{"thread":{"id":"34900","subject":"breakage in revision traversal with pathspec","startedAt":"2013-09-10T17:19:20Z","lastAt":"2013-09-25T09:12:59Z","messageCount":14,"participants":["Junio C Hamano","Kevin Bracey","Jonathan Nieder","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"227347","messageId":"xmqqy574y4pz.fsf@gitster.dls.corp.google.com","threadId":"34900","inReplyTo":null,"subject":"breakage in revision traversal with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-10T17:19:20Z","receivedAt":"2013-09-10T17:19:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I am grumpy X-<.\n\nIt appears that we introduced a large breakage during 1.8.4 cycle to\nthe revision traversal machinery and made pathspec-limited \"git log\"\npretty much useless.\n\nThis command\n\n $ git log v1.8.3.1..v1.8.4 -- git-cvsserver.perl\n\nreports that a merge 766f0f8ef7 (which did not touch the specified\npath at all) touches it.\n\nBisecting points at d0af663e (revision.c: Make --full-history\nconsider more merges, 2013-05-16).\n"},{"id":"227383","messageId":"522F8ED2.9000408@bracey.fi","threadId":"34900","inReplyTo":"xmqqy574y4pz.fsf@gitster.dls.corp.google.com","subject":"Re: breakage in revision traversal with pathspec","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-09-10T21:27:46Z","receivedAt":"2013-09-10T21:27:46Z","isPatch":false,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 10/09/2013 20:19, Junio C Hamano wrote:\n> I am grumpy X-<.\n>\n> It appears that we introduced a large breakage during 1.8.4 cycle to\n> the revision traversal machinery and made pathspec-limited \"git log\"\n> pretty much useless.\n>\n> This command\n>\n> $ git log v1.8.3.1..v1.8.4 -- git-cvsserver.perl\n>\n> reports that a merge 766f0f8ef7 (which did not touch the specified\n> path at all) touches it.\n>\n> Bisecting points at d0af663e (revision.c: Make --full-history\n> consider more merges, 2013-05-16).\n>\nThat merge appearing *with* --full-history would seem like correct \nbehaviour to me. Or at least it's what I intended.\n\nThat merge *did* touch that path - that file differs between the two \nparents, and it resolved on one of them.\n\nThis goes back to my original motivation for the change - the ability to \nfind merges that resolved in unexpected ways - I want \"--full-history\" \nto show every merge where the end result is not identical to every parent.\n\nThis does mean that \"--full-history\" will show more merges than it used \nto for a pathspec - it will show merges in from topic branches which \ndidn't touch that pathspec, but where the mainline did change it.\n\nThese extra merges can be pared back by \"--simplify-merges\", which will \ngenerally eliminate any irrelevant topic branches, although not for \ntopic branches that are rooted older than your bottom commit, like in \nthis example.\n\n\nHowever, your particular example occurs *without*--full-history, which \nsuggests a problem.\n\nThat merge is right near the bottom of the range. Its first parent is \nv1.8.3. It has an incoming pre-1.8.3 topic branch \n(fc/transport-helper-error-reporting) with an old version of \ngit-cvsserver.perl (which the merge correctly didn't take). Display of \nthat sort of bottom-of-range merge is a problem area I did try to \naddress - it was problematic in older Git versions, but that problem was \npartially concealed by the overly-permissive \"hide any merge identical \nto any one parent\" logic, and became more exposed by my \"show more merges\".\n\nI'm pretty certain non-full \"git log v1.8.3..v1.8.4\" shouldn't show that \nmerge. And indeed it doesn't. Yay! At this point the various \"follow \nfirst parent if identical\", \"prioritise on-graph treesame\" and \"treat \nbottom commits as on-graph\" rules are working. That merge is identical \nto v1.8.3, its first parent, and we've expressed an interest in v1.8.3, \nso that's treated as on-graph, so it doesn't get shown.\n\n(Oddly, \"gitk v1.8.3..v1.8.4\" fails and shows the merge. It seems gitk \nfails here because of the annotated tag: \"gitk v1.8.3^0..v1.8.4\" \ncorrectly shows nothing. So one apparent \"failure to peel\" bug \nsomewhere. Can't seem to provoke this with git log.)\n\n\"git log v1.8.3.1..v1.8.4\" on the other hand I'm not so sure about. The \n\"follow first treesame parent\" logic doesn't kick in because the merge's \nonly treesame parent (v1.8.3) is off-graph. The merge is not treesame to \nits only on-graph parent (fc/transport-helper-error-reporting), so that \n\"default following\" rule doesn't activate.\n\nI'm going to have to think a bit. \"git log (--ancestry-path?) \nfc/transport-helper-error-reporting..v1.8.4\" should definitely show that \nmerge - we want to see how we got from the version of the file on the \nspecified topic branch to the different version in v1.8.4.\n\nMaybe if the first-treesame-parent rule was reapplied again later after \nrewriting? After we've pruned away the topic branch by rewriting because \nits commits don't touch the pathspec, then our merge is left with two \noff-graph rewritten parents. At which point maybe it would be reasonable \nfor the default log to reapply the \"follow first treesame parent\" rule \nif all remaining parents are off-graph.\n\nDoes that make sense? Going to have to think harder.\n\nI note that \"gitk v1.8.3^0..v1.8.4\" and \"git log --parents \nv1.8.3..v1.8.4\" show that merge in Git 1.8.3, but not in Git 1.8.4. So \nwe're going partially forwards, at least.\n\nKevin\n"},{"id":"227380","messageId":"xmqq38pcwc21.fsf@gitster.dls.corp.google.com","threadId":"34900","inReplyTo":"522F8ED2.9000408@bracey.fi","subject":"Re: breakage in revision traversal with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-10T22:23:50Z","receivedAt":"2013-09-10T22:23:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> On 10/09/2013 20:19, Junio C Hamano wrote:\n>> I am grumpy X-<.\n>>\n>> It appears that we introduced a large breakage during 1.8.4 cycle to\n>> the revision traversal machinery and made pathspec-limited \"git log\"\n>> pretty much useless.\n>>\n>> This command\n>>\n>> $ git log v1.8.3.1..v1.8.4 -- git-cvsserver.perl\n>>\n>> reports that a merge 766f0f8ef7 (which did not touch the specified\n>> path at all) touches it.\n>>\n>> Bisecting points at d0af663e (revision.c: Make --full-history\n>> consider more merges, 2013-05-16).\n>>\n> That merge appearing *with* --full-history would seem like correct\n> behaviour to me. Or at least it's what I intended.\n\nOh, of course. \"--full-history\" is about showing any pointless\nchange, \"the mainline was a lot more up-to-date and there were\nchanges relative to a fork based on an older baseline\", so your\nupdated \"log\" should show that in the mainline git-cvsserver.perl\nhas been more fresh when that merge happened. But it shouldn't\nappear if the user does not ask for \"--full-history\".\n\n> However, your particular example occurs *without*--full-history, which\n> suggests a problem.\n\nYes.\n\n> I note that \"gitk v1.8.3^0..v1.8.4\" and \"git log --parents\n> v1.8.3..v1.8.4\" show that merge in Git 1.8.3, but not in Git 1.8.4. So\n> we're going partially forwards, at least.\n\nWith the testcases demonstrating the cases your series fixed that\nall look sensible, I think it is not really an option for us to\nrevert them; you do not have to defend it with \"we are going\npartially forwards\" ;-).\n"},{"id":"227470","messageId":"5230AD23.2050009@bracey.fi","threadId":"34900","inReplyTo":"xmqq38pcwc21.fsf@gitster.dls.corp.google.com","subject":"Re: breakage in revision traversal with pathspec","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-09-11T17:49:23Z","receivedAt":"2013-09-11T17:49:23Z","isPatch":false,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 11/09/2013 01:23, Junio C Hamano wrote:\n> Kevin Bracey writes:\n>\n>> On 10/09/2013 20:19, Junio C Hamano wrote:\n>>> This command\n>>>\n>>> $ git log v1.8.3.1..v1.8.4 -- git-cvsserver.perl\n>>>\n>>> reports that a merge 766f0f8ef7 (which did not touch the specified\n>>> path at all) touches it.\n>>>\n>>> Bisecting points at d0af663e (revision.c: Make --full-history\n>>> consider more merges, 2013-05-16).\n>>>\n>> That merge appearing *with* --full-history would seem like correct\n>> behaviour to me. Or at least it's what I intended.\n> ... But it shouldn't\n> appear if the user does not ask for \"--full-history\".\n\nWell, there is a functioning semi-work-around for now: avoid difficult \nnon-linear questions like \"v1.8.3.1..v1.8.4\". A question like \n\"v1.8.3..v1.8.4\" is a lot easier to visualise, and it does already omit \nthe merge.\n\nOn reflection I'm not sure what we should for the \"simple history\" view \nof v1.8.3.1..v1.8.4. We're not rewriting parents, so we don't get a \nchance to reconsider the merge as being zero-parent, and we do have this \nlittle section of graph to traverse at the bottom:\n\n 1.8.3\n o----x----x----x----x---x--- (x = included, o = \nexcluded, *=!treesame)\n /\n /*\n o--x--x--x--x\n\nIn effect, we do have a linear section of history to follow, and the \nfile does change in the middle of that line. It may be quite hard to \ncome up with a solid rule to hide the merge that doesn't go wrong \nsomewhere else.\n\nThe current rules for this are\n\n1) if identical to any on-graph parent, follow that one, and rewrite the \nmerge as a non-merge. We currently do not follow to an identical \noff-graph parent. This long-standing comment in try_to_simplify_commit \napplies: \"Even if a merge with an uninteresting side branch brought the \nentire change we are interested in, we do not want to lose the other \nbranches of this merge, so we just keep going.\" For this query, the \nmainline link to 1.8.3 is the \"uninteresting side branch\"! If you do \nspecify v1.8.3..v1.8.4, then v1.8.3 becomes \"on-graph\" thanks to other \nnew rules, and this rule does kick in, hiding the merge.\n\n2) If rule 1 doesn't activate, and it remains as a merge, hide it if \ntreesame to all on-graph parents. Previously this rule was \"hide if \ntreesame to any parent\", and so that would have hidden the merge.\n\nNow, when I changed rule 2, I did not think this would affect the \ndefault log. See my commit message:\n\n \"Now redefine a commit's TREESAME flag to be true only if a commit is\n TREESAME to _all_ of its [later: on-graph] parent. This doesn't \naffect ... the default\n simplify_history behaviour (because partially TREESAME merges are \nturned\n into normal commits)...\"\n\nWhoops - partially TREESAME merges are not always turned into normal \ncommits.\n\nMaybe the fix is to define TREESAME differently for simplify_history - \nto use the old definition of \"identical to any parent\" in that case. I'm \nnot sure that's right though.\n\nI currently feel instinctively more disposed to dropping the older \n\"don't follow off-graph identical parents\" rule. Let the default history \ngo straight to v1.8.3 even though it goes off the graph, stopping us \ntraversing the topic branch.\n\nKevin\n"},{"id":"227476","messageId":"20130911182444.GD4326@google.com","threadId":"34900","inReplyTo":"5230AD23.2050009@bracey.fi","subject":"Re: breakage in revision traversal with pathspec","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-11T18:24:44Z","receivedAt":"2013-09-11T18:24:44Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Kevin Bracey wrote:\n\n> On reflection I'm not sure what we should for the \"simple history\"\n> view of v1.8.3.1..v1.8.4. We're not rewriting parents, so we don't\n> get a chance to reconsider the merge as being zero-parent, and we do\n> have this little section of graph to traverse at the bottom:\n>\n> 1.8.3\n> o----x----x----x----x---x--- (x = included, o = excluded, *=!treesame)\n> /\n> /*\n> o--x--x--x--x\n[...]\n> 1) if identical to any on-graph parent, follow that one, and rewrite\n> the merge as a non-merge. We currently do not follow to an identical\n> off-graph parent. This long-standing comment in try_to_simplify_commit\n> applies: \"Even if a merge with an uninteresting side branch brought\n> the entire change we are interested in, we do not want to lose the\n> other branches of this merge, so we just keep going.\"\n[...]\n> 2) If rule 1 doesn't activate, and it remains as a merge, hide it if\n> treesame to all on-graph parents. Previously this rule was \"hide if\n> treesame to any parent\", and so that would have hidden the merge.\n>\n> Now, when I changed rule 2, I did not think this would affect the\n> default log. See my commit message:\n[...]\n> I currently feel instinctively more disposed to dropping the older\n> \"don't follow off-graph identical parents\" rule. Let the default\n> history go straight to v1.8.3 even though it goes off the graph,\n> stopping us traversing the topic branch.\n\nThanks for this analysis. Interesting.\n\nThe rule (1) comes from v1.3.0-rc1~13^2~6:\n\n\tcommit f3219fbbba32b5100430c17468524b776eb869d6\n\tAuthor: Junio C Hamano \n\tDate: Fri Mar 10 21:59:37 2006 -0800\n\n\t try_to_simplify_commit(): do not skip inspecting tree change at boundary.\n\t \n\t When git-rev-list (and git-log) collapsed ancestry chain to\n\t commits that touch specified paths, we failed to inspect and\n\t notice tree changes when we are about to hit uninteresting\n\t parent. This resulted in \"git rev-list since.. -- file\" to\n\t always show the child commit after the lower bound, even if it\n\t does not touch the file. This commit fixes it.\n\t \n\t Thanks for Catalin for reporting this.\n\t \n\t See also:\n\t\t461cf59f8924f174d7a0dcc3d77f576d93ed29a4\n\t \n\t Signed-off-by: Junio C Hamano \n\nI think you're right that dropping the \"don't follow off-graph\ntreesame parents\" rule would be a sensible change. The usual point of\nthe \"follow the treesame parent\" rule is to avoid drawing undue\nattention to merges of ancient history where some of the parents are\nside-branches with an old version of the files being tracked and did\nnot actually change those files. That rationale applies just as much\nfor a merge on top of an UNINTERESTING rev as any other merge.\n\nThanks,\nJonathan\n"},{"id":"227486","messageId":"xmqqmwnjrwov.fsf@gitster.dls.corp.google.com","threadId":"34900","inReplyTo":"20130911182444.GD4326@google.com","subject":"Re: breakage in revision traversal with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-11T19:21:36Z","receivedAt":"2013-09-11T19:21:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I think you're right that dropping the \"don't follow off-graph\n> treesame parents\" rule would be a sensible change. The usual point of\n> the \"follow the treesame parent\" rule is to avoid drawing undue\n> attention to merges of ancient history where some of the parents are\n> side-branches with an old version of the files being tracked and did\n> not actually change those files. That rationale applies just as much\n> for a merge on top of an UNINTERESTING rev as any other merge.\n\nSounds sensible. Thanks.\n"},{"id":"227487","messageId":"5230C6E3.3080406@bracey.fi","threadId":"34900","inReplyTo":"20130911182444.GD4326@google.com","subject":"Re: breakage in revision traversal with pathspec","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-09-11T19:39:15Z","receivedAt":"2013-09-11T19:39:15Z","isPatch":false,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 11/09/2013 21:24, Jonathan Nieder wrote:\n> Kevin Bracey wrote:\n>\n>> On reflection I'm not sure what we should for the \"simple history\"\n>> view of v1.8.3.1..v1.8.4. We're not rewriting parents, so we don't\n>> get a chance to reconsider the merge as being zero-parent, and we do\n>> have this little section of graph to traverse at the bottom:\n>>\n>> 1.8.3\n>> o----x----x----x----x---x--- (x = included, o = excluded, *=!treesame)\n>> /\n>> /*\n>> o--x--x--x--x\n> [...]\n>> 1) if identical to any on-graph parent, follow that one, and rewrite\n>> the merge as a non-merge. We currently do not follow to an identical\n>> off-graph parent. This long-standing comment in try_to_simplify_commit\n>> applies: \"Even if a merge with an uninteresting side branch brought\n>> the entire change we are interested in, we do not want to lose the\n>> other branches of this merge, so we just keep going.\"\n>>\n> [...]\n>> I currently feel instinctively more disposed to dropping the older\n>> \"don't follow off-graph identical parents\" rule. Let the default\n>> history go straight to v1.8.3 even though it goes off the graph,\n>> stopping us traversing the topic branch.\n> Thanks for this analysis. Interesting.\n>\n> The rule (1) comes from v1.3.0-rc1~13^2~6: ...\n>\n> I think you're right that dropping the \"don't follow off-graph\n> treesame parents\" rule would be a sensible change. The usual point of\n> the \"follow the treesame parent\" rule is to avoid drawing undue\n> attention to merges of ancient history where some of the parents are\n> side-branches with an old version of the files being tracked and did\n> not actually change those files. That rationale applies just as much\n> for a merge on top of an UNINTERESTING rev as any other merge.\nI agree about the rationale still applying - why not follow off-graph, \nunless you're doing --ancestry-path? (Fortunately ancestry_path already \ndisables simplify_history). That makes more sense if you try to ignore \nthe misleading comment. In a typical \"v1..v3\" range, the temporal \nlimiting means that it's paths to the mainline that will tend to be \nmarked UNINTERESTING, not to the topic branches...\n\nBut I can imagine going off graph it may previously have tripped up \nother parts of the code. It could be that this Git 1.3.0 rule ended up \ncovering over some of the older merge hiding logic flakiness. Maybe it's \nno longer necessary. I'll do some experiments.\n\nNow, one bit of news - I have just figured out why gitk is behaving \ndifferently. It transforms \"..\" before it reaches git.\n\nTo see the effect at the command line: \"git log v1.8.3..v.1.8.4\" hides \nthe merge, but \"git log ^v1.8.3 v1.8.4\" shows it. Whoops. A new example \nof a dotty shorthand not being exactly equivalent.\n\nIn the \"..\" case the v1.8.3 tag gets peeled before being sent to \nadd_rev_cmdline , and the \"mark bottom commits\" logic works. But in the \n\"^\" case, the v1.8.3 doesn't get peeled. Junio - any thoughts on the \ncorrect place to fix that? (And gitk actually does ^, just to \nbe odd, so that needs to be handled too). Should these things be peeled \nin revs->cmdline or not? We should be consistent.\n\nKevin\n"},{"id":"227492","messageId":"xmqqa9jjrrfb.fsf@gitster.dls.corp.google.com","threadId":"34900","inReplyTo":"5230C6E3.3080406@bracey.fi","subject":"Re: breakage in revision traversal with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-11T21:15:20Z","receivedAt":"2013-09-11T21:15:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> To see the effect at the command line: \"git log v1.8.3..v.1.8.4\" hides\n> the merge, but \"git log ^v1.8.3 v1.8.4\" shows it. Whoops. A new\n> example of a dotty shorthand not being exactly equivalent.\n>\n> In the \"..\" case the v1.8.3 tag gets peeled before being sent to\n> add_rev_cmdline , and the \"mark bottom commits\" logic works. But in\n> the \"^\" case, the v1.8.3 doesn't get peeled.\n\nThat sounds like a bug. ^v1.8.3 should mark v1.8.3^0 as\nuninteresting.\n"},{"id":"227901","messageId":"xmqq38p0sdeb.fsf@gitster.dls.corp.google.com","threadId":"34900","inReplyTo":"xmqqa9jjrrfb.fsf@gitster.dls.corp.google.com","subject":"Re: breakage in revision traversal with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-19T21:35:40Z","receivedAt":"2013-09-19T21:35:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Kevin Bracey writes:\n>\n>> To see the effect at the command line: \"git log v1.8.3..v.1.8.4\" hides\n>> the merge, but \"git log ^v1.8.3 v1.8.4\" shows it. Whoops. A new\n>> example of a dotty shorthand not being exactly equivalent.\n>>\n>> In the \"..\" case the v1.8.3 tag gets peeled before being sent to\n>> add_rev_cmdline , and the \"mark bottom commits\" logic works. But in\n>> the \"^\" case, the v1.8.3 doesn't get peeled.\n>\n> That sounds like a bug. ^v1.8.3 should mark v1.8.3^0 as\n> uninteresting.\n\nOK, so \"git rev-list ^v1.8.3 v1.8.4\" throws two objects into\nrevs->pending.objects[] array. Two tags, v1.8.3 marked as\nUNINTERESTING and v1.8.4. The revision walking machinery will peel\nthe tag by calling handle_commit() (which by the way arguably is\nmisnamed because it has to be called for any type of object) when it\nstarts to walk in prepare_revision_walk().\n\nBut \"git rev-list v1.8.3..v1.8.4\" throws two commits (v1.8.3^0 with\nUNINTERESTING bit and v1.8.4^0) to revs->pending.objects[] after\npeeling. I _think_ it is wrong. Because the range is only defined\nover commit DAG, and because the same codepath handles the symmetric\ndifference v1.8.3...v1.8.4 as well, both ends of dots operator do\nneed to be peeled to commits, but I think it is wrong to throw these\npeeled results into revs->pending.objects[].\n\nWhere it makes a difference is when rev-list is used with --objects.\n\n $ git rev-list --objects v1.8.4^1..v1.8.4 | grep $(git rev-parse v1.8.4)\n $ git rev-list --objects v1.8.4 ^v1.8.4^1 | grep $(git rev-parse v1.8.4)\n 04f013dc38d7512eadb915eba22efc414f18b869 v1.8.4\n\n-- >8 --\nSubject: revision: do not peel tags used in range notation\n\nA range notation \"A..B\" means exactly the same thing as what \"^A B\"\nmeans, i.e. the set of commits that are reachable from B but not\nfrom A. But the internal representation after the revision parser\nparsed these two notations are subtly different.\n\n - \"rev-list ^A B\" leaves A and B in the revs->pending.objects[]\n array, with the former marked as UNINTERESTING and the revision\n traversal machinery propagates the mark to underlying commit\n objects A^0 and B^0.\n\n - \"rev-list A..B\" peels tags and leaves A^0 (marked as\n UNINTERESTING) and B^0 in revs->pending.objects[] array before\n the traversal machinery kicks in.\n\nThis difference usually does not matter, but starts to matter when\nthe --objects option is used. For example, we see this:\n\n $ git rev-list --objects v1.8.4^1..v1.8.4 | grep $(git rev-parse v1.8.4)\n $ git rev-list --objects v1.8.4 ^v1.8.4^1 | grep $(git rev-parse v1.8.4)\n 04f013dc38d7512eadb915eba22efc414f18b869 v1.8.4\n\nWith the former invocation, the revision traversal machinery never\nhears about the tag v1.8.4 (it only sees the result of peeling it,\ni.e. the commit v1.8.4^0), and the tag itself does not appear in the\noutput. The latter does send the tag object itself to the output.\n\nMake the range notation keep the unpeeled objects and feed them to\nthe traversal machinery to fix this inconsistency.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Made against maint-1.8.0 just in case we may want to fix older\n maintenance series.\n\n revision.c | 19 +++++++++++++------\n t/t6000-rev-list-misc.sh | 8 ++++++++\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 68545c8..a6d2150 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1159,6 +1159,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t !get_sha1_committish(next, sha1)) {\n \t\t\tstruct commit *a, *b;\n \t\t\tstruct commit_list *exclude;\n+\t\t\tstruct object *a_obj, *b_obj;\n \n \t\t\ta = lookup_commit_reference(from_sha1);\n \t\t\tb = lookup_commit_reference(sha1);\n@@ -1184,14 +1185,20 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\ta_flags = flags | SYMMETRIC_LEFT;\n \t\t\t} else\n \t\t\t\ta_flags = flags_exclude;\n-\t\t\ta->object.flags |= a_flags;\n-\t\t\tb->object.flags |= flags;\n-\t\t\tadd_rev_cmdline(revs, &a->object, this,\n+\t\t\ta_obj = (!hashcmp(a->object.sha1, from_sha1)\n+\t\t\t\t ? &a->object\n+\t\t\t\t : lookup_object(from_sha1));\n+\t\t\tb_obj = (!hashcmp(b->object.sha1, sha1)\n+\t\t\t\t ? &b->object\n+\t\t\t\t : lookup_object(sha1));\n+\t\t\ta_obj->flags |= a_flags;\n+\t\t\tb_obj->flags |= flags;\n+\t\t\tadd_rev_cmdline(revs, a_obj, this,\n \t\t\t\t\tREV_CMD_LEFT, a_flags);\n-\t\t\tadd_rev_cmdline(revs, &b->object, next,\n+\t\t\tadd_rev_cmdline(revs, b_obj, next,\n \t\t\t\t\tREV_CMD_RIGHT, flags);\n-\t\t\tadd_pending_object(revs, &a->object, this);\n-\t\t\tadd_pending_object(revs, &b->object, next);\n+\t\t\tadd_pending_object(revs, a_obj, this);\n+\t\t\tadd_pending_object(revs, b_obj, next);\n \t\t\treturn 0;\n \t\t}\n \t\t*dotdot = '.';\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex b10685a..15e3d64 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -48,4 +48,12 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' '\n \t! grep one output\n '\n \n+test_expect_success 'rev-list A..B and rev-list ^A B are the same' '\n+\tgit commit --allow-empty -m another &&\n+\tgit tag -a -m \"annotated\" v1.0 &&\n+\tgit rev-list --objects ^v1.0^ v1.0 >expect &&\n+\tgit rev-list --objects v1.0^..v1.0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"227922","messageId":"20130920033541.GC15101@sigill.intra.peff.net","threadId":"34900","inReplyTo":"xmqq38p0sdeb.fsf@gitster.dls.corp.google.com","subject":"Re: breakage in revision traversal with pathspec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-20T03:35:41Z","receivedAt":"2013-09-20T03:35:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 19, 2013 at 02:35:40PM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: revision: do not peel tags used in range notation\n> \n> A range notation \"A..B\" means exactly the same thing as what \"^A B\"\n> means, i.e. the set of commits that are reachable from B but not\n> from A. But the internal representation after the revision parser\n> parsed these two notations are subtly different.\n> [...]\n\nThanks for a very clear explanation. This definitely seems like an\nimprovement, and the patch looks good to me.\n\nOne question, though. With your patch, if I do \"tag1..tag2\", I get both\nthe tags and the peeled commits in the pending object list. Whereas with\n\"^tag1 tag2\", we put only the tags into the list, and we expect the\ntraversal machinery to peel them later. I cannot off-hand think of a\nreason this difference should be a problem, but I am wondering if there\nis some code path that does not traverse, but just looks at pending\nobjects, that might care.\n\n-Peff\n"},{"id":"227925","messageId":"xmqqioxwqec0.fsf@gitster.dls.corp.google.com","threadId":"34900","inReplyTo":"20130920033541.GC15101@sigill.intra.peff.net","subject":"Re: breakage in revision traversal with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-20T04:58:23Z","receivedAt":"2013-09-20T04:58:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> One question, though. With your patch, if I do \"tag1..tag2\", I get both\n> the tags and the peeled commits in the pending object list. Whereas with\n> \"^tag1 tag2\", we put only the tags into the list, and we expect the\n> traversal machinery to peel them later. I cannot off-hand think of a\n> reason this difference should be a problem, but I am wondering if there\n> is some code path that does not traverse, but just looks at pending\n> objects, that might care.\n\nDid I really do that?\n\nI thought that the original was pushing peeled tag1^0 and tag2^0\n(and nothing else) for \"tag1..tag2\", and the intent of the patch was\nto see if \"a\" (which is \"tag1^0\" in this case) has the same object\nname as the object originally given on the side of the dots\n(i.e. \"tag1\"). If they differ, that means \"a\" is the peeled object,\nand instead use the original \"tag1\" for \"a_obj\" that is pushed into\nthe pending (and if they are the same, \"a_obj\" is just \"&a->object\",\nthe object itself). The same for \"b\", \"tag2\" and \"b_obj\". So at\nleast I didn't mean to push four objects into the pending list\nbefore prepare_revision_walk() kicks in.\n\nPerhaps I missed something?\n\nNow, when prepare_revision_walk() picks up objects from the pending\nlist, they are fed to handle_commit(), and these two tags will be\npeeled and their commits are returned to be queued in revs->commits\nlinked list, while the tags themselves are sent to the pending list\nto be emitted in \"--objects\" output. But that should be the same\nbetween \"tag1..tag2\" and \"^tag1 tag2\".\n\nA possible difference in behaviour is that with \"^tag1 tag2\", we do\nnot instantiate the commit objects pointed at by these tags until\nprepare_revision_walk() sends these tags to handle_commit(), while\nwith \"tag1..tag2\", these tags and the commit objects would already\nbe parsed when setup_revisions() returns (and the updated code does\nrely on this behaviour by saying \"if a->object.sha1 and from_sha1\nare different, we know the tag whose name is from_sha1 is already\nparsed, so we can just call lookup_object() on from_sha1 to grab\nit\"). But I do not think any code just tries to grab an object\nusing a random object name outside the revision traversal and decide\nto do things that results in semantically different behaviour if the\nresulting object has (or has not) already been parsed.\n"},{"id":"227926","messageId":"20130920051107.GA17609@sigill.intra.peff.net","threadId":"34900","inReplyTo":"xmqqioxwqec0.fsf@gitster.dls.corp.google.com","subject":"Re: breakage in revision traversal with pathspec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-20T05:11:07Z","receivedAt":"2013-09-20T05:11:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 19, 2013 at 09:58:23PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > One question, though. With your patch, if I do \"tag1..tag2\", I get both\n> > the tags and the peeled commits in the pending object list. Whereas with\n> > \"^tag1 tag2\", we put only the tags into the list, and we expect the\n> > traversal machinery to peel them later. I cannot off-hand think of a\n> > reason this difference should be a problem, but I am wondering if there\n> > is some code path that does not traverse, but just looks at pending\n> > objects, that might care.\n> \n> Did I really do that?\n> \n> I thought that the original was pushing peeled tag1^0 and tag2^0\n> (and nothing else) for \"tag1..tag2\", and the intent of the patch was\n> to see if \"a\" (which is \"tag1^0\" in this case) has the same object\n> name as the object originally given on the side of the dots\n> (i.e. \"tag1\"). If they differ, that means \"a\" is the peeled object,\n> and instead use the original \"tag1\" for \"a_obj\" that is pushed into\n> the pending (and if they are the same, \"a_obj\" is just \"&a->object\",\n> the object itself). The same for \"b\", \"tag2\" and \"b_obj\". So at\n> least I didn't mean to push four objects into the pending list\n> before prepare_revision_walk() kicks in.\n> \n> Perhaps I missed something?\n\nHrm, no, it is me misreading the diff.\n\nMy original question was going to be: why bother peeling at all if we\nare just going to push the outer objects, anyway?\n\nAnd after staring at it, I somehow convinced myself that the answer was\nthat you were pushing both. But that is not the case. Sorry for the\nnoise.\n\nThe other reason I considered is that we want to make sure they do peel\nto commits. I do not think that is technically required for \"A..B\",\nwhich can operate on non-commits. But it is for \"A...B\", and I do not\nsee any advantage in loosening \"A..B\" for the non-commit case. It would\njust complicate the code.\n\n> Now, when prepare_revision_walk() picks up objects from the pending\n> list, they are fed to handle_commit(), and these two tags will be\n> peeled and their commits are returned to be queued in revs->commits\n> linked list, while the tags themselves are sent to the pending list\n> to be emitted in \"--objects\" output. But that should be the same\n> between \"tag1..tag2\" and \"^tag1 tag2\".\n\nYes, I was specifically concerned about sites that did not call\nprepare_revision_walk(), but since the state is the same for both cases,\nit's a non-issue.\n\n> it\"). But I do not think any code just tries to grab an object\n> using a random object name outside the revision traversal and decide\n> to do things that results in semantically different behaviour if the\n> resulting object has (or has not) already been parsed.\n\nYeah, I think any code relying on that would be insane, from a\nmodularity perspective. The caching of parsed object state is an\noptimization, and callers have no business making assumptions about it.\nOtherwise they are fragile with respect to a previous traversal being\nadded in the same in-memory process.\n\nSo I think your patch is doing the right thing, and my concern was just\nfrom mis-reading. Again, sorry for the confusion.\n\n-Peff\n"},{"id":"227939","messageId":"xmqqeh8jqt38.fsf@gitster.dls.corp.google.com","threadId":"34900","inReplyTo":"20130920051107.GA17609@sigill.intra.peff.net","subject":"Re: breakage in revision traversal with pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-20T17:51:55Z","receivedAt":"2013-09-20T17:51:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> My original question was going to be: why bother peeling at all if we\n> are just going to push the outer objects, anyway?\n>\n> And after staring at it, I somehow convinced myself that the answer was\n> that you were pushing both. But that is not the case. Sorry for the\n> noise.\n\nBut that is still a valid point, and the patch to avoid peeling for\nnon symmetric diff does not look too bad, either.\n\n revision.c | 59 ++++++++++++++++++++++++++++++------------------\n t/t6000-rev-list-misc.sh | 8 +++++++\n 2 files changed, 45 insertions(+), 22 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 68545c8..7010aff 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1157,41 +1157,56 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t}\n \t\tif (!get_sha1_committish(this, from_sha1) &&\n \t\t !get_sha1_committish(next, sha1)) {\n-\t\t\tstruct commit *a, *b;\n-\t\t\tstruct commit_list *exclude;\n-\n-\t\t\ta = lookup_commit_reference(from_sha1);\n-\t\t\tb = lookup_commit_reference(sha1);\n-\t\t\tif (!a || !b) {\n-\t\t\t\tif (revs->ignore_missing)\n-\t\t\t\t\treturn 0;\n-\t\t\t\tdie(symmetric ?\n-\t\t\t\t \"Invalid symmetric difference expression %s...%s\" :\n-\t\t\t\t \"Invalid revision range %s..%s\",\n-\t\t\t\t arg, next);\n-\t\t\t}\n+\t\t\tstruct object *a_obj, *b_obj;\n \n \t\t\tif (!cant_be_filename) {\n \t\t\t\t*dotdot = '.';\n \t\t\t\tverify_non_filename(revs->prefix, arg);\n \t\t\t}\n \n-\t\t\tif (symmetric) {\n+\t\t\ta_obj = parse_object(from_sha1);\n+\t\t\tb_obj = parse_object(sha1);\n+\t\t\tif (!a_obj || !b_obj) {\n+\t\t\tmissing:\n+\t\t\t\tif (revs->ignore_missing)\n+\t\t\t\t\treturn 0;\n+\t\t\t\tdie(symmetric\n+\t\t\t\t ? \"Invalid symmetric difference expression %s\"\n+\t\t\t\t : \"Invalid revision range %s\", arg);\n+\t\t\t}\n+\n+\t\t\tif (!symmetric) {\n+\t\t\t\t/* just A..B */\n+\t\t\t\ta_flags = flags_exclude;\n+\t\t\t} else {\n+\t\t\t\t/* A...B -- find merge bases between the two */\n+\t\t\t\tstruct commit *a, *b;\n+\t\t\t\tstruct commit_list *exclude;\n+\n+\t\t\t\ta = (a_obj->type == OBJ_COMMIT\n+\t\t\t\t ? (struct commit *)a_obj\n+\t\t\t\t : lookup_commit_reference(a_obj->sha1));\n+\t\t\t\tb = (b_obj->type == OBJ_COMMIT\n+\t\t\t\t ? (struct commit *)b_obj\n+\t\t\t\t : lookup_commit_reference(b_obj->sha1));\n+\t\t\t\tif (!a || !b)\n+\t\t\t\t\tgoto missing;\n \t\t\t\texclude = get_merge_bases(a, b, 1);\n \t\t\t\tadd_pending_commit_list(revs, exclude,\n \t\t\t\t\t\t\tflags_exclude);\n \t\t\t\tfree_commit_list(exclude);\n+\n \t\t\t\ta_flags = flags | SYMMETRIC_LEFT;\n-\t\t\t} else\n-\t\t\t\ta_flags = flags_exclude;\n-\t\t\ta->object.flags |= a_flags;\n-\t\t\tb->object.flags |= flags;\n-\t\t\tadd_rev_cmdline(revs, &a->object, this,\n+\t\t\t}\n+\n+\t\t\ta_obj->flags |= a_flags;\n+\t\t\tb_obj->flags |= flags;\n+\t\t\tadd_rev_cmdline(revs, a_obj, this,\n \t\t\t\t\tREV_CMD_LEFT, a_flags);\n-\t\t\tadd_rev_cmdline(revs, &b->object, next,\n+\t\t\tadd_rev_cmdline(revs, b_obj, next,\n \t\t\t\t\tREV_CMD_RIGHT, flags);\n-\t\t\tadd_pending_object(revs, &a->object, this);\n-\t\t\tadd_pending_object(revs, &b->object, next);\n+\t\t\tadd_pending_object(revs, a_obj, this);\n+\t\t\tadd_pending_object(revs, b_obj, next);\n \t\t\treturn 0;\n \t\t}\n \t\t*dotdot = '.';\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex b10685a..15e3d64 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -48,4 +48,12 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' '\n \t! grep one output\n '\n \n+test_expect_success 'rev-list A..B and rev-list ^A B are the same' '\n+\tgit commit --allow-empty -m another &&\n+\tgit tag -a -m \"annotated\" v1.0 &&\n+\tgit rev-list --objects ^v1.0^ v1.0 >expect &&\n+\tgit rev-list --objects v1.0^..v1.0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"228198","messageId":"20130925091259.GA5844@sigill.intra.peff.net","threadId":"34900","inReplyTo":"xmqqeh8jqt38.fsf@gitster.dls.corp.google.com","subject":"Re: breakage in revision traversal with pathspec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-25T09:12:59Z","receivedAt":"2013-09-25T09:12:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 20, 2013 at 10:51:55AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > My original question was going to be: why bother peeling at all if we\n> > are just going to push the outer objects, anyway?\n> >\n> > And after staring at it, I somehow convinced myself that the answer was\n> > that you were pushing both. But that is not the case. Sorry for the\n> > noise.\n> \n> But that is still a valid point, and the patch to avoid peeling for\n> non symmetric diff does not look too bad, either.\n> \n> revision.c | 59 ++++++++++++++++++++++++++++++------------------\n> t/t6000-rev-list-misc.sh | 8 +++++++\n> 2 files changed, 45 insertions(+), 22 deletions(-)\n\nFWIW, the flow of this version makes more sense to me. It also allows\nthings like:\n\n git rev-list --objects $blob..$tree\n\nwhich I cannot see anybody actually wanting, but it somehow seems\nsimpler to me to say \"A..B\" is syntactic sugar for \"^B A\", without\nqualifying \"except that A and B must be commit-ishes\".\n\n-Peff\n"}]}