* [PATCH] fetch: print an error when declining to request an unadvertised object @ 2017-02-10 17:26 Matt McCutchen 2017-02-10 18:36 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Matt McCutchen @ 2017-02-10 17:26 UTC (permalink / raw) To: git 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 <matt@mattmccutchen.net> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] fetch: print an error when declining to request an unadvertised object 2017-02-10 17:26 [PATCH] fetch: print an error when declining to request an unadvertised object Matt McCutchen @ 2017-02-10 18:36 ` Junio C Hamano 2017-02-12 21:13 ` Matt McCutchen 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2017-02-10 18:36 UTC (permalink / raw) To: Matt McCutchen; +Cc: git Matt McCutchen <matt@mattmccutchen.net> 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 <matt@mattmccutchen.net> > --- > > 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 && > test_must_fail git rev-parse --verify refs/heads/copy && > > # the server side can allow it to succeed ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] fetch: print an error when declining to request an unadvertised object 2017-02-10 18:36 ` Junio C Hamano @ 2017-02-12 21:13 ` Matt McCutchen 2017-02-12 23:49 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Matt McCutchen @ 2017-02-12 21:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 NNNN", 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] fetch: print an error when declining to request an unadvertised object 2017-02-12 21:13 ` Matt McCutchen @ 2017-02-12 23:49 ` Junio C Hamano 2017-02-19 1:55 ` Matt McCutchen 2017-02-19 2:07 ` Matt McCutchen 0 siblings, 2 replies; 17+ messages in thread From: Junio C Hamano @ 2017-02-12 23:49 UTC (permalink / raw) To: Matt McCutchen; +Cc: git Matt McCutchen <matt@mattmccutchen.net> 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] fetch: print an error when declining to request an unadvertised object 2017-02-12 23:49 ` Junio C Hamano @ 2017-02-19 1:55 ` Matt McCutchen 2017-02-21 6:36 ` Junio C Hamano 2017-02-19 2:07 ` Matt McCutchen 1 sibling, 1 reply; 17+ messages in thread From: Matt McCutchen @ 2017-02-19 1:55 UTC (permalink / raw) To: git 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 <matt@mattmccutchen.net> --- 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(&si); 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; + } + ret = 1; + } + return ret; +} diff --git a/fetch-pack.h b/fetch-pack.h index c912e3d..fd4d80e 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -45,4 +45,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args, struct sha1_array *shallow, char **pack_lockfile); +/* + * Print an appropriate error message for each sought ref that wasn't + * matched. Return 0 if all sought refs were matched, otherwise 1. + * + * The type of "sought" should be "const struct ref *const *" but for + * http://stackoverflow.com/questions/5055655/double-pointer-const-correctness-warnings-in-c . + */ +int report_unmatched_refs(struct ref **sought, int nr_sought); + #endif diff --git a/remote.h b/remote.h index 9248811..0b9d8c4 100644 --- a/remote.h +++ b/remote.h @@ -89,8 +89,13 @@ struct ref { force:1, forced_update:1, expect_old_sha1:1, - deletion:1, - matched:1; + deletion:1; + + enum { + REF_NOT_MATCHED = 0, /* initial value */ + REF_MATCHED, + REF_UNADVERTISED_NOT_ALLOWED + } match_status; /* * Order is important here, as we write to FETCH_HEAD diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 26b2caf..78f3b8e 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 diff --git a/transport.c b/transport.c index 04e5d66..c377907 100644 --- a/transport.c +++ b/transport.c @@ -204,6 +204,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus static int fetch_refs_via_pack(struct transport *transport, int nr_heads, struct ref **to_fetch) { + int ret = 0; struct git_transport_data *data = transport->data; struct ref *refs; char *dest = xstrdup(transport->url); @@ -241,19 +242,22 @@ static int fetch_refs_via_pack(struct transport *transport, &transport->pack_lockfile); close(data->fd[0]); close(data->fd[1]); - if (finish_connect(data->conn)) { - free_refs(refs); - refs = NULL; - } + if (finish_connect(data->conn)) + ret = -1; data->conn = NULL; data->got_remote_heads = 0; data->options.self_contained_and_connected = args.self_contained_and_connected; + if (refs == NULL) + ret = -1; + if (report_unmatched_refs(to_fetch, nr_heads)) + ret = -1; + free_refs(refs_tmp); free_refs(refs); free(dest); - return (refs ? 0 : -1); + return ret; } static int push_had_errors(struct ref *ref) -- 2.9.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] fetch: print an error when declining to request an unadvertised object 2017-02-19 1:55 ` Matt McCutchen @ 2017-02-21 6:36 ` Junio C Hamano 2017-02-22 16:01 ` [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function Matt McCutchen ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Junio C Hamano @ 2017-02-21 6:36 UTC (permalink / raw) To: Matt McCutchen; +Cc: git Matt McCutchen <matt@mattmccutchen.net> 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 <matt@mattmccutchen.net> > --- 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function 2017-02-21 6:36 ` Junio C Hamano @ 2017-02-22 16:01 ` Matt McCutchen 2017-02-22 17:11 ` Junio C Hamano 2017-02-22 16:02 ` [PATCH 2/3] fetch_refs_via_pack: call report_unmatched_refs Matt McCutchen ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Matt McCutchen @ 2017-02-22 16:01 UTC (permalink / raw) To: git We're preparing to reuse this code in transport.c for "git fetch". While I'm here, internationalize the existing error message. --- builtin/fetch-pack.c | 7 +------ fetch-pack.c | 13 +++++++++++++ fetch-pack.h | 9 +++++++++ t/t5500-fetch-pack.sh | 6 +++--- 4 files changed, 26 insertions(+), 9 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..7c8d44c 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1094,3 +1094,16 @@ struct ref *fetch_pack(struct fetch_pack_args *args, clear_shallow_info(&si); 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] || sought[i]->matched) + continue; + error(_("no such remote ref %s"), sought[i]->name); + ret = 1; + } + return ret; +} diff --git a/fetch-pack.h b/fetch-pack.h index c912e3d..fd4d80e 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -45,4 +45,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args, struct sha1_array *shallow, char **pack_lockfile); +/* + * Print an appropriate error message for each sought ref that wasn't + * matched. Return 0 if all sought refs were matched, otherwise 1. + * + * The type of "sought" should be "const struct ref *const *" but for + * http://stackoverflow.com/questions/5055655/double-pointer-const-correctness-warnings-in-c . + */ +int report_unmatched_refs(struct ref **sought, int nr_sought); + #endif diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 505e1b4..b5865b3 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -484,7 +484,7 @@ test_expect_success 'test lonely missing ref' ' cd client && test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy ) >/dev/null 2>error-m && - test_cmp expect-error error-m + test_i18ncmp expect-error error-m ' test_expect_success 'test missing ref after existing' ' @@ -492,7 +492,7 @@ test_expect_success 'test missing ref after existing' ' cd client && test_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy ) >/dev/null 2>error-em && - test_cmp expect-error error-em + test_i18ncmp expect-error error-em ' test_expect_success 'test missing ref before existing' ' @@ -500,7 +500,7 @@ test_expect_success 'test missing ref before existing' ' cd client && test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A ) >/dev/null 2>error-me && - test_cmp expect-error error-me + test_i18ncmp expect-error error-me ' test_expect_success 'test --all, --depth, and explicit head' ' -- 2.9.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function 2017-02-22 16:01 ` [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function Matt McCutchen @ 2017-02-22 17:11 ` Junio C Hamano 2017-02-22 16:01 ` [PATCH v2 " Matt McCutchen ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Junio C Hamano @ 2017-02-22 17:11 UTC (permalink / raw) To: Matt McCutchen; +Cc: git Matt McCutchen <matt@mattmccutchen.net> writes: > We're preparing to reuse this code in transport.c for "git fetch". > > While I'm here, internationalize the existing error message. > --- Sounds good. Please just say it is OK for me to forge your sign-off ;-) > diff --git a/fetch-pack.h b/fetch-pack.h > index c912e3d..fd4d80e 100644 > --- a/fetch-pack.h > +++ b/fetch-pack.h > @@ -45,4 +45,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args, > struct sha1_array *shallow, > char **pack_lockfile); > > +/* > + * Print an appropriate error message for each sought ref that wasn't > + * matched. Return 0 if all sought refs were matched, otherwise 1. > + * > + * The type of "sought" should be "const struct ref *const *" but for > + * http://stackoverflow.com/questions/5055655/double-pointer-const-correctness-warnings-in-c . > + */ This is an unfinished sentence, but I wonder if we even need to have it here? I'd be surprised if this function was unique in the codebase that takes an array pointer whose type is looser than necessary because of well-known language rules. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/3] fetch-pack: move code to report unmatched refs to a function 2017-02-22 17:11 ` Junio C Hamano @ 2017-02-22 16:01 ` Matt McCutchen 2017-02-22 16:02 ` [PATCH v2 2/3] fetch_refs_via_pack: call report_unmatched_refs Matt McCutchen ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: Matt McCutchen @ 2017-02-22 16:01 UTC (permalink / raw) To: git We're preparing to reuse this code in transport.c for "git fetch". While I'm here, internationalize the existing error message. Signed-off-by: Matt McCutchen <matt@mattmccutchen.net> --- builtin/fetch-pack.c | 7 +------ fetch-pack.c | 13 +++++++++++++ fetch-pack.h | 6 ++++++ t/t5500-fetch-pack.sh | 6 +++--- 4 files changed, 23 insertions(+), 9 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..7c8d44c 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1094,3 +1094,16 @@ struct ref *fetch_pack(struct fetch_pack_args *args, clear_shallow_info(&si); 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] || sought[i]->matched) + continue; + error(_("no such remote ref %s"), sought[i]->name); + ret = 1; + } + return ret; +} diff --git a/fetch-pack.h b/fetch-pack.h index c912e3d..a2d46e6 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -45,4 +45,10 @@ struct ref *fetch_pack(struct fetch_pack_args *args, struct sha1_array *shallow, char **pack_lockfile); +/* + * Print an appropriate error message for each sought ref that wasn't + * matched. Return 0 if all sought refs were matched, otherwise 1. + */ +int report_unmatched_refs(struct ref **sought, int nr_sought); + #endif diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 505e1b4..b5865b3 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -484,7 +484,7 @@ test_expect_success 'test lonely missing ref' ' cd client && test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy ) >/dev/null 2>error-m && - test_cmp expect-error error-m + test_i18ncmp expect-error error-m ' test_expect_success 'test missing ref after existing' ' @@ -492,7 +492,7 @@ test_expect_success 'test missing ref after existing' ' cd client && test_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy ) >/dev/null 2>error-em && - test_cmp expect-error error-em + test_i18ncmp expect-error error-em ' test_expect_success 'test missing ref before existing' ' @@ -500,7 +500,7 @@ test_expect_success 'test missing ref before existing' ' cd client && test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A ) >/dev/null 2>error-me && - test_cmp expect-error error-me + test_i18ncmp expect-error error-me ' test_expect_success 'test --all, --depth, and explicit head' ' -- 2.9.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] fetch_refs_via_pack: call report_unmatched_refs 2017-02-22 17:11 ` Junio C Hamano 2017-02-22 16:01 ` [PATCH v2 " Matt McCutchen @ 2017-02-22 16:02 ` Matt McCutchen 2017-02-22 16:05 ` [PATCH v2 3/3] fetch-pack: add specific error for fetching an unadvertised object Matt McCutchen 2017-02-22 17:26 ` [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function Matt McCutchen 3 siblings, 0 replies; 17+ messages in thread From: Matt McCutchen @ 2017-02-22 16:02 UTC (permalink / raw) To: git "git fetch" currently doesn't bother to check that it got all refs it sought, because the common case of requesting a nonexistent ref triggers a die() in get_fetch_map. However, there's at least one case that slipped through: "git fetch REMOTE SHA1" if the server doesn't allow requests for unadvertised objects. Make fetch_refs_via_pack (which is on the "git fetch" code path) call report_unmatched_refs so that we at least get an error message in that case. Signed-off-by: Matt McCutchen <matt@mattmccutchen.net> --- t/t5516-fetch-push.sh | 3 ++- transport.c | 14 +++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 26b2caf..0d13a45 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 "no such remote ref" err && test_must_fail git rev-parse --verify refs/heads/copy && # the server side can allow it to succeed diff --git a/transport.c b/transport.c index 04e5d66..c377907 100644 --- a/transport.c +++ b/transport.c @@ -204,6 +204,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus static int fetch_refs_via_pack(struct transport *transport, int nr_heads, struct ref **to_fetch) { + int ret = 0; struct git_transport_data *data = transport->data; struct ref *refs; char *dest = xstrdup(transport->url); @@ -241,19 +242,22 @@ static int fetch_refs_via_pack(struct transport *transport, &transport->pack_lockfile); close(data->fd[0]); close(data->fd[1]); - if (finish_connect(data->conn)) { - free_refs(refs); - refs = NULL; - } + if (finish_connect(data->conn)) + ret = -1; data->conn = NULL; data->got_remote_heads = 0; data->options.self_contained_and_connected = args.self_contained_and_connected; + if (refs == NULL) + ret = -1; + if (report_unmatched_refs(to_fetch, nr_heads)) + ret = -1; + free_refs(refs_tmp); free_refs(refs); free(dest); - return (refs ? 0 : -1); + return ret; } static int push_had_errors(struct ref *ref) -- 2.9.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] fetch-pack: add specific error for fetching an unadvertised object 2017-02-22 17:11 ` Junio C Hamano 2017-02-22 16:01 ` [PATCH v2 " Matt McCutchen 2017-02-22 16:02 ` [PATCH v2 2/3] fetch_refs_via_pack: call report_unmatched_refs Matt McCutchen @ 2017-02-22 16:05 ` Matt McCutchen 2017-02-22 17:26 ` [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function Matt McCutchen 3 siblings, 0 replies; 17+ messages in thread From: Matt McCutchen @ 2017-02-22 16:05 UTC (permalink / raw) To: git Enhance filter_refs (which decides whether a request for an unadvertised object should be sent to the server) to record a new match status on the "struct ref" when a 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". Signed-off-by: Matt McCutchen <matt@mattmccutchen.net> --- fetch-pack.c | 42 +++++++++++++++++++++++++++--------------- remote.h | 9 +++++++-- t/t5516-fetch-push.sh | 2 +- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 7c8d44c..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; @@ -1100,9 +1102,19 @@ int report_unmatched_refs(struct ref **sought, int nr_sought) int i, ret = 0; for (i = 0; i < nr_sought; i++) { - if (!sought[i] || sought[i]->matched) + if (!sought[i]) continue; - error(_("no such remote ref %s"), sought[i]->name); + 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; + } ret = 1; } return ret; diff --git a/remote.h b/remote.h index 9248811..0b9d8c4 100644 --- a/remote.h +++ b/remote.h @@ -89,8 +89,13 @@ struct ref { force:1, forced_update:1, expect_old_sha1:1, - deletion:1, - matched:1; + deletion:1; + + enum { + REF_NOT_MATCHED = 0, /* initial value */ + REF_MATCHED, + REF_UNADVERTISED_NOT_ALLOWED + } match_status; /* * Order is important here, as we write to FETCH_HEAD diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 0d13a45..78f3b8e 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1099,7 +1099,7 @@ test_expect_success 'fetch exact SHA1' ' # fetching the hidden object should fail by default test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err && - test_i18ngrep "no such remote ref" 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function 2017-02-22 17:11 ` Junio C Hamano ` (2 preceding siblings ...) 2017-02-22 16:05 ` [PATCH v2 3/3] fetch-pack: add specific error for fetching an unadvertised object Matt McCutchen @ 2017-02-22 17:26 ` Matt McCutchen 3 siblings, 0 replies; 17+ messages in thread From: Matt McCutchen @ 2017-02-22 17:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, 2017-02-22 at 09:11 -0800, Junio C Hamano wrote: > Matt McCutchen <matt@mattmccutchen.net> writes: > > > We're preparing to reuse this code in transport.c for "git fetch". > > > > While I'm here, internationalize the existing error message. > > --- > > Sounds good. Please just say it is OK for me to forge your sign-off > ;-) Oops. Given the other issue below, I'll just regenerate the patch series. > > diff --git a/fetch-pack.h b/fetch-pack.h > > index c912e3d..fd4d80e 100644 > > --- a/fetch-pack.h > > +++ b/fetch-pack.h > > @@ -45,4 +45,13 @@ struct ref *fetch_pack(struct fetch_pack_args > > *args, > > struct sha1_array *shallow, > > char **pack_lockfile); > > > > +/* > > + * Print an appropriate error message for each sought ref that > > wasn't > > + * matched. Return 0 if all sought refs were matched, otherwise > > 1. > > + * > > + * The type of "sought" should be "const struct ref *const *" but > > for > > + * http://stackoverflow.com/questions/5055655/double-pointer-const > > -correctness-warnings-in-c . > > + */ > > This is an unfinished sentence, but I wonder if we even need to have > it here? I'd be surprised if this function was unique in the > codebase that takes an array pointer whose type is looser than > necessary because of well-known language rules. You're probably right. I'm in the habit of documenting things that were unknown to me, but I'll take your word for what's well-known to the average git developer. I'll remove the remark. Matt ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] fetch_refs_via_pack: call report_unmatched_refs 2017-02-21 6:36 ` Junio C Hamano 2017-02-22 16:01 ` [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function Matt McCutchen @ 2017-02-22 16:02 ` Matt McCutchen 2017-02-22 16:05 ` [PATCH 3/3] fetch-pack: add specific error for fetching an unadvertised object Matt McCutchen 2017-02-22 16:17 ` [PATCH] fetch: print an error when declining to request " Matt McCutchen 3 siblings, 0 replies; 17+ messages in thread From: Matt McCutchen @ 2017-02-22 16:02 UTC (permalink / raw) To: git "git fetch" currently doesn't bother to check that it got all refs it sought, because the common case of requesting a nonexistent ref triggers a die() in get_fetch_map. However, there's at least one case that slipped through: "git fetch REMOTE SHA1" if the server doesn't allow requests for unadvertised objects. Make fetch_refs_via_pack (which is on the "git fetch" code path) call report_unmatched_refs so that we at least get an error message in that case. --- t/t5516-fetch-push.sh | 3 ++- transport.c | 14 +++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 26b2caf..0d13a45 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 "no such remote ref" err && test_must_fail git rev-parse --verify refs/heads/copy && # the server side can allow it to succeed diff --git a/transport.c b/transport.c index 04e5d66..c377907 100644 --- a/transport.c +++ b/transport.c @@ -204,6 +204,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus static int fetch_refs_via_pack(struct transport *transport, int nr_heads, struct ref **to_fetch) { + int ret = 0; struct git_transport_data *data = transport->data; struct ref *refs; char *dest = xstrdup(transport->url); @@ -241,19 +242,22 @@ static int fetch_refs_via_pack(struct transport *transport, &transport->pack_lockfile); close(data->fd[0]); close(data->fd[1]); - if (finish_connect(data->conn)) { - free_refs(refs); - refs = NULL; - } + if (finish_connect(data->conn)) + ret = -1; data->conn = NULL; data->got_remote_heads = 0; data->options.self_contained_and_connected = args.self_contained_and_connected; + if (refs == NULL) + ret = -1; + if (report_unmatched_refs(to_fetch, nr_heads)) + ret = -1; + free_refs(refs_tmp); free_refs(refs); free(dest); - return (refs ? 0 : -1); + return ret; } static int push_had_errors(struct ref *ref) -- 2.9.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/3] fetch-pack: add specific error for fetching an unadvertised object 2017-02-21 6:36 ` Junio C Hamano 2017-02-22 16:01 ` [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function Matt McCutchen 2017-02-22 16:02 ` [PATCH 2/3] fetch_refs_via_pack: call report_unmatched_refs Matt McCutchen @ 2017-02-22 16:05 ` Matt McCutchen 2017-02-22 16:17 ` [PATCH] fetch: print an error when declining to request " Matt McCutchen 3 siblings, 0 replies; 17+ messages in thread From: Matt McCutchen @ 2017-02-22 16:05 UTC (permalink / raw) To: git Enhance filter_refs (which decides whether a request for an unadvertised object should be sent to the server) to record a new match status on the "struct ref" when a 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". --- fetch-pack.c | 42 +++++++++++++++++++++++++++--------------- remote.h | 9 +++++++-- t/t5516-fetch-push.sh | 2 +- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 7c8d44c..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; @@ -1100,9 +1102,19 @@ int report_unmatched_refs(struct ref **sought, int nr_sought) int i, ret = 0; for (i = 0; i < nr_sought; i++) { - if (!sought[i] || sought[i]->matched) + if (!sought[i]) continue; - error(_("no such remote ref %s"), sought[i]->name); + 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; + } ret = 1; } return ret; diff --git a/remote.h b/remote.h index 9248811..0b9d8c4 100644 --- a/remote.h +++ b/remote.h @@ -89,8 +89,13 @@ struct ref { force:1, forced_update:1, expect_old_sha1:1, - deletion:1, - matched:1; + deletion:1; + + enum { + REF_NOT_MATCHED = 0, /* initial value */ + REF_MATCHED, + REF_UNADVERTISED_NOT_ALLOWED + } match_status; /* * Order is important here, as we write to FETCH_HEAD diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 0d13a45..78f3b8e 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1099,7 +1099,7 @@ test_expect_success 'fetch exact SHA1' ' # fetching the hidden object should fail by default test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err && - test_i18ngrep "no such remote ref" 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] fetch: print an error when declining to request an unadvertised object 2017-02-21 6:36 ` Junio C Hamano ` (2 preceding siblings ...) 2017-02-22 16:05 ` [PATCH 3/3] fetch-pack: add specific error for fetching an unadvertised object Matt McCutchen @ 2017-02-22 16:17 ` Matt McCutchen 2017-02-22 17:07 ` Junio C Hamano 3 siblings, 1 reply; 17+ messages in thread From: Matt McCutchen @ 2017-02-22 16:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] fetch: print an error when declining to request an unadvertised object 2017-02-22 16:17 ` [PATCH] fetch: print an error when declining to request " Matt McCutchen @ 2017-02-22 17:07 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2017-02-22 17:07 UTC (permalink / raw) To: Matt McCutchen; +Cc: git Matt McCutchen <matt@mattmccutchen.net> 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] fetch: print an error when declining to request an unadvertised object 2017-02-12 23:49 ` Junio C Hamano 2017-02-19 1:55 ` Matt McCutchen @ 2017-02-19 2:07 ` Matt McCutchen 1 sibling, 0 replies; 17+ messages in thread From: Matt McCutchen @ 2017-02-19 2:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-02-22 17:29 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-10 17:26 [PATCH] fetch: print an error when declining to request an unadvertised object Matt McCutchen 2017-02-10 18:36 ` Junio C Hamano 2017-02-12 21:13 ` Matt McCutchen 2017-02-12 23:49 ` Junio C Hamano 2017-02-19 1:55 ` Matt McCutchen 2017-02-21 6:36 ` Junio C Hamano 2017-02-22 16:01 ` [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function Matt McCutchen 2017-02-22 17:11 ` Junio C Hamano 2017-02-22 16:01 ` [PATCH v2 " Matt McCutchen 2017-02-22 16:02 ` [PATCH v2 2/3] fetch_refs_via_pack: call report_unmatched_refs Matt McCutchen 2017-02-22 16:05 ` [PATCH v2 3/3] fetch-pack: add specific error for fetching an unadvertised object Matt McCutchen 2017-02-22 17:26 ` [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function Matt McCutchen 2017-02-22 16:02 ` [PATCH 2/3] fetch_refs_via_pack: call report_unmatched_refs Matt McCutchen 2017-02-22 16:05 ` [PATCH 3/3] fetch-pack: add specific error for fetching an unadvertised object Matt McCutchen 2017-02-22 16:17 ` [PATCH] fetch: print an error when declining to request " Matt McCutchen 2017-02-22 17:07 ` Junio C Hamano 2017-02-19 2:07 ` Matt McCutchen
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).