Re: [PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1
Junio C Hamano wrote: > Jonathan Nieder writes: >> This patch adds a test to check this behavior that notices another >> behavior difference between protocol v0 and v2 in the process. Add a >> NEEDSWORK comment to clear it up. > > Thanks. > > I wonder if there is a more effective way to smoke out other bugs > remaining in proto v2. When the fetch-by-SHA1 feature was added > originally, we certainly would have added a test or two to make sure > it won't break. The root cause of this breakage is that we lack the > ability to easily exercise proto v2 on these existing tests that > were written back in the proto v0 days. It there were such a way > (like, a common set of tests that are run with all supported > protos), we would have caught the breakge even before the topic hit > 'next'. I had a similar thought. I am not sure I agree about the root cause, but root causes are generally slippery to define. Because this bug had significant internal impact, we came up with a few next steps: - shore up protocol v2 test coverage, as you described - arrange for long refactoring series we submit to be divided up for the team to review, to avoid reviewer fatigue. Hopefully this will make us a better example for other submitters of long series. We're open to cooperating with others --- maybe we can set up a volunteer reviewer brigade to get a more diverse set of eyes on each series --- though organizing that is harder. - improve telemetry for our internal deployment, to get earlier notice when Git is producing more errors. I suspect other installations may want something like this too --- e.g. I think this is one of the benefits of what Jeff Hostetler is starting to build with json-writer. - help internal users triage errors from Git (like those decision trees parents have to help decide when to bring a child to the doctor), so that we get earlier notice and can roll back and report upstream more quickly when they've run into a Git bug Or in other words, please expect more in this area soon, and feel free to pester me if the test coverage doesn't arrive. :) Thanks, Jonathan
Re: [PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1
Jonathan Nieder writes: > When v2.18.0-rc0~10^2~1 (refspec: consolidate ref-prefix generation > logic, 2018-05-16) factored out the ref-prefix generation code for > reuse, it left out the 'if (!item->exact_sha1)' test in the original > ref-prefix generation code. As a result, fetches by SHA-1 generate > ref-prefixes as though the SHA-1 being fetched were an abbreviated ref > name: > > $ GIT_TRACE_PACKET=1 bin-wrappers/git -c protocol.version=2 \ > fetch origin 12039e008f9a4e3394f3f94f8ea897785cb09448 > [...] > packet:fetch> ref-prefix 12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/tags/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/heads/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448/HEAD > packet:fetch> > > If there is another ref name on the command line or the object being > fetched is already available locally, then that's mostly harmless. > But otherwise, we error out with > > fatal: no matching remote head > > since the server did not send any refs we are interested in. Filter > out the exact_sha1 refspecs to avoid this. > > This patch adds a test to check this behavior that notices another > behavior difference between protocol v0 and v2 in the process. Add a > NEEDSWORK comment to clear it up. Thanks. I wonder if there is a more effective way to smoke out other bugs remaining in proto v2. When the fetch-by-SHA1 feature was added originally, we certainly would have added a test or two to make sure it won't break. The root cause of this breakage is that we lack the ability to easily exercise proto v2 on these existing tests that were written back in the proto v0 days. It there were such a way (like, a common set of tests that are run with all supported protos), we would have caught the breakge even before the topic hit 'next'.
Re: [PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1
Thanks for finding this, I don't know how I missed moving that bit over when factoring it out. Well I guess I sort of rewrote it and combined two pieces of logic so that's how. Anyway, this looks right and thanks for adding the test. On Thu, May 31, 2018 at 12:23 AM, Jonathan Nieder wrote: > When v2.18.0-rc0~10^2~1 (refspec: consolidate ref-prefix generation > logic, 2018-05-16) factored out the ref-prefix generation code for > reuse, it left out the 'if (!item->exact_sha1)' test in the original > ref-prefix generation code. As a result, fetches by SHA-1 generate > ref-prefixes as though the SHA-1 being fetched were an abbreviated ref > name: > > $ GIT_TRACE_PACKET=1 bin-wrappers/git -c protocol.version=2 \ > fetch origin 12039e008f9a4e3394f3f94f8ea897785cb09448 > [...] > packet:fetch> ref-prefix 12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/tags/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/heads/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448/HEAD > packet:fetch> > > If there is another ref name on the command line or the object being > fetched is already available locally, then that's mostly harmless. > But otherwise, we error out with > > fatal: no matching remote head > > since the server did not send any refs we are interested in. Filter > out the exact_sha1 refspecs to avoid this. > > This patch adds a test to check this behavior that notices another > behavior difference between protocol v0 and v2 in the process. Add a > NEEDSWORK comment to clear it up. > > Signed-off-by: Jonathan Nieder > --- > Here's the change described in > https://public-inbox.org/git/20180531010739.gb36...@aiede.svl.corp.google.com/ > as a proper patch. > > Thoughts of all kinds welcome, as always. > > refspec.c | 2 ++ > refspec.h | 4 > t/t5516-fetch-push.sh | 19 +++ > 3 files changed, 25 insertions(+) > > diff --git a/refspec.c b/refspec.c > index c59a4ccf1e..ada7854f7a 100644 > --- a/refspec.c > +++ b/refspec.c > @@ -202,6 +202,8 @@ void refspec_ref_prefixes(const struct refspec *rs, > const struct refspec_item *item = &rs->items[i]; > const char *prefix = NULL; > > + if (item->exact_sha1) > + continue; > if (rs->fetch == REFSPEC_FETCH) > prefix = item->src; > else if (item->dst) > diff --git a/refspec.h b/refspec.h > index 01b700e094..3a9363887c 100644 > --- a/refspec.h > +++ b/refspec.h > @@ -42,6 +42,10 @@ void refspec_clear(struct refspec *rs); > int valid_fetch_refspec(const char *refspec); > > struct argv_array; > +/* > + * Determine what values to pass to the peer in ref-prefix lines > + * (see Documentation/technical/protocol-v2.txt). > + */ > void refspec_ref_prefixes(const struct refspec *rs, > struct argv_array *ref_prefixes); > > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index f4d28288f0..a5077d8b7c 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1121,6 +1121,25 @@ test_expect_success 'fetch exact SHA1' ' > ) > ' > > +test_expect_success 'fetch exact SHA1 in protocol v2' ' > + mk_test testrepo heads/master hidden/one && > + git push testrepo master:refs/hidden/one && > + git -C testrepo config transfer.hiderefs refs/hidden && > + check_push_result testrepo $the_commit hidden/one && > + > + mk_child testrepo child && > + git -C child config protocol.version 2 && > + > + # make sure $the_commit does not exist here > + git -C child repack -a -d && > + git -C child prune && > + test_must_fail git -C child cat-file -t $the_commit && > + > + # fetching the hidden object succeeds by default > + # NEEDSWORK: should this match the v0 behavior instead? > + git -C child fetch -v ../testrepo $the_commit:refs/heads/copy > +' > + > for configallowtipsha1inwant in true false > do > test_expect_success "shallow fetch reachable SHA1 (but not a ref), > allowtipsha1inwant=$configallowtipsha1inwant" ' > -- > 2.17.1.1185.g55be947832 >
[PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1
When v2.18.0-rc0~10^2~1 (refspec: consolidate ref-prefix generation logic, 2018-05-16) factored out the ref-prefix generation code for reuse, it left out the 'if (!item->exact_sha1)' test in the original ref-prefix generation code. As a result, fetches by SHA-1 generate ref-prefixes as though the SHA-1 being fetched were an abbreviated ref name: $ GIT_TRACE_PACKET=1 bin-wrappers/git -c protocol.version=2 \ fetch origin 12039e008f9a4e3394f3f94f8ea897785cb09448 [...] packet:fetch> ref-prefix 12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/tags/12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/heads/12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448 packet:fetch> ref-prefix refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448/HEAD packet:fetch> If there is another ref name on the command line or the object being fetched is already available locally, then that's mostly harmless. But otherwise, we error out with fatal: no matching remote head since the server did not send any refs we are interested in. Filter out the exact_sha1 refspecs to avoid this. This patch adds a test to check this behavior that notices another behavior difference between protocol v0 and v2 in the process. Add a NEEDSWORK comment to clear it up. Signed-off-by: Jonathan Nieder --- Here's the change described in https://public-inbox.org/git/20180531010739.gb36...@aiede.svl.corp.google.com/ as a proper patch. Thoughts of all kinds welcome, as always. refspec.c | 2 ++ refspec.h | 4 t/t5516-fetch-push.sh | 19 +++ 3 files changed, 25 insertions(+) diff --git a/refspec.c b/refspec.c index c59a4ccf1e..ada7854f7a 100644 --- a/refspec.c +++ b/refspec.c @@ -202,6 +202,8 @@ void refspec_ref_prefixes(const struct refspec *rs, const struct refspec_item *item = &rs->items[i]; const char *prefix = NULL; + if (item->exact_sha1) + continue; if (rs->fetch == REFSPEC_FETCH) prefix = item->src; else if (item->dst) diff --git a/refspec.h b/refspec.h index 01b700e094..3a9363887c 100644 --- a/refspec.h +++ b/refspec.h @@ -42,6 +42,10 @@ void refspec_clear(struct refspec *rs); int valid_fetch_refspec(const char *refspec); struct argv_array; +/* + * Determine what values to pass to the peer in ref-prefix lines + * (see Documentation/technical/protocol-v2.txt). + */ void refspec_ref_prefixes(const struct refspec *rs, struct argv_array *ref_prefixes); diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index f4d28288f0..a5077d8b7c 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1121,6 +1121,25 @@ test_expect_success 'fetch exact SHA1' ' ) ' +test_expect_success 'fetch exact SHA1 in protocol v2' ' + mk_test testrepo heads/master hidden/one && + git push testrepo master:refs/hidden/one && + git -C testrepo config transfer.hiderefs refs/hidden && + check_push_result testrepo $the_commit hidden/one && + + mk_child testrepo child && + git -C child config protocol.version 2 && + + # make sure $the_commit does not exist here + git -C child repack -a -d && + git -C child prune && + test_must_fail git -C child cat-file -t $the_commit && + + # fetching the hidden object succeeds by default + # NEEDSWORK: should this match the v0 behavior instead? + git -C child fetch -v ../testrepo $the_commit:refs/heads/copy +' + for configallowtipsha1inwant in true false do test_expect_success "shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant" ' -- 2.17.1.1185.g55be947832
