Revert "fetch: default to protocol version 2"

Commit Message

Jonathan Nieder April 22, 2020, 3:50 p.m. UTC

This reverts commit 684ceae32dae726c6a5c693b257b156926aba8b7.

Users fetching from linux-next and other kernel remotes are reporting
that the limited ref advertisement causes negotiation to reach
MAX_IN_VAIN, resulting in too-large fetches.

Reported-by: Lubomir Rintel <[e-mail]>
Reported-by: "Dixit, Ashutosh" <[e-mail]>
Reported-by: Jiri Slaby <[e-mail]>
Reported-by: Konstantin Ryabitsev <[e-mail]>
Signed-off-by: Jonathan Nieder <[e-mail]>
---
Hi again,

Jeff King wrote:

> Thanks for this report. We've been tracking the issue but have had
> trouble reproducing it.
>
> To get you unstuck, the immediate workaround is to drop back to the
> older protocol, like:
>
>   git -c protocol.version=0 fetch --all

By the way, I'd recommend the immediate workaround of

	git fetch --negotiation-tip=refs/remotes/xo/* xo

instead.  But that's a separate subject.

[...]
> There are a few data points we've been wanting to collect:
[...]
> I'm attaching for-each-ref output before and after the xo fetch. That
> should be sufficient to recreate the situation synthetically even once
> these repos have moved on.

Excellent --- I think this is enough for us to have something to use
to investigate, switching users to protocol v0 in the meantime.

Thanks,
Jonathan

 Documentation/config/protocol.txt | 2 +-
 protocol.c                        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Junio C Hamano April 22, 2020, 6:23 p.m. UTC | #1

Jonathan Nieder <[e-mail]> writes:

> This reverts commit 684ceae32dae726c6a5c693b257b156926aba8b7.
>
> Users fetching from linux-next and other kernel remotes are reporting
> that the limited ref advertisement causes negotiation to reach
> MAX_IN_VAIN, resulting in too-large fetches.
>
> Reported-by: Lubomir Rintel <[e-mail]>
> Reported-by: "Dixit, Ashutosh" <[e-mail]>
> Reported-by: Jiri Slaby <[e-mail]>
> Reported-by: Konstantin Ryabitsev <[e-mail]>
> Signed-off-by: Jonathan Nieder <[e-mail]>
> ---

With the one-liner fix for resetting the "we haven't heard an Ack"
timer, I am no longer so worried by hurting users by keeping them on
v2 by default, but it will take some time for the fix to trickle
down from the 'master' track to a maintenance release, so I am OK to
apply this patch in the meantime.  But let's flip the default back
to v2 after the one-liner fix lands to see if (1) the fix truly
helps and (2) people hit other issues in the difference between v0
and v2.

Thanks.

Jeff King April 22, 2020, 7:40 p.m. UTC | #2

On Wed, Apr 22, 2020 at 08:50:47AM -0700, Jonathan Nieder wrote:

> This reverts commit 684ceae32dae726c6a5c693b257b156926aba8b7.
> 
> Users fetching from linux-next and other kernel remotes are reporting
> that the limited ref advertisement causes negotiation to reach
> MAX_IN_VAIN, resulting in too-large fetches.

OK, now that we have data I think this strategy is reasonable.

That said, it will take a while to make it to a release, so we very well
may have brought v2 and v0 to parity in the meantime.

> > To get you unstuck, the immediate workaround is to drop back to the
> > older protocol, like:
> >
> >   git -c protocol.version=0 fetch --all
> 
> By the way, I'd recommend the immediate workaround of
> 
> 	git fetch --negotiation-tip=refs/remotes/xo/* xo
> 
> instead.  But that's a separate subject.

It seems like if we are fetching with refspec X/*:Y/* that we should
perhaps automatically select our local Y/* negotiation tips.

That said, neither it (nor the manual version above) would help the case
I've been testing with. It's a first fetch from "xo", which can reuse
history we already have from other remotes.

I agree it's a good workaround for folks doing their daily fetches,
though.

-Peff

Jeff King April 22, 2020, 7:47 p.m. UTC | #3

On Wed, Apr 22, 2020 at 03:40:47PM -0400, Jeff King wrote:

> > By the way, I'd recommend the immediate workaround of
> > 
> > 	git fetch --negotiation-tip=refs/remotes/xo/* xo
> > 
> > instead.  But that's a separate subject.
> 
> It seems like if we are fetching with refspec X/*:Y/* that we should
> perhaps automatically select our local Y/* negotiation tips.

Actually, I guess that option is not "please prioritize these tips", but
rather "use only these tips". So using it can sometimes hurt.

> That said, neither it (nor the manual version above) would help the case
> I've been testing with. It's a first fetch from "xo", which can reuse
> history we already have from other remotes.
> 
> I agree it's a good workaround for folks doing their daily fetches,
> though.

I did confirm that re-running my test with --negotiation-tip=master (to
point to Linus's tree, avoiding the clogging of "next" commits) results
in the usual good pack in a reasonable time.

-Peff

Patch

diff --git a/Documentation/config/protocol.txt b/Documentation/config/protocol.txt
index 756591d77b0..0b40141613e 100644
--- a/Documentation/config/protocol.txt
+++ b/Documentation/config/protocol.txt
@@ -48,7 +48,7 @@  protocol.version::
 	If set, clients will attempt to communicate with a server
 	using the specified protocol version.  If the server does
 	not support it, communication falls back to version 0.
-	If unset, the default is `2`.
+	If unset, the default is `0`.
 	Supported versions:
 +
 --
diff --git a/protocol.c b/protocol.c
index 803bef5c87e..d390391ebac 100644
--- a/protocol.c
+++ b/protocol.c
@@ -39,7 +39,7 @@  enum protocol_version get_protocol_version_config(void)
 		return env;
 	}
 
-	return protocol_v2;
+	return protocol_v0;
 }
 
 enum protocol_version determine_protocol_version_server(void)