Re: [PATCH] clone: add remote.cloneDefault config option

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 27, 2020, 04:21 UTC
Message-ID: <xmqqpn7csoqd.fsf@gitster.c.googlers.com>
In-Reply-To: <20200827033854.130005-1-sean@barag.org>

Sean Barag <sean@barag.org> writes:

Show 17 quoted lines

>> It is somewhat sad that we have the git_config(git_default_config)
>> call so late in the control flow.  I wonder if we can update the
>> start-up sequence to match the usual flow
>> ...
>> One oddity "git clone" has is that it wants to delay the reading of
>> configuration files (they are read only once, and second and
>> subsequent git_config() calls will reuse what was read before [*]) so
>> that it can read what clone.c::write_config() wrote, so if we were to
>> "fix" the start-up sequence to match the usual flow, we need to
>> satisfy what that odd arrangement wanted to achieve in some other way
>> (e.g. feed what is in option_config to git_default_config ourselves,
>> without using git_config(), as part of the "main control flow uses the
>> variable" part), but it should be doable.
>
> Sounds like a pretty big change! I'm willing to take a crack at it,
> but given that this is my first patch I'm frankly a bit intimidated :)
> How would you feel about that being a separate patch?

It definitely needs to be a separate patch.

In order to realize any new feature that needs to read the existing (i.e. per-machine or per-user) configuration files to affect the behaviour of "git clone", whether it is the "give default to --origin option" or any other thing, first needs to fix the start-up sequence so that the configuration is read once before we process command line options, which is the norm. Only after that is done, we can build the clone.defaultRemoteName and other features that would be affected by the settings of clone.* variables on top, and it is not wise to mix the foundation with a new feature that uses the foundation. So I would imagine it would at least be 3-patch series:

 - [1/3] would be to whip the start-up sequence into the normal
   order.  This may be the most tricky part.  I identified that the
   handling of option_config might need adjustment, but there may be
   others.  This may not need any new tests, but if the existing
   tests for "clone -c var=val" do not catch breakage when we
   naively move the git_config(git_default_config) call early
   without doing any other adjustment, we might need to add more
   tests to cover the option.  If we find things other than
   option_config needs adjustment, they also need test coverage.

 - [2/3] would be to tighten the error checking of option_origin
   that was lost when the command was reimplemented in C (already
   discussed).  This would need new tests to see "--origin $bogus"
   command line option is flagged as an error.

 - [3/3] would be to read option_origin from the configuration file.
   The start-up sequence fixed by [1/3] would allow us to write the
   config callback in a more natural way, compared to what you wrote
   and what I suggested to rewrite.  Error checking code in [2/3]
   would directly be reusable in the code added in this step.  We'd
   need tests similar to we add in [2/3] for the configuration, and
   combination of configuration and command line (you already wrote
   most of these).

Thanks.