[PATCH 0/2] handle empty spec-compliant remote repos correctly

[PATCH 0/2] handle empty spec-compliant remote repos correctly

[Jonathan Tan]

This issue was noticed when trying to clone an empty repository served by a
server that uses the JGit library.

Reference Discovery in Documentation/technical/pack-protocol.txt dictates that
servers should send a zero ID when there are no refs in the queried repository,
and implementations like JGit do, but the C client does not handle them
correctly (treating them as an actual ref and subsequently returning incorrect
responses or errors).

These patches fix those while maintaining backwards compatibility with existing
implementations that do not send the zero ID in such a case.

Jonathan Tan (2):
  tests: move test_lazy_prereq JGIT to test-lib.sh
  connect: know that zero-ID is not a ref

 connect.c               |  7 +++++++
 t/t5310-pack-bitmaps.sh |  4 ----
 t/t5512-ls-remote.sh    | 22 ++++++++++++++++++++++
 t/test-lib.sh           |  4 ++++
 4 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh

[Jonathan Tan]

This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5310-pack-bitmaps.sh | 4 ----
 t/test-lib.sh           | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
 	git pack-objects --stdout --revs <revs >/dev/null
 '
 
-test_lazy_prereq JGIT '
-	type jgit
-'
-
 test_expect_success JGIT 'we can read jgit bitmaps' '
 	git clone . compat-jgit &&
 	(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
 	test "$uid" != 0
 '
 
+test_lazy_prereq JGIT '
+	type jgit
+'
+
 # SANITY is about "can you correctly predict what the filesystem would
 # do by only looking at the permission bits of the files and
 # directories?"  A typical example of !SANITY is running the test
-- 
2.8.0.rc3.226.g39d4020

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

[Jonathan Tan]

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.

(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.)

The test uses JGit's daemon feature, which is specification-compliant.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 connect.c            |  7 +++++++
 t/t5512-ls-remote.sh | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/connect.c b/connect.c
index 722dc3f..d4a58de 100644
--- a/connect.c
+++ b/connect.c
@@ -165,6 +165,13 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			continue;
 		}
 
+		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;
+		}
+
 		if (!check_ref(name, flags))
 			continue;
 		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
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
+'
+
+JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}
+
+# 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
+	test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
+'
 
 test_done
-- 
2.8.0.rc3.226.g39d4020

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

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> 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.

For example, this means I get the following confusing message when
cloning an empty repository served by "jgit daemon":

 $ git clone https://googlers.googlesource.com/jrn/empty
 Cloning into 'empty'...
 Checking connectivity... done.
 warning: remote HEAD refers to nonexistent ref, unable to checkout.

 $

It also means that standard "git daemon" is not able to advertise
capabilities on a fetch from a repository without advertising some
refs, so I cannot fetch-by-sha1 from a C git server unless some refs
are advertised.

This fix should solve the former problem and paves the way for the
latter to be solved once a year or two passes and people's clients are
up to date (or earlier if a server operator chooses).

> 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.

This description focuses on the code.  When deciding whether to apply
the patch (or whether to revert it if it comes up while trying to
investigate another problem), I am likely to wonder "what effect will
the patch have on me?" instead of "what standards does it follow?"

Flipping the emphasis, we could say something like

	connect: treat ref advertisement with capabilities^{} line as one with no refs

	When cloning an empty repository served by standard git, "git clone"
	produces the following reassuring message:

		$ git clone git://localhost/tmp/empty
		Cloning into 'empty'...
		warning: You appear to have cloned an empty repository.
		Checking connectivity... done.
	
	Meanwhile when cloning an empty repository served by JGit, the output
	is more haphazard:

		$ git clone git://localhost/tmp/empty
		Cloning into 'empty'...
		Checking connectivity... done.
		warning: remote HEAD refers to nonexistent ref, unable to checkout.

	This is a common command to run immediately after creating a remote
	repository as preparation for adding content to populate it and pushing.
	The warning is confusing and needlessly worrying.

	The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
	with no refs in upload service., 2013-08-08), JGit's ref advertisement
	includes a ref named capabilities^{} to advertise its refs on, while git's
	ref advertisement is empty in this case.  This allows the client to learn
	about the server's capabilities and is need, for example, for fetch-by-sha1
	to work when no refs are advertised.

	Git advertises the same capabilities^{} ref in its ref advertisement for
	push but since it never remembered to do so for fetch, the client forgot
	to handle this case. Handle it.

	So now you can see the same friendly message whether the server runs C
	git or JGit.

	The pack-protocol.txt documentation gives some guidance about the expected
	server behavior: what JGit does is currently required, since a list-of-refs
	is required to be non-empty.

		  advertised-refs  =  (no-refs / list-of-refs)
				      *shallow
				      flush-pkt

		  no-refs          =  PKT-LINE(zero-id SP "capabilities^{}"
				      NUL capability-list)

		  list-of-refs     =  first-ref *other-ref

	Because git client versions without this fix are expected to exist in the
	wild for a while, we should not change the server to always send the
	capabilities^{} line when there are no refs to advertise yet.  A transition
	will take multiple steps:

	 1. This patch, which updates the client

	 2. Update pack-protocol to clarify that both server behaviors must be
	    tolerated.

	 3. Add a configuration variable to allow git upload-pack to advertise
	    capabilities when there are no refs to advertise.  Leave it disabled
	    by default since git clients can't be counted on to have this patch (1)
	    yet.

	 4. After a year or so, flip the default for that server configuration
	    variable to true.

[...]
> --- a/connect.c
> +++ b/connect.c
> @@ -165,6 +165,13 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  			continue;
>  		}
>  
> +		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'd rather this be stricter.  Though it's not your fault: the code
before it is very lax in letting each line specify capabilities again.

Could this behave more like the ".have" case?  That is, just

		if (!strcmp(name, "capabilities^{}"))
			continue;

Or if we want to sanity-check the line further,

		if (!strcmp(name, "capabilities^{}")) {
			if (len == name_len + GIT_SHA1_HEX_SZ + 1)
				die("protocol error: expected capabilities after nul, got none");
			if (!is_null_oid(&old_oid))
				die("protocol error: expected zero-id, got %s", oid_to_hex(&old_oid));
			continue;
		}

The protocol definition and other clients already handle capabilities^{} differently
from other cases of zero-id.

[...]
> --- 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_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

Can this sleep be avoided?  E.g. start_git_daemon in lib-git-daemon.sh
uses output from the daemon to tell when it is serving.

Thanks for writing this.  I'll be happy when this protocol blip is gone.

Sincerely,
Jonathan

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

[Shawn Pearce]

