Re: [PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Dec 9, 2021, 22:12 UTC
- Message-ID
- <20211209221250.508257-1-jonathantanmy@google.com>
- In-Reply-To
- <20211207192925.67680-1-chooglen@google.com>
Glen Choo <chooglen@google.com> writes:
Show 33 quoted lines
> `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.
Show 9 quoted lines
> + 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.
> + /* 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.
Besides these code comments and the fact that the commit message could be improved, this patch looks good to me.