Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Junio C Hamano]

Matt McCutchen  writes:

> Anyway, I made a split series and will send it in a moment.  I don't
> know if all the commit messages include exactly the information you
> want; hopefully you're happy to edit them as desired.  Compared to the
> previous patch, there is one fix in the net result: fixing t5500-fetch-
> pack.sh to deal with the internationalized "no such remote ref"
> message.

Thanks for going an extra mile.  I think many developers in the
future who reads "git log" will thank you, too.  The changes,
especially the one in the last one, are very much more
understandable compared to the original, even if the end result is
the same.

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Matt McCutchen]

On Mon, 2017-02-20 at 22:36 -0800, Junio C Hamano wrote:
> Hmph, I would have expected this to be done as a three-patch series,
> 
>  * move the loop at the end of cmd_fetch_pack() to a separate helper
>    function report_unmatched_refs() and call it;
> 
>  * add a call to report_unmatched_refs() to the transport layer;
> 
>  * enhance report_unmatched_refs() by introducing match_status
>    field and adding new code to filter_refs() to diagnose other
>    kinds of errors.

Sure.

> The result looks reasonable from a cursory read, though.
> 
> Thanks for following it up to the completion.

This remark led me to believe you were satisfied with the single patch,
but the last "What's cooking in git.git" mail says "Expecting a split
series?".

Anyway, I made a split series and will send it in a moment.  I don't
know if all the commit messages include exactly the information you
want; hopefully you're happy to edit them as desired.  Compared to the
previous patch, there is one fix in the net result: fixing t5500-fetch-
pack.sh to deal with the internationalized "no such remote ref"
message.

Matt

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Junio C Hamano]

Matt McCutchen  writes:

> Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
> allow requests for unadvertised objects by sha1.  The more common case
> of requesting a nonexistent ref normally triggers a die() in
> get_fetch_map, so "git fetch" wasn't bothering to check after the fetch
> that it got all the refs it sought, like "git fetch-pack" does near the
> end of cmd_fetch_pack.
>
> Move the code from cmd_fetch_pack to a new function,
> report_unmatched_refs, that is called by fetch_refs_via_pack as part of
> "git fetch".  Also, change filter_refs (which checks whether a request
> for an unadvertised object should be sent to the server) to set a new
> match status on the "struct ref" when the request is not allowed, and
> have report_unmatched_refs check for this status and print a special
> error message, "Server does not allow request for unadvertised object".
>
> Finally, add a simple test case for "git fetch REMOTE SHA1".
>
> Signed-off-by: Matt McCutchen 
> ---

Hmph, I would have expected this to be done as a three-patch series,

 * move the loop at the end of cmd_fetch_pack() to a separate helper
   function report_unmatched_refs() and call it;

 * add a call to report_unmatched_refs() to the transport layer;

 * enhance report_unmatched_refs() by introducing match_status
   field and adding new code to filter_refs() to diagnose other
   kinds of errors.

The result looks reasonable from a cursory read, though.

Thanks for following it up to the completion.

[PATCH] fetch: print an error when declining to request an unadvertised object

[Matt McCutchen]

Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
allow requests for unadvertised objects by sha1.  The more common case
of requesting a nonexistent ref normally triggers a die() in
get_fetch_map, so "git fetch" wasn't bothering to check after the fetch
that it got all the refs it sought, like "git fetch-pack" does near the
end of cmd_fetch_pack.

Move the code from cmd_fetch_pack to a new function,
report_unmatched_refs, that is called by fetch_refs_via_pack as part of
"git fetch".  Also, change filter_refs (which checks whether a request
for an unadvertised object should be sent to the server) to set a new
match status on the "struct ref" when the request is not allowed, and
have report_unmatched_refs check for this status and print a special
error message, "Server does not allow request for unadvertised object".

Finally, add a simple test case for "git fetch REMOTE SHA1".

Signed-off-by: Matt McCutchen 
---
 builtin/fetch-pack.c  |  7 +--
 fetch-pack.c  | 51 ++-
 fetch-pack.h  |  9 +
 remote.h  |  9 +++--
 t/t5516-fetch-push.sh |  3 ++-
 transport.c   | 14 +-
 6 files changed, 66 insertions(+), 27 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cfe9e44..2a1c1c2 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -219,12 +219,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
 * remote no-such-ref' would silently succeed without issuing
 * an error.
 */