On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>
> +               if (is_null_oid(&old_oid)) {
> +                       if (strcmp(name, "capabilities^{}"))

Its not the zero ID that is special, its the "capabilities^{}" name
that is special when its the first entry in the stream. In the wire
protocol a "x^{}" line is a modifier to a prior "x" line to add a
peeled object to the prior line. But if we see "^{}" on the first line
that is non-sense, there is no prior line to modify with this
identifier.

Further ^{} is used here because its invalid in a name. A server
really cannot have a reference that ends with the sequence ^{}. And a
server should not have a reference named "capabilities" without a
"refs/" prefix on it.

So the entire "capabilities^{}" on the first line is a bunch of
contradictions that violate a number of things about the protocol,
which is why clients should ignore it.

I think the test should be about:

  !*list && !strcmp(name, "capabilities^{}")

> +                               warning("zero object ID received that is not accompanied by a "
> +                                       "capability declaration, ignoring and continuing anyway");

Annoyingly a zero object ID is sort of possible; with a probability of
1/2^160 or something. Its just a very very unlikely value. Slightly
stronger to test against the known invalid name.

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

[Stefan Beller]

On Fri, Sep 2, 2016 at 12:39 PM, Shawn Pearce <spearce@spearce.org> wrote:
> On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>>
>> +               if (is_null_oid(&old_oid)) {
>> +                       if (strcmp(name, "capabilities^{}"))
>
> Its not the zero ID that is special, its the "capabilities^{}" name
> that is special when its the first entry in the stream. In the wire
> protocol a "x^{}" line is a modifier to a prior "x" line to add a
> peeled object to the prior line. But if we see "^{}" on the first line
> that is non-sense, there is no prior line to modify with this
> identifier.
>
> Further ^{} is used here because its invalid in a name. A server
> really cannot have a reference that ends with the sequence ^{}. And a
> server should not have a reference named "capabilities" without a
> "refs/" prefix on it.
>
> So the entire "capabilities^{}" on the first line is a bunch of
> contradictions that violate a number of things about the protocol,
> which is why clients should ignore it.
>
> I think the test should be about:
>
>   !*list && !strcmp(name, "capabilities^{}")
>
>> +                               warning("zero object ID received that is not accompanied by a "
>> +                                       "capability declaration, ignoring and continuing anyway");
>
> Annoyingly a zero object ID is sort of possible; with a probability of
> 1/2^160 or something. Its just a very very unlikely value. Slightly
> stronger to test against the known invalid name.

That ship has sailed long ago I would think?
There are tests for null sha1 in the refs code, e.g. for
deletion/creation of a branch
we consider the null sha1 as the counterpart.

So while it may be possible to have SHA1 producing a "0"x40, you
cannot e.g. push it?

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

[Shawn Pearce]

On Fri, Sep 2, 2016 at 12:56 PM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Sep 2, 2016 at 12:39 PM, Shawn Pearce <spearce@spearce.org> wrote:
>> On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>>>
>>> +               if (is_null_oid(&old_oid)) {
>>> +                       if (strcmp(name, "capabilities^{}"))
>>
>> Its not the zero ID that is special, its the "capabilities^{}" name
>> that is special when its the first entry in the stream. In the wire
>> protocol a "x^{}" line is a modifier to a prior "x" line to add a
>> peeled object to the prior line. But if we see "^{}" on the first line
>> that is non-sense, there is no prior line to modify with this
>> identifier.
>>
>> Further ^{} is used here because its invalid in a name. A server
>> really cannot have a reference that ends with the sequence ^{}. And a
>> server should not have a reference named "capabilities" without a
>> "refs/" prefix on it.
>>
>> So the entire "capabilities^{}" on the first line is a bunch of
>> contradictions that violate a number of things about the protocol,
>> which is why clients should ignore it.
>>
>> I think the test should be about:
>>
>>   !*list && !strcmp(name, "capabilities^{}")
>>
>>> +                               warning("zero object ID received that is not accompanied by a "
>>> +                                       "capability declaration, ignoring and continuing anyway");
>>
>> Annoyingly a zero object ID is sort of possible; with a probability of
>> 1/2^160 or something. Its just a very very unlikely value. Slightly
>> stronger to test against the known invalid name.
>
> That ship has sailed long ago I would think?
> There are tests for null sha1 in the refs code, e.g. for
> deletion/creation of a branch
> we consider the null sha1 as the counterpart.
>
> So while it may be possible to have SHA1 producing a "0"x40, you
> cannot e.g. push it?

Indeed, you are correct.

I'm just stating the JGit client behavior is to look at
"capabilities^{}" in the first line as special, not the SHA-1.

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

[Jeff King]

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

> 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...

> (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?

> +		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).

> 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?

> +# 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

[PATCH v2 0/2] handle empty spec-compliant remote repos correctly

[Jonathan Tan]

Thanks - I've updated the following:
o better patch description as suggested by Jonathan Nieder
o checking for capabilities^{} - all such lines are ignored without any further
  checks, similar to the behavior for .have 
o minor fix to test_lazy_prereq GIT_DAEMON
o test waits for output of `jgit daemon` (instead of sleeping)

Jonathan Tan (2):
  tests: move test_lazy_prereq JGIT to test-lib.sh
  connect: advertized capability is not a ref

 connect.c               |  3 +++
 t/t5310-pack-bitmaps.sh |  4 ----
 t/t5512-ls-remote.sh    | 39 +++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh           |  4 ++++
 4 files changed, 46 insertions(+), 4 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh

[Jonathan Tan]

This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5310-pack-bitmaps.sh | 4 ----
 t/test-lib.sh           | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
 	git pack-objects --stdout --revs <revs >/dev/null
 '
 
-test_lazy_prereq JGIT '
-	type jgit
-'
-
 test_expect_success JGIT 'we can read jgit bitmaps' '
 	git clone . compat-jgit &&
 	(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
 	test "$uid" != 0
 '
 
+test_lazy_prereq JGIT '
+	type jgit
+'
+
 # SANITY is about "can you correctly predict what the filesystem would
 # do by only looking at the permission bits of the files and
 # directories?"  A typical example of !SANITY is running the test
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 2/2] connect: advertized capability is not a ref

[Jonathan Tan]

When cloning an empty repository served by standard git, "git clone" produces
the following reassuring message:

	$ git clone git://localhost/tmp/empty
	Cloning into 'empty'...
	warning: You appear to have cloned an empty repository.
	Checking connectivity... done.

Meanwhile when cloning an empty repository served by JGit, the output is more
haphazard:

	$ git clone git://localhost/tmp/empty
	Cloning into 'empty'...
	Checking connectivity... done.
	warning: remote HEAD refers to nonexistent ref, unable to checkout.

This is a common command to run immediately after creating a remote repository
as preparation for adding content to populate it and pushing. The warning is
confusing and needlessly worrying.

The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
with no refs in upload service., 2013-08-08), JGit's ref advertisement includes
a ref named capabilities^{} to advertise its capabilities on, while git's ref
advertisement is empty in this case. This allows the client to learn about the
server's capabilities and is needed, for example, for fetch-by-sha1 to work
when no refs are advertised.

This also affects "ls-remote". For example, against an empty repository served
by JGit:

	$ git ls-remote git://localhost/tmp/empty
	0000000000000000000000000000000000000000        capabilities^{}

Git advertises the same capabilities^{} ref in its ref advertisement for push
but since it never remembered to do so for fetch, the client forgot to handle
this case. Handle it.

In this aspect, JGit is compliant with the specification in pack-protocol.txt.
Because git client versions without this fix are expected to exist in the wild
for a while, we should not change the server to always send the capabilities^{}
line when there are no refs to advertise yet.  A transition will take multiple
steps:

 1. This patch, which updates the client

 2. Update pack-protocol to clarify that both server behaviors must be
    tolerated.

 3. Add a configuration variable to allow git upload-pack to advertise
    capabilities when there are no refs to advertise.  Leave it disabled
    by default since git clients can't be counted on to have this patch (1)
    yet.

 4. After a year or so, flip the default for that server configuration
    variable to true.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 connect.c            |  3 +++
 t/t5512-ls-remote.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/connect.c b/connect.c
index 722dc3f..0c2221e 100644
--- a/connect.c
+++ b/connect.c
@@ -165,6 +165,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			continue;
 		}
 
+		if (!strcmp(name, "capabilities^{}"))
+			continue;
+
 		if (!check_ref(name, flags))
 			continue;
 		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..2de52f5 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,44 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 	test_cmp expect actual
 '
 
+test_lazy_prereq GIT_DAEMON '
+	test_tristate GIT_TEST_GIT_DAEMON &&
+	test "$GIT_TEST_GIT_DAEMON" != false
+'
+
+JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}
+
+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success PIPE,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 &&
+	mkfifo jgit_daemon_output &&
+	{
+		jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
+		JGIT_DAEMON_PID=$!
+	} &&
+	test_when_finished kill "$JGIT_DAEMON_PID" &&
+	{
+		read line &&
+		case $line in
+		Exporting*)
+			;;
+		*)
+			echo "Expected: Exporting" &&
+			false;;
+		esac &&
+		read line &&
+		case $line in
+		"Listening on"*)
+			;;
+		*)
+			echo "Expected: Listening on" &&
+			false;;
+		esac
+	} <jgit_daemon_output &&
+	test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
+'
 
 test_done
-- 
2.8.0.rc3.226.g39d4020

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

[Jonathan Tan]

On 09/02/2016 01:13 PM, Jeff King wrote:
> On Fri, Sep 02, 2016 at 10:15:39AM -0700, Jonathan Tan wrote:
>> (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?

I was comparing the behavior of git daemon and jgit daemon - when 
serving the same repository, the former does not send the zero ID and 
capabilities^{} line, whereas the latter does; and I was stating that 
git daemon's behavior should be changed to JGit's behavior, but not 
necessarily immediately.

(In one of the replies to that email, Jonathan Nieder has suggested a 
more detailed transition plan.)

>> 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?

The JGIT line shouldn't be there - thanks for catching this.

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jonathan Nieder]

Jonathan Tan wrote:

> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  connect.c            |  3 +++
>  t/t5512-ls-remote.sh | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

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

[Jeff King]

On Fri, Sep 02, 2016 at 03:11:16PM -0700, Jonathan Tan wrote:

> On 09/02/2016 01:13 PM, Jeff King wrote:
> > On Fri, Sep 02, 2016 at 10:15:39AM -0700, Jonathan Tan wrote:
> > > (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?
> 
> I was comparing the behavior of git daemon and jgit daemon - when serving
> the same repository, the former does not send the zero ID and
> capabilities^{} line, whereas the latter does; and I was stating that git
> daemon's behavior should be changed to JGit's behavior, but not necessarily
> immediately.

Ah, I see. I was confused because it's git-upload-pack, not git-daemon,
which would be modified (git-daemon may also spawn other tools like
receive-pack, which _does_ already have this behavior).

I remain unconvinced that we should transition to the JGit behavior on
the server side (as opposed to "fixing" the documentation). However,
given that JGit versions with this behavior are in the wild, it seems
like a no-brainer to make the receiver more liberal (i.e., this series).

-Peff

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jeff King]

