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

From: Jonathan Tan <jonathantanmy@google.com>
To: chooglen@google.com
Cc: git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only
Date: Thu,  9 Dec 2021 14:12:50 -0800	[thread overview]
Message-ID: <20211209221250.508257-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20211207192925.67680-1-chooglen@google.com>

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.

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