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  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 
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 
Signed-off-by: Junio C Hamano 
---
 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(>old_oid, _oid);
*list = ref;
list = >next;
-   got_at_least_one_head = 1;
}
 
annotate_refs_with_symref_info(*orig_list);
-- 
2.10.0-267-g7db2ae3

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

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

[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 
---
 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(>old_oid, _oid);
*list = ref;
list = >next;
-   got_at_least_one_head = 1;
}
 
annotate_refs_with_symref_info(*orig_list);
--