All of lore.kernel.org
 help / color / mirror / Atom feed
From: <rsbecker@nexbridge.com>
To: "'Kevin Puetz'" <PuetzKevinA@johndeere.com>,
	"'Jeff King'" <peff@peff.net>
Cc: "'brian m. carlson'" <sandals@crustytoothpaste.net>,
	<git@vger.kernel.org>
Subject: RE: [Bug] git fetch --dry-run --filter makes changes to .git/config
Date: Thu, 18 Sep 2025 18:21:08 -0400	[thread overview]
Message-ID: <022401dc28ea$8be87d40$a3b977c0$@nexbridge.com> (raw)
In-Reply-To: <DS0PR05MB10013AC6090CEE7A562B56CBCB516A@DS0PR05MB10013.namprd05.prod.outlook.com>

On September 18, 2025 5:55 PM, Kevin Puetz wrote:
>> > I did find one odd quirk. When the remote and the requested commit
>> > are unrelated e.g. I'm taking to the wrong remote, or have done `git
>> > checkout --orphan`), I get
>> >
>> > $ git fetch https://github.com/git/git
>
> --negotiate-only --negotiation-tip=$COMMIT
>> > fatal: expected 'acknowledgments', received 'packfile'
>> >
>> > This still works for conan's purpose (exiting with an error means it
>> > didn't print a matching commit hash), but I expected something more like the
>"fatal: remote error: upload-pack: not our ref: ..."
>> > error that you get from git fetch {remote} --refetch $COMMIT. Of
>> > course, unexpectly sending a packfile could be a problem with github's server
>implementation, rather than the git client.
>>
>> I had never used --negotiate-only before, and did a simple test which
>> ended up with the exact same problem. I agree it's probably a bug, but
>> it's present in git.git itself (my fetch was to a local test repo).
>>
>> I'm not sure if the bug is in the server or client, though. If I
>> understand correctly, the server does not know anything about this
>> "negotiate-only" mode, but is just responding to the client. The
>> client is supposed to say "done" to tell the server that it is not
>> sending any more "have" negotiation, at which point the server sends the
>packfile.
>> But in this case the packfile comes anyway. Running with
>> GIT_TRACE_PACKET in the environment shows the client making an extra
>> request with no "have" lines at all, which the server then takes as an
>> indication it can send the packfile.
>
>Yep. In the cases where things work as expected, the client sends various `have`
>lines for ancestors of the negotiation-tips, and then the server sends "ACK ..." with
>the one it knows.
>
>When they are unrelated (none of the have lines are comits known to the remote),
>it replies with NAK (which is sensible enough). But then the client doesn't just error
>out after the NAK it keeps going. I guess because if this really was a fetch, rather
>than negotiate-only, and we have nothing in common with the server, we'd still
>need to fetch the wanted ref (and receive everything).
>But it *is* --negotiate-only, so we don't actually want anything either.
>Which leads to this weird empty request-for-nothing. Seems like the client Really
>should have just exited early after the NAK, given that it doesn't want anything.
>
>> So I think there is at least one client bug, which is making that
>> extra request. There might _also_ be a server bug, because it is
>> sending the packfile without a "done" line, even though it was told
>> the client wants the wait-for-done option (and the client does not even send a
>"want"
>> line at all, so what does it think it is sending?).
>
>Looking closer, I think the server bug is at
>
>https://github.com/git/git/blob/ca2559c1d630eb4f04cdee2328aaf1c768907a9e/
>upload-pack.c#L1805-L1813
>
>There's an early exit to UPLOAD_DONE (without sending any packfile) if you didn't
>want anything, But it's skipped over if you use wait-for-done, because that's the
>negotiate-only case, which still wants the acknowledgements.
>But then (during the weird empty client request) seen_haves isn't true either, So it
>skips the UPLOAD_SEND_ACKS case too. And it ends up just falling through into the
>final else { state = UPLOAD_SEND_PACK; }
>
>I think here the client was expecting the server to go into UPLOAD_SEND_ACKS, and
>just send an empty set of acknowledgements, which seems legal.
>https://git-scm.com/docs/gitprotocol-v2 says
>
>- If the server has found a suitable cut point and has decided to send a "ready" line,
>  then the server can decide to (as an optimization) omit any "ACK" lines it would
>have sent
>
>So I think the server could/should just reply with no "ACK" lines and "ready"
>(i.e. you don't want anything, but sure, I can send you an empty pack file).
>But it's definitely a corner case. And right now it wouldn't work, because
>https://github.com/git/git/blob/master/upload-pack.c#L1712-L1713
>
>would send NAK instead (which also seems legal, depending on how one interprets
>
>- The server will respond with "NAK" if none of the object ids sent as have lines
>were common.
>
>I think the client could deal with either "ready" or "NAK", it's just the empty pack it's
>not expecting.

If I might ask, and I realize this is an edge-condition, but what is the intent when
the clone has been done with a --depth=1 or similar? The single commit may not
be known locally but may be in the history as far as the remote is concerned. I am
just curious.

--Randall


  reply	other threads:[~2025-09-18 22:21 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-09-17 14:44 [Bug] git fetch --dry-run --filter makes changes to .git/config Kevin Puetz
2025-09-17 21:21 ` brian m. carlson
2025-09-17 23:23   ` Kevin Puetz
2025-09-18 19:20     ` Jeff King
2025-09-18 20:28       ` Junio C Hamano
2025-09-18 20:39         ` Jeff King
2025-09-18 20:46           ` Junio C Hamano
2025-09-18 21:55       ` Kevin Puetz
2025-09-18 22:21         ` rsbecker [this message]
2025-09-19 15:31           ` Kevin Puetz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='022401dc28ea$8be87d40$a3b977c0$@nexbridge.com' \
    --to=rsbecker@nexbridge.com \
    --cc=PuetzKevinA@johndeere.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.