{"thread":{"id":"35633","subject":"git-log --cherry-pick gives different results when using tag or tag^{}","startedAt":"2014-01-10T13:15:40Z","lastAt":"2014-01-15T22:41:21Z","messageCount":9,"participants":["Francis Moreau","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"232996","messageId":"52CFF27C.1090108@gmail.com","threadId":"35633","inReplyTo":null,"subject":"git-log --cherry-pick gives different results when using tag or tag^{}","fromName":"Francis Moreau","fromEmail":"francis.moro@gmail.com","sentAt":"2014-01-10T13:15:40Z","receivedAt":"2014-01-10T13:15:40Z","isPatch":false,"sender":{"key":"francis.moro@gmail.com","avatar":null},"body":"Hello,\n\nIn mykernel repository, I'm having 2 different behaviours with git-log\nbut I don't understand why:\n\nDoing:\n\n    $ git log --oneline --cherry-pick --left-right v3.4.71-1^{}...next\n\nand\n\n    $ git log --oneline --cherry-pick --left-right v3.4.71-1...next\n\ngive something different (where v3.4.71-1 is a tag).\n\nThe command using ^{} looks the one that gives correct result I think.\n\nCould anybody enlight me ?\n\nThanks.\n"},{"id":"233138","messageId":"20140115094945.GD14335@sigill.intra.peff.net","threadId":"35633","inReplyTo":"52CFF27C.1090108@gmail.com","subject":"Re: git-log --cherry-pick gives different results when using tag or tag^{}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T09:49:45Z","receivedAt":"2014-01-15T09:49:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Junio, as the bug blames to him]\n\nOn Fri, Jan 10, 2014 at 02:15:40PM +0100, Francis Moreau wrote:\n\n> In mykernel repository, I'm having 2 different behaviours with git-log\n> but I don't understand why:\n> \n> Doing:\n> \n>     $ git log --oneline --cherry-pick --left-right v3.4.71-1^{}...next\n> \n> and\n> \n>     $ git log --oneline --cherry-pick --left-right v3.4.71-1...next\n> \n> give something different (where v3.4.71-1 is a tag).\n> \n> The command using ^{} looks the one that gives correct result I think.\n\nYeah, this looks like a bug. Here's a simple reproduction recipe:\n\n  commit() {\n    echo content >$1 &&\n    git add $1 &&\n    git commit -m $1\n  }\n\n  git init repo && cd repo &&\n  commit one &&\n  commit two &&\n  sleep 1 &&\n  git tag -m foo mytag &&\n  git checkout -b side HEAD^ &&\n  git cherry-pick mytag &&\n  commit three\n\nThe sleep seems to be necessary, to give the commit and its\ncherry-picked version different commit times (presumably because it\nimpacts the order in which we visit them during the traversal).\n\nRunning:\n\n  git log --oneline --decorate --cherry-pick --left-right mytag^{}...HEAD\n\nproduces the expected:\n\n  > e36cc32 (HEAD, side) three\n\nbut running it with the tag, as:\n\n  git log --oneline --decorate --cherry-pick --left-right mytag...HEAD\n\nyields:\n\n  > e36cc32 (HEAD, side) three\n  > 5e96f7d two\n  > db92fca (tag: mytag, master) two\n\nNot only do we get the cherry-pick wrong (we should omit both \"twos\"),\nbut we seem to erroneously count the tagged \"two\" as being on the\nright-hand side, which it clearly is not (and which is probably why we\ndon't find the match via --cherry-pick).\n\nThis worked in v1.8.4, but is broken in v1.8.5. It bisects to Junio's\n895c5ba (revision: do not peel tags used in range notation, 2013-09-19),\nwhich sounds promising.\n\nI think what is happening is that we used to apply the SYMMETRIC_LEFT\nflag directly to the commit. Now we apply it to the tag, and it does not\nseem to get propagated. The patch below fixes it for me, but I have no\nidea if we actually need to be setting the other flags, or just\nSYMMETRIC_LEFT. I also wonder if the non-symmetric two-dot case needs to\naccess any pointed-to commit and propagate flags in a similar way.\n\ndiff --git a/revision.c b/revision.c\nindex 7010aff..1d99bfc 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1197,6 +1197,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\tfree_commit_list(exclude);\n \n \t\t\t\ta_flags = flags | SYMMETRIC_LEFT;\n+\t\t\t\ta->object.flags |= a_flags;\n+\t\t\t\tb->object.flags |= flags;\n \t\t\t}\n \n \t\t\ta_obj->flags |= a_flags;\n\n-Peff\n"},{"id":"233139","messageId":"52D65C19.2000209@gmail.com","threadId":"35633","inReplyTo":"20140115094945.GD14335@sigill.intra.peff.net","subject":"Re: git-log --cherry-pick gives different results when using tag or tag^{}","fromName":"Francis Moreau","fromEmail":"francis.moro@gmail.com","sentAt":"2014-01-15T09:59:53Z","receivedAt":"2014-01-15T09:59:53Z","isPatch":false,"sender":{"key":"francis.moro@gmail.com","avatar":null},"body":"On 01/15/2014 10:49 AM, Jeff King wrote:\n> [+cc Junio, as the bug blames to him]\n> \n> On Fri, Jan 10, 2014 at 02:15:40PM +0100, Francis Moreau wrote:\n> \n>> In mykernel repository, I'm having 2 different behaviours with git-log\n>> but I don't understand why:\n>>\n>> Doing:\n>>\n>>     $ git log --oneline --cherry-pick --left-right v3.4.71-1^{}...next\n>>\n>> and\n>>\n>>     $ git log --oneline --cherry-pick --left-right v3.4.71-1...next\n>>\n>> give something different (where v3.4.71-1 is a tag).\n>>\n>> The command using ^{} looks the one that gives correct result I think.\n> \n> Yeah, this looks like a bug. Here's a simple reproduction recipe:\n\nThanks a lot Jeff for your good analyze.\n"},{"id":"233166","messageId":"xmqq1u092f2k.fsf@gitster.dls.corp.google.com","threadId":"35633","inReplyTo":"20140115094945.GD14335@sigill.intra.peff.net","subject":"Re: git-log --cherry-pick gives different results when using tag or tag^{}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-15T19:57:39Z","receivedAt":"2014-01-15T19:57:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> [+cc Junio, as the bug blames to him]\n> ...\n> I think what is happening is that we used to apply the SYMMETRIC_LEFT\n> flag directly to the commit. Now we apply it to the tag, and it does not\n> seem to get propagated. The patch below fixes it for me, but I have no\n> idea if we actually need to be setting the other flags, or just\n> SYMMETRIC_LEFT. I also wonder if the non-symmetric two-dot case needs to\n> access any pointed-to commit and propagate flags in a similar way.\n\nThanks.\n\nWhere do we pass down other flags from tags to commits?  For\nexample, if we do this:\n\n\t$ git log ^v1.8.5 master\n\nwe mark v1.8.5 tag as UNINTERESTING, and throw that tag (not commit\nv1.8.5^0) into revs->pending.objects[].  We do the same for 'master',\nwhich is a commit.\n\nLater, in prepare_revision_walk(), we call handle_commit() on them,\nand unwrap the tag v1.8.5 to get v1.8.5^0, and then handles that\ncommit object with flags obtained from the tag object.  This code\nonly cares about UNINTERESTING and manually propagates it.\n\nPerhaps that code needs to propagate at least SYMMETRIC_LEFT down to\nthe commit object as well, no?  With your patch, the topmost level\nof tag object and the eventual commit object are marked with the\nflag, but if we were dealing with a tag that points at another tag\nthat in turn points at a commit, the intermediate tag will not be\nmarked with SYMMETRIC_LEFT (nor UNINTERESTING for that matter),\nwhich may not affect the final outcome, but it somewhat feels wrong.\n\nHow about doing it this way instead (totally untested, though)?\n\n revision.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex a68fde6..def070e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -276,6 +276,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\t\treturn NULL;\n \t\t\tdie(\"bad object %s\", sha1_to_hex(tag->tagged->sha1));\n \t\t}\n+\t\tobject->flags |= flags;\n \t}\n \n \t/*\n@@ -287,7 +288,6 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\tif (parse_commit(commit) < 0)\n \t\t\tdie(\"unable to parse commit %s\", name);\n \t\tif (flags & UNINTERESTING) {\n-\t\t\tcommit->object.flags |= UNINTERESTING;\n \t\t\tmark_parents_uninteresting(commit);\n \t\t\trevs->limited = 1;\n \t\t}\n"},{"id":"233167","messageId":"xmqqwqi10z6i.fsf_-_@gitster.dls.corp.google.com","threadId":"35633","inReplyTo":"xmqq1u092f2k.fsf@gitster.dls.corp.google.com","subject":"revision: propagate flag bits from tags to pointees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-15T20:26:13Z","receivedAt":"2014-01-15T20:26:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"With the previous fix 895c5ba3 (revision: do not peel tags used in\nrange notation, 2013-09-19), handle_revision_arg() that processes\ncommand line arguments for the \"git log\" family of commands no\nlonger directly places the object pointed by the tag in the pending\nobject array when it sees a tag object.  We used to place pointee\nthere after copying the flag bits like UNINTERESTING and\nSYMMETRIC_LEFT.\n\nThis change meant that any flag that is relevant to later history\ntraversal must now be propagated to the pointed objects (most often\nthese are commits) while starting the traversal, which is partly\ndone by handle_commit() that is called from prepare_revision_walk().\nWe did propagate UNINTERESTING, but did not do so for others, most\nnotably SYMMETRIC_LEFT.  This caused \"git log --left-right v1.0...\"\n(where \"v1.0\" is a tag) to start losing the \"leftness\" from the\ncommit the tag points at.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Comes directly on top of the faulty commit, so that we could\n   backport it to 1.8.4.x series.\n\n revision.c               |  2 +-\n t/t6000-rev-list-misc.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 7010aff..6d1c8f9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -265,6 +265,7 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\t\t\treturn NULL;\n \t\t\tdie(\"bad object %s\", sha1_to_hex(tag->tagged->sha1));\n \t\t}\n+\t\tobject->flags |= flags;\n \t}\n \n \t/*\n@@ -276,7 +277,6 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\tif (parse_commit(commit) < 0)\n \t\t\tdie(\"unable to parse commit %s\", name);\n \t\tif (flags & UNINTERESTING) {\n-\t\t\tcommit->object.flags |= UNINTERESTING;\n \t\t\tmark_parents_uninteresting(commit);\n \t\t\trevs->limited = 1;\n \t\t}\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 15e3d64..b84d6b0 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -56,4 +56,15 @@ test_expect_success 'rev-list A..B and rev-list ^A B are the same' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'symleft flag bit is propagated down from tag' '\n+\tgit log --format=\"%m %s\" --left-right v1.0...master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t> two\n+\t> one\n+\t< another\n+\t< that\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"233173","messageId":"20140115215343.GA16401@sigill.intra.peff.net","threadId":"35633","inReplyTo":"xmqq1u092f2k.fsf@gitster.dls.corp.google.com","subject":"Re: git-log --cherry-pick gives different results when using tag or tag^{}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T21:53:43Z","receivedAt":"2014-01-15T21:53:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2014 at 11:57:39AM -0800, Junio C Hamano wrote:\n\n> Where do we pass down other flags from tags to commits?  For\n> example, if we do this:\n> \n> \t$ git log ^v1.8.5 master\n> \n> we mark v1.8.5 tag as UNINTERESTING, and throw that tag (not commit\n> v1.8.5^0) into revs->pending.objects[].  We do the same for 'master',\n> which is a commit.\n> \n> Later, in prepare_revision_walk(), we call handle_commit() on them,\n> and unwrap the tag v1.8.5 to get v1.8.5^0, and then handles that\n> commit object with flags obtained from the tag object.  This code\n> only cares about UNINTERESTING and manually propagates it.\n\nThanks for picking up this line of thought. I had some notion that the\nright solution would be in propagating the flags later from the pending\ntags to the commits, but I didn't quite know where to look. Knowing that\nwe explicitly propagate UNINTERESTING but nothing else makes what I was\nseeing make a lot more sense.\n\n> Perhaps that code needs to propagate at least SYMMETRIC_LEFT down to\n> the commit object as well, no?  With your patch, the topmost level\n> of tag object and the eventual commit object are marked with the\n> flag, but if we were dealing with a tag that points at another tag\n> that in turn points at a commit, the intermediate tag will not be\n> marked with SYMMETRIC_LEFT (nor UNINTERESTING for that matter),\n> which may not affect the final outcome, but it somewhat feels wrong.\n\nAgreed. I think the lack of flags on intermediate tags has always been\nthat way, even before 895c5ba, and I do not know of any case where it\ncurrently matters. But it seems like the obvious right thing to mark\nthose intermediate tags.\n\n> How about doing it this way instead (totally untested, though)?\n\nMakes sense. It also means we will propagate flags down to any\npointed-to trees and blobs. I can't think of a case where that will\nmatter either (and they cannot be SYMMETRIC_LEFT, as that only makes\nsense for commit objects).\n\nI do notice that when we have a tree, we explicitly propagate\nUNINTERESTING to the rest of the tree. Should we be propagating all\nflags instead? Again, I can't think of a reason to do so (and if it is\nnot UNINTERESTING, it is a non-trivial amount of time to mark all paths\nin the tree).\n\n\n> @@ -287,7 +288,6 @@ static struct commit *handle_commit(struct rev_info *revs,\n>  \t\tif (parse_commit(commit) < 0)\n>  \t\t\tdie(\"unable to parse commit %s\", name);\n>  \t\tif (flags & UNINTERESTING) {\n> -\t\t\tcommit->object.flags |= UNINTERESTING;\n>  \t\t\tmark_parents_uninteresting(commit);\n>  \t\t\trevs->limited = 1;\n>  \t\t}\n\nWe don't need to propagate the UNINTERESTING flag here, because either:\n\n  - \"object\" pointed to the commit, in which case flags comes from\n    object->flags, and we already have it set\n\nor\n\n  - \"object\" was a tag, and we propagated the flags as we peeled (from\n    your earlier hunk)\n\nMakes sense. I think the \"mark_blob_uninteresting\" call later in the\nfunction is now irrelevant for the same reasons. The\nmark_tree_uninteresting call is not, though, because it recurses.\n\n-Peff\n"},{"id":"233174","messageId":"20140115215641.GB16401@sigill.intra.peff.net","threadId":"35633","inReplyTo":"xmqqwqi10z6i.fsf_-_@gitster.dls.corp.google.com","subject":"Re: revision: propagate flag bits from tags to pointees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T21:56:41Z","receivedAt":"2014-01-15T21:56:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2014 at 12:26:13PM -0800, Junio C Hamano wrote:\n\n> With the previous fix 895c5ba3 (revision: do not peel tags used in\n> range notation, 2013-09-19), handle_revision_arg() that processes\n> command line arguments for the \"git log\" family of commands no\n> longer directly places the object pointed by the tag in the pending\n> object array when it sees a tag object.  We used to place pointee\n> there after copying the flag bits like UNINTERESTING and\n> SYMMETRIC_LEFT.\n> \n> This change meant that any flag that is relevant to later history\n> traversal must now be propagated to the pointed objects (most often\n> these are commits) while starting the traversal, which is partly\n> done by handle_commit() that is called from prepare_revision_walk().\n> We did propagate UNINTERESTING, but did not do so for others, most\n> notably SYMMETRIC_LEFT.  This caused \"git log --left-right v1.0...\"\n> (where \"v1.0\" is a tag) to start losing the \"leftness\" from the\n> commit the tag points at.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n\nLooks good to me. As per my previous mail, I _think_ you could squash\nin:\n\ndiff --git a/revision.c b/revision.c\nindex f786b51..2db906c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -316,13 +316,10 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t * Blob object? You know the drill by now..\n \t */\n \tif (object->type == OBJ_BLOB) {\n-\t\tstruct blob *blob = (struct blob *)object;\n \t\tif (!revs->blob_objects)\n \t\t\treturn NULL;\n-\t\tif (flags & UNINTERESTING) {\n-\t\t\tmark_blob_uninteresting(blob);\n+\t\tif (flags & UNINTERESTING)\n \t\t\treturn NULL;\n-\t\t}\n \t\tadd_pending_object(revs, object, \"\");\n \t\treturn NULL;\n \t}\n\nbut that is not very much code reduction (and mark_blob_uninteresting is\nvery cheap). So it may not be worth the risk that my analysis is wrong.\n:)\n\n-Peff\n"},{"id":"233175","messageId":"xmqqk3e0288d.fsf@gitster.dls.corp.google.com","threadId":"35633","inReplyTo":"20140115215641.GB16401@sigill.intra.peff.net","subject":"Re: revision: propagate flag bits from tags to pointees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-15T22:25:22Z","receivedAt":"2014-01-15T22:25:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Looks good to me. As per my previous mail, I _think_ you could squash\n> in:\n>\n> diff --git a/revision.c b/revision.c\n> index f786b51..2db906c 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -316,13 +316,10 @@ static struct commit *handle_commit(struct rev_info *revs,\n>  \t * Blob object? You know the drill by now..\n>  \t */\n>  \tif (object->type == OBJ_BLOB) {\n> -\t\tstruct blob *blob = (struct blob *)object;\n>  \t\tif (!revs->blob_objects)\n>  \t\t\treturn NULL;\n> -\t\tif (flags & UNINTERESTING) {\n> -\t\t\tmark_blob_uninteresting(blob);\n> +\t\tif (flags & UNINTERESTING)\n>  \t\t\treturn NULL;\n> -\t\t}\n>  \t\tadd_pending_object(revs, object, \"\");\n>  \t\treturn NULL;\n>  \t}\n>\n> but that is not very much code reduction (and mark_blob_uninteresting is\n> very cheap). So it may not be worth the risk that my analysis is wrong.\n> :)\n\nYour analysis is correct, but I think the pros-and-cons of the your\nsquashable change boils down to the choice between:\n\n - leaving it in will keep similarity between tree and blob\n   codepaths (both have mark_X_uninteresting(); and\n\n - reducing cycles by taking advantage of the explicit knowledge\n   that mark_X_uninteresting() recurses for a tree while it does not\n   for a blob.\n\nBut I have a suspicion that my patch may break if any codepath looks\nat the current flag on the object and decides \"ah, it already is\nmarked\" and punts.\n\nIt indeed looks like mark_tree_uninteresting() does have that\nproperty.  When an uninteresting tag directly points at a tree, if\nwe propagate the UNINTERESTING bit to the pointee while peeling,\nwouldn't we end up calling mark_tree_uninteresting() on a tree,\nwhose flags already have UNINTERESTING bit set, causing it not to\nrecurse?\n"},{"id":"233176","messageId":"xmqqfvoo27hq.fsf@gitster.dls.corp.google.com","threadId":"35633","inReplyTo":"xmqqk3e0288d.fsf@gitster.dls.corp.google.com","subject":"Re: revision: propagate flag bits from tags to pointees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-15T22:41:21Z","receivedAt":"2014-01-15T22:41:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> But I have a suspicion that my patch may break if any codepath looks\n> at the current flag on the object and decides \"ah, it already is\n> marked\" and punts.\n>\n> It indeed looks like mark_tree_uninteresting() does have that\n> property.  When an uninteresting tag directly points at a tree, if\n> we propagate the UNINTERESTING bit to the pointee while peeling,\n> wouldn't we end up calling mark_tree_uninteresting() on a tree,\n> whose flags already have UNINTERESTING bit set, causing it not to\n> recurse?\n\nExtending that line of thought further, what should this do?\n\n    git rev-list --objects ^HEAD^^{tree} HEAD^{tree} |\n    git pack-object --stdin pack\n\nIt says \"I am interested in the objects that is used in the tree of\nHEAD, but I do not need those that already appear in HEAD^\".\n\nWith the current code (with or without the fix under discussion, or\neven without the faulty \"do not peel tags used in range notation\"),\nthe tree of the HEAD^ is marked in handle_revision_arg() as\nUNINTERESTING when it is placed in revs->pending.objects[], and the\nhandle_commit() --- we should rename it to handle_pending_object()\nor something, by the way --- will call mark_tree_uninteresting() on\nthat tree, which then would say \"It is already uninteresting\" and\nreturn without marking the objects common to these two trees\nuninteresting, no?\n\nI think that is a related but separate bug that dates back to\nprehistoric times, and the asymmetry between handle_commit() deals\nwith commits and trees should have been a clear clue that tells us\nsomething is fishy.  It calls \"mark PARENTS uninteresting\", leaving\nthe responsibility of marking the commit itself to the caller, but\nit calls mark_tree_uninteresting() whose caller is not supposed to\nmark the tree itself.\n\nWhich suggest me that a right fix for this separate bug would be to\nintroduce mark_tree_contents_uninteresting() or something, which has\nthe parallel semantics to mark_parents_uninteresting().  Then\nmark_blob_uninteresting() call in the function can clearly go.  Such\na change will make it clear that handle_commit() is responsible for\nhandling the flags for the given object, and any helper functions\ncalled by it should not peek and stop the flag of the object itself\nwhen deciding to recurse into the objects linked to it.\n"}]}