-   for (i = 0; i < nr_sought; i++) {
-   if (!sought[i] || sought[i]->matched)
-   continue;
-   error("no such remote ref %s", sought[i]->name);
-   ret = 1;
-   }
+   ret |= report_unmatched_refs(sought, nr_sought);
 
while (ref) {
printf("%s %s\n",
diff --git a/fetch-pack.c b/fetch-pack.c
index 601f077..f12bfcd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -578,7 +578,7 @@ static void filter_refs(struct fetch_pack_args *args,
break; /* definitely do not have it */
else if (cmp == 0) {
keep = 1; /* definitely have it */
-   sought[i]->matched = 1;
+   sought[i]->match_status = REF_MATCHED;
}
i++;
}
@@ -598,22 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
}
 
/* Append unmatched requests to the list */
-   if ((allow_unadvertised_object_request &
-   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
-   for (i = 0; i < nr_sought; i++) {
-   unsigned char sha1[20];
+   for (i = 0; i < nr_sought; i++) {
+   unsigned char sha1[20];
 
-   ref = sought[i];
-   if (ref->matched)
-   continue;
-   if (get_sha1_hex(ref->name, sha1) ||
-   ref->name[40] != '\0' ||
-   hashcmp(sha1, ref->old_oid.hash))
-   continue;
+   ref = sought[i];
+   if (ref->match_status != REF_NOT_MATCHED)
+   continue;
+   if (get_sha1_hex(ref->name, sha1) ||
+   ref->name[40] != '\0' ||
+   hashcmp(sha1, ref->old_oid.hash))
+   continue;
 
-   ref->matched = 1;
+   if ((allow_unadvertised_object_request &
+   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
+   ref->match_status = REF_MATCHED;
*newtail = copy_ref(ref);
newtail = &(*newtail)->next;
+   } else {
+   ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
}
}
*refs = newlist;
@@ -1094,3 +1096,26 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
clear_shallow_info();
return ref_cpy;
 }
+
+int report_unmatched_refs(struct ref **sought, int nr_sought)
+{
+   int i, ret = 0;
+
+   for (i = 0; i < nr_sought; i++) {
+   if (!sought[i])
+   continue;
+   switch (sought[i]->match_status) {
+   case REF_MATCHED:
+   continue;
+   case REF_NOT_MATCHED:
+   error(_("no such remote ref %s"), sought[i]->name);
+   break;
+   case REF_UNADVERTISED_NOT_ALLOWED:
+   error(_("Server does not allow request for unadvertised 
object %s"),
+ sought[i]->name);
+   break;
+   

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Matt McCutchen]

On Sun, 2017-02-12 at 15:49 -0800, Junio C Hamano wrote:
> The fact that we have the above two choices tells me that a two-step
> approach may be an appropriate approach. [...]

> Even if you did only the first step, as long as the second step can
> be done without reverting what the first step did [*4*] by somebody
> who cares the "specific error" deeply enough, I am OK with that.  Of
> course if you did both steps, that is fine by me as well ;-)

I appreciate the flexibility, but now that I've spent the time to
understand all the code involved, it would be a pity not to go for the
complete solution.  New patch coming.

Matt

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Junio C Hamano]

Matt McCutchen  writes:

> What do you think?  Do you not care about having a more specific error,
> in which case I can copy the code from builtin/fetch-pack.c to
> fetch_refs_via_pack?  Or shall I add code to filter_refs to set a flag
> and add code to builtin/fetch-pack.c and fetch_refs_via_pack to check
> the flag?  Or what?

The fact that we have the above two choices tells me that a two-step
approach may be an appropriate approach.

The first step is to teach fetch_refs_via_pack() that it should not
ignore the information returned in sought[].  It would add new code
similar to what cmd_fetch_pack() uses to notice and report errors
[*1*] to the function.  It would be a sensible first step, but would
not let the user know which of multiple causes of "not matched" we
noticed.

By "a more specific error", I think you are envisioning that the
current boolean "matched" is made into an enum that allows the
caller to tell how each request did not match [*2*].  That can be
the topic of the second patch and would have to touch filter_refs()
[*3*], cmd_fetch_pack() and fetch_refs_via_pack().

I do not have strong preference myself offhand between stopping at
the first step or completing both.

Even if you did only the first step, as long as the second step can
be done without reverting what the first step did [*4*] by somebody
who cares the "specific error" deeply enough, I am OK with that.  Of
course if you did both steps, that is fine by me as well ;-)


[Footnote]

*1* While I know that it is not right to die() in filter_refs(), and
fetch_refs_via_pack() is a better place to notice errors, I do
not offhand know if it is the right place to report errors, or a
caller higher in the callchain may want the callee to be silent
and wants to show its own error message (in which case the error
may have to percolate upwards in the callchain).

*2* e.g. "was it a ref but they did not advertise?  Did it request
an explicit object name and they did not allow it?"  We may want
to support other "more specific" errors that can be detected in
the future.

*3* The current code flips the sought[i]->matched bit on for matched
ones (relying on the initial state of the bit being false), but
it now needs to stuff different kind of "not matched" to the
field to allow the caller to act on it.

*4* IOW, I am OK with an initial "small" improvement, but I'd want
to make sure that such an initial step does not make future
enhancements by others harder.

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Matt McCutchen]

On Fri, 2017-02-10 at 10:36 -0800, Junio C Hamano wrote:
> > 
> There is this piece of code near the end of builtin/fetch-pack.c:
> 
> [...]
> 
> that happens before the command shows the list of fetched refs, and
> this code is prepared to inspect what happend to the requests it (in
> response to the user request) made to the underlying fetch
> machinery, and issue the error message.
> If you change your command line to "git fetch-pack REMOTE SHA1", you
> do see an error from the above.

Yes, "error: no such remote ref ", which at least makes clear that
the operation didn't work, though it would be nice to give a more
specific error message.

> This all happens in transport.c::fetch_refs_via_pack().
> I think that function is a much better place to error or die than
> filter_refs().

I confirmed that checking the sought refs there works.  However, in
filter_refs, it's easy to give a more specific error message that the
server doesn't allow requests for unadvertised objects, and that code
works for "git fetch-pack" too.  To do the same in fetch_refs_via_pack,
we'd have to duplicate a few lines of code from filter_refs and expose
the allow_unadvertised_object_request variable, or just set a flag on
the "struct ref" in filter_refs and check it in fetch_refs_via_pack.

What do you think?  Do you not care about having a more specific error,
in which case I can copy the code from builtin/fetch-pack.c to
fetch_refs_via_pack?  Or shall I add code to filter_refs to set a flag
and add code to builtin/fetch-pack.c and fetch_refs_via_pack to check
the flag?  Or what?

Matt

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Junio C Hamano]

Matt McCutchen  writes:

> Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
> allow requests for unadvertised objects by sha1.  Change it to print a
> meaningful error message.
>
> Signed-off-by: Matt McCutchen 
> ---
>
> The fetch code looks unbelievably complicated to me and I don't understand all
> the cases that can arise.  Hopefully this patch is an acceptable solution to 
> the
> problem.

At first I thought that this should be caught on the sending end
(grep for "not our ref" in upload-pack.c), but you found a case
where we do not even ask the sender on the requesting side.

I am not convinced that modifying filter_refs() is needed or even
desirable, though.

There is this piece of code near the end of builtin/fetch-pack.c:

/*
 * If the heads to pull were given, we should have consumed
 * all of them by matching the remote.  Otherwise, 'git fetch
 * remote no-such-ref' would silently succeed without issuing
 * an error.
 */
for (i = 0; i < nr_sought; i++) {
if (!sought[i] || sought[i]->matched)
continue;
error("no such remote ref %s", sought[i]->name);
ret = 1;
}

that happens before the command shows the list of fetched refs, and
this code is prepared to inspect what happend to the requests it (in
response to the user request) made to the underlying fetch
machinery, and issue the error message.
If you change your command line to "git fetch-pack REMOTE SHA1", you
do see an error from the above.

That is a good indication that the underlying fetch machinery called
by this caller is already doing diagnosis that is necessary for the
caller to issue such an error.  So why do we fail to say anything in
"git fetch"?

I think the real issue is that when fetch-pack machinery is used via
the transport layer, the transport layer discards the information on
these original request (i.e. what is returned in sought[]).

Instead, the caller of the fetch-pack machinery receives the list of
filtered refs as the return value of fetch_pack() function, and only
looks at "refs" without checking what happened to the original
request to_fetch[] (which corresponds to sought[] in the fetch-pack
command).  This all happens in transport.c::fetch_refs_via_pack().
I think that function is a much better place to error or die than
filter_refs().


>  fetch-pack.c  | 31 ---
>  t/t5516-fetch-push.sh |  3 ++-
>  2 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 601f077..117874c 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -598,23 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
>   }
>  
>   /* Append unmatched requests to the list */
> - if ((allow_unadvertised_object_request &
> - (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
> - for (i = 0; i < nr_sought; i++) {
> - unsigned char sha1[20];
> + for (i = 0; i < nr_sought; i++) {
> + unsigned char sha1[20];
>  
> - ref = sought[i];
> - if (ref->matched)
> - continue;
> - if (get_sha1_hex(ref->name, sha1) ||
> - ref->name[40] != '\0' ||
> - hashcmp(sha1, ref->old_oid.hash))
> - continue;
> + ref = sought[i];
> + if (ref->matched)
> + continue;
> + if (get_sha1_hex(ref->name, sha1) ||
> + ref->name[40] != '\0' ||
> + hashcmp(sha1, ref->old_oid.hash))
> + continue;
>  
> - ref->matched = 1;
> - *newtail = copy_ref(ref);
> - newtail = &(*newtail)->next;
> - }
> + if (!(allow_unadvertised_object_request &
> + (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)))
> + die(_("Server does not allow request for unadvertised 
> object %s"), ref->name);
> +
> + ref->matched = 1;
> + *newtail = copy_ref(ref);
> + newtail = &(*newtail)->next;
>   }
>   *refs = newlist;
>  }
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 0fc5a7c..177897e 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
>   test_must_fail git cat-file -t $the_commit &&
>  
>   # fetching the hidden object should fail by default
> - test_must_fail git fetch -v ../testrepo 
> $the_commit:refs/heads/copy &&
> + test_must_fail git fetch -v ../testrepo 
> $the_commit:refs/heads/copy 2>err &&
> + test_i18ngrep "Server does not allow request for unadvertised 
> object" err &&
> 

[PATCH] fetch: print an error when declining to request an unadvertised object

[Matt McCutchen]

Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
allow requests for unadvertised objects by sha1.  Change it to print a
meaningful error message.

Signed-off-by: Matt McCutchen 
---

The fetch code looks unbelievably complicated to me and I don't understand all
the cases that can arise.  Hopefully this patch is an acceptable solution to the
problem.

 fetch-pack.c  | 31 ---
 t/t5516-fetch-push.sh |  3 ++-
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 601f077..117874c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -598,23 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
}
 
/* Append unmatched requests to the list */
-   if ((allow_unadvertised_object_request &
-   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
-   for (i = 0; i < nr_sought; i++) {
-   unsigned char sha1[20];
+   for (i = 0; i < nr_sought; i++) {
+   unsigned char sha1[20];
 
-   ref = sought[i];
-   if (ref->matched)
-   continue;
-   if (get_sha1_hex(ref->name, sha1) ||
-   ref->name[40] != '\0' ||
-   hashcmp(sha1, ref->old_oid.hash))
-   continue;
+   ref = sought[i];
+   if (ref->matched)
+   continue;
+   if (get_sha1_hex(ref->name, sha1) ||
+   ref->name[40] != '\0' ||
+   hashcmp(sha1, ref->old_oid.hash))
+   continue;
 
-   ref->matched = 1;
-   *newtail = copy_ref(ref);
-   newtail = &(*newtail)->next;
-   }
+   if (!(allow_unadvertised_object_request &
+   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)))
+   die(_("Server does not allow request for unadvertised 
object %s"), ref->name);
+
+   ref->matched = 1;
+   *newtail = copy_ref(ref);
+   newtail = &(*newtail)->next;
}
*refs = newlist;
 }
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 0fc5a7c..177897e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
test_must_fail git cat-file -t $the_commit &&
 
# fetching the hidden object should fail by default
-   test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy &&
+   test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy 2>err &&
+   test_i18ngrep "Server does not allow request for unadvertised 
object" err &&
test_must_fail git rev-parse --verify refs/heads/copy &&
 
# the server side can allow it to succeed
-- 
2.9.3