Re: [PATCH] fetch: print an error when declining to request an unadvertised object
Matt McCutchenwrites: > 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
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
Matt McCutchenwrites: > 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.
Re: [PATCH] fetch: print an error when declining to request an unadvertised object
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
Matt McCutchenwrites: > 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
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
Matt McCutchenwrites: > 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 && >