Thread (27 messages) 27 messages, 4 authors, 3d ago

Re: [PATCH v4 1/1] environment: move excludes_file into repo_config_values

From: Tian Yuchen <hidden>
Date: 2026-06-30 16:21:06

Hi Christian and Junio,

On 6/29/26 22:47, Junio C Hamano wrote:
Christian Couder [off-list ref] writes:
quoted
I agree that the best end state would be to have no `if (!repo ||
!repo->initialized)` check, but we shouldn't have to get there right
away. I think it's fine to proceed in several steps:

1) `if (!repo || !repo->initialized) return NULL;` allows us to remove
the global variable and use getters which will help us in the next
step.

2) `if (!repo || !repo->initialized) return BUG("repo must be an
initialized repository");` now we want to find and fix callers
(including tests) that haven't properly initialized things.

3) No `if (!repo || !repo->initialized)` check, as we are sure that
all the callers that didn't properly initialized things have been
found and fixed.

So I think 1) is fine for now as long as we properly explain in the
commit messages and in code comments (maybe using NEEDSWORK comments)
that we know there is more work to do on this in the future.
I am OK with the progressive improvements, but if "wean us away from
global variables" topic stops at step 1 I would have to say that is
a failed experiment.  Not doing (2) means you made the system bug-to-bug
compatible from the old world where these things weren't in repo-config
but were still globals, which is code churn for nothing good to show
in the end result.  We need to get to (2) before declaring victory.

But of course, we need to start somewhere.  (1) with in-code
comments sprinkled to such places that say that we'd need to revisit
would be a good place to start.

Thanks.
Thank you both for paving a clear way forward.

For the upcoming V5 patch, I will implement:

1. Revert to the shields ('return NULL' for the getter, and bypassing 
'repo != repository' for the _clear()).
2. Add 'NEEDSWORK' comments above them documenting that these are 
temporary changes.
3. Update the commit message to reflect this.

Once this initial migration safely lands, the next goal will be to 
investigate those failing CI tests.

Will send out V5 shortly.

Regards, yuchen

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help