Glen Choo <chooglen@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> `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.
