[PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects

[PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects

[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

Re: [PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects

[Jeff King]

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

[PATCH] fetch-pack: don't try to fetch peeled values with --all

[Jeff King]

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

Re: [PATCH] fetch-pack: don't try to fetch peeled values with --all

[Eric Sunshine]

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
> +       )
> +'

[PATCH v2] fetch-pack: don't try to fetch peel values with --all

[Jeff King]

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

Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all

[Kirill Smelkov]

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

Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all

[Jeff King]

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

Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all

[Kirill Smelkov]

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

[PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[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

Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[Junio C Hamano]

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 &&
>  	(

Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[Kirill Smelkov]

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

Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[Jeff King]

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

Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[Jeff King]

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

Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[Kirill Smelkov]

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

Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[Junio C Hamano]

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.

Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

[Kirill Smelkov]

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

[PATCH] fetch-pack: demonstrate --all failure when remote is empty

[Kirill Smelkov]

( 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

Re: [PATCH] fetch-pack: demonstrate --all failure when remote is empty

[Junio C Hamano]

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
> +	)
> +'

Re: [PATCH] fetch-pack: demonstrate --all failure when remote is empty

[Kirill Smelkov]

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
> > +	)
> > +'

Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all

[Jeff King]

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

Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all

[Kirill Smelkov]

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