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

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> 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.