On Fri, Sep 02, 2016 at 03:06:12PM -0700, Jonathan Tan wrote:

> The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
> with no refs in upload service., 2013-08-08), JGit's ref advertisement includes
> a ref named capabilities^{} to advertise its capabilities on, while git's ref
> advertisement is empty in this case. This allows the client to learn about the
> server's capabilities and is needed, for example, for fetch-by-sha1 to work
> when no refs are advertised.

So does JGit actually have a fetch-by-sha1 that works in such a case
(obviously the repository has to have _some_ objects, so is this a
feature where the server hides some of the refs?).

I was thinking we did not have one in git (i.e., we have nothing to
allow fetching arbitrary sha1s). But combining hideRefs with
allowTipSHA1InWant could trigger this case.

> This also affects "ls-remote". For example, against an empty repository served
> by JGit:
> 
> 	$ git ls-remote git://localhost/tmp/empty
> 	0000000000000000000000000000000000000000        capabilities^{}
> 
> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never remembered to do so for fetch, the client forgot to handle
> this case. Handle it.

As you can probably guess from my previous emails in this thread, I
don't think it is "never remembered to do so". It is more like "never
intended to do so and was documented incorrectly".

That doesn't make things clear cut, of course. But I think the real
story is more like (I dug a little in the history, as you'll see, but
didn't look for conversations in the list archive, so take this with the
appropriate grain of salt):

  0. Upload-pack existed without this capabilities^{} trick for some time.

  1. Receive-pack learned the capabilities^{} trick, and send-pack on
     the client side learned to accept it (this looks like it came along
     with the first capability in cfee10a (send-pack/receive-pack: allow
     errors to be reported back to pusher., 2005-12-25).

  2. Later, b31222c (Update packfile transfer protocol documentation,
     2009-11-03) tried to document the upload-pack and receive-pack
     protocols, but mistakenly documented both as having
     capabilities^{}.

  3. In ae1f469 (Advertise capabilities with no refs in upload service.,
     2013-08-08), JGit started sending these in its upload-pack
     equivalent, according to the documentation from (3).

So now we are in a state where JGit behavior does not match Git
behavior. Since JGit versions have existed in the wild for a few years,
it's a good idea for all clients to be liberal and accept both cases.

> In this aspect, JGit is compliant with the specification in pack-protocol.txt.
> Because git client versions without this fix are expected to exist in the wild
> for a while, we should not change the server to always send the capabilities^{}
> line when there are no refs to advertise yet.  A transition will take multiple
> steps:
> 
>  1. This patch, which updates the client
> 
>  2. Update pack-protocol to clarify that both server behaviors must be
>     tolerated.

These two seem like obvious improvements.

>  3. Add a configuration variable to allow git upload-pack to advertise
>     capabilities when there are no refs to advertise.  Leave it disabled
>     by default since git clients can't be counted on to have this patch (1)
>     yet.
> 
>  4. After a year or so, flip the default for that server configuration
>     variable to true.

I think "a year or so" is not nearly long enough, as this is not a
backwards-compatible change. The only thing that mitigates it is that an
older client doesn't barf totally, but just generates funny output.

I'd be more interested in the pain of this transition if there was a
concrete use case for "hide literally all refs, but still allow
fetches". Is that a thing that people do?

-Peff

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Stefan Beller]

On Fri, Sep 2, 2016 at 4:35 PM, Jeff King <peff@peff.net> wrote:
> I'd be more interested in the pain of this transition if there was a
> concrete use case for "hide literally all refs, but still allow
> fetches". Is that a thing that people do?
>

Not yet, I would think.

Not to derail the discussion to much, but let's talk about protocol v2
for a second:

    One of the ideas we floated around for protocol v2 would be exactly
    that: the server advertises only a small portion (could be just master
    or no branch eventually) with a capability "v2" and then the client
    selects that capability and after that there comes protocol 2.

    The advantage of this approach would be have a functional
    v1 server still running, but the meat is found only in v2: e.g. via
    v2 you can obtain all pull requests/changes or even wiki/meta
    information stuff that would be too large to advertise in v1.

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jonathan Nieder]

Jeff King wrote:
> On Fri, Sep 02, 2016 at 03:06:12PM -0700, Jonathan Tan wrote:

>                                  But combining hideRefs with
> allowTipSHA1InWant could trigger this case.

Yes.

[...]
> I'd be more interested in the pain of this transition if there was a
> concrete use case for "hide literally all refs, but still allow
> fetches". Is that a thing that people do?

Sure, it is a thing that people do.  For example I've seen replication
systems that learn what SHA-1s to fetch out-of-band and then use this
approach to avoid the overhead of a long ref advertisement.

However, that is not my motivation.  My motivation is being able to
extend the protocol in the future.  The capabilities line has been
important for that historically.

Do you have any objection to the server gaining support for this
guarded by a configuration option?  Then when the time comes to
consider flipping the default we can use real-world statistics about
what git client versions people use to make an informed decision.

Thanks,
Jonathan

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jonathan Nieder]

Stefan Beller wrote:
> On Fri, Sep 2, 2016 at 4:35 PM, Jeff King <peff@peff.net> wrote:

>> I'd be more interested in the pain of this transition if there was a
>> concrete use case for "hide literally all refs, but still allow
>> fetches". Is that a thing that people do?
[...]
> Not to derail the discussion to much, but let's talk about protocol v2
> for a second:

Uh oh. ;-)

>     One of the ideas we floated around for protocol v2 would be exactly
>     that: the server advertises only a small portion (could be just master
>     or no branch eventually) with a capability "v2" and then the client
>     selects that capability and after that there comes protocol 2.

Sounds scary to me.  What would happen when I try to clone a
repository with a v1 client?  I'd see nothing.  I'd want at least a
"master" branch with a README file (or an ERR packet?) saying "please
update your client".

>     The advantage of this approach would be have a functional
>     v1 server still running, but the meat is found only in v2: e.g. via
>     v2 you can obtain all pull requests/changes or even wiki/meta
>     information stuff that would be too large to advertise in v1.

This sounds less scary, but it doesn't answer the question Peff
raised.  Wouldn't it still be typical to advertise at least one ref,
which can contain a capabilities line?

However, another idea I think you've mentioned before on-list about
changing the ref advertisement could answer it.  Suppose that I always
include the ref advertisement in my first reply, but I provide a
capability saying that further requests to this server can use a
different mechanism that skips the long advertisement.

Normally that would work great --- I only pay the cost of the large
advertisement once, and from then on I can cache what the server told
me about how it prefers to be contacted.  Except what happens if this
was a new repository and my first contact with the server was to clone
that empty repository?

In that case, getting capabilities with the ref advertisement would
benefit me.

Likewise for other capabilities that may come with such an empty
fetch: for example the server could tell which unborn branch the HEAD
symref points to.

Thanks,
Jonathan

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jeff King]

On Fri, Sep 02, 2016 at 04:51:45PM -0700, Jonathan Nieder wrote:

> > I'd be more interested in the pain of this transition if there was a
> > concrete use case for "hide literally all refs, but still allow
> > fetches". Is that a thing that people do?
> 
> Sure, it is a thing that people do.  For example I've seen replication
> systems that learn what SHA-1s to fetch out-of-band and then use this
> approach to avoid the overhead of a long ref advertisement.

I know that's how those features work. I was more wondering if it ever
comes up that somebody actually has hidden refs, but _no_ non-hidden
ones. Do the systems you've seen hide all the refs?

> However, that is not my motivation.  My motivation is being able to
> extend the protocol in the future.  The capabilities line has been
> important for that historically.

