threads / patch / 41028

patchrevision.c: propagate tag names from pending array

Subject: [PATCH] revision.c: propagate tag names from pending array

## tl;dr

3 messages between Dec 17, 2015 and Dec 17, 2015. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Jeff King· Dec 17, 2015, 06:47 UTC · lore

When we unwrap a tag to find its commit for a traversal, we do not propagate the "name" field of the tag in the pending array (i.e., the ref name the user gave us in the first place) to the commit (instead, we use an empty string). This means that "git log --source" will never show the tag-name for commits we reach through it.

This was broken in 2073949 (traverse_commit_list: support pending blobs/trees with paths, 2014-10-15). That commit tried to be careful and avoid propagating the path information for a tag (which would be nonsensical) to trees and blobs. But it should not have cut off the "name" field, which should carry forward to children.

Note that this does mean that the "name" field will carry forward to blobs and trees, too. Whereas prior to 2073949, we always gave them an empty string. This is the right thing to do, but in practice no callers probably use it (since now we have an explicit separate "path" field, which was the point of 2073949).

We add tests here not only for the broken case, but also a basic sanity test of "log --source" in general, which did not have any coverage in the test suite.

Reported-by: Raymundo <gypark@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
This was reported several weeks ago, but I needed to take the time to
convince myself this wasn't regressing any cases. I'm pretty sure it's
the right thing to do.

The regression is in v2.2.0, so this is not urgent to make it into v2.7 before release, but it is definitely maint-worthy.

 revision.c     |  3 +--
 t/t4202-log.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)
Show changes to 2 files +30 −2

revision.c, t/t4202-log.sh

diff --git a/revision.c b/revision.c
index 9404a05..0a282f5 100644
--- a/revision.c
+++ b/revision.c
@@ -294,9 +294,8 @@ static struct commit *handle_commit(struct rev_info *revs,
 		/*
 		 * We'll handle the tagged object by looping or dropping
 		 * through to the non-tag handlers below. Do not
-		 * propagate data from the tag's pending entry.
+		 * propagate path data from the tag's pending entry.
 		 */
-		name = "";
 		path = NULL;
 		mode = 0;
 	}
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 6ede069..a0f80af 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -908,4 +908,33 @@ test_expect_success 'log diagnoses bogus HEAD' '
 	test_i18ngrep broken stderr
 '
 
+test_expect_success 'set up --source tests' '
+	git checkout --orphan source-a &&
+	test_commit one &&
+	test_commit two &&
+	git checkout -b source-b HEAD^ &&
+	test_commit three
+'
+
+test_expect_success 'log --source paints branch names' '
+	cat >expect <<-\EOF &&
+	09e12a9	source-b three
+	8e393e1	source-a two
+	1ac6c77	source-b one
+	EOF
+	git log --oneline --source source-a source-b >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --source paints tag names' '
+	git tag -m tagged source-tag &&
+	cat >expect <<-\EOF &&
+	09e12a9	source-tag three
+	8e393e1	source-a two
+	1ac6c77	source-tag one
+	EOF
+	git log --oneline --source source-tag source-a >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.7.0.rc1.339.gca82f22
Junio C Hamano· Dec 17, 2015, 20:28 UTC · re: Jeff King · lore

Re: [PATCH] revision.c: propagate tag names from pending array

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> When we unwrap a tag to find its commit for a traversal, we
> do not propagate the "name" field of the tag in the pending
> array (i.e., the ref name the user gave us in the first
> place) to the commit (instead, we use an empty string). This
> means that "git log --source" will never show the tag-name
> for commits we reach through it.
>
> This was broken in 2073949 (traverse_commit_list: support
> pending blobs/trees with paths, 2014-10-15). That commit
> tried to be careful and avoid propagating the path
> information for a tag (which would be nonsensical) to trees
> and blobs. But it should not have cut off the "name" field,
> which should carry forward to children.
> ...
> This was reported several weeks ago, but I needed to take the time to
> convince myself this wasn't regressing any cases. I'm pretty sure it's
> the right thing to do.
>
> The regression is in v2.2.0, so this is not urgent to make it into v2.7
> before release, but it is definitely maint-worthy.
Makes sense, and I agree.
By the way, a totally unrelated niggle I have with 2073949 is this.
    $ git describe --contains 2073949
    v2.3.1~3^2~4
while as you said, this dates back to at least v2.2.0-rc0
    $ git tag --contains 2073949
    v2.2.0
    v2.2.0-rc0
    ...
    v2.7.0-rc1

That "describe --contains" output comes from "name-rev --tags", and I need to force it to use v2.2.0-rc0 as the source of naming, i.e.

    $ git name-rev --refs=refs/tags/v2.2.0-rc0 2073949
    2073949 tags/v2.2.0-rc0~13^2~9
to get what I would expect to be more useful.

I know "name-rev --contains" wants to describe a commit based on an anchor point that is topologically closest, and even though I do not offhand think of any, I am sure there are valid use cases that want to see the current behaviour. But from time to time, I wish it did its naming taking the topological age of the anchor points into account. If a commit is contained in v2.2.0-rc0 and onward, even though v2.0.0-rc0~13^2~9 describes a longer path from v2.0.0-rc0 than v2.3.1~3^2~4 is from v2.3.1, I often want to see the name based on the "oldest" tag (if such a thing exists, and for older commits in this project, it always is the case, I think).

Jeff King· Dec 17, 2015, 22:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] revision.c: propagate tag names from pending array

On Thu, Dec 17, 2015 at 12:28:48PM -0800, Junio C Hamano wrote:
Show 31 quoted lines
> By the way, a totally unrelated niggle I have with 2073949 is this.
> 
>     $ git describe --contains 2073949
>     v2.3.1~3^2~4
> 
> while as you said, this dates back to at least v2.2.0-rc0
> 
>     $ git tag --contains 2073949
>     v2.2.0
>     v2.2.0-rc0
>     ...
>     v2.7.0-rc1
> 
> That "describe --contains" output comes from "name-rev --tags", and
> I need to force it to use v2.2.0-rc0 as the source of naming, i.e.
> 
>     $ git name-rev --refs=refs/tags/v2.2.0-rc0 2073949
>     2073949 tags/v2.2.0-rc0~13^2~9
> 
> to get what I would expect to be more useful.
> 
> I know "name-rev --contains" wants to describe a commit based on an
> anchor point that is topologically closest, and even though I do not
> offhand think of any, I am sure there are valid use cases that want
> to see the current behaviour.  But from time to time, I wish it did
> its naming taking the topological age of the anchor points into
> account.  If a commit is contained in v2.2.0-rc0 and onward, even
> though v2.0.0-rc0~13^2~9 describes a longer path from v2.0.0-rc0
> than v2.3.1~3^2~4 is from v2.3.1, I often want to see the name based
> on the "oldest" tag (if such a thing exists, and for older commits
> in this project, it always is the case, I think).

Yes, I agree (and judging by the number of "git describe is confusing" threads over the years, I think it is not just us). I rarely use "git describe --contains" myself. What I typically use (and how I arrived at v2.2.0 in this instance) is:

  git tag --contains "$@" |
  grep ^v |
  grep -v -- -rc |
  sort -V |
  head -1
But there are some git-specific assumptions in there.
-Peff

← back to recent threads