* [PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects @ 2018-06-10 14:32 Kirill Smelkov 2018-06-11 4:20 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Kirill Smelkov @ 2018-06-10 14:32 UTC (permalink / raw) To: Junio C Hamano Cc: Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff King, Jeff Hostetler, Johannes Schindelin, git, Kirill Smelkov Added test shows remote with two tag objects pointing to a blob and a tree. The tag objects themselves are referenced from under regular refs/tags/* namespace. If test_expect_failure is changed to test_expect_success the test fails: Initialized empty Git repository in /home/kirr/src/tools/git/git/t/trash directory.t5500-fetch-pack/fetchall/.git/ fatal: git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf fatal: The remote end hung up unexpectedly not ok 56 - test --all wrt tag to non-commits # # blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) && # git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 && # tree_sha1=$(echo -e "100644 blob $blob_sha1\tfile" | git mktree) && # git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 && # mkdir fetchall && # ( # cd fetchall && # git init && # git fetch-pack --all .. && # git cat-file blob $blob_sha1 >/dev/null && # git cat-file tree $tree_sha1 >/dev/null # ) and manual investigation from under "trash directory.t5500-fetch-pack/fetchall/" shows: .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. 440858748ae905d48259d4fb67a12a7aa1520cf7 HEAD f85e353c1b377970afbb804118d9135948598eea refs/heads/A 440858748ae905d48259d4fb67a12a7aa1520cf7 refs/heads/B 7f3cb539fbce926dd99233cfc9b6966f1d69747e refs/heads/C b3401427a9637a35f6a203d635e5677e76ad409d refs/heads/D 4928b093c801d36be5cdb3ed3ab572fa0d4c93bf refs/heads/E c1375be492d3716839043d7f7e9a593f8e80c668 refs/heads/F f85e353c1b377970afbb804118d9135948598eea refs/tags/A 440858748ae905d48259d4fb67a12a7aa1520cf7 refs/tags/B 7f3cb539fbce926dd99233cfc9b6966f1d69747e refs/tags/C b3401427a9637a35f6a203d635e5677e76ad409d refs/tags/D 4928b093c801d36be5cdb3ed3ab572fa0d4c93bf refs/tags/E c1375be492d3716839043d7f7e9a593f8e80c668 refs/tags/F 27f494dfb7e67d2f9cd2282404adf1d97581aa34 refs/tags/OLDTAG 10e1d7b51cacf2f0478498681177f0e6f1e8392d refs/tags/TAGA1 f85e353c1b377970afbb804118d9135948598eea refs/tags/TAGA1^{} f85e353c1b377970afbb804118d9135948598eea refs/tags/TAGA2 a540a4ddd2b16a9fe66e9539d5ec103c68052eaa refs/tags/TAGB1 9ca64d8fd8038b086badca1d11ccd8bbcfdeace1 refs/tags/TAGB1^{} 9ca64d8fd8038b086badca1d11ccd8bbcfdeace1 refs/tags/TAGB2 bc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob # <-- NOTE 038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{} 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree # <-- NOTE 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{} .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. fatal: A git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf fatal: The remote end hung up unexpectedly however the remote has all referenced objects and is generally ok: .../git/t/trash directory.t5500-fetch-pack/fetchall$ cd .. .../git/t/trash directory.t5500-fetch-pack$ git show tag-to-blob tag tag-to-blob Tagger: C O Mitter <committer@example.com> Date: Thu Apr 7 16:36:13 2005 -0700 tag -> blob hello blob .../git/t/trash directory.t5500-fetch-pack$ git show tag-to-tree tag tag-to-tree Tagger: C O Mitter <committer@example.com> Date: Thu Apr 7 16:36:13 2005 -0700 tag -> tree tree tag-to-tree file .../git/t/trash directory.t5500-fetch-pack$ git fsck Checking object directories: 100% (256/256), done. For the reference, porcelain fetch 'refs/*:refs/origin/*' works: .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch .. 'refs/*:refs/origin/*' remote: Enumerating objects: 259, done. remote: Counting objects: 100% (259/259), done. remote: Compressing objects: 100% (89/89), done. remote: Total 259 (delta 1), reused 0 (delta 0) Receiving objects: 100% (259/259), 21.64 KiB | 1.97 MiB/s, done. Resolving deltas: 100% (1/1), done. From .. * [new branch] A -> refs/origin/heads/A * [new branch] B -> refs/origin/heads/B * [new branch] C -> refs/origin/heads/C * [new branch] D -> refs/origin/heads/D * [new branch] E -> refs/origin/heads/E * [new branch] F -> refs/origin/heads/F * [new tag] A -> refs/origin/tags/A * [new tag] B -> refs/origin/tags/B * [new tag] C -> refs/origin/tags/C * [new tag] D -> refs/origin/tags/D * [new tag] E -> refs/origin/tags/E * [new tag] F -> refs/origin/tags/F * [new tag] OLDTAG -> refs/origin/tags/OLDTAG * [new tag] TAGA1 -> refs/origin/tags/TAGA1 * [new tag] TAGA2 -> refs/origin/tags/TAGA2 * [new tag] TAGB1 -> refs/origin/tags/TAGB1 * [new tag] TAGB2 -> refs/origin/tags/TAGB2 * [new tag] tag-to-blob -> refs/origin/tags/tag-to-blob * [new tag] tag-to-tree -> refs/origin/tags/tag-to-tree * [new tag] A -> A * [new tag] B -> B * [new tag] C -> C * [new tag] D -> D * [new tag] E -> E * [new tag] F -> F * [new tag] OLDTAG -> OLDTAG * [new tag] TAGA1 -> TAGA1 * [new tag] TAGA2 -> TAGA2 * [new tag] TAGB1 -> TAGB1 * [new tag] TAGB2 -> TAGB2 * [new tag] tag-to-blob -> tag-to-blob # <-- NOTE * [new tag] tag-to-tree -> tag-to-tree # <-- NOTE Signed-off-by: Kirill Smelkov <kirr@nexedi.com> --- t/t5500-fetch-pack.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index d4f435155f..000f338172 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -518,6 +518,21 @@ test_expect_success 'test --all, --depth, and explicit tag' ' ) >out-adt 2>error-adt ' +test_expect_failure 'test --all wrt tag to non-commits' ' + blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) && + git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 && + tree_sha1=$(echo -e "100644 blob $blob_sha1\tfile" | git mktree) && + git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 && + mkdir fetchall && + ( + cd fetchall && + git init && + git fetch-pack --all .. && + git cat-file blob $blob_sha1 >/dev/null && + git cat-file tree $tree_sha1 >/dev/null + ) +' + test_expect_success 'shallow fetch with tags does not break the repository' ' mkdir repo1 && ( -- 2.18.0.rc1.242.g61856ae69a.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects 2018-06-10 14:32 [PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects Kirill Smelkov @ 2018-06-11 4:20 ` Jeff King 2018-06-11 4:47 ` [PATCH] fetch-pack: don't try to fetch peeled values with --all Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2018-06-11 4:20 UTC (permalink / raw) To: Kirill Smelkov Cc: Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, git On Sun, Jun 10, 2018 at 02:32:57PM +0000, Kirill Smelkov wrote: > Added test shows remote with two tag objects pointing to a blob and a > tree. The tag objects themselves are referenced from under regular > refs/tags/* namespace. If test_expect_failure is changed to > test_expect_success the test fails: Interesting case. The problem is actually that upload-pack complains: > fatal: git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf And that sha1 is the tagged blob: > bc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob # <-- NOTE > 038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{} > 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree # <-- NOTE > 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{} So it seems like upload-pack is at fault for not marking the object as a tip when it peels the tag. > For the reference, porcelain fetch 'refs/*:refs/origin/*' works: That's because it doesn't actually issue a "want" for the peeled blob (it doesn't need to, because it's fetching the tag itself). So it happens to work, but I still think upload-pack is at fault for not accepting the "want" on the blob it advertised. Doubly interesting, it looks like this case _used_ to work, but was broken by 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09). Which only changed the fetch-pack side. It moved the handling of --all so that it was no longer in the "else" for check_refname_format(). I guess the original code was rejecting those peeled bits as "not a ref" (which makes sense). So that seems like a bug in fetch-pack. But I'm still not convinced that upload-pack doesn't also have a bug. > +test_expect_failure 'test --all wrt tag to non-commits' ' > + blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) && > + git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 && > + tree_sha1=$(echo -e "100644 blob $blob_sha1\tfile" | git mktree) && I had to switch this "echo -e" to: printf "100644 blob $blob_sha1\tfile\n" since "-e" is a bash-ism (and my /bin/sh is dash). -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] fetch-pack: don't try to fetch peeled values with --all 2018-06-11 4:20 ` Jeff King @ 2018-06-11 4:47 ` Jeff King 2018-06-11 5:28 ` Eric Sunshine 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2018-06-11 4:47 UTC (permalink / raw) To: Kirill Smelkov Cc: Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, git On Mon, Jun 11, 2018 at 12:20:16AM -0400, Jeff King wrote: > Doubly interesting, it looks like this case _used_ to work, but was > broken by 5f0fc64513 (fetch-pack: eliminate spurious error messages, > 2012-09-09). Which only changed the fetch-pack side. It moved the > handling of --all so that it was no longer in the "else" for > check_refname_format(). I guess the original code was rejecting those > peeled bits as "not a ref" (which makes sense). > > So that seems like a bug in fetch-pack. But I'm still not convinced that > upload-pack doesn't also have a bug. Here's a patch which fixes fetch-pack. I just rolled the test into the same commit; I hope that's OK. I'm somewhat on the fence regarding the upload-pack behavior. It would probably be pretty easy to fix, but since this is how it has always worked, I'm not sure if it's worth changing (and I think it is consistent in a sense -- it just means that the peeled tips we advertise are meant only as information, and not to be explicitly requested). One other funny thing I noticed about this code. For ill-formed refs, it checks that they begin with "refs/" and that they fail check_refname_format(). But I think that means I could advertise "foobar^{}" and fetch-pack would consider it a possible ref to fetch. That seems odd. I guess that's perhaps how it handles HEAD, though. I didn't dig in further. -- >8 -- Subject: fetch-pack: don't try to fetch peeled values with --all When "fetch-pack --all" sees a tag-to-blob on the remote, it tries to fetch both the tag itself ("refs/tags/foo") and the peeled value that the remote advertises ("refs/tags/foo^{}"). Asking for the object pointed to by the latter can cause upload-pack to complain with "not our ref", since it does not mark the peeled objects with the OUR_REF. Arguably upload-pack _should_ be marking those peeled objects. But it never has in the past, since clients would generally just ask for the tag and expect to get the peeled value along with it. And that's how "git fetch" works, as well as older versions of "fetch-pack --all". The problem was introduced by 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09). Before then, the matching logic was something like: if (refname is ill-formed) do nothing else if (doing --all) always consider it matched else look through list of sought refs for a match That commit wanted to flip the order of the second two arms of that conditional. But we ended up with: if (refname is ill-formed) do nothing else look through list of sought refs for a match if (--all and no match so far) always consider it matched That means tha an ill-formed ref will trigger the --all conditional block, even though we should just be ignoring it. We can fix that by having a single "else" with all of the well-formed logic, that checks the sought refs and "--all" in the correct order. Original report and test from Kirill Smelkov. Signed-off-by: Kirill Smelkov <kirr@nexedi.com> Signed-off-by: Jeff King <peff@peff.net> --- I just stuck with your same test, but thinking about it, I guess this would be a problem even for a tag-to-commit. Diff is -U15 to better show the context (in case you are wondering why it is so big ;) ). fetch-pack.c | 8 ++++---- t/t5500-fetch-pack.sh | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index a320ce9872..cc7a42fee9 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -645,35 +645,35 @@ static void filter_refs(struct fetch_pack_args *args, if (starts_with(ref->name, "refs/") && check_refname_format(ref->name, 0)) ; /* trash */ else { while (i < nr_sought) { int cmp = strcmp(ref->name, sought[i]->name); if (cmp < 0) break; /* definitely do not have it */ else if (cmp == 0) { keep = 1; /* definitely have it */ sought[i]->match_status = REF_MATCHED; } i++; } - } - if (!keep && args->fetch_all && - (!args->deepen || !starts_with(ref->name, "refs/tags/"))) - keep = 1; + if (!keep && args->fetch_all && + (!args->deepen || !starts_with(ref->name, "refs/tags/"))) + keep = 1; + } if (keep) { *newtail = ref; ref->next = NULL; newtail = &ref->next; } else { ref->next = unmatched; unmatched = ref; } } /* Append unmatched requests to the list */ for (i = 0; i < nr_sought; i++) { struct object_id oid; const char *p; diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index d4f435155f..74641e8870 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' ' test_expect_success 'test --all, --depth, and explicit head' ' ( cd client && git fetch-pack --no-progress --all --depth=1 .. refs/heads/A ) >out-adh 2>error-adh ' test_expect_success 'test --all, --depth, and explicit tag' ' git tag OLDTAG refs/heads/B~5 && ( cd client && git fetch-pack --no-progress --all --depth=1 .. refs/tags/OLDTAG ) >out-adt 2>error-adt ' +test_expect_success 'test --all wrt tag to non-commits' ' + blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) && + git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 && + tree_sha1=$(printf "100644 blob $blob_sha1\tfile\n" | git mktree) && + git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 && + mkdir fetchall && + ( + cd fetchall && + git init && + git fetch-pack --all .. && + git cat-file blob $blob_sha1 >/dev/null && + git cat-file tree $tree_sha1 >/dev/null + ) +' + test_expect_success 'shallow fetch with tags does not break the repository' ' mkdir repo1 && ( cd repo1 && git init && test_commit 1 && test_commit 2 && test_commit 3 && mkdir repo2 && cd repo2 && git init && git fetch --depth=2 ../.git master:branch && git fsck ) ' -- 2.18.0.rc1.446.g4486251e51 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: don't try to fetch peeled values with --all 2018-06-11 4:47 ` [PATCH] fetch-pack: don't try to fetch peeled values with --all Jeff King @ 2018-06-11 5:28 ` Eric Sunshine 2018-06-11 5:53 ` [PATCH v2] fetch-pack: don't try to fetch peel " Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Eric Sunshine @ 2018-06-11 5:28 UTC (permalink / raw) To: Jeff King Cc: Kirill Smelkov, Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List On Mon, Jun 11, 2018 at 12:47 AM, Jeff King <peff@peff.net> wrote: > Subject: fetch-pack: don't try to fetch peeled values with --all > [...] > Original report and test from Kirill Smelkov. > > Signed-off-by: Kirill Smelkov <kirr@nexedi.com> > Signed-off-by: Jeff King <peff@peff.net> > --- > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' ' > +test_expect_success 'test --all wrt tag to non-commits' ' > + blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) && > + git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 && > + tree_sha1=$(printf "100644 blob $blob_sha1\tfile\n" | git mktree) && Perhaps modernize these names to 'blob_oid' and 'tree_oid', or even simpler, just 'blob' and 'tree'. > + git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 && > + mkdir fetchall && > + ( > + cd fetchall && > + git init && > + git fetch-pack --all .. && Simpler: git init fetchall && ( cd fetchall && git fetch-pack --all .. && Although, I see that this script already has a mix of the two styles (simpler and not-so-simple), so... > + git cat-file blob $blob_sha1 >/dev/null && > + git cat-file tree $tree_sha1 >/dev/null > + ) > +' ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] fetch-pack: don't try to fetch peel values with --all 2018-06-11 5:28 ` Eric Sunshine @ 2018-06-11 5:53 ` Jeff King 2018-06-11 9:43 ` Kirill Smelkov 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2018-06-11 5:53 UTC (permalink / raw) To: Eric Sunshine Cc: Kirill Smelkov, Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List On Mon, Jun 11, 2018 at 01:28:23AM -0400, Eric Sunshine wrote: > On Mon, Jun 11, 2018 at 12:47 AM, Jeff King <peff@peff.net> wrote: > > Subject: fetch-pack: don't try to fetch peeled values with --all > > [...] > > Original report and test from Kirill Smelkov. > > > > Signed-off-by: Kirill Smelkov <kirr@nexedi.com> > > Signed-off-by: Jeff King <peff@peff.net> > > --- > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > > @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' ' > > +test_expect_success 'test --all wrt tag to non-commits' ' > > + blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) && > > + git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 && > > + tree_sha1=$(printf "100644 blob $blob_sha1\tfile\n" | git mktree) && > > Perhaps modernize these names to 'blob_oid' and 'tree_oid', or even > simpler, just 'blob' and 'tree'. Looking deeper, we do not need these trees and blobs at all. The problem is really just a tag that peels to an object that is not otherwise a ref tip, regardless of its type. So below is a patch that simplifies the test even further (the actual code change is the same). > > + git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 && > > + mkdir fetchall && > > + ( > > + cd fetchall && > > + git init && > > + git fetch-pack --all .. && > > Simpler: > > git init fetchall && > ( > cd fetchall && > git fetch-pack --all .. && > > Although, I see that this script already has a mix of the two styles > (simpler and not-so-simple), so... The nearby tests actually reuse the "client" directory. We can do that, too, if we simply create new objects for our test, to make sure they still need fetching. See below (we could also use "git -C" there, but the subshell matches the other tests). -- >8 -- Subject: fetch-pack: don't try to fetch peel values with --all When "fetch-pack --all" sees a tag-to-blob on the remote, it tries to fetch both the tag itself ("refs/tags/foo") and the peeled value that the remote advertises ("refs/tags/foo^{}"). Asking for the object pointed to by the latter can cause upload-pack to complain with "not our ref", since it does not mark the peeled objects with the OUR_REF (unless they were at the tip of some other ref). Arguably upload-pack _should_ be marking those peeled objects. But it never has in the past, since clients would generally just ask for the tag and expect to get the peeled value along with it. And that's how "git fetch" works, as well as older versions of "fetch-pack --all". The problem was introduced by 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09). Before then, the matching logic was something like: if (refname is ill-formed) do nothing else if (doing --all) always consider it matched else look through list of sought refs for a match That commit wanted to flip the order of the second two arms of that conditional. But we ended up with: if (refname is ill-formed) do nothing else look through list of sought refs for a match if (--all and no match so far) always consider it matched That means tha an ill-formed ref will trigger the --all conditional block, even though we should just be ignoring it. We can fix that by having a single "else" with all of the well-formed logic, that checks the sought refs and "--all" in the correct order. Reported-by: Kirill Smelkov <kirr@nexedi.com> Signed-off-by: Jeff King <peff@peff.net> --- fetch-pack.c | 8 ++++---- t/t5500-fetch-pack.sh | 10 ++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index a320ce9872..cc7a42fee9 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -657,11 +657,11 @@ static void filter_refs(struct fetch_pack_args *args, } i++; } - } - if (!keep && args->fetch_all && - (!args->deepen || !starts_with(ref->name, "refs/tags/"))) - keep = 1; + if (!keep && args->fetch_all && + (!args->deepen || !starts_with(ref->name, "refs/tags/"))) + keep = 1; + } if (keep) { *newtail = ref; diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index d4f435155f..5726f83ea3 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -518,6 +518,16 @@ test_expect_success 'test --all, --depth, and explicit tag' ' ) >out-adt 2>error-adt ' +test_expect_success 'test --all with tag to non-tip' ' + git commit --allow-empty -m non-tip && + git commit --allow-empty -m tip && + git tag -m "annotated" non-tip HEAD^ && + ( + cd client && + git fetch-pack --all .. + ) +' + test_expect_success 'shallow fetch with tags does not break the repository' ' mkdir repo1 && ( -- 2.18.0.rc1.446.g4486251e51 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all 2018-06-11 5:53 ` [PATCH v2] fetch-pack: don't try to fetch peel " Jeff King @ 2018-06-11 9:43 ` Kirill Smelkov 2018-06-12 9:48 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Kirill Smelkov @ 2018-06-11 9:43 UTC (permalink / raw) To: Jeff King Cc: Eric Sunshine, Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List Jeff, On Mon, Jun 11, 2018 at 01:53:57AM -0400, Jeff King wrote: > On Mon, Jun 11, 2018 at 01:28:23AM -0400, Eric Sunshine wrote: > > > On Mon, Jun 11, 2018 at 12:47 AM, Jeff King <peff@peff.net> wrote: > > > Subject: fetch-pack: don't try to fetch peeled values with --all > > > [...] > > > Original report and test from Kirill Smelkov. > > > > > > Signed-off-by: Kirill Smelkov <kirr@nexedi.com> > > > Signed-off-by: Jeff King <peff@peff.net> > > > --- > > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > > > @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' ' > > > +test_expect_success 'test --all wrt tag to non-commits' ' > > > + blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) && > > > + git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 && > > > + tree_sha1=$(printf "100644 blob $blob_sha1\tfile\n" | git mktree) && > > > > Perhaps modernize these names to 'blob_oid' and 'tree_oid', or even > > simpler, just 'blob' and 'tree'. > > Looking deeper, we do not need these trees and blobs at all. The problem > is really just a tag that peels to an object that is not otherwise a ref > tip, regardless of its type. Thanks for feedback and for coming up with the fix. Sure, I'm ok with moving the test into your patch. However, even if a test becomes different - narrowing down root of _current_ problem, I suggest to also keep explicitly testing tag-to-blob and tag-to-tree (and if we really also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip those now, they can potentially break in the future. I would also suggest to fix upload-pack, as it is just not consistent to reject sending objects that were advertised, and so can strike again some way in the future. After all git.git's fetch-pack is not the only git client that should be possible to interact with git.git's upload-pack on remote side, right? By the way, another problem I noticed with fetch-pack is that fetching with --all from completely empty repository also fails: .../r$ git init --bare repo.git Initialized empty Git repository in /home/kirr/tmp/trashme/r/repo.git/ .../r$ mkdir clone .../r$ cd clone/ .../r/clone$ git init Initialized empty Git repository in /home/kirr/tmp/trashme/r/clone/.git/ .../r/clone$ git fetch-pack --all ../repo.git/ fatal: no matching remote head .../r/clone$ echo $? 128 .../r/clone$ git ls-remote ../repo.git/ .../r/clone$ echo $? 0 .../r/clone$ git fetch ../repo.git/ 'refs/*:refs/repo/*' .../r/clone$ echo $? 0 I'm not sure, but I would say that `fetch-pack --all` from an empty repository should not fail and should just give empty output as fetch does. For the reference all the cases presented here are real - they appear in our repositories on lab.nexedi.com for which I maintain the backup, and I've noticed them in the process of switching git-backup from using fetch to fetch-pack here: https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436 Kirill > So below is a patch that simplifies the test even further (the actual > code change is the same). > > > > + git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 && > > > + mkdir fetchall && > > > + ( > > > + cd fetchall && > > > + git init && > > > + git fetch-pack --all .. && > > > > Simpler: > > > > git init fetchall && > > ( > > cd fetchall && > > git fetch-pack --all .. && > > > > Although, I see that this script already has a mix of the two styles > > (simpler and not-so-simple), so... > > The nearby tests actually reuse the "client" directory. We can do that, > too, if we simply create new objects for our test, to make sure they > still need fetching. See below (we could also use "git -C" there, but > the subshell matches the other tests). > > -- >8 -- > Subject: fetch-pack: don't try to fetch peel values with --all > > When "fetch-pack --all" sees a tag-to-blob on the remote, it > tries to fetch both the tag itself ("refs/tags/foo") and the > peeled value that the remote advertises ("refs/tags/foo^{}"). > Asking for the object pointed to by the latter can cause > upload-pack to complain with "not our ref", since it does > not mark the peeled objects with the OUR_REF (unless they > were at the tip of some other ref). > > Arguably upload-pack _should_ be marking those peeled > objects. But it never has in the past, since clients would > generally just ask for the tag and expect to get the peeled > value along with it. And that's how "git fetch" works, as > well as older versions of "fetch-pack --all". > > The problem was introduced by 5f0fc64513 (fetch-pack: > eliminate spurious error messages, 2012-09-09). Before then, > the matching logic was something like: > > if (refname is ill-formed) > do nothing > else if (doing --all) > always consider it matched > else > look through list of sought refs for a match > > That commit wanted to flip the order of the second two arms > of that conditional. But we ended up with: > > if (refname is ill-formed) > do nothing > else > look through list of sought refs for a match > > if (--all and no match so far) > always consider it matched > > That means tha an ill-formed ref will trigger the --all > conditional block, even though we should just be ignoring > it. We can fix that by having a single "else" with all of > the well-formed logic, that checks the sought refs and > "--all" in the correct order. > > Reported-by: Kirill Smelkov <kirr@nexedi.com> > Signed-off-by: Jeff King <peff@peff.net> > --- > fetch-pack.c | 8 ++++---- > t/t5500-fetch-pack.sh | 10 ++++++++++ > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/fetch-pack.c b/fetch-pack.c > index a320ce9872..cc7a42fee9 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -657,11 +657,11 @@ static void filter_refs(struct fetch_pack_args *args, > } > i++; > } > - } > > - if (!keep && args->fetch_all && > - (!args->deepen || !starts_with(ref->name, "refs/tags/"))) > - keep = 1; > + if (!keep && args->fetch_all && > + (!args->deepen || !starts_with(ref->name, "refs/tags/"))) > + keep = 1; > + } > > if (keep) { > *newtail = ref; > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index d4f435155f..5726f83ea3 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -518,6 +518,16 @@ test_expect_success 'test --all, --depth, and explicit tag' ' > ) >out-adt 2>error-adt > ' > > +test_expect_success 'test --all with tag to non-tip' ' > + git commit --allow-empty -m non-tip && > + git commit --allow-empty -m tip && > + git tag -m "annotated" non-tip HEAD^ && > + ( > + cd client && > + git fetch-pack --all .. > + ) > +' > + > test_expect_success 'shallow fetch with tags does not break the repository' ' > mkdir repo1 && > ( > -- > 2.18.0.rc1.446.g4486251e51 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all 2018-06-11 9:43 ` Kirill Smelkov @ 2018-06-12 9:48 ` Jeff King 2018-06-12 18:54 ` Kirill Smelkov 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2018-06-12 9:48 UTC (permalink / raw) To: Kirill Smelkov Cc: Eric Sunshine, Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote: > > Looking deeper, we do not need these trees and blobs at all. The problem > > is really just a tag that peels to an object that is not otherwise a ref > > tip, regardless of its type. > > Thanks for feedback and for coming up with the fix. Sure, I'm ok with > moving the test into your patch. However, even if a test becomes > different - narrowing down root of _current_ problem, I suggest to also > keep explicitly testing tag-to-blob and tag-to-tree (and if we really > also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip > those now, they can potentially break in the future. Yeah, I have no problem testing these cases separately. There's no bug with them now, but it is a slightly uncommon case. My suggestion would be to submit a patch that goes on top of mine that covers these cases. > I would also suggest to fix upload-pack, as it is just not consistent to > reject sending objects that were advertised, and so can strike again > some way in the future. After all git.git's fetch-pack is not the only > git client that should be possible to interact with git.git's > upload-pack on remote side, right? No, it's not the only client. At the same time, I am on the fence over whether upload-pack's behavior is wrong or not. It depends what you take a peeled advertisement line to mean. Does it mean: this object has been advertised and clients should be able to fetch it? Or does it mean: by the way, you may be interested to know the peeled value of this tag in case you want to do tag-following? So far I think it has only meant the latter. I could see an argument for the former, but any client depending on that would never have worked, AFAICT. We could _make_ it work, but how would a client know which server version it's talking to (and therefore whether it is safe to make the request?). I think you'd have to add a capability to negotiate. > I'm not sure, but I would say that `fetch-pack --all` from an empty > repository should not fail and should just give empty output as fetch > does. Yeah, that seems reasonable to me. The die() that catches this dates back to 2005-era, and we later taught the "fetch" porcelain to handle this. I don't _think_ anybody would be upset that the plumbing learned to treat this as a noop. It's probably a one-liner change in fetch_pack() to return early instead of dying. > For the reference all the cases presented here are real - they appear in > our repositories on lab.nexedi.com for which I maintain the backup, and > I've noticed them in the process of switching git-backup from using > fetch to fetch-pack here: > > https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436 I applaud you using the porcelain for your scripts, but I suspect that fetch-pack by itself is not at all well-used or well-tested these days (certainly this --all bug has been around for almost 6 years and is not very hard to trigger in practice). If an extra connection isn't a problem, you might be better off with "git ls-remote", and then picking through the results for refs of interest, and then "git fetch-pack" to actually get the pack. That's how git-fetch worked when it was a shell script (e.g., see c3a200120d, the last shell version). It may also be sane to just use "git fetch", which I'd say is _fairly_ safe to script. Of course I have no problem if you want to fix all of the corner cases in fetch-pack. Just giving you fair warning. :) -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all 2018-06-12 9:48 ` Jeff King @ 2018-06-12 18:54 ` Kirill Smelkov 2018-06-13 11:18 ` [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits Kirill Smelkov ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Kirill Smelkov @ 2018-06-12 18:54 UTC (permalink / raw) To: Jeff King Cc: Eric Sunshine, Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote: > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote: > > > > Looking deeper, we do not need these trees and blobs at all. The problem > > > is really just a tag that peels to an object that is not otherwise a ref > > > tip, regardless of its type. > > > > Thanks for feedback and for coming up with the fix. Sure, I'm ok with > > moving the test into your patch. However, even if a test becomes > > different - narrowing down root of _current_ problem, I suggest to also > > keep explicitly testing tag-to-blob and tag-to-tree (and if we really > > also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip > > those now, they can potentially break in the future. > > Yeah, I have no problem testing these cases separately. There's no bug > with them now, but it is a slightly uncommon case. My suggestion would > be to submit a patch that goes on top of mine that covers these cases. Ok, I will try to do it. > > I would also suggest to fix upload-pack, as it is just not consistent to > > reject sending objects that were advertised, and so can strike again > > some way in the future. After all git.git's fetch-pack is not the only > > git client that should be possible to interact with git.git's > > upload-pack on remote side, right? > > No, it's not the only client. At the same time, I am on the fence over > whether upload-pack's behavior is wrong or not. It depends what you take > a peeled advertisement line to mean. Does it mean: this object has been > advertised and clients should be able to fetch it? Or does it mean: by > the way, you may be interested to know the peeled value of this tag in > case you want to do tag-following? > > So far I think it has only meant the latter. I could see an argument for > the former, but any client depending on that would never have worked, > AFAICT. We could _make_ it work, but how would a client know which > server version it's talking to (and therefore whether it is safe to make > the request?). I think you'd have to add a capability to negotiate. I see. I don't know the details of the exchange, just it was surprising for outside observer that fetching what was advertised is rejected. For the reference there is no strong need for me for this to work anymore (please see below). > > I'm not sure, but I would say that `fetch-pack --all` from an empty > > repository should not fail and should just give empty output as fetch > > does. > > Yeah, that seems reasonable to me. The die() that catches this dates > back to 2005-era, and we later taught the "fetch" porcelain to handle > this. I don't _think_ anybody would be upset that the plumbing learned > to treat this as a noop. It's probably a one-liner change in > fetch_pack() to return early instead of dying. Ok, I will try to send related testcase, and it is indeed easy to find - the fix itself. > > For the reference all the cases presented here are real - they appear in > > our repositories on lab.nexedi.com for which I maintain the backup, and > > I've noticed them in the process of switching git-backup from using > > fetch to fetch-pack here: > > > > https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436 > > I applaud you using the porcelain for your scripts, but I suspect that > fetch-pack by itself is not at all well-used or well-tested these days > (certainly this --all bug has been around for almost 6 years and is not > very hard to trigger in practice). I see; thanks for the warning. > If an extra connection isn't a problem, you might be better off with > "git ls-remote", and then picking through the results for refs of > interest, and then "git fetch-pack" to actually get the pack. That's how > git-fetch worked when it was a shell script (e.g., see c3a200120d, the > last shell version). Yes, this is what I ended up doing: https://lab.nexedi.com/kirr/git-backup/commit/899103bf but for another reason - to avoid repeating for every fetched repository slow (in case of my "big" destination backup repository) quickfetch() checking in every spawned `git fetch`: git-backup can build index of objects we already have ourselves only once at startup, and then in fetch, after checking lsremote output, consult that index, and if we see we already have everything for an advertised reference - just avoid giving it to fetch-pack to process. It turns out for many pulled repositories there is usually no references changed at all and this way fetch-pack can be skipped completely: https://lab.nexedi.com/kirr/git-backup/commit/3efed898 > It may also be sane to just use "git fetch", which I'd say is _fairly_ > safe to script. Of course I have no problem if you want to fix all of > the corner cases in fetch-pack. Just giving you fair warning. :) Thanks again for the warning. I'm happy the switch to fetch plumbing happenned on my side, and so far it is working well. Like I said above I cannot use `git fetch` as is, because quickfetch() overhead for my case became dominant and very slow, taking ~ 30 seconds of the time just to check whether we have everything from one fetched repository, which, if there are 100x or 1000x of such, adds up to hours. If ls-remote has to be used anyway switching to plumbing seems natural. Let's see if I hit any more corner place or not - I will be keeping your warning in mind. Thanks again, Kirill ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits 2018-06-12 18:54 ` Kirill Smelkov @ 2018-06-13 11:18 ` Kirill Smelkov 2018-06-13 17:42 ` Junio C Hamano 2018-06-13 12:55 ` [PATCH] fetch-pack: demonstrate --all failure when remote is empty Kirill Smelkov 2018-06-13 21:13 ` [PATCH v2] fetch-pack: don't try to fetch peel values with --all Jeff King 2 siblings, 1 reply; 21+ messages in thread From: Kirill Smelkov @ 2018-06-13 11:18 UTC (permalink / raw) To: Junio C Hamano Cc: Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff King, Jeff Hostetler, Johannes Schindelin, git, Kirill Smelkov Fetch-pack --all became broken with respect to unusual tags in 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09), and was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch peel values with --all, 2018-06-11). However the test added in e9502c0a7f does not explicitly cover all funky cases. In order to be sure fetching funky tags will never break, let's explicitly test all relevant cases with 4 tag objects pointing to 1) a blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced tag objects themselves are referenced from under regular refs/tags/* namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way: .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. 440858748ae905d48259d4fb67a12a7aa1520cf7 HEAD ... bc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob # <-- NOTE 038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{} 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree # <-- NOTE 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{} .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. fatal: A git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf fatal: The remote end hung up unexpectedly Signed-off-by: Kirill Smelkov <kirr@nexedi.com> --- t/t5500-fetch-pack.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index f20bb59d22..b560d90c7b 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -528,6 +528,34 @@ test_expect_success 'test --all with tag to non-tip' ' ) ' +test_expect_success 'test --all wrt tag to non-commits' ' + blob=$(echo "hello blob" | git hash-object -t blob -w --stdin) && + git tag -a -m "tag -> blob" tag-to-blob $blob && + \ + tree=$(printf "100644 blob $blob\tfile" | git mktree) && + git tag -a -m "tag -> tree" tag-to-tree $tree && + \ + tree2=$(printf "100644 blob $blob\tfile2" | git mktree) && + commit=$(git commit-tree -m "hello commit" $tree) && + git tag -a -m "tag -> commit" tag-to-commit $commit && + \ + blob2=$(echo "hello blob2" | git hash-object -t blob -w --stdin) && + tag=$(printf "object $blob2\ntype blob\ntag tag-to-blob2\n\ +tagger author A U Thor <author@example.com> 0 +0000\n\nhello tag" | git mktag) && + git tag -a -m "tag -> tag" tag-to-tag $tag && + \ + mkdir fetchall && + ( + cd fetchall && + git init && + git fetch-pack --all .. && + git cat-file blob $blob >/dev/null && + git cat-file tree $tree >/dev/null && + git cat-file commit $commit >/dev/null && + git cat-file tag $tag >/dev/null + ) +' + test_expect_success 'shallow fetch with tags does not break the repository' ' mkdir repo1 && ( -- 2.18.0.rc1.253.gf85a566b11.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits 2018-06-13 11:18 ` [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits Kirill Smelkov @ 2018-06-13 17:42 ` Junio C Hamano 2018-06-13 18:43 ` Kirill Smelkov 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2018-06-13 17:42 UTC (permalink / raw) To: Kirill Smelkov Cc: Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff King, Jeff Hostetler, Johannes Schindelin, git Kirill Smelkov <kirr@nexedi.com> writes: > Fetch-pack --all became broken with respect to unusual tags in > 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09), > and was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch > peel values with --all, 2018-06-11). However the test added in > e9502c0a7f does not explicitly cover all funky cases. Thanks. Very much appreciated > > In order to be sure fetching funky tags will never break, let's > explicitly test all relevant cases with 4 tag objects pointing to 1) a > blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced > tag objects themselves are referenced from under regular refs/tags/* > namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way: > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. > 440858748ae905d48259d4fb67a12a7aa1520cf7 HEAD > ... > bc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob # <-- NOTE > 038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{} > 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree # <-- NOTE > 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{} > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. > fatal: A git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf > fatal: The remote end hung up unexpectedly ... except for this bit. I am not sure readers understand what these two overlong lines want to say. Would it be easier to understand if you wrote the above like this? .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. 44085874... HEAD ... bc4e9e1f... refs/tags/tag-to-blob 038f48ad... refs/tags/tag-to-blob^{} # peeled 520db1f5... refs/tags/tag-to-tree 7395c100... refs/tags/tag-to-tree^{} # peeled .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. fatal: A git upload-pack: not our ref 038f48ad... fatal: The remote end hung up unexpectedly Instead of marking the tag-to-blob and tag-to-tree entries (which are not where the 'fatal' breakage comes from), I think it makes more sense to mark the entries that show peeled version (which also matches the object name in the error message), and by shortening the line, readers can see more easily which lines are highlighted. > +test_expect_success 'test --all wrt tag to non-commits' ' > + blob=$(echo "hello blob" | git hash-object -t blob -w --stdin) && > + git tag -a -m "tag -> blob" tag-to-blob $blob && > + \ > + tree=$(printf "100644 blob $blob\tfile" | git mktree) && > + git tag -a -m "tag -> tree" tag-to-tree $tree && > + \ > + tree2=$(printf "100644 blob $blob\tfile2" | git mktree) && > + commit=$(git commit-tree -m "hello commit" $tree) && > + git tag -a -m "tag -> commit" tag-to-commit $commit && > + \ > + blob2=$(echo "hello blob2" | git hash-object -t blob -w --stdin) && > + tag=$(printf "object $blob2\ntype blob\ntag tag-to-blob2\n\ > +tagger author A U Thor <author@example.com> 0 +0000\n\nhello tag" | git mktag) && > + git tag -a -m "tag -> tag" tag-to-tag $tag && All of the above, while may not be techincallly wrong per-se, look unnecessarily complex. I guess the reason why you do the above is because you do not want to use any object that is reachable via other existing refs and instead want to ensure these objects are reachable only via the tags you are creating in this test. Otherwise using HEAD~4:test.txt and HEAD^{tree} instead of $blob and $tree constructed via hash-object and mktree would be sufficient and more readable. Oh well. > + mkdir fetchall && > + ( > + cd fetchall && > + git init && > + git fetch-pack --all .. && > + git cat-file blob $blob >/dev/null && > + git cat-file tree $tree >/dev/null && > + git cat-file commit $commit >/dev/null && > + git cat-file tag $tag >/dev/null > + ) > +' > + > test_expect_success 'shallow fetch with tags does not break the repository' ' > mkdir repo1 && > ( ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits 2018-06-13 17:42 ` Junio C Hamano @ 2018-06-13 18:43 ` Kirill Smelkov 2018-06-13 21:05 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Kirill Smelkov @ 2018-06-13 18:43 UTC (permalink / raw) To: Junio C Hamano Cc: Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff King, Jeff Hostetler, Johannes Schindelin, git On Wed, Jun 13, 2018 at 10:42:33AM -0700, Junio C Hamano wrote: > Kirill Smelkov <kirr@nexedi.com> writes: > > > Fetch-pack --all became broken with respect to unusual tags in > > 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09), > > and was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch > > peel values with --all, 2018-06-11). However the test added in > > e9502c0a7f does not explicitly cover all funky cases. > > Thanks. Very much appreciated Thanks. > > In order to be sure fetching funky tags will never break, let's > > explicitly test all relevant cases with 4 tag objects pointing to 1) a > > blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced > > tag objects themselves are referenced from under regular refs/tags/* > > namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way: > > > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. > > 440858748ae905d48259d4fb67a12a7aa1520cf7 HEAD > > ... > > bc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob # <-- NOTE > > 038f48ad0beaffbea71d186a05084b79e3870cbf refs/tags/tag-to-blob^{} > > 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree # <-- NOTE > > 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6 refs/tags/tag-to-tree^{} > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. > > fatal: A git upload-pack: not our ref 038f48ad0beaffbea71d186a05084b79e3870cbf > > fatal: The remote end hung up unexpectedly > > ... except for this bit. I am not sure readers understand what > these two overlong lines want to say. Would it be easier to > understand if you wrote the above like this? > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. > 44085874... HEAD > ... > bc4e9e1f... refs/tags/tag-to-blob > 038f48ad... refs/tags/tag-to-blob^{} # peeled > 520db1f5... refs/tags/tag-to-tree > 7395c100... refs/tags/tag-to-tree^{} # peeled > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. > fatal: A git upload-pack: not our ref 038f48ad... > fatal: The remote end hung up unexpectedly > > Instead of marking the tag-to-blob and tag-to-tree entries (which > are not where the 'fatal' breakage comes from), I think it makes > more sense to mark the entries that show peeled version (which also > matches the object name in the error message), and by shortening the > line, readers can see more easily which lines are highlighted. Makes sense. I've amended the patch description with your suggestion. > > +test_expect_success 'test --all wrt tag to non-commits' ' > > + blob=$(echo "hello blob" | git hash-object -t blob -w --stdin) && > > + git tag -a -m "tag -> blob" tag-to-blob $blob && > > + \ > > + tree=$(printf "100644 blob $blob\tfile" | git mktree) && > > + git tag -a -m "tag -> tree" tag-to-tree $tree && > > + \ > > + tree2=$(printf "100644 blob $blob\tfile2" | git mktree) && > > + commit=$(git commit-tree -m "hello commit" $tree) && > > + git tag -a -m "tag -> commit" tag-to-commit $commit && > > + \ > > + blob2=$(echo "hello blob2" | git hash-object -t blob -w --stdin) && > > + tag=$(printf "object $blob2\ntype blob\ntag tag-to-blob2\n\ > > +tagger author A U Thor <author@example.com> 0 +0000\n\nhello tag" | git mktag) && > > + git tag -a -m "tag -> tag" tag-to-tag $tag && > > All of the above, while may not be techincallly wrong per-se, look > unnecessarily complex. > > I guess the reason why you do the above is because you do not want > to use any object that is reachable via other existing refs and > instead want to ensure these objects are reachable only via the tags > you are creating in this test. Otherwise using HEAD~4:test.txt and > HEAD^{tree} instead of $blob and $tree constructed via hash-object > and mktree would be sufficient and more readable. Oh well. Yes, it is exactly the reason here. I've added corresponding comment explaining this to the test case. Kirill ---- 8< ---- From: Kirill Smelkov <kirr@nexedi.com> Date: Wed, 13 Jun 2018 12:28:21 +0300 Subject: [PATCH v2] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits Fetch-pack --all became broken with respect to unusual tags in 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09), and was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch peel values with --all, 2018-06-11). However the test added in e9502c0a7f does not explicitly cover all funky cases. In order to be sure fetching funky tags will never break, let's explicitly test all relevant cases with 4 tag objects pointing to 1) a blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced tag objects themselves are referenced from under regular refs/tags/* namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way: .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. 44085874... HEAD ... bc4e9e1f... refs/tags/tag-to-blob 038f48ad... refs/tags/tag-to-blob^{} # peeled 520db1f5... refs/tags/tag-to-tree 7395c100... refs/tags/tag-to-tree^{} # peeled .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. fatal: A git upload-pack: not our ref 038f48ad... fatal: The remote end hung up unexpectedly Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Kirill Smelkov <kirr@nexedi.com> --- t/t5500-fetch-pack.sh | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index f20bb59d22..5879ee44d7 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -528,6 +528,37 @@ test_expect_success 'test --all with tag to non-tip' ' ) ' +test_expect_success 'test --all wrt tag to non-commits' ' + # create tag-to-{blob,tree,commit,tag}, making sure all tagged objects + # are reachable only via created tag references. + blob=$(echo "hello blob" | git hash-object -t blob -w --stdin) && + git tag -a -m "tag -> blob" tag-to-blob $blob && + \ + tree=$(printf "100644 blob $blob\tfile" | git mktree) && + git tag -a -m "tag -> tree" tag-to-tree $tree && + \ + tree2=$(printf "100644 blob $blob\tfile2" | git mktree) && + commit=$(git commit-tree -m "hello commit" $tree) && + git tag -a -m "tag -> commit" tag-to-commit $commit && + \ + blob2=$(echo "hello blob2" | git hash-object -t blob -w --stdin) && + tag=$(printf "object $blob2\ntype blob\ntag tag-to-blob2\n\ +tagger author A U Thor <author@example.com> 0 +0000\n\nhello tag" | git mktag) && + git tag -a -m "tag -> tag" tag-to-tag $tag && + \ + # `fetch-pack --all` should succeed fetching all those objects. + mkdir fetchall && + ( + cd fetchall && + git init && + git fetch-pack --all .. && + git cat-file blob $blob >/dev/null && + git cat-file tree $tree >/dev/null && + git cat-file commit $commit >/dev/null && + git cat-file tag $tag >/dev/null + ) +' + test_expect_success 'shallow fetch with tags does not break the repository' ' mkdir repo1 && ( -- 2.18.0.rc1.256.g331a1db143 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits 2018-06-13 18:43 ` Kirill Smelkov @ 2018-06-13 21:05 ` Jeff King 2018-06-13 23:11 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2018-06-13 21:05 UTC (permalink / raw) To: Kirill Smelkov Cc: Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, git On Wed, Jun 13, 2018 at 06:43:04PM +0000, Kirill Smelkov wrote: > From: Kirill Smelkov <kirr@nexedi.com> > Date: Wed, 13 Jun 2018 12:28:21 +0300 > Subject: [PATCH v2] fetch-pack: test explicitly that --all can fetch tag > references pointing to non-commits > > Fetch-pack --all became broken with respect to unusual tags in > 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09), > and was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch > peel values with --all, 2018-06-11). However the test added in > e9502c0a7f does not explicitly cover all funky cases. > > In order to be sure fetching funky tags will never break, let's > explicitly test all relevant cases with 4 tag objects pointing to 1) a > blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced > tag objects themselves are referenced from under regular refs/tags/* > namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way: > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. > 44085874... HEAD > ... > bc4e9e1f... refs/tags/tag-to-blob > 038f48ad... refs/tags/tag-to-blob^{} # peeled > 520db1f5... refs/tags/tag-to-tree > 7395c100... refs/tags/tag-to-tree^{} # peeled > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. > fatal: A git upload-pack: not our ref 038f48ad... > fatal: The remote end hung up unexpectedly TBH, I do not find this snippet all that compelling. We know that e9502c0a7f already fixed the bug, and that it had nothing to do with non-commits at all. The primary reason to add these tests is that in general we do not cover fetch-pack over tags to non-commits. And I think the reason to use otherwise unreferenced objects is that it they are more likely to have detectable symptoms if they tickle a bug. So why don't we say that, instead of re-hashing output from the earlier fix? -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits 2018-06-13 21:05 ` Jeff King @ 2018-06-13 23:11 ` Jeff King 2018-06-14 5:25 ` Kirill Smelkov 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2018-06-13 23:11 UTC (permalink / raw) To: Kirill Smelkov Cc: Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, git On Wed, Jun 13, 2018 at 05:05:09PM -0400, Jeff King wrote: > > In order to be sure fetching funky tags will never break, let's > > explicitly test all relevant cases with 4 tag objects pointing to 1) a > > blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced > > tag objects themselves are referenced from under regular refs/tags/* > > namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way: > > > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. > > 44085874... HEAD > > ... > > bc4e9e1f... refs/tags/tag-to-blob > > 038f48ad... refs/tags/tag-to-blob^{} # peeled > > 520db1f5... refs/tags/tag-to-tree > > 7395c100... refs/tags/tag-to-tree^{} # peeled > > > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. > > fatal: A git upload-pack: not our ref 038f48ad... > > fatal: The remote end hung up unexpectedly > > TBH, I do not find this snippet all that compelling. We know that > e9502c0a7f already fixed the bug, and that it had nothing to do with > non-commits at all. > > The primary reason to add these tests is that in general we do not cover > fetch-pack over tags to non-commits. And I think the reason to use > otherwise unreferenced objects is that it they are more likely to have > detectable symptoms if they tickle a bug. > > So why don't we say that, instead of re-hashing output from the earlier > fix? Hmm, it looks like this already hit 'next', so it is too late to change the commit message (although 'next' will get rewound after the release, so we _could_ do it then). I also was going to suggest these style fixes, which could be applied on top (or squashed if we end up going that route). I actually wonder if the final tag one could just use two invocations of "git tag -m", but it's probably not worth spending too much time on polishing. -- >8 -- Subject: [PATCH] t5500: prettify non-commit tag tests We don't need to use backslash continuation, as the "&&" already provides continuation (and happily soaks up empty lines between commands). We can also expand the multi-line printf into a here-document, which lets us use line breaks more naturally (and avoids another continuation that required us to break the natural indentation). Signed-off-by: Jeff King <peff@peff.net> --- t/t5500-fetch-pack.sh | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index ea6570e819..3d33ab3875 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -533,19 +533,26 @@ test_expect_success 'test --all wrt tag to non-commits' ' # are reachable only via created tag references. blob=$(echo "hello blob" | git hash-object -t blob -w --stdin) && git tag -a -m "tag -> blob" tag-to-blob $blob && - \ + tree=$(printf "100644 blob $blob\tfile" | git mktree) && git tag -a -m "tag -> tree" tag-to-tree $tree && - \ + tree2=$(printf "100644 blob $blob\tfile2" | git mktree) && commit=$(git commit-tree -m "hello commit" $tree) && git tag -a -m "tag -> commit" tag-to-commit $commit && - \ + blob2=$(echo "hello blob2" | git hash-object -t blob -w --stdin) && - tag=$(printf "object $blob2\ntype blob\ntag tag-to-blob2\n\ -tagger author A U Thor <author@example.com> 0 +0000\n\nhello tag" | git mktag) && + tag=$(git mktag <<-EOF + object $blob2 + type blob + tag tag-to-blob2 + tagger author A U Thor <author@example.com> 0 +0000 + + hello tag + EOF + ) && git tag -a -m "tag -> tag" tag-to-tag $tag && - \ + # `fetch-pack --all` should succeed fetching all those objects. mkdir fetchall && ( -- 2.18.0.rc2.519.gb87ed92113 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits 2018-06-13 23:11 ` Jeff King @ 2018-06-14 5:25 ` Kirill Smelkov 2018-06-14 16:07 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Kirill Smelkov @ 2018-06-14 5:25 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, git On Wed, Jun 13, 2018 at 07:11:47PM -0400, Jeff King wrote: > On Wed, Jun 13, 2018 at 05:05:09PM -0400, Jeff King wrote: > > > > In order to be sure fetching funky tags will never break, let's > > > explicitly test all relevant cases with 4 tag objects pointing to 1) a > > > blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced > > > tag objects themselves are referenced from under regular refs/tags/* > > > namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way: > > > > > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. > > > 44085874... HEAD > > > ... > > > bc4e9e1f... refs/tags/tag-to-blob > > > 038f48ad... refs/tags/tag-to-blob^{} # peeled > > > 520db1f5... refs/tags/tag-to-tree > > > 7395c100... refs/tags/tag-to-tree^{} # peeled > > > > > > .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. > > > fatal: A git upload-pack: not our ref 038f48ad... > > > fatal: The remote end hung up unexpectedly > > > > TBH, I do not find this snippet all that compelling. We know that > > e9502c0a7f already fixed the bug, and that it had nothing to do with > > non-commits at all. > > > > The primary reason to add these tests is that in general we do not cover > > fetch-pack over tags to non-commits. And I think the reason to use > > otherwise unreferenced objects is that it they are more likely to have > > detectable symptoms if they tickle a bug. > > > > So why don't we say that, instead of re-hashing output from the earlier > > fix? > > Hmm, it looks like this already hit 'next', so it is too late to change > the commit message (although 'next' will get rewound after the release, > so we _could_ do it then). > > I also was going to suggest these style fixes, which could be applied on > top (or squashed if we end up going that route). I actually wonder if > the final tag one could just use two invocations of "git tag -m", but > it's probably not worth spending too much time on polishing. Jeff, thanks for corrections. I originally tried to look into invoking "git tag" two times, but since git tag always creates a reference it would not be semantically the same as having one referenced tag object pointing to another tag object which does not have anything in refs/ pointing to it directly. Maybe the commit text is not good but I tried to explain the reason you are talking about with "In order to be sure fetching funky tags will never break ..." Kirill > -- >8 -- > Subject: [PATCH] t5500: prettify non-commit tag tests > > We don't need to use backslash continuation, as the "&&" > already provides continuation (and happily soaks up empty > lines between commands). > > We can also expand the multi-line printf into a > here-document, which lets us use line breaks more naturally > (and avoids another continuation that required us to break > the natural indentation). > > Signed-off-by: Jeff King <peff@peff.net> > --- > t/t5500-fetch-pack.sh | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index ea6570e819..3d33ab3875 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -533,19 +533,26 @@ test_expect_success 'test --all wrt tag to non-commits' ' > # are reachable only via created tag references. > blob=$(echo "hello blob" | git hash-object -t blob -w --stdin) && > git tag -a -m "tag -> blob" tag-to-blob $blob && > - \ > + > tree=$(printf "100644 blob $blob\tfile" | git mktree) && > git tag -a -m "tag -> tree" tag-to-tree $tree && > - \ > + > tree2=$(printf "100644 blob $blob\tfile2" | git mktree) && > commit=$(git commit-tree -m "hello commit" $tree) && > git tag -a -m "tag -> commit" tag-to-commit $commit && > - \ > + > blob2=$(echo "hello blob2" | git hash-object -t blob -w --stdin) && > - tag=$(printf "object $blob2\ntype blob\ntag tag-to-blob2\n\ > -tagger author A U Thor <author@example.com> 0 +0000\n\nhello tag" | git mktag) && > + tag=$(git mktag <<-EOF > + object $blob2 > + type blob > + tag tag-to-blob2 > + tagger author A U Thor <author@example.com> 0 +0000 > + > + hello tag > + EOF > + ) && > git tag -a -m "tag -> tag" tag-to-tag $tag && > - \ > + > # `fetch-pack --all` should succeed fetching all those objects. > mkdir fetchall && > ( > -- > 2.18.0.rc2.519.gb87ed92113 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits 2018-06-14 5:25 ` Kirill Smelkov @ 2018-06-14 16:07 ` Junio C Hamano 2018-06-14 17:51 ` Kirill Smelkov 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2018-06-14 16:07 UTC (permalink / raw) To: Kirill Smelkov Cc: Jeff King, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, git Kirill Smelkov <kirr@nexedi.com> writes: > Jeff, thanks for corrections. I originally tried to look into invoking > "git tag" two times, but since git tag always creates a reference it > would not be semantically the same as having one referenced tag object > pointing to another tag object which does not have anything in refs/ > pointing to it directly. Well, then you could remove it after you are done, no? I.e. git tag -a -m "tag to commit" tag-to-commit HEAD git tag -a -m "tag to commit (1)" temp-tag HEAD~1 git tag -a -m "tag to tag" tag-to-tag temp-tag git tag -d temp-tag would make the temp-tag object reachable only via refs/tags/tag-to-tag I think. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits 2018-06-14 16:07 ` Junio C Hamano @ 2018-06-14 17:51 ` Kirill Smelkov 0 siblings, 0 replies; 21+ messages in thread From: Kirill Smelkov @ 2018-06-14 17:51 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, git On Thu, Jun 14, 2018 at 09:07:26AM -0700, Junio C Hamano wrote: > Kirill Smelkov <kirr@nexedi.com> writes: > > > Jeff, thanks for corrections. I originally tried to look into invoking > > "git tag" two times, but since git tag always creates a reference it > > would not be semantically the same as having one referenced tag object > > pointing to another tag object which does not have anything in refs/ > > pointing to it directly. > > Well, then you could remove it after you are done, no? I.e. > > git tag -a -m "tag to commit" tag-to-commit HEAD > git tag -a -m "tag to commit (1)" temp-tag HEAD~1 > git tag -a -m "tag to tag" tag-to-tag temp-tag > git tag -d temp-tag > > would make the temp-tag object reachable only via refs/tags/tag-to-tag > I think. Yes, I could remove, and I though about it originally, but to me it is less clean compared to just creating new tag object without any reference to it in the first place. Kirill ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] fetch-pack: demonstrate --all failure when remote is empty 2018-06-12 18:54 ` Kirill Smelkov 2018-06-13 11:18 ` [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits Kirill Smelkov @ 2018-06-13 12:55 ` Kirill Smelkov 2018-06-13 17:13 ` Junio C Hamano 2018-06-13 21:13 ` [PATCH v2] fetch-pack: don't try to fetch peel values with --all Jeff King 2 siblings, 1 reply; 21+ messages in thread From: Kirill Smelkov @ 2018-06-13 12:55 UTC (permalink / raw) To: Junio C Hamano, Jeff King Cc: Eric Sunshine, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List ( Junio, please pick up the patch provided in the end ) On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote: > On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote: > > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote: [...] > > > I'm not sure, but I would say that `fetch-pack --all` from an empty > > > repository should not fail and should just give empty output as fetch > > > does. > > > > Yeah, that seems reasonable to me. The die() that catches this dates > > back to 2005-era, and we later taught the "fetch" porcelain to handle > > this. I don't _think_ anybody would be upset that the plumbing learned > > to treat this as a noop. It's probably a one-liner change in > > fetch_pack() to return early instead of dying. > > Ok, I will try to send related testcase, and it is indeed easy to find > - the fix itself. I started doing it in full with the following --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1581,6 +1581,8 @@ struct ref *fetch_pack(struct fetch_pack_args *args, if (!ref) { packet_flush(fd[1]); + if (nr_sought == 0) // XXX or better args->fetch_all + return NULL; /* nothing to fetch */ die(_("no matching remote head")); } prepare_shallow_info(&si, shallow); but then came to the fact that !ref fetch_pack() return is analyzed in 2 places: - in builtin/fetch-pack.c itself: ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought, &shallow, pack_lockfile_ptr, protocol_v0); ... ret = !ref; - and in transport.c in fetch_refs_via_pack(): case protocol_v1: case protocol_v0: refs = fetch_pack(&args, data->fd, data->conn, refs_tmp ? refs_tmp : transport->remote_refs, dest, to_fetch, nr_heads, &data->shallow, &transport->pack_lockfile, data->version); break; ... if (refs == NULL) ret = -1; As I don't know git codebase well-enough I don't see offhand how to distinguish empty result from a real error when something was requested and not fetched. If it would be only builtin/fetch-pack I could start to play ugly games with analyzing too in the calling site args.fetch_all and nr_though and if at that level we also know we requested nothing, don't treat !refs as an error. However with transport.c being there too, since I'm no longer using `fetch-pack --all`, now it is best for me to not delve into this story and just stop with attached patch. Thanks, Kirill ---- 8< ---- From 76d80ffcfd4574715545c62413d64d40af063d09 Mon Sep 17 00:00:00 2001 From: Kirill Smelkov <kirr@nexedi.com> Date: Wed, 13 Jun 2018 15:46:00 +0300 Subject: [PATCH] fetch-pack: demonstrate --all failure when remote is empty Currently `fetch-pack --all` from an empty repository gives: fatal: no matching remote head However it would be logical for this fetch operation to succeed with empty result. Add test showing the failure. Signed-off-by: Kirill Smelkov <kirr@nexedi.com> --- t/t5500-fetch-pack.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 82aee1c2d8..2234bad411 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -528,6 +528,14 @@ test_expect_success 'test --all with tag to non-tip' ' ) ' +test_expect_failure 'test --all wrt empty.git' ' + git init --bare empty.git && + ( + cd client && + git fetch-pack --all ../empty.git + ) +' + test_expect_success 'shallow fetch with tags does not break the repository' ' mkdir repo1 && ( -- 2.18.0.rc1.253.gf85a566b11.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: demonstrate --all failure when remote is empty 2018-06-13 12:55 ` [PATCH] fetch-pack: demonstrate --all failure when remote is empty Kirill Smelkov @ 2018-06-13 17:13 ` Junio C Hamano 2018-06-13 18:21 ` Kirill Smelkov 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2018-06-13 17:13 UTC (permalink / raw) To: Kirill Smelkov Cc: Jeff King, Eric Sunshine, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List Kirill Smelkov <kirr@nexedi.com> writes: > ( Junio, please pick up the patch provided in the end ) > > On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote: >> On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote: >> > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote: > [...] > >> > > I'm not sure, but I would say that `fetch-pack --all` from an empty >> > > repository should not fail and should just give empty output as fetch >> > > does. >> > >> > Yeah, that seems reasonable to me. The die() that catches this dates >> > back to 2005-era, and we later taught the "fetch" porcelain to handle >> > this. I don't _think_ anybody would be upset that the plumbing learned >> > to treat this as a noop. It's probably a one-liner change in >> > fetch_pack() to return early instead of dying. I actually have a slight preference to the current "attempting to fetch from a total emptiness is so rare that it is worth grabbing attention of whoever does so" behaviour, to be honest. Oh, wait, is this specific to "fetch-pack" and the behaviour of end-user-facing "git fetch" is kept same as before? If then, I'd be somewhat sympathetic to the cause---it would be more convenient for the calling Porcelain script if this turned into a silent noop (even though it would probably make it harder to diagnose when such a Porcelain is set up incorrectly e.g. pointing at an empty repository that is not the one the Porcelain writer intended to fetch from). > However with transport.c being there too, since I'm no longer using > `fetch-pack --all`, now it is best for me to not delve into this story > and just stop with attached patch. If we do not plan to change the behaviour later ourselves, I do not think it makes sense, nor it is fair to those future developers who inherit this project, to declare that the established behaviour is wrong with an 'expect-failure' test like this, to be honest. > +test_expect_failure 'test --all wrt empty.git' ' > + git init --bare empty.git && > + ( > + cd client && > + git fetch-pack --all ../empty.git > + ) > +' ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] fetch-pack: demonstrate --all failure when remote is empty 2018-06-13 17:13 ` Junio C Hamano @ 2018-06-13 18:21 ` Kirill Smelkov 0 siblings, 0 replies; 21+ messages in thread From: Kirill Smelkov @ 2018-06-13 18:21 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Eric Sunshine, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List On Wed, Jun 13, 2018 at 10:13:07AM -0700, Junio C Hamano wrote: > Kirill Smelkov <kirr@nexedi.com> writes: > > > ( Junio, please pick up the patch provided in the end ) > > > > On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote: > >> On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote: > >> > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote: > > [...] > > > >> > > I'm not sure, but I would say that `fetch-pack --all` from an empty > >> > > repository should not fail and should just give empty output as fetch > >> > > does. > >> > > >> > Yeah, that seems reasonable to me. The die() that catches this dates > >> > back to 2005-era, and we later taught the "fetch" porcelain to handle > >> > this. I don't _think_ anybody would be upset that the plumbing learned > >> > to treat this as a noop. It's probably a one-liner change in > >> > fetch_pack() to return early instead of dying. > > I actually have a slight preference to the current "attempting to > fetch from a total emptiness is so rare that it is worth grabbing > attention of whoever does so" behaviour, to be honest. I see. > Oh, wait, is this specific to "fetch-pack" and the behaviour of > end-user-facing "git fetch" is kept same as before? If then, I'd be > somewhat sympathetic to the cause---it would be more convenient for > the calling Porcelain script if this turned into a silent noop (even > though it would probably make it harder to diagnose when such a > Porcelain is set up incorrectly e.g. pointing at an empty repository > that is not the one the Porcelain writer intended to fetch from). Yes, it is only for fetch-pack, and behaviour of porcelain fetch is kept as it was before. > > However with transport.c being there too, since I'm no longer using > > `fetch-pack --all`, now it is best for me to not delve into this story > > and just stop with attached patch. > > If we do not plan to change the behaviour later ourselves, I do not > think it makes sense, nor it is fair to those future developers who > inherit this project, to declare that the established behaviour is > wrong with an 'expect-failure' test like this, to be honest. I see. Let's please cancel this patch then. > > +test_expect_failure 'test --all wrt empty.git' ' > > + git init --bare empty.git && > > + ( > > + cd client && > > + git fetch-pack --all ../empty.git > > + ) > > +' ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all 2018-06-12 18:54 ` Kirill Smelkov 2018-06-13 11:18 ` [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits Kirill Smelkov 2018-06-13 12:55 ` [PATCH] fetch-pack: demonstrate --all failure when remote is empty Kirill Smelkov @ 2018-06-13 21:13 ` Jeff King 2018-06-14 5:29 ` Kirill Smelkov 2 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2018-06-13 21:13 UTC (permalink / raw) To: Kirill Smelkov Cc: Eric Sunshine, Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote: > > If an extra connection isn't a problem, you might be better off with > > "git ls-remote", and then picking through the results for refs of > > interest, and then "git fetch-pack" to actually get the pack. That's how > > git-fetch worked when it was a shell script (e.g., see c3a200120d, the > > last shell version). > > Yes, this is what I ended up doing: > > https://lab.nexedi.com/kirr/git-backup/commit/899103bf > > but for another reason - to avoid repeating for every fetched repository > slow (in case of my "big" destination backup repository) quickfetch() > checking in every spawned `git fetch`: git-backup can build index of > objects we already have ourselves only once at startup, and then in > fetch, after checking lsremote output, consult that index, and if we see > we already have everything for an advertised reference - just avoid > giving it to fetch-pack to process. It turns out for many pulled > repositories there is usually no references changed at all and this way > fetch-pack can be skipped completely: > > https://lab.nexedi.com/kirr/git-backup/commit/3efed898 Thanks for sharing that, it's an interesting case. I'd hope that git-fetch is smart enough not to even bother with quickfetch() if there are no refs to update. But if we have even one change to fetch, then yeah, in the general case it makes sense to me that you could do better by amortizing the scan of local objects across many operations. -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all 2018-06-13 21:13 ` [PATCH v2] fetch-pack: don't try to fetch peel values with --all Jeff King @ 2018-06-14 5:29 ` Kirill Smelkov 0 siblings, 0 replies; 21+ messages in thread From: Kirill Smelkov @ 2018-06-14 5:29 UTC (permalink / raw) To: Jeff King Cc: Eric Sunshine, Junio C Hamano, Jonathan Tan, Brandon Williams, Takuto Ikuta, Jeff Hostetler, Johannes Schindelin, Michael Haggerty, Git List On Wed, Jun 13, 2018 at 05:13:02PM -0400, Jeff King wrote: > On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote: > > > > If an extra connection isn't a problem, you might be better off with > > > "git ls-remote", and then picking through the results for refs of > > > interest, and then "git fetch-pack" to actually get the pack. That's how > > > git-fetch worked when it was a shell script (e.g., see c3a200120d, the > > > last shell version). > > > > Yes, this is what I ended up doing: > > > > https://lab.nexedi.com/kirr/git-backup/commit/899103bf > > > > but for another reason - to avoid repeating for every fetched repository > > slow (in case of my "big" destination backup repository) quickfetch() > > checking in every spawned `git fetch`: git-backup can build index of > > objects we already have ourselves only once at startup, and then in > > fetch, after checking lsremote output, consult that index, and if we see > > we already have everything for an advertised reference - just avoid > > giving it to fetch-pack to process. It turns out for many pulled > > repositories there is usually no references changed at all and this way > > fetch-pack can be skipped completely: > > > > https://lab.nexedi.com/kirr/git-backup/commit/3efed898 > > Thanks for sharing that, it's an interesting case. I'd hope that > git-fetch is smart enough not to even bother with quickfetch() if there > are no refs to update. But if we have even one change to fetch, then > yeah, in the general case it makes sense to me that you could do better > by amortizing the scan of local objects across many operations. Thanks for feedback. For the reference in case of git-backup `git fetch` or `git fetch-pack` would have to always do quickfetch scan or equivalent, because in case of backup repo there is only one reference in it - its master - and references of backed up repositories do not have anything representing them in backup.git/refs/ . Kirill ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2018-06-14 18:06 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-10 14:32 [PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects Kirill Smelkov 2018-06-11 4:20 ` Jeff King 2018-06-11 4:47 ` [PATCH] fetch-pack: don't try to fetch peeled values with --all Jeff King 2018-06-11 5:28 ` Eric Sunshine 2018-06-11 5:53 ` [PATCH v2] fetch-pack: don't try to fetch peel " Jeff King 2018-06-11 9:43 ` Kirill Smelkov 2018-06-12 9:48 ` Jeff King 2018-06-12 18:54 ` Kirill Smelkov 2018-06-13 11:18 ` [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits Kirill Smelkov 2018-06-13 17:42 ` Junio C Hamano 2018-06-13 18:43 ` Kirill Smelkov 2018-06-13 21:05 ` Jeff King 2018-06-13 23:11 ` Jeff King 2018-06-14 5:25 ` Kirill Smelkov 2018-06-14 16:07 ` Junio C Hamano 2018-06-14 17:51 ` Kirill Smelkov 2018-06-13 12:55 ` [PATCH] fetch-pack: demonstrate --all failure when remote is empty Kirill Smelkov 2018-06-13 17:13 ` Junio C Hamano 2018-06-13 18:21 ` Kirill Smelkov 2018-06-13 21:13 ` [PATCH v2] fetch-pack: don't try to fetch peel values with --all Jeff King 2018-06-14 5:29 ` Kirill Smelkov
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).