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

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

89 more lines

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

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.

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

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)

36 more lines

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

On Mon, Jun 11, 2018 at 12:47 AM, Jeff King [off-list ref] wrote:

Perhaps modernize these names to 'blob_oid' and 'tree_oid', or even
simpler, just 'blob' and 'tree'.

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...

On Mon, Jun 11, 2018 at 01:28:23AM -0400, Eric Sunshine wrote:

On Mon, Jun 11, 2018 at 12:47 AM, Jeff King [off-list ref] wrote:

quoted

Subject: fetch-pack: don't try to fetch peeled values with --all
[...]
Original report and test from Kirill Smelkov.

Signed-off-by: Kirill Smelkov <redacted>
Signed-off-by: Jeff King <redacted>
---

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).

quoted

+       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

18 more lines

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

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:

quoted

On Mon, Jun 11, 2018 at 12:47 AM, Jeff King [off-list ref] wrote:

quoted

Subject: fetch-pack: don't try to fetch peeled values with --all
[...]
Original report and test from Kirill Smelkov.

Signed-off-by: Kirill Smelkov <redacted>
Signed-off-by: Jeff King <redacted>
---

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

9 more lines

So below is a patch that simplifies the test even further (the actual
code change is the same).

quoted

quoted

+       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

80 more quoted lines

On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:

quoted

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

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:

quoted

quoted

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.

quoted

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).

quoted

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.

quoted

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

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 <redacted>
---
 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

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

quoted

On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:

[...]

quoted

quoted

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,

21 more lines

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

On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:

quoted

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

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:

quoted

quoted

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