Re: [PATCH 03/13] setup: unify setup of shallow file

[Patrick Steinhardt <ps@pks.im>]

On Mon, Jul 06, 2026 at 05:02:08PM -0500, Justin Tobler wrote:
> On 26/06/30 01:47PM, Patrick Steinhardt wrote:
> > It is possible to configure an arbitrary "shallow" file via two
> > mechanisms, and the respective logic to handle these is split across two
> > locations:
> > 
> >   - Via the "GIT_SHALLOW_FILE" environment variable, which is handled in
> >     `setup_git_env_internal()`.
> > 
> >   - Via the global "--shallow-file=" command line option, which is
> >     handled in `handle_options()`.
> 
> Ok.
> 
> > We can rather easily unify this logic by not configuring the shallow
> > file in `handle_options()`, but instead overwriting the environment
> > variable. The environment variable itself is then handled inside of
> > `apply_repository_format()`, which is responsible for configuring a
> > discovered Git directory.
> 
> What is supposed to be the correct order for processing shallow file
> configuration here? Does this mean that the `--shallow-file` option now
> overwrites the environment variable? Was this how it already was?

That's a good question. The command line switch does override the
environment variable, but it's not a change in behaviour: the last
parameter of `set_alternate_shallow_file()` controls whether or not we
want to override an already-configured shallow file. So even though we
used to call that function with the value of the environment variable at
a much later point in time, we had that parameter set to `0` there. So
if we've already configured the shallow file before via "--shallow-file"
it wouldn't have been overwritten.

We can remove this logic now though, as it's essentially unused after
this patch. And it certainly warrants a mention in the commit message.

> > This new logic is similar in nature to how we handle the other global
> > options already, all of which end up setting an environment variable.
> > So for one this gives us more consistency. But more importantly, this
> > change means that `the_repository` will not contain any relevant state
> > anymore before we hit `apply_repository_format()` once we're at the end
> > of this patch series. Consequently, it will become possible for us to
> > completely discard `the_repository` and populate it anew.
> 
> I can't say that I'm a fan of using environment variables to store
> global state in this manner, but I guess if there is precdent and this
> is making us more consistent, it is probably fine. I guess the other
> option would be to store the read configuration is some intermediate
> structure to be applied later, but that may not be worth it here.

I agree, I'm not much of a fan of this either. I'd also love to
eventually refactor the argument handling in "git.c" to not rely on
global state anymore, but that's going to be a bigger refactoring (if
it's feasible at all).

Patrick