git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Kirill Smelkov <kirr@nexedi.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Brandon Williams <bmwill@google.com>,
	Takuto Ikuta <tikuta@chromium.org>,
	Jeff Hostetler <jeffhost@microsoft.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Michael Haggerty <mhagger@alum.mit.edu>,
	git@vger.kernel.org
Subject: [PATCH] fetch-pack: don't try to fetch peeled values with --all
Date: Mon, 11 Jun 2018 00:47:11 -0400
Message-ID: <20180611044710.GB31642@sigill.intra.peff.net> (raw)
In-Reply-To: <20180611042016.GA31642@sigill.intra.peff.net>

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


  reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-10 14:32 [PATCH] fetch-pack: demonstrate --all breakage when remote have tags to non-commit objects Kirill Smelkov
2018-06-11  4:20 ` Jeff King
2018-06-11  4:47   ` Jeff King [this message]
2018-06-11  5:28     ` [PATCH] fetch-pack: don't try to fetch peeled values with --all Eric Sunshine
2018-06-11  5:53       ` [PATCH v2] fetch-pack: don't try to fetch peel " Jeff King
2018-06-11  9:43         ` Kirill Smelkov
2018-06-12  9:48           ` Jeff King
2018-06-12 18:54             ` Kirill Smelkov
2018-06-13 11:18               ` [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits Kirill Smelkov
2018-06-13 17:42                 ` Junio C Hamano
2018-06-13 18:43                   ` Kirill Smelkov
2018-06-13 21:05                     ` Jeff King
2018-06-13 23:11                       ` Jeff King
2018-06-14  5:25                         ` Kirill Smelkov
2018-06-14 16:07                           ` Junio C Hamano
2018-06-14 17:51                             ` Kirill Smelkov
2018-06-13 12:55               ` [PATCH] fetch-pack: demonstrate --all failure when remote is empty Kirill Smelkov
2018-06-13 17:13                 ` Junio C Hamano
2018-06-13 18:21                   ` Kirill Smelkov
2018-06-13 21:13               ` [PATCH v2] fetch-pack: don't try to fetch peel values with --all Jeff King
2018-06-14  5:29                 ` Kirill Smelkov

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180611044710.GB31642@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=jonathantanmy@google.com \
    --cc=kirr@nexedi.com \
    --cc=mhagger@alum.mit.edu \
    --cc=tikuta@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox