Jonathan Tan [off-list ref] writes:
Glen Choo [off-list ref] writes:
quoted
Jonathan Tan [off-list ref] writes:
quoted
Glen Choo [off-list ref] writes:
quoted
`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.
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.
quoted
quoted
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@google.com (local)
[2] https://lore.kernel.org/git/20211216003213.99135-1-chooglen@google.com (local)