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:
>> `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]...

>> +	if (negotiate_only) {
>> +		/*
>> +		 * --negotiate-only should never recurse into
>> +		 * submodules, so there is no need to read .gitmodules.
>> +		 */
>> +		recurse_submodules = RECURSE_SUBMODULES_OFF;
>> +		if (!negotiation_tip.nr)
>> +			die(_("--negotiate-only needs one or more --negotiate-tip=*"));
>> +	}
>
> 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.

>
>> +	/* skip irrelevant tasks if objects were not fetched  */
>> +	if (negotiate_only)
>> +		return result;
>
> There are other reasons too why objects were not fetched (e.g. if we
> already have all of them). Maybe add a NEEDSWORK explaining this.

Good point, the comment doesn't distinguish between cases where we know
that objects won't be fetched beforehand vs cases where we found out
that objects weren't fetched after the fact. I'll add the NEEDSWORK.

[1] https://lore.kernel.org/git/20211209184928.71413-5-chooglen@google.com