Sure, I agree it's a nice move forward for compatibility. But that
argues for teaching the clients to handle it (for the future), and then
turning it on in the server only when it becomes useful (i.e., the "in a
year or so" can become "when we find a use for it").

In a similar vein, I'd think that a config to enable this in upload-pack
today could have an "auto" mode, which enables it _only_ when you know
something productive might come of it (namely that you have hidden refs,
one of the uploadpack.allow* features is enabled, and the ref
advertisement is empty). Then requests which could not benefit from it
at all do not have to pay the potential compatibility cost.

> Do you have any objection to the server gaining support for this
> guarded by a configuration option?  Then when the time comes to
> consider flipping the default we can use real-world statistics about
> what git client versions people use to make an informed decision.

Guarded by config, no. It's the flipping of the default I care more
about. The config is not necessary in the meantime for getting
real-world statistics; you can add the config and flip the default as
one unit at any time (the thing that is time-critical is teaching the
client to handle _both_ cases gracefully).

The config is useful in the meantime if there are people who could
benefit from it immediately, and don't mind paying the compatibility
cost. With an "auto" as I described above, using that as the default
seems like a decent interim behavior (i.e., why would you set up such a
repository if you didn't expect most clients to use the allowTipSHA1
feature?). I'd still probably give some lag between shipping the client,
and flipping the server default to "auto".

I hoped to share some numbers on what versions people are currently
using against GitHub, to get a sense of how far back most people are.
But I haven't been actively involved in keeping those numbers for a
while, and I'm not sure what we have readily stored. I did show some
numbers a few years ago[1], and it looks like about 2/3 of people were
within 6-12 months of the latest version, but the rest was a long tail.

I don't know if that will have changed with the advent of more client
versions (e.g., lots more people are using libgit2 now via GUI clients,
Visual Studio, etc; how does it fare with the proposed change?).

-Peff

[1] http://public-inbox.org/git/20120531114801.GA21367@sigill.intra.peff.net/

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

[Shawn Pearce]

On Fri, Sep 2, 2016 at 1:13 PM, Jeff King <peff@peff.net> wrote:
>
> 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?

No, JGit needs to keep sending the capabilities^{} line in upload-pack
if there were no refs advertised to the client. (If it advertises at
least one ref it does not send this 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).

Its exactly these sort of extra capabilities. We run JGit in modes
where "out of band" (e.g. URL or higher level protocol framing like an
undocumented HTTP header) allows the fetch-pack client to say "do not
send me advertisements, but I want to learn your capabilities". The
fetch-pack client typically activates the allow-reachable-sha1-in-want
feature and names specific SHA-1s it wants.

This allows the fetch-pack client to bypass a very large advertisement
if it wants only a specific SHA-1 and doesn't care about the ref name
its bound to, or reachable through.


This is also perhaps a stepping stone towards "client speaks first".
If we can later standardize an HTTP query parameter or extra HTTP
header, the server may be able to avoid sending a lot of ref
advertisements, but would still need to advertise capabilities.

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

[Jeff King]

On Fri, Sep 02, 2016 at 07:03:30PM -0700, Shawn Pearce wrote:

> > 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).
> 
> Its exactly these sort of extra capabilities. We run JGit in modes
> where "out of band" (e.g. URL or higher level protocol framing like an
> undocumented HTTP header) allows the fetch-pack client to say "do not
> send me advertisements, but I want to learn your capabilities". The
> fetch-pack client typically activates the allow-reachable-sha1-in-want
> feature and names specific SHA-1s it wants.

So it sounds like you _could_ enable this only in out-of-band mode,
without loss of functionality.

I don't particularly care either way (I do not run any JGit servers
myself), but if we do it in Git, I think that is the path we should go
for maximum compatibility (and then if we jump to protocol v2, we get to
shed all of the compatibility cruft).

> This allows the fetch-pack client to bypass a very large advertisement
> if it wants only a specific SHA-1 and doesn't care about the ref name
> its bound to, or reachable through.

Yep, definitely a useful thing.

> This is also perhaps a stepping stone towards "client speaks first".
> If we can later standardize an HTTP query parameter or extra HTTP
> header, the server may be able to avoid sending a lot of ref
> advertisements, but would still need to advertise capabilities.

Yes, but that is a big enough jump that we can design it to look like
whatever we want, not shoe-horning it into a pretend ref. Older clients
will be left behind either way, we will need a transition plan, etc.

-Peff

Re: [PATCH v2 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> This enables JGIT to be used as a prereq in invocations of
> test_expect_success (and other functions) in other test scripts.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  t/t5310-pack-bitmaps.sh | 4 ----
>  t/test-lib.sh           | 4 ++++
>  2 files changed, 4 insertions(+), 4 deletions(-)

Makes sense.  Thanks.

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never remembered to do so for fetch, the client forgot to handle
> this case. Handle it.
> ...
> In this aspect, JGit is compliant with the specification in pack-protocol.txt.

I agree with Peff that the above explanation distorts the history.
It should be reworded.

I do not have an issue with being more lenient to what JGit servers
have been doing for a few years, though.

> Because git client versions without this fix are expected to exist in the wild
> for a while, we should not change the server to always send the capabilities^{}
> line when there are no refs to advertise yet.  A transition will take multiple
> steps:
>
>  1. This patch, which updates the client
>
>  2. Update pack-protocol to clarify that both server behaviors must be
>     tolerated.
>
>  3. Add a configuration variable to allow git upload-pack to advertise
>     capabilities when there are no refs to advertise.  Leave it disabled
>     by default since git clients can't be counted on to have this patch (1)
>     yet.
>
>  4. After a year or so, flip the default for that server configuration
>     variable to true.

The above assumes it is an unconditionally good thing to send
capabilities^{}; I do not think we established that in this
discussion, and more importantly, this client-side change is a good
thing to do regardless of the outcome of the discussion.

I'd suggest dropping everything below "Because Git client versions
without...".

> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 819b9dd..2de52f5 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -207,5 +207,44 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
>  	test_cmp expect actual
>  '
>  
> +test_lazy_prereq GIT_DAEMON '
> +	test_tristate GIT_TEST_GIT_DAEMON &&
> +	test "$GIT_TEST_GIT_DAEMON" != false
> +'
> +
> +JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}

Shouldn't this be inside the next expect_success?

> +# This test spawns a daemon, so run it only if the user would be OK with
> +# testing with git-daemon.
> +test_expect_success PIPE,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 &&

To make it clear that the existence of the file is the thing you
care the most, not that the file having a recent timestamp:

	>empty.git/git-daemon-export-ok &&

> +	mkfifo jgit_daemon_output &&
> +	{
> +		jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
> +		JGIT_DAEMON_PID=$!
> +	} &&
> +	test_when_finished kill "$JGIT_DAEMON_PID" &&
> +	{
> +		read line &&
> +		case $line in
> +		Exporting*)
> +			;;
> +		*)
> +			echo "Expected: Exporting" &&
> +			false;;
> +		esac &&
> +		read line &&
> +		case $line in
> +		"Listening on"*)
> +			;;
> +		*)
> +			echo "Expected: Listening on" &&
> +			false;;
> +		esac

OK, so this is a nice way to wait until the daemon becomes ready to
serve.

> +	} <jgit_daemon_output &&

An in-code comment that explains what the significance of "2" here

	# --exit-code asks the command to exit with 2 when no
        # matching refs are found.

would be nice.

> +	test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
> +'

Thanks.

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> diff --git a/connect.c b/connect.c
> index 722dc3f..0c2221e 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -165,6 +165,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  			continue;
>  		}
>  
> +		if (!strcmp(name, "capabilities^{}"))
> +			continue;

While it is true that ignoring this line anywhere in the ref
advertisement is safe, it feels a bit strange to do so, when we know
that it can appear _only_ when there is no other ref advertised.  I
guess you can argue that it is good to be lenient to accept what
others produce, but on the other hand, it can also be argued that
having this among real ref advertisement would be a protocol
violation that we may want to diagnose and prod the other side to
fix their software (but still not fail).

> +
>  		if (!check_ref(name, flags))
>  			continue;
>  		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:

>> diff --git a/connect.c b/connect.c
>> index 722dc3f..0c2221e 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -165,6 +165,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>>  			continue;
>>  		}
>>  
>> +		if (!strcmp(name, "capabilities^{}"))
>> +			continue;
>
> While it is true that ignoring this line anywhere in the ref
> advertisement is safe, it feels a bit strange to do so, when we know
> that it can appear _only_ when there is no other ref advertised.  I
> guess you can argue that it is good to be lenient to accept what
> others produce, but on the other hand, it can also be argued that
> having this among real ref advertisement would be a protocol
> violation that we may want to diagnose and prod the other side to
> fix their software (but still not fail).

By "it can also be argued", do you mean that you would prefer that
behavior?

It sounds like the worst of both worlds to me --- git would allow the
buggy server behavior, leading people not to fix their servers, but it
would print an ugly error message, so end-users would associate git
with confusing messages.

Given that there aren't any servers that are going to produce this
kind of bad input anyway, I prefer a die().

Thanks,
Jonathan

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Given that there aren't any servers that are going to produce this
> kind of bad input anyway, I prefer a die().

That would certainly put bigger pressure on the folks who write
buggy stuff in the future.  If we know that nobody produces such
output, I'd prefer to forbid it, of course.  It's just that is not
what this 2/2 implements ;-).

[PATCH v3 0/2] handle empty spec-compliant remote repos correctly

[Jonathan Tan]

Updated, taking into account review comments.

This patch set uses warnings (instead of errors using "die") to indicate
protocol errors that we can recover from. There is a discussion on a sibling
thread about whether such protocol errors should be errors ("die") instead - I
can change it if consensus is that these should be errors.

Jonathan Tan (2):
  tests: move test_lazy_prereq JGIT to test-lib.sh
  connect: advertized capability is not a ref

 connect.c               | 17 +++++++++++++++++
 t/t5310-pack-bitmaps.sh |  4 ----
 t/t5512-ls-remote.sh    | 40 ++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh           |  4 ++++
 4 files changed, 61 insertions(+), 4 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh

