{"thread":{"id":"41028","subject":"[PATCH] revision.c: propagate tag names from pending array","startedAt":"2015-12-17T06:47:07Z","lastAt":"2015-12-17T22:14:59Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"274647","messageId":"20151217064706.GA3531@sigill.intra.peff.net","threadId":"41028","inReplyTo":null,"subject":"[PATCH] revision.c: propagate tag names from pending array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-17T06:47:07Z","receivedAt":"2015-12-17T06:47:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we unwrap a tag to find its commit for a traversal, we\ndo not propagate the \"name\" field of the tag in the pending\narray (i.e., the ref name the user gave us in the first\nplace) to the commit (instead, we use an empty string). This\nmeans that \"git log --source\" will never show the tag-name\nfor commits we reach through it.\n\nThis was broken in 2073949 (traverse_commit_list: support\npending blobs/trees with paths, 2014-10-15). That commit\ntried to be careful and avoid propagating the path\ninformation for a tag (which would be nonsensical) to trees\nand blobs. But it should not have cut off the \"name\" field,\nwhich should carry forward to children.\n\nNote that this does mean that the \"name\" field will carry\nforward to blobs and trees, too. Whereas prior to 2073949,\nwe always gave them an empty string. This is the right thing\nto do, but in practice no callers probably use it (since now\nwe have an explicit separate \"path\" field, which was the\npoint of 2073949).\n\nWe add tests here not only for the broken case, but also a\nbasic sanity test of \"log --source\" in general, which did\nnot have any coverage in the test suite.\n\nReported-by: Raymundo <gypark@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis was reported several weeks ago, but I needed to take the time to\nconvince myself this wasn't regressing any cases. I'm pretty sure it's\nthe right thing to do.\n\nThe regression is in v2.2.0, so this is not urgent to make it into v2.7\nbefore release, but it is definitely maint-worthy.\n\n revision.c     |  3 +--\n t/t4202-log.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 9404a05..0a282f5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -294,9 +294,8 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t/*\n \t\t * We'll handle the tagged object by looping or dropping\n \t\t * through to the non-tag handlers below. Do not\n-\t\t * propagate data from the tag's pending entry.\n+\t\t * propagate path data from the tag's pending entry.\n \t\t */\n-\t\tname = \"\";\n \t\tpath = NULL;\n \t\tmode = 0;\n \t}\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 6ede069..a0f80af 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -908,4 +908,33 @@ test_expect_success 'log diagnoses bogus HEAD' '\n \ttest_i18ngrep broken stderr\n '\n \n+test_expect_success 'set up --source tests' '\n+\tgit checkout --orphan source-a &&\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\tgit checkout -b source-b HEAD^ &&\n+\ttest_commit three\n+'\n+\n+test_expect_success 'log --source paints branch names' '\n+\tcat >expect <<-\\EOF &&\n+\t09e12a9\tsource-b three\n+\t8e393e1\tsource-a two\n+\t1ac6c77\tsource-b one\n+\tEOF\n+\tgit log --oneline --source source-a source-b >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log --source paints tag names' '\n+\tgit tag -m tagged source-tag &&\n+\tcat >expect <<-\\EOF &&\n+\t09e12a9\tsource-tag three\n+\t8e393e1\tsource-a two\n+\t1ac6c77\tsource-tag one\n+\tEOF\n+\tgit log --oneline --source source-tag source-a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.7.0.rc1.339.gca82f22\n"},{"id":"274688","messageId":"xmqqoadobz0f.fsf@gitster.mtv.corp.google.com","threadId":"41028","inReplyTo":"20151217064706.GA3531@sigill.intra.peff.net","subject":"Re: [PATCH] revision.c: propagate tag names from pending array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-17T20:28:48Z","receivedAt":"2015-12-17T20:28:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When we unwrap a tag to find its commit for a traversal, we\n> do not propagate the \"name\" field of the tag in the pending\n> array (i.e., the ref name the user gave us in the first\n> place) to the commit (instead, we use an empty string). This\n> means that \"git log --source\" will never show the tag-name\n> for commits we reach through it.\n>\n> This was broken in 2073949 (traverse_commit_list: support\n> pending blobs/trees with paths, 2014-10-15). That commit\n> tried to be careful and avoid propagating the path\n> information for a tag (which would be nonsensical) to trees\n> and blobs. But it should not have cut off the \"name\" field,\n> which should carry forward to children.\n> ...\n> This was reported several weeks ago, but I needed to take the time to\n> convince myself this wasn't regressing any cases. I'm pretty sure it's\n> the right thing to do.\n>\n> The regression is in v2.2.0, so this is not urgent to make it into v2.7\n> before release, but it is definitely maint-worthy.\n\nMakes sense, and I agree.\n\nBy the way, a totally unrelated niggle I have with 2073949 is this.\n\n    $ git describe --contains 2073949\n    v2.3.1~3^2~4\n\nwhile as you said, this dates back to at least v2.2.0-rc0\n\n    $ git tag --contains 2073949\n    v2.2.0\n    v2.2.0-rc0\n    ...\n    v2.7.0-rc1\n\nThat \"describe --contains\" output comes from \"name-rev --tags\", and\nI need to force it to use v2.2.0-rc0 as the source of naming, i.e.\n\n    $ git name-rev --refs=refs/tags/v2.2.0-rc0 2073949\n    2073949 tags/v2.2.0-rc0~13^2~9\n\nto get what I would expect to be more useful.\n\nI know \"name-rev --contains\" wants to describe a commit based on an\nanchor point that is topologically closest, and even though I do not\noffhand think of any, I am sure there are valid use cases that want\nto see the current behaviour.  But from time to time, I wish it did\nits naming taking the topological age of the anchor points into\naccount.  If a commit is contained in v2.2.0-rc0 and onward, even\nthough v2.0.0-rc0~13^2~9 describes a longer path from v2.0.0-rc0\nthan v2.3.1~3^2~4 is from v2.3.1, I often want to see the name based\non the \"oldest\" tag (if such a thing exists, and for older commits\nin this project, it always is the case, I think).\n"},{"id":"274697","messageId":"20151217221459.GB8150@sigill.intra.peff.net","threadId":"41028","inReplyTo":"xmqqoadobz0f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] revision.c: propagate tag names from pending array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-17T22:14:59Z","receivedAt":"2015-12-17T22:14:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 17, 2015 at 12:28:48PM -0800, Junio C Hamano wrote:\n\n> By the way, a totally unrelated niggle I have with 2073949 is this.\n> \n>     $ git describe --contains 2073949\n>     v2.3.1~3^2~4\n> \n> while as you said, this dates back to at least v2.2.0-rc0\n> \n>     $ git tag --contains 2073949\n>     v2.2.0\n>     v2.2.0-rc0\n>     ...\n>     v2.7.0-rc1\n> \n> That \"describe --contains\" output comes from \"name-rev --tags\", and\n> I need to force it to use v2.2.0-rc0 as the source of naming, i.e.\n> \n>     $ git name-rev --refs=refs/tags/v2.2.0-rc0 2073949\n>     2073949 tags/v2.2.0-rc0~13^2~9\n> \n> to get what I would expect to be more useful.\n> \n> I know \"name-rev --contains\" wants to describe a commit based on an\n> anchor point that is topologically closest, and even though I do not\n> offhand think of any, I am sure there are valid use cases that want\n> to see the current behaviour.  But from time to time, I wish it did\n> its naming taking the topological age of the anchor points into\n> account.  If a commit is contained in v2.2.0-rc0 and onward, even\n> though v2.0.0-rc0~13^2~9 describes a longer path from v2.0.0-rc0\n> than v2.3.1~3^2~4 is from v2.3.1, I often want to see the name based\n> on the \"oldest\" tag (if such a thing exists, and for older commits\n> in this project, it always is the case, I think).\n\nYes, I agree (and judging by the number of \"git describe is confusing\"\nthreads over the years, I think it is not just us). I rarely use \"git\ndescribe --contains\" myself. What I typically use (and how I arrived at\nv2.2.0 in this instance) is:\n\n  git tag --contains \"$@\" |\n  grep ^v |\n  grep -v -- -rc |\n  sort -V |\n  head -1\n\nBut there are some git-specific assumptions in there.\n\n-Peff\n"}]}