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

[Junio C Hamano]

Jonathan Nieder  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(>old_oid, _oid);

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

[Junio C Hamano]

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

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

[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
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 
---
 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(>old_oid, _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
+   }