Re: [PATCH] builtin/fetch: skip unnecessary tasks when using --negotiate-only
flat view
From: Jonathan Tan <hidden>
Date: 2021-12-09 22:12:54
Glen Choo [off-list ref] 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 <redacted> --- `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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ /* 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.