Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > Glen Choo <chooglen@xxxxxxxxxx> writes: >> Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: >> >> > 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. >> >> 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). Good suggestion. I'll clean it up for brevity. > >> > 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? This would be similar to what I did for branch --recurse-submodules [1], which, as I noted in my cover letter [2], is easier to understand, but a little inconsistent with the rest of the --recurse-submodules parsing. I'm not very enthusiastic about adding more inconsistency, and since this check isn't critical to this bugfix (and I think there is very little chance that anyone would run afoul of this check any time soon) I'd like to hold off on it until RECURSE_SUBMODULES_EXPLICIT. Unless you think I'm missing something of course :) [1] https://lore.kernel.org/git/20211216003213.99135-6-chooglen@xxxxxxxxxx [2] https://lore.kernel.org/git/20211216003213.99135-1-chooglen@xxxxxxxxxx