[Jonathan Tan]

This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5310-pack-bitmaps.sh | 4 ----
 t/test-lib.sh           | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
 	git pack-objects --stdout --revs <revs >/dev/null
 '
 
-test_lazy_prereq JGIT '
-	type jgit
-'
-
 test_expect_success JGIT 'we can read jgit bitmaps' '
 	git clone . compat-jgit &&
 	(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
 	test "$uid" != 0
 '
 
+test_lazy_prereq JGIT '
+	type jgit
+'
+
 # SANITY is about "can you correctly predict what the filesystem would
 # do by only looking at the permission bits of the files and
 # directories?"  A typical example of !SANITY is running the test
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 2/2] connect: advertized capability is not a ref

[Jonathan Tan]

When cloning an empty repository served by standard git, "git clone" produces
the following reassuring message:

	$ git clone git://localhost/tmp/empty
	Cloning into 'empty'...
	warning: You appear to have cloned an empty repository.
	Checking connectivity... done.

Meanwhile when cloning an empty repository served by JGit, the output is more
haphazard:

	$ git clone git://localhost/tmp/empty
	Cloning into 'empty'...
	Checking connectivity... done.
	warning: remote HEAD refers to nonexistent ref, unable to checkout.

This is a common command to run immediately after creating a remote repository
as preparation for adding content to populate it and pushing. The warning is
confusing and needlessly worrying.

The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
with no refs in upload service., 2013-08-08), JGit's ref advertisement includes
a ref named capabilities^{} to advertise its capabilities on, while git's ref
advertisement is empty in this case. This allows the client to learn about the
server's capabilities and is needed, for example, for fetch-by-sha1 to work
when no refs are advertised.

This also affects "ls-remote". For example, against an empty repository served
by JGit:

	$ git ls-remote git://localhost/tmp/empty
	0000000000000000000000000000000000000000        capabilities^{}

Git advertises the same capabilities^{} ref in its ref advertisement for push
but since it never remembered to do so for fetch, the client forgot to handle
this case. Handle it.

In this aspect, JGit is compliant with the specification in pack-protocol.txt.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 connect.c            | 17 +++++++++++++++++
 t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/connect.c b/connect.c
index 722dc3f..0bb8103 100644
--- a/connect.c
+++ b/connect.c
@@ -116,6 +116,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 {
 	struct ref **orig_list = list;
 	int got_at_least_one_head = 0;
+	int got_dummy_ref_with_capabilities_declaration = 0;
 
 	*list = NULL;
 	for (;;) {
@@ -165,8 +166,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			continue;
 		}
 
+		if (!strcmp(name, "capabilities^{}")) {
+			if (got_at_least_one_head)
+				warning("protocol error: unexpected dummy ref for "
+				        "capabilities declaration, continuing anyway");
+			if (got_dummy_ref_with_capabilities_declaration)
+				warning("protocol error: multiple dummy refs for "
+				        "capabilities declaration, continuing anyway");
+			got_dummy_ref_with_capabilities_declaration = 1;
+			continue;
+		}
+
 		if (!check_ref(name, flags))
 			continue;
+
+		if (got_dummy_ref_with_capabilities_declaration)
+			warning("protocol error: unexpected ref after "
+			        "dummy ref, using this ref and continuing anyway");
+
 		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
 		oidcpy(&ref->old_oid, &old_oid);
 		*list = ref;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..befdfee 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,45 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 	test_cmp expect actual
 '
 
+test_lazy_prereq GIT_DAEMON '
+	test_tristate GIT_TEST_GIT_DAEMON &&
+	test "$GIT_TEST_GIT_DAEMON" != false
+'
+
+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
+	JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&
+	JGIT_DAEMON_PID= &&
+	git init --bare empty.git &&
+	>empty.git/git-daemon-export-ok &&
+	mkfifo jgit_daemon_output &&
+	{
+		jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
+		JGIT_DAEMON_PID=$!
+	} &&
+	test_when_finished kill "$JGIT_DAEMON_PID" &&
+	{
+		read line &&
+		case $line in
+		Exporting*)
+			;;
+		*)
+			echo "Expected: Exporting" &&
+			false;;
+		esac &&
+		read line &&
+		case $line in
+		"Listening on"*)
+			;;
+		*)
+			echo "Expected: Listening on" &&
+			false;;
+		esac
+	} <jgit_daemon_output &&
+	# --exit-code asks the command to exit with 2 when no
+	# matching refs are found.
+	test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
+'
 
 test_done
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v3 2/2] connect: advertized capability is not a ref

[Jonathan Nieder]

Jonathan Tan wrote:

> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never remembered to do so for fetch, the client forgot to handle
> this case. Handle it.

The comment in the previous review was that this doesn't describe the
history correctly.  It can instead say something like

	Git advertises the same capabilities^{} ref in its ref advertisement for push
	but since it never did so for fetch, the client didn't need to handle this
	case. Handle it.

[...]
> @@ -165,8 +166,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  			continue;
>  		}
>  
> +		if (!strcmp(name, "capabilities^{}")) {
> +			if (got_at_least_one_head)
> +				warning("protocol error: unexpected dummy ref for "
> +				        "capabilities declaration, continuing anyway");

Can this die() instead of warning?

I think mentioning capabilities^{} in the error message would make it
easier to debug.

> +			if (got_dummy_ref_with_capabilities_declaration)
> +				warning("protocol error: multiple dummy refs for "
> +				        "capabilities declaration, continuing anyway");

Likewise.

> +			got_dummy_ref_with_capabilities_declaration = 1;
> +			continue;

I think we can make this stricter.  The capabilities^{} line is supposed
to be the first advertised ref, before any 'shallow' lines or .have
extra refs.

(Alas, Documentation/technical/pack-protocol.txt doesn't describe
.have refs --- v1.6.1-rc1~203^2~1, push: prepare sender to receive
extended ref information from the receiver, 2008-09-09.)

'die_initial_contact' uses got_at_least_one_head to determine whether
it was on the first line but code paths added later that use
'continue' don't populate it properly (see b06dcd7d, 40c155ff, and
1a7141ff).  We could do

	int first_line = 1;

	for (;; first_line = 0) {
		...
	}

and use !first_line instead of got_at_least_one_head (removing
got_at_least_one_head in the process since it has no other purpose).

Thanks,
Jonathan

[PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)

[Jonathan Nieder]

(+cc: Heiko)
Jonathan Nieder wrote:

> 'die_initial_contact' uses got_at_least_one_head to determine whether
> it was on the first line but code paths added later that use
> 'continue' don't populate it properly (see b06dcd7d, 40c155ff, and
> 1a7141ff).  We could do
>
> 	int first_line = 1;
>
> 	for (;; first_line = 0) {
> 		...
> 	}
>
> and use !first_line instead of got_at_least_one_head (removing
> got_at_least_one_head in the process since it has no other purpose).

I got the history wrong.  It looks like this was always confused
by the 'continue' cases.  Unless I'm missing something subtle ---
thoughts?

Thanks,
Jonathan

-- >8 --
Subject: connect: tighten check for unexpected early hang up

A server hanging up immediately to mark access being denied does not
send any .have refs, shallow lines, or anything else before hanging
up.  If the server has sent anything, then the hangup is unexpected.

That is, if the server hangs up after a shallow line but before sending
any refs, then git should tell me so:

	fatal: The remote end hung up upon initial contact

instead of suggesting an access control problem:

	fatal: Could not read from remote repository.
	Please make sure you have the correct access rights
	and the repository exists.

Noticed while examining this code.  This case isn't likely to come up
in practice but tightening the check makes the code easier to read and
manipulate.

Change-Id: I3cec2c160eb6c6f3efdce7dab38a4c78592f6c7f
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 connect.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/connect.c b/connect.c
index 722dc3f..2c2ebef 100644
--- a/connect.c
+++ b/connect.c
@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int got_at_least_one_head)
+static void die_initial_contact(int unexpected)
 {
-	if (got_at_least_one_head)
+	if (unexpected)
 		die("The remote end hung up upon initial contact");
 	else
 		die("Could not read from remote repository.\n\n"
@@ -115,10 +115,10 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			      struct sha1_array *shallow_points)
 {
 	struct ref **orig_list = list;
-	int got_at_least_one_head = 0;
+	int first_line = 1;
 
 	*list = NULL;
-	for (;;) {
+	for (;; first_line = 0) {
 		struct ref *ref;
 		struct object_id old_oid;
 		char *name;
@@ -131,7 +131,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 				  PACKET_READ_GENTLE_ON_EOF |
 				  PACKET_READ_CHOMP_NEWLINE);
 		if (len < 0)
-			die_initial_contact(got_at_least_one_head);
+			die_initial_contact(first_line);
 
 		if (!len)
 			break;
@@ -171,7 +171,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		oidcpy(&ref->old_oid, &old_oid);
 		*list = ref;
 		list = &ref->next;
-		got_at_least_one_head = 1;
 	}
 
 	annotate_refs_with_symref_info(*orig_list);
-- 

Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)

[Jonathan Nieder]

Jonathan Nieder wrote:

> Change-Id: I3cec2c160eb6c6f3efdce7dab38a4c78592f6c7f

Gah --- sorry about that.  Please remove this line if applying (or
I'll be happy to resend without it after review).

Jonathan

Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)

[Jonathan Nieder]

Jonathan Nieder wrote:

> Subject: connect: tighten check for unexpected early hang up
[...]
> @@ -131,7 +131,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  				  PACKET_READ_GENTLE_ON_EOF |
>  				  PACKET_READ_CHOMP_NEWLINE);
>  		if (len < 0)
> -			die_initial_contact(got_at_least_one_head);
> +			die_initial_contact(first_line);

