Re: [PATCH 2/2] connect: know that zero-ID is not a ref

From: Jeff King <peff@peff.net>
Date: Sep 2, 2016, 20:13 UTC
Message-ID: <20160902201321.35egsg5l6r2fvrtw@sigill.intra.peff.net>
In-Reply-To: <2bea354c6218a33db3972e42baa75676fdcbc598.1472836026.git.jonathantanmy@google.com>

On Fri, Sep 02, 2016 at 10:15:39AM -0700, Jonathan Tan wrote:

Show 13 quoted lines

> connect.c, when processing packfiles, treats a zero ID (with
> `capabilities^{}` in place of the refname) as an actual ref instead of a
> placeholder for a capability declaration, contrary to the specification
> in Reference Discovery in Documentation/technical/pack-protocol.txt.
> This is an issue when interacting with Git implementations that follow
> this specification. For example, `ls-remote` (against a git://
> repository served by such a Git implementation) will report a ref when
> there are none, and `clone` (against something similar) will fail its
> checkout.
> 
> Make connect.c follow the specification with respect to this, while
> maintaining compatibility with existing implementations that do not
> serve the zero ID when a repository has no refs.

Hmm. So since this is backwards-compatible, I'm not overly concerned with changing the client. But I wonder if you considered that the documentation is wrong, and that JGit should stop sending the extra capabilities line?

In either case, there will still be breakage until _somebody_ upgrades (with your patch, until clients upgrade; with a JGit patch, until the server upgrades). So perhaps an interesting question would be: if we were writing this now, what is the correct behavior?

For pushing, it is obviously useful to send capabilities even though there are no refs (because the client is going to send _you_ something). And that is why "capabilities^{}" exists; it is a receive-pack feature (that is actually implemented!), and the documentation (which came after the implementation) blindly mentioned it for upload-pack, as well.

Is it useful for upload-pack? If we have no refs, there's traditionally been nothing to fetch. Perhaps that's something that could change, though. For example, there could be a capability to allow fetching arbitrary sha1s (we have allowTIPSH1InWant and allowReachableSHA1InWant, which obviously both require some refs, but allowArbitrarySHA1 does not seem outside the realm of possibility).

I'll review the rest assuming that this is the direction we want to go...

Show 6 quoted lines

> (git-daemon should probably also be changed to serve zero IDs, but such
> a change can be considered independently from this change; even if both
> the client and server changes were made in one commit, it is nearly
> impossible that all Git installations are updated at the same time - an
> updated client would still need to deal with unupdated servers and vice
> versa.)

I'm really not sure what you mean here. How does git-daemon enter into this at all?

Show 6 quoted lines

> +		if (is_null_oid(&old_oid)) {
> +			if (strcmp(name, "capabilities^{}"))
> +				warning("zero object ID received that is not accompanied by a "
> +				        "capability declaration, ignoring and continuing anyway");
> +			continue;
> +		}

I agree with Shawn that this should be looking for "capabilities^{}" as the name of the first entry (and the warning can go away; if we see any other null sha1, it just gets handled in the usual error code paths).

Show 13 quoted lines

> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 819b9dd..c6f8b6f 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
>  	test_cmp expect actual
>  '
>  
> +test_lazy_prereq GIT_DAEMON '
> +	test_have_prereq JGIT &&
> +	test_tristate GIT_TEST_GIT_DAEMON &&
> +	test "$GIT_TEST_GIT_DAEMON" != false
> +'

GIT_DAEMON depends on JGIT? Should this really be the JGIT_DAEMON prerequisite?

Show 12 quoted lines

> +# This test spawns a daemon, so run it only if the user would be OK with
> +# testing with git-daemon.
> +test_expect_success JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
> +	JGIT_DAEMON_PID= &&
> +	git init --bare empty.git &&
> +	touch empty.git/git-daemon-export-ok &&
> +	{
> +		jgit daemon --port="$JGIT_DAEMON_PORT" . &
> +		JGIT_DAEMON_PID=$!
> +	} &&
> +	test_when_finished kill "$JGIT_DAEMON_PID" &&
> +	sleep 1 && # allow jgit daemon some time to set up

We definitely need something more robust than this "sleep". This test is going to fail racily when the system is under load.

-Peff