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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Dec 13, 2021, 22:58 UTC
Message-ID: <20211213225842.768852-1-jonathantanmy@google.com>
In-Reply-To: <kl6lbl1p9zjf.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 45 quoted lines

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

Show 13 quoted lines

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