This should say !first_line.

I'll add tests if the patch seems like a good idea.

Thanks,
Jonathan

Re: [PATCH v3 2/2] connect: advertized capability is not a ref

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I think we can make this stricter.  The capabilities^{} line is supposed
> to be the first advertised ref, before any 'shallow' lines or .have
> extra refs.

"The first", or "the first and only"?  I thought that it would be
the latter.

Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)

[Stefan Beller]

On Wed, Sep 7, 2016 at 6:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (+cc: Heiko)
> Jonathan Nieder wrote:
>
>> 'die_initial_contact' uses got_at_least_one_head to determine whether
>> it was on the first line but code paths added later that use
>> 'continue' don't populate it properly (see b06dcd7d, 40c155ff, and
>> 1a7141ff).  We could do
>>
>>       int first_line = 1;
>>
>>       for (;; first_line = 0) {
>>               ...
>>       }
>>
>> and use !first_line instead of got_at_least_one_head (removing
>> got_at_least_one_head in the process since it has no other purpose).
>
> I got the history wrong.  It looks like this was always confused
> by the 'continue' cases.  Unless I'm missing something subtle ---
> thoughts?

I was a bit confused by the line

    for (;; first_line = 0) {

at first, but the explanation of 'continue's make sense for this pattern.
However I'd rather prefer if we'd have

    int first_line = 1;
    for(;;) {
        ... // stuff with no continue here
        if (len < 0)
            die_initial_contact(!first_line);
        first_line = 0;
        ... // here we may have some continues, but that doesn't matter
        // w.r.t. first_line
    }

Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jonathan Nieder wrote:
>
>> Subject: connect: tighten check for unexpected early hang up
> [...]
>> @@ -131,7 +131,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>>  				  PACKET_READ_GENTLE_ON_EOF |
>>  				  PACKET_READ_CHOMP_NEWLINE);
>>  		if (len < 0)
>> -			die_initial_contact(got_at_least_one_head);
>> +			die_initial_contact(first_line);
>
> This should say !first_line.
>
> I'll add tests if the patch seems like a good idea.

I tried to write one-paragraph comment for the die_initial_contact()
function, like so:

    +/*
    + * A remote end that is unwilling to talk to us would not give
    + * any response to us before hanging up.  After seeing some
    + * response, we know the hang-up is unexpected.
    + */
    +static void die_initial_contact(int saw_any_response)

but then I got stuck.

We may know that after seeing any response (not necessarily a ref,
but .have or shallow) the other end is willing to talk to us, but
the reverse is not necessarily true (it may be willing to talk to
us, but the network between us may have prevented it from doing so).
For that reason, the above comment is inappropriate for a function
that takes a bool and gives an "unexpected hung-up" or an
"unreachable, possible ACL or problems" message.

So my second attempt was to comment on the variable that keeps track
of the status of the conversation, which turned out to be better
(attached).

I think I fixed your "oops, the bool needs polarity flip".  A test
may be a good idea, but I am not sure how you plan to produce a
failure after sending some response.

-- >8 --
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 7 Sep 2016 18:45:55 -0700
Subject: [PATCH] connect: tighten check for unexpected early hang up

A server hanging up immediately to mark access being denied does not
send any .have refs, shallow lines, or anything else before hanging
up.  If the server has sent anything, then the hangup is unexpected.

That is, if the server hangs up after a shallow line but before sending
any refs, then git should tell me so:

	fatal: The remote end hung up upon initial contact

instead of suggesting an access control problem:

	fatal: Could not read from remote repository.
	Please make sure you have the correct access rights
	and the repository exists.

