Re: [PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only

[Glen Choo <chooglen@google.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> Glen Choo <chooglen@google.com> writes:
>> Jonathan Tan <jonathantanmy@google.com> writes:
>> 
>> > Glen Choo <chooglen@google.com> writes:
>> >> `git fetch --negotiate-only` does not fetch objects and thus, it should
>> >> not perform certain auxiliary tasks like updating submodules, updating
>> >> the commit graph, or running gc. Although send_pack() invokes `git fetch
>> >> --negotiate-only` correctly, cmd_fetch() also reads config variables,
>> >> leading to undesirable behavior, like updating submodules if
>> >> `submodule.recurse=true`.
>> >> 
>> >> Make cmd_fetch() return early if --negotiate-only was specified so that
>> >> these auxiliary tasks are skipped.
>> >> 
>> >> Signed-off-by: Glen Choo <chooglen@google.com>
>> >> ---
>> >> `git fetch --negotiate-only` is used during push negotiation to
>> >> determine the reachability of commits. As its name implies, only
>> >> negotiation is performed, not the actual fetching of objects. However,
>> >> cmd_fetch() performs certain tasks with the assumption that objects are
>> >> fetched:
>> >> 
>> >> * Submodules are updated if enabled by recurse.submodules=true, but
>> >>   negotiation fetch doesn't actually update the repo, so this doesn't
>> >>   make sense (introduced in [1]).
>> >> * Commit graphs will be written if enabled by
>> >>   fetch.writeCommitGraph=true. But according to
>> >>   Documentation/config/fetch.txt [2], this should only be done if a
>> >>   pack-file is downloaded
>> >> * gc is run, but according to [3], we only do this because we expect
>> >>   `git fetch` to introduce objects
>> >> 
>> >> Instead of disabling these tasks piecemeal, let's just make cmd_fetch()
>> >> return early if --negotiate-only was given. To accommodate possible
>> >> future options that don't fetch objects, I opted to introduce another
>> >> `if` statement instead of putting the early return in the existing
>> >> `if (negotiate_only)` block.
>> >
>> > Some of this probably should be in the commit message too.
>> 
>> I suppose you mean the explanation of why the tasks are irrelevant to
>> negotiation fetch? i.e. 
>> 
>>    * Submodules are updated if enabled by recurse.submodules=true...
>>    * Commit graphs will be written if enabled by...
>>    * gc is run, but according to [3]...
>
> Yes - why the behavior is undesirable, and the way you're doing it (by
> adding another "if" statement).
>
> After looking at this, more concretely, it might be better to use the
> part below the "---" as your commit message. :-) Just note that we're
> not just accommodating future options that don't fetch objects - "fetch"
> already may not fetch objects (e.g. if the ref we want doesn't exist or
> if we already have all the objects).

Good suggestion. I'll clean it up for brevity.

>
>> > Maybe add a check here that --recurse-submodules was not explicitly
>> > given.
>> 
>> Hm, that's not a bad idea, but it's not so easy because we don't have
>> RECURSE_SUBMODULES_EXPLICIT so it's not easy to tell whether or not
>> submodule recursion was enabled by CLI option or config.
>> 
>> This is the exact same use case I encountered with "branch
>> --recurse-submodules" [1]. I think this means that we should consider
>> standardizing the parsing of submodule.recurse + --recurse-submodules. I
>> haven't done it yet because it's a little tricky and hard to review.
>> 
>> So I'll punt on this check until we get RECURSE_SUBMODULES_EXPLICIT.
>
> Hmm...can we separate out the recurse_submodules variable into one
> that's given by config and one that's given by CLI argument?

This would be similar to what I did for branch --recurse-submodules [1],
which, as I noted in my cover letter [2], is easier to understand, but a
little inconsistent with the rest of the --recurse-submodules parsing.

I'm not very enthusiastic about adding more inconsistency, and since
this check isn't critical to this bugfix (and I think there is very
little chance that anyone would run afoul of this check any time soon)
I'd like to hold off on it until RECURSE_SUBMODULES_EXPLICIT.

Unless you think I'm missing something of course :)

[1] https://lore.kernel.org/git/20211216003213.99135-6-chooglen@google.com
[2] https://lore.kernel.org/git/20211216003213.99135-1-chooglen@google.com