Possible regression in git 2.26.0

Today I'd noticed a failure in some custom automation that seems to came as a side effect after of a git client update.

The issue issue I'd spotted is the following:

$ git clone git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/bionic linux/bionic Cloning into 'linux/bionic'... fatal: remote error: Parameters must have values

Reverting to git 2.17.1 didn't show the issue.

I'd asked at #git in freenode and @rafasc was able to reproduce and bisect, pointing to

https://github.com/git/git/commit/684ceae32dae726c6a5c693b257b156926aba8b7

For the time being I'd added `-c protocol.version=0` as a temporary workaround.

Is this a regression?

Thanks for looking into this.

Josep Torra

On Tue, Mar 31, 2020 at 07:13:25PM +0200, Josep Torra wrote:

I'd like to relay the following conversation I had recently:

06:00 <sfr> anyone know why git recently has fetched basically a whole kernel
            repo when I try to update some trees/branches
06:00 <sfr> this has happened to me for three separate trees over the past few days
06:01 <sfr> none were on git.kernel.org
11:31 <mricon> sfr: if you have an example of the tree/branch that caused you to
               download a lot for a simple pull, I'd be interested in knowing what it
               is so I can poke.
16:26 <sfr> mricon: last evening it was fetching the drm tree
            (git://git.freedesktop.org/git/drm/drm.git branch drm-next):
16:26 <sfr> remote: Enumerating objects: 7237633, done.
16:26 <sfr> remote: Counting objects: 100% (7237633/7237633), done.
16:26 <sfr> remote: Compressing objects: 100% (1069838/1069838), done.
16:26 <sfr> remote: Total 7237633 (delta 6121918), reused 7236008 (delta 6120350)
16:26 <sfr> Receiving objects: 100% (7237633/7237633), 1.32 GiB | 642.00 KiB/s, done.
16:26 <sfr> Resolving deltas: 100% (6121918/6121918), done.
16:26 <sfr> From git://git.freedesktop.org/git/drm/drm
16:26 <sfr>    c2556238120bc..700d6ab987f3b  drm-next   -> drm/drm-next
16:30 <mricon> sfr: interesting. What's your git version?
16:30 <sfr> $ git --version
16:30 <sfr> git version 2.26.0
16:31 <sfr> Debian, so may have some extra patches
16:35 <mricon> sfr: git-2.26.0 switched to protocol v2 by default, so I'm curious
               if that is what changed
16:35 <mricon> but I've been using protocol v2 for a while now, and I don't see
               this problem
16:36 <mricon> I also don't yet have git-2.26.0
16:37 <mricon> sfr: if you come across another repo that shows this problem, 
               I suggest you set protocol.version=1 in your .gitconfig to see
               if that makes any difference
16:37 <sfr> mricon: ok, thanks
16:38 <mricon> total shot in the dark
16:46 <sfr> mricon: that *seems* to make a difference
16:47 <sfr> repo: git://anongit.freedesktop.org/drm/drm-misc branch: for-linux-next
16:48 <sfr> went from trying to transfer 7226542 to just 19
16:48 <sfr> even after I did a reset of the remote branch at my end and a gc
16:49 <sfr> it updated b1e7396a1d0e..98878d9dfc7a
16:50 <sfr> in case it matters, I have "tagopt = --no-tags" set for all the repos
            I fetch (for obvious reasons)
16:52 <sfr> removing the protocol.version=1 (and resetting/gcing) gets me the bad 
            behaviour again :-(
16:52 <sfr> mricon: so good shot! :-)

It appears that there are cases where protocol.version=2 is causing weird results during ref negotiation?

-K

On Tue, Mar 31, 2020 at 01:22:18PM -0400, Konstantin Ryabitsev wrote:

Possibly. The underlying negotiation technique is roughly the same, but it's possible there's a subtle difference in the v2 code.

I spent a little time trying to reproduce with the branch mentioned above, but couldn't. If somebody hits it again, the most useful thing would be:

  - a dump of "git for-each-ref" in the client repo

  - the exact fetch (or pull) command which behaves differently between
    the two protocols

-Peff

On Tue, Mar 31, 2020 at 07:13:25PM +0200, Josep Torra wrote:

I can reproduce here, and it's definitely related to using protocol v2. Running:

  GIT_TRACE_PACKET=1 git -c protocol.version=2 ls-remote \
    git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/bionic \
    >/dev/null

yields:

  [...]git> git-upload-pack ~ubuntu-kernel/ubuntu/+source/linux/+git/bionic\0host=git.launchpad.net\0\0version=2\0
  [...]git< ERR Parameters must have values

So it's complaining at that initial contact probing for v2. The behavior is the same with v1 (which was an interim step between v0 and v1 to do this probing and check for compatibility).

We checked that the probe works sensibly against old versions of Git, as well as a few others (JGit, and I can't remember the rest).

Do you know what software powers git.launchpad.net? That "Parameters must have values" message has never appeared in any Git version, though the server claims to be v2.17.1:

  $ GIT_TRACE_PACKET=1 git -c protocol.version=0 ls-remote \
     git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/bionic \
     2>&1 >/dev/null | grep agent=
  [...]git< a78d21bd8bb58c158f73108eb7d7402619fcae3d HEAD\0multi_ack thin-pack side-band side-band-64k ofs-delta shallow deepen-since deepen-not deepen-relative no-progress include-tag multi_ack_detailed symref=HEAD:refs/heads/master agent=git/2.17.1

My guess is that it may be v2.17.1 with some kind of load-balancing or proxy layer in it. It's rather unfortunate that nobody noticed the problem during the year and a half testing phase. But at this point, I think the best path forward is probably to try to fix that server side (preferably to speak v2, but if not, to at least ignore the v2 probe marker and continue to speak v0; the client will gracefully handle that).

By the way, swapping out git:// for https:// in the URL works just fine for this server. In git-over-http, the v2 probe goes in an HTTP request header, and they seem to ignore that and speak v0 (which is an expected and fine outcome for pre-v2-aware servers). That may be why nobody noticed before.

-Peff

On Fri, Apr 03, 2020 at 02:58:04PM -0400, Jeff King wrote:

I guess it's this:

  https://code.launchpad.net/launchpad

but I can't find any trace of the "Parameters must have values" error message in there. It looks like it builds on dulwich, but I can't find the message there either. So I'm at a loss.

-Peff

Jeff King <peff@peff.net> writes:

Let me apologize in advance for a totally useless distraction, but nevertheless I found it funny. My search engine found

    https://ntrs.nasa.gov/archive/nasa/casi.ntrs.nasa.gov/19780010630.pdf

when I asked

    launchpad "parameters must have values"

https://www.google.com/search?q=launchpad+%22parameters+must+have+values%22

;-)

On Fri, Apr 03, 2020 at 12:53:50PM -0700, Junio C Hamano wrote:

Heh, wow. I knew vaguely what to expect from the URL, but was quite pleased to see a PDF full of scanned FORTRAN code. I have a new appreciation for working with our newfangled "C" code.

I also searched for that error but didn't come up with much. It seems to be common in some AWS workflows. I wonder if it's just coincidence, or if launchpad's git:// support is somehow just proxying to a service run through AWS.

I think the next step is to report it to launchpad folks and let them figure it out. I opened a bug there:

  https://bugs.launchpad.net/launchpad/+bug/1870804

-Peff

On Sat, Apr 04, 2020 at 11:20:33AM -0400, Jeff King wrote:

Looks like this is now fixed:

  https://bugs.launchpad.net/turnip/+bug/1870080

-Peff