Noticed while examining this code.  This case isn't likely to come up
in practice but tightening the check makes the code easier to read and
manipulate.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 connect.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/connect.c b/connect.c
index c53f3f1..067cf40 100644
--- a/connect.c
+++ b/connect.c
@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int got_at_least_one_head)
+static void die_initial_contact(int unexpected)
 {
-	if (got_at_least_one_head)
+	if (unexpected)
 		die("The remote end hung up upon initial contact");
 	else
 		die("Could not read from remote repository.\n\n"
@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			      struct sha1_array *shallow_points)
 {
 	struct ref **orig_list = list;
-	int got_at_least_one_head = 0;
+
+	/*
+	 * A hang-up after seeing some response from the other end
+	 * means that it is unexpected, as we know the other end is
+	 * willing to talk to us.  A hang-up before seeing any
+	 * response does not necessarily mean an ACL problem, though.
+	 */
+	int saw_response;
 
 	*list = NULL;
-	for (;;) {
+	for (saw_response = 0; ; saw_response = 1) {
 		struct ref *ref;
 		struct object_id old_oid;
 		char *name;
@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 				  PACKET_READ_GENTLE_ON_EOF |
 				  PACKET_READ_CHOMP_NEWLINE);
 		if (len < 0)
-			die_initial_contact(got_at_least_one_head);
+			die_initial_contact(saw_response);
 
 		if (!len)
 			break;
@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		oidcpy(&ref->old_oid, &old_oid);
 		*list = ref;
 		list = &ref->next;
-		got_at_least_one_head = 1;
 	}
 
 	annotate_refs_with_symref_info(*orig_list);
-- 
2.10.0-267-g7db2ae3

[PATCH v4 0/3] handle empty spec-compliant remote repos correctly

[Jonathan Tan]

Updates:
o Included tighten-check patch from Jonathan Nieder and Junio C Hamano
  in this patch set
o Updated commit message following Jonathan Nieder's suggestion
o Updated warning messages to mention capabilities^{}

As for warning vs die, I would prefer the "liberal" approach of continuing on
when facing a recoverable error (that is, "warning"). But I agree that there
are good points in favor of using fatal errors ("die") and I can switch to that
if there is consensus.

Jonathan Nieder (1):
  connect: tighten check for unexpected early hang up

Jonathan Tan (2):
  tests: move test_lazy_prereq JGIT to test-lib.sh
  connect: advertized capability is not a ref

 connect.c               | 35 +++++++++++++++++++++++++++++------
 t/t5310-pack-bitmaps.sh |  4 ----
 t/t5512-ls-remote.sh    | 40 ++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh           |  4 ++++
 4 files changed, 73 insertions(+), 10 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh

[Jonathan Tan]

This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5310-pack-bitmaps.sh | 4 ----
 t/test-lib.sh           | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
 	git pack-objects --stdout --revs <revs >/dev/null
 '
 
-test_lazy_prereq JGIT '
-	type jgit
-'
-
 test_expect_success JGIT 'we can read jgit bitmaps' '
 	git clone . compat-jgit &&
 	(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
 	test "$uid" != 0
 '
 
+test_lazy_prereq JGIT '
+	type jgit
+'
+
 # SANITY is about "can you correctly predict what the filesystem would
 # do by only looking at the permission bits of the files and
 # directories?"  A typical example of !SANITY is running the test
-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 2/3] connect: tighten check for unexpected early hang up

[Jonathan Tan]

From: Jonathan Nieder <jrnieder@gmail.com>

A server hanging up immediately to mark access being denied does not
send any .have refs, shallow lines, or anything else before hanging
up.  If the server has sent anything, then the hangup is unexpected.

That is, if the server hangs up after a shallow line but before sending
any refs, then git should tell me so:

	fatal: The remote end hung up upon initial contact

instead of suggesting an access control problem:

	fatal: Could not read from remote repository.
	Please make sure you have the correct access rights
	and the repository exists.

Noticed while examining this code.  This case isn't likely to come up
in practice but tightening the check makes the code easier to read and
manipulate.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 connect.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/connect.c b/connect.c
index 722dc3f..0c01a49 100644
--- a/connect.c
+++ b/connect.c
@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int got_at_least_one_head)
+static void die_initial_contact(int unexpected)
 {
-	if (got_at_least_one_head)
+	if (unexpected)
 		die("The remote end hung up upon initial contact");
 	else
 		die("Could not read from remote repository.\n\n"
@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			      struct sha1_array *shallow_points)
 {
 	struct ref **orig_list = list;
-	int got_at_least_one_head = 0;
+
+	/*
+	 * A hang-up after seeing some response from the other end
+	 * means that it is unexpected, as we know the other end is
+	 * willing to talk to us.  A hang-up before seeing any
+	 * response does not necessarily mean an ACL problem, though.
+	 */
+	int saw_response;
 
 	*list = NULL;
-	for (;;) {
+	for (saw_response = 0; ; saw_response = 1) {
 		struct ref *ref;
 		struct object_id old_oid;
 		char *name;
@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 				  PACKET_READ_GENTLE_ON_EOF |
 				  PACKET_READ_CHOMP_NEWLINE);
 		if (len < 0)
-			die_initial_contact(got_at_least_one_head);
+			die_initial_contact(saw_response);
 
 		if (!len)
 			break;
@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		oidcpy(&ref->old_oid, &old_oid);
 		*list = ref;
 		list = &ref->next;
-		got_at_least_one_head = 1;
 	}
 
 	annotate_refs_with_symref_info(*orig_list);
-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 3/3] connect: advertized capability is not a ref

[Jonathan Tan]

When cloning an empty repository served by standard git, "git clone" produces
the following reassuring message:

	$ git clone git://localhost/tmp/empty
	Cloning into 'empty'...
	warning: You appear to have cloned an empty repository.
	Checking connectivity... done.

Meanwhile when cloning an empty repository served by JGit, the output is more
haphazard:

	$ git clone git://localhost/tmp/empty
	Cloning into 'empty'...
	Checking connectivity... done.
	warning: remote HEAD refers to nonexistent ref, unable to checkout.

This is a common command to run immediately after creating a remote repository
as preparation for adding content to populate it and pushing. The warning is
confusing and needlessly worrying.

The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
with no refs in upload service., 2013-08-08), JGit's ref advertisement includes
a ref named capabilities^{} to advertise its capabilities on, while git's ref
advertisement is empty in this case. This allows the client to learn about the
server's capabilities and is needed, for example, for fetch-by-sha1 to work
when no refs are advertised.

This also affects "ls-remote". For example, against an empty repository served
by JGit:

	$ git ls-remote git://localhost/tmp/empty
	0000000000000000000000000000000000000000        capabilities^{}

Git advertises the same capabilities^{} ref in its ref advertisement for push
but since it never did so for fetch, the client didn't need to handle this
case. Handle it.

In this aspect, JGit is compliant with the specification in pack-protocol.txt.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 connect.c            | 17 +++++++++++++++++
 t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/connect.c b/connect.c
index 0c01a49..cb3cd97 100644
--- a/connect.c
+++ b/connect.c
@@ -123,6 +123,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * response does not necessarily mean an ACL problem, though.
 	 */
 	int saw_response;
+	int got_dummy_ref_with_capabilities_declaration = 0;
 
 	*list = NULL;
 	for (saw_response = 0; ; saw_response = 1) {
@@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			continue;
 		}
 
+		if (!strcmp(name, "capabilities^{}")) {
+			if (saw_response)
+				warning("protocol error: unexpected capabilities^{}, "
+					"continuing anyway");
+			if (got_dummy_ref_with_capabilities_declaration)
+				warning("protocol error: multiple capabilities^{}, "
+					"continuing anyway");
+			got_dummy_ref_with_capabilities_declaration = 1;
+			continue;
+		}
+
 		if (!check_ref(name, flags))
 			continue;
+
+		if (got_dummy_ref_with_capabilities_declaration)
+			warning("protocol error: unexpected ref after capabilities^{}, "
+				"using this ref and continuing anyway");
+
 		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
 		oidcpy(&ref->old_oid, &old_oid);
 		*list = ref;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..befdfee 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,45 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 	test_cmp expect actual
 '
 
+test_lazy_prereq GIT_DAEMON '
+	test_tristate GIT_TEST_GIT_DAEMON &&
+	test "$GIT_TEST_GIT_DAEMON" != false
+'
+
+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
+	JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&
+	JGIT_DAEMON_PID= &&
+	git init --bare empty.git &&
+	>empty.git/git-daemon-export-ok &&
+	mkfifo jgit_daemon_output &&
+	{
+		jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
+		JGIT_DAEMON_PID=$!
+	} &&
+	test_when_finished kill "$JGIT_DAEMON_PID" &&
+	{
+		read line &&
+		case $line in
+		Exporting*)
+			;;
+		*)
+			echo "Expected: Exporting" &&
+			false;;
+		esac &&
+		read line &&
+		case $line in
+		"Listening on"*)
+			;;
+		*)
+			echo "Expected: Listening on" &&
+			false;;
+		esac
+	} <jgit_daemon_output &&
+	# --exit-code asks the command to exit with 2 when no
+	# matching refs are found.
+	test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
+'
 
 test_done
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v4 3/3] connect: advertized capability is not a ref

[Jonathan Nieder]

Jonathan Tan wrote:

> --- a/connect.c
> +++ b/connect.c
> @@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  			continue;
>  		}
>  
> +		if (!strcmp(name, "capabilities^{}")) {
> +			if (saw_response)
> +				warning("protocol error: unexpected capabilities^{}, "
> +					"continuing anyway");

Please use die() for these.

The warning is directed at the wrong user.  The end-user isn't going
to be able to fix the server.  The server owner is going to say "Git
works fine --- I'll ignore this".  Client authors are going to
*eventually* discover the bad server and have to work around it.  So
everyone suffers.

I feel strongly about this: because there are no servers that violate
this, it should be a fatal error.  If we find a server that violates
this, we should weaken the spec and make all violations of the spec
still a fatal error.

The rest looks good.

Thanks for your patience,
Jonathan

Re: [PATCH v4 3/3] connect: advertized capability is not a ref

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never did so for fetch, the client didn't need to handle this
> case. Handle it.
>
> In this aspect, JGit is compliant with the specification in pack-protocol.txt.

The last sentence somehow looks out of place.

> +	int got_dummy_ref_with_capabilities_declaration = 0;
>  
>  	*list = NULL;
>  	for (saw_response = 0; ; saw_response = 1) {
> @@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  			continue;
>  		}
>  
> +		if (!strcmp(name, "capabilities^{}")) {
> +			if (saw_response)
> +				warning("protocol error: unexpected capabilities^{}, "
> +					"continuing anyway");

OK.  saw_response tells us that we saw ".have", a valid ref, or "shallow",
but "capabilities^{}" should happen before any of them, so that is a
protocol violation.  Makes perfect sense.

> +			if (got_dummy_ref_with_capabilities_declaration)
> +				warning("protocol error: multiple capabilities^{}, "
> +					"continuing anyway");
> +			got_dummy_ref_with_capabilities_declaration = 1;
> +			continue;
> +		}
> +
>  		if (!check_ref(name, flags))
>  			continue;
> +
> +		if (got_dummy_ref_with_capabilities_declaration)
> +			warning("protocol error: unexpected ref after capabilities^{}, "
> +				"using this ref and continuing anyway");

Likewise. "capabilities^{}" is used when we cannot piggyback the
capability list after a real ref, so it is unusual to see a real ref
after seeing one.  Makes perfect sense.

Do we want to abort the connection in these cases, I wonder, though?

Thanks.

[PATCH v5 0/3] handle empty spec-compliant remote repos correctly

[Jonathan Tan]

Updates from PATCH v4:
o use fatal errors ("die") instead of warnings for protocol errors
o reworded commit message - removed last sentence and wrote "(following the
  specification in pack-protocol.txt)" in one of the earlier paragraphs

Jonathan Nieder (1):
  connect: tighten check for unexpected early hang up

Jonathan Tan (2):
  tests: move test_lazy_prereq JGIT to test-lib.sh
  connect: advertized capability is not a ref

 connect.c               | 32 ++++++++++++++++++++++++++------
 t/t5310-pack-bitmaps.sh |  4 ----
 t/t5512-ls-remote.sh    | 40 ++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh           |  4 ++++
 4 files changed, 70 insertions(+), 10 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh

[Jonathan Tan]

This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5310-pack-bitmaps.sh | 4 ----
 t/test-lib.sh           | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
 	git pack-objects --stdout --revs <revs >/dev/null
 '
 
-test_lazy_prereq JGIT '
-	type jgit
-'
-
 test_expect_success JGIT 'we can read jgit bitmaps' '
 	git clone . compat-jgit &&
 	(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
 	test "$uid" != 0
 '
 
+test_lazy_prereq JGIT '
+	type jgit
+'
+
 # SANITY is about "can you correctly predict what the filesystem would
 # do by only looking at the permission bits of the files and
 # directories?"  A typical example of !SANITY is running the test
-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 2/3] connect: tighten check for unexpected early hang up

[Jonathan Tan]

From: Jonathan Nieder <jrnieder@gmail.com>

A server hanging up immediately to mark access being denied does not
send any .have refs, shallow lines, or anything else before hanging
up.  If the server has sent anything, then the hangup is unexpected.

That is, if the server hangs up after a shallow line but before sending
any refs, then git should tell me so:

	fatal: The remote end hung up upon initial contact

instead of suggesting an access control problem:

	fatal: Could not read from remote repository.
	Please make sure you have the correct access rights
	and the repository exists.

Noticed while examining this code.  This case isn't likely to come up
in practice but tightening the check makes the code easier to read and
manipulate.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 connect.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/connect.c b/connect.c
index 722dc3f..0c01a49 100644
--- a/connect.c
+++ b/connect.c
@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int got_at_least_one_head)
+static void die_initial_contact(int unexpected)
 {
-	if (got_at_least_one_head)
+	if (unexpected)
 		die("The remote end hung up upon initial contact");
 	else
 		die("Could not read from remote repository.\n\n"
@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			      struct sha1_array *shallow_points)
 {
 	struct ref **orig_list = list;
-	int got_at_least_one_head = 0;
+
+	/*
+	 * A hang-up after seeing some response from the other end
+	 * means that it is unexpected, as we know the other end is
+	 * willing to talk to us.  A hang-up before seeing any
+	 * response does not necessarily mean an ACL problem, though.
+	 */
+	int saw_response;
 
 	*list = NULL;
-	for (;;) {
+	for (saw_response = 0; ; saw_response = 1) {
 		struct ref *ref;
 		struct object_id old_oid;
 		char *name;
@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 				  PACKET_READ_GENTLE_ON_EOF |
 				  PACKET_READ_CHOMP_NEWLINE);
 		if (len < 0)
-			die_initial_contact(got_at_least_one_head);
+			die_initial_contact(saw_response);
 
 		if (!len)
 			break;
@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		oidcpy(&ref->old_oid, &old_oid);
 		*list = ref;
 		list = &ref->next;
-		got_at_least_one_head = 1;
 	}
 
 	annotate_refs_with_symref_info(*orig_list);
-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 3/3] connect: advertized capability is not a ref

[Jonathan Tan]

When cloning an empty repository served by standard git, "git clone" produces
the following reassuring message:

	$ git clone git://localhost/tmp/empty
	Cloning into 'empty'...
	warning: You appear to have cloned an empty repository.
	Checking connectivity... done.

Meanwhile when cloning an empty repository served by JGit, the output is more
haphazard:

	$ git clone git://localhost/tmp/empty
	Cloning into 'empty'...
	Checking connectivity... done.
	warning: remote HEAD refers to nonexistent ref, unable to checkout.

This is a common command to run immediately after creating a remote repository
as preparation for adding content to populate it and pushing. The warning is
confusing and needlessly worrying.

The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
with no refs in upload service., 2013-08-08), JGit's ref advertisement includes
a ref named capabilities^{} to advertise its capabilities on (following the
specification in pack-protocol.txt), while git's ref advertisement is empty in
this case. This allows the client to learn about the server's capabilities and
is needed, for example, for fetch-by-sha1 to work when no refs are advertised.

This also affects "ls-remote". For example, against an empty repository served
by JGit:

	$ git ls-remote git://localhost/tmp/empty
	0000000000000000000000000000000000000000        capabilities^{}

Git advertises the same capabilities^{} ref in its ref advertisement for push
but since it never did so for fetch, the client didn't need to handle this
case. Handle it.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 connect.c            | 14 ++++++++++++++
 t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/connect.c b/connect.c
index 0c01a49..7224b5e 100644
--- a/connect.c
+++ b/connect.c
@@ -123,6 +123,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * response does not necessarily mean an ACL problem, though.
 	 */
 	int saw_response;
+	int got_dummy_ref_with_capabilities_declaration = 0;
 
 	*list = NULL;
 	for (saw_response = 0; ; saw_response = 1) {
@@ -172,8 +173,21 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			continue;
 		}
 
+		if (!strcmp(name, "capabilities^{}")) {
+			if (saw_response)
+				die("protocol error: unexpected capabilities^{}");
+			if (got_dummy_ref_with_capabilities_declaration)
+				die("protocol error: multiple capabilities^{}");
+			got_dummy_ref_with_capabilities_declaration = 1;
+			continue;
+		}
+
 		if (!check_ref(name, flags))
 			continue;
+
+		if (got_dummy_ref_with_capabilities_declaration)
+			die("protocol error: unexpected ref after capabilities^{}");
+
 		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
 		oidcpy(&ref->old_oid, &old_oid);
 		*list = ref;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..befdfee 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,45 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 	test_cmp expect actual
 '
 
+test_lazy_prereq GIT_DAEMON '
+	test_tristate GIT_TEST_GIT_DAEMON &&
+	test "$GIT_TEST_GIT_DAEMON" != false
+'
+
+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
+	JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&
+	JGIT_DAEMON_PID= &&
+	git init --bare empty.git &&
+	>empty.git/git-daemon-export-ok &&
+	mkfifo jgit_daemon_output &&
+	{
+		jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
+		JGIT_DAEMON_PID=$!
+	} &&
+	test_when_finished kill "$JGIT_DAEMON_PID" &&
+	{
+		read line &&
+		case $line in
+		Exporting*)
+			;;
+		*)
+			echo "Expected: Exporting" &&
+			false;;
+		esac &&
+		read line &&
+		case $line in
+		"Listening on"*)
+			;;
+		*)
+			echo "Expected: Listening on" &&
+			false;;
+		esac
+	} <jgit_daemon_output &&
+	# --exit-code asks the command to exit with 2 when no
+	# matching refs are found.
+	test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
+'
 
 test_done
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v4 3/3] connect: advertized capability is not a ref

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jonathan Tan wrote:
>
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>>  			continue;
>>  		}
>>  
>> +		if (!strcmp(name, "capabilities^{}")) {
>> +			if (saw_response)
>> +				warning("protocol error: unexpected capabilities^{}, "
>> +					"continuing anyway");
>
> Please use die() for these.
> ...
> The rest looks good.

Will squash this in, then.

 connect.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/connect.c b/connect.c
index df25d21..5ccbd10 100644
--- a/connect.c
+++ b/connect.c
@@ -175,11 +175,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 
 		if (!strcmp(name, "capabilities^{}")) {
 			if (saw_response)
-				warning("protocol error: unexpected capabilities^{}, "
-					"continuing anyway");
+				die("protocol error: unexpected capabilities^{}");
 			if (got_dummy_ref_with_capabilities_declaration)
-				warning("protocol error: multiple capabilities^{}, "
-					"continuing anyway");
+				die("protocol error: multiple capabilities^{}");
 			got_dummy_ref_with_capabilities_declaration = 1;
 			continue;
 		}
@@ -188,8 +186,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			continue;
 
 		if (got_dummy_ref_with_capabilities_declaration)
-			warning("protocol error: unexpected ref after capabilities^{}, "
-				"using this ref and continuing anyway");
+			die("protocol error: unexpected ref after capabilities^{}");
 
 		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
 		oidcpy(&ref->old_oid, &old_oid);

Re: [PATCH v5 0/3] handle empty spec-compliant remote repos correctly

[Jonathan Nieder]

Jonathan Tan wrote:

>  connect.c               | 32 ++++++++++++++++++++++++++------
>  t/t5310-pack-bitmaps.sh |  4 ----
>  t/t5512-ls-remote.sh    | 40 ++++++++++++++++++++++++++++++++++++++++
>  t/test-lib.sh           |  4 ++++
>  4 files changed, 70 insertions(+), 10 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thank you,
Jonathan

Re: [PATCH v4 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh

[Torsten Bögershausen]

On Fri, Sep 09, 2016 at 10:36:28AM -0700, Jonathan Tan wrote:
[]
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index d731d66..c9c1037 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
>  	test "$uid" != 0
>  '
>  
> +test_lazy_prereq JGIT '
> +	type jgit
> +'
> +

Minor note: 
Typically the stdout of `which` is suppressed like this:

if ! type cvs >/dev/null 2>&1

Re: [PATCH v4 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh

[Jeff King]

On Sat, Sep 10, 2016 at 05:51:19AM +0000, Torsten Bögershausen wrote:

> > +test_lazy_prereq JGIT '
> > +	type jgit
> > +'
> > +
> 
> Minor note: 
> Typically the stdout of `which` is suppressed like this:
> 
> if ! type cvs >/dev/null 2>&1

But we don't want to suppress the output here; the test harness
redirects the lazy_prereq output to /dev/null unless you specify "-v".
And there is no need for "if", because what we care about is the exit
code of the commands inside the lazy_prereq.

-Peff