Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits
On Thu, Jun 14, 2018 at 09:07:26AM -0700, Junio C Hamano wrote: > Kirill Smelkov 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
Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits
Kirill Smelkov 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
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
> ---
> 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 0 +\n\nhello tag" | git
> mktag) &&
> + tag=$(git mktag <<-EOF
> + object $blob2
> + type blob
> + tag tag-to-blob2
> + tagger author A U Thor 0 +
> +
> + 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
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
---
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 0 +\n\nhello tag" | git mktag)
&&
+ tag=$(git mktag <<-EOF
+ object $blob2
+ type blob
+ tag tag-to-blob2
+ tagger author A U Thor 0 +
+
+ 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
On Wed, Jun 13, 2018 at 06:43:04PM +, Kirill Smelkov wrote:
> From: Kirill Smelkov
> 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
On Wed, Jun 13, 2018 at 10:42:33AM -0700, Junio C Hamano wrote:
> Kirill Smelkov 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
> > ..
> > 440858748ae905d48259d4fb67a12a7aa1520cf7HEAD
> > ...
> > 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 0 +\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
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
Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits
Kirill Smelkov 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 ..
> 440858748ae905d48259d4fb67a12a7aa1520cf7HEAD
> ...
> bc4e9e1fa80662b449805b1ac29fc9b1e4c49187refs/tags/tag-to-blob
> # <-- NOTE
> 038f48ad0beaffbea71d186a05084b79e3870cbf
> refs/tags/tag-to-blob^{}
> 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1refs/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 0 +\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 &&
> (
