{"thread":{"id":"29404","subject":"[RFC] Making pathspec limited log play nicer with --first-parent","startedAt":"2012-01-19T19:58:45Z","lastAt":"2012-01-19T20:48:46Z","messageCount":4,"participants":["Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"182803","messageId":"7v1uqvjwga.fsf@alter.siamese.dyndns.org","threadId":"29404","inReplyTo":null,"subject":"[RFC] Making pathspec limited log play nicer with --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-19T19:58:45Z","receivedAt":"2012-01-19T19:58:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I often find myself frustrated when I receive an update to a part of the\nproject and want to find the latest commit that merges the topic branch\nthat touched the same area.\n\nFor example, I wanted to find the latest merge of any git-p4 related topic\nso that I can fork a new topic branch to keep Luke's updates posted today.\n\nWithout pathspec, \"git log --first-parent\" traverses the first-parent\nchain just fine, and \"-m --stat\" shows the list of paths touched by the\nmerge, so we _could_ ask the question this way:\n\n    $ git log --first-parent --oneline -m --stat master |\n      sed -e '/^ contrib\\/fast-import\\/git-p4 /q' | tail\n      \nThe above finds that 8cbfc11 (Merge branch 'pw/p4-view-updates',\n2012-01-06) was such a commit.\n\nBut a more natural way to spell it does not work as expected:\n\n    $ git log --first-parent --oneline -m --stat -1 master -- \\\n      contrib/fast-import/git-p4\n\nThis finds ecb7cf9 (git-p4: rewrite view handling, 2012-01-02), which is a\npart of the merged topic branch and is _not_ on the first-parent path from\nthe 'master':\n\n    $ git show-branch 8cbfc11 ecb7cf9\n    ! [8cbfc11] Merge branch 'pw/p4-view-updates'\n     ! [ecb7cf9] git-p4: rewrite view handling\n    --\n    -  [8cbfc11] Merge branch 'pw/p4-view-updates'\n    +  [8cbfc11^2] git-p4: view spec documentation\n    ++ [ecb7cf9] git-p4: rewrite view handling\n\nThe problem happens when the merge simplification logic kicks in when the\ntraversal inspects the merge 8cbfc11. In this case, the history leading to\nthe tip of 'master' did not touch git-p4 since 'pw/p4-view-updates' topic\nforked from it, and the result of the merge is simply a copy from the tip\nof the topic branch in the view limited by the given pathspec.  The merge\nsimplification logic discards the first-parent of the merge and pretends\nas if the sole parent of the merge is its second parent, i.e. the tip of\nthe topic. The history traversal veers off to the side branch, possibly\nskipping a large swath of the mainline history if the topic forked from it\nlong in the past, but that only happens when the mainline did not touch\nthe paths in the limited view since the side topic forked, so it is not\nlosing information---but it still is wrong to show the commits on the side\ntopic when we are explicitly asked to show the first-parent chain.\n\nHere is an attempt to fix this issue, by not allowing us to compare the\nmerge result with anything but the first parent when --first-parent is in\neffect, to avoid the history traversal veering off to the side branch.\n\nAs this touches deep innards of a scary hairball that is the revision\ntraversal machinery, I am obviously not considering it as a 1.7.9\nmaterial, but I think we would want to fix it at some point, hopefully\nsoon.\n\nWith this patch, the \"more natural way\" finds the merge commit I am\nlooking for.\n\nComments?\n\n---\n revision.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 064e351..9e4596d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -416,7 +416,7 @@ static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)\n static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n {\n \tstruct commit_list **pp, *parent;\n-\tint tree_changed = 0, tree_same = 0;\n+\tint tree_changed = 0, tree_same = 0, nth_parent = 0;\n \n \t/*\n \t * If we don't do pruning, everything is interesting\n@@ -444,6 +444,8 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n \twhile ((parent = *pp) != NULL) {\n \t\tstruct commit *p = parent->item;\n \n+\t\tif (revs->first_parent_only && nth_parent++)\n+\t\t\tbreak;\n \t\tif (parse_commit(p) < 0)\n \t\t\tdie(\"cannot simplify commit %s (because of %s)\",\n \t\t\t    sha1_to_hex(commit->object.sha1),\n"},{"id":"182807","messageId":"CA+55aFxucaeX7it_Kj7WV3ZbwCukN+wvbuxqJzh3V5Rxz4ib1g@mail.gmail.com","threadId":"29404","inReplyTo":"7v1uqvjwga.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Making pathspec limited log play nicer with --first-parent","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2012-01-19T20:15:53Z","receivedAt":"2012-01-19T20:15:53Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Jan 19, 2012 at 11:58 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Comments?\n\nLooks conceptually right, but I do have to admit to hating that new variable.\n\nI don't see a better way to do it, though. Sure, you could do it with just\n\n   if (revs->first_parent_only && pp != &commit->parents)\n             break;\n\nand avoid the new variable that way, but that replaces the annoying\nvariable with a pretty subtle thing.\n\nOr we could re-write that while() loop and move the 'parent' variable\ninto it. Like the appended untested thing.\n\nBut maybe your patch is better, and my dislike for that parent counter\nis just irrational.\n\n                  Linus\n\n\n revision.c |   16 ++++++++++------\n 1 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 064e35108478..5e8eb379c369 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -415,7 +415,7 @@ static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)\n \n static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n {\n-\tstruct commit_list **pp, *parent;\n+\tstruct commit_list **pp;\n \tint tree_changed = 0, tree_same = 0;\n \n \t/*\n@@ -441,8 +441,14 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n \t\treturn;\n \n \tpp = &commit->parents;\n-\twhile ((parent = *pp) != NULL) {\n-\t\tstruct commit *p = parent->item;\n+\tdo {\n+\t\tstruct commit_list *parent = *pp;\n+\t\tstruct commit *p;\n+\n+\t\tif (!parent)\n+\t\t\tbreak;\n+\t\tpp = &parent->next;\n+\t\tp = parent->item;\n \n \t\tif (parse_commit(p) < 0)\n \t\t\tdie(\"cannot simplify commit %s (because of %s)\",\n@@ -458,7 +464,6 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n \t\t\t\t * to lose the other branches of this\n \t\t\t\t * merge, so we just keep going.\n \t\t\t\t */\n-\t\t\t\tpp = &parent->next;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tparent->next = NULL;\n@@ -487,11 +492,10 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)\n \t\tcase REV_TREE_OLD:\n \t\tcase REV_TREE_DIFFERENT:\n \t\t\ttree_changed = 1;\n-\t\t\tpp = &parent->next;\n \t\t\tcontinue;\n \t\t}\n \t\tdie(\"bad tree compare for commit %s\", sha1_to_hex(commit->object.sha1));\n-\t}\n+\t} while (!revs->first_parent_only);\n \tif (tree_changed && !tree_same)\n \t\treturn;\n \tcommit->object.flags |= TREESAME;\n"},{"id":"182809","messageId":"7vwr8niftt.fsf@alter.siamese.dyndns.org","threadId":"29404","inReplyTo":"CA+55aFxucaeX7it_Kj7WV3ZbwCukN+wvbuxqJzh3V5Rxz4ib1g@mail.gmail.com","subject":"Re: [RFC] Making pathspec limited log play nicer with --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-19T20:43:10Z","receivedAt":"2012-01-19T20:43:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Thu, Jan 19, 2012 at 11:58 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Comments?\n>\n> Looks conceptually right, but I do have to admit to hating that new variable.\n>\n> I don't see a better way to do it, though. Sure, you could do it with just\n>\n>    if (revs->first_parent_only && pp != &commit->parents)\n>              break;\n>\n> and avoid the new variable that way, but that replaces the annoying\n> variable with a pretty subtle thing.\n>\n> Or we could re-write that while() loop and move the 'parent' variable\n> into it. Like the appended untested thing.\n>\n> But maybe your patch is better, and my dislike for that parent counter\n> is just irrational.\n\nI didn't like that parent counter that _only_ increments when we are\nrunning under first-parent-only mode at the conceptual level. At the\nimplementation level, of course it is the right thing to do because\noutside first-parent-only mode nobody cares about the parent counter,\nso it is a valid but subtle optimization.\n\nBut I personally find your loop\n\n\tdo {\n        \t...\n\t} while (!revs->first_parent_only);\n\nis even more disgusting. It is misleading to have something that is not\nsupposed to change inside the loop as the terminating condition as if we\nare saying \"loop until somebody flips that bit\" which is clearly not the\ncase.\n\nSo obviously I am saying that I do not think either patch is pretty\nwithout offering a better alternative implementation, which is my usual\nbadness. As this is not an ultra urgent fix, I'll wait and see if somebody\nelse comes up with a more readable version.\n\nThanks for eyeballing the logic side of it, anyway. That was what I was\nworried about the change the most.\n"},{"id":"182810","messageId":"CA+55aFyR02DyzwxrTdr360U+Pa6S3a40yBXegc_TuOsq_Z84qA@mail.gmail.com","threadId":"29404","inReplyTo":"7vwr8niftt.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Making pathspec limited log play nicer with --first-parent","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2012-01-19T20:48:46Z","receivedAt":"2012-01-19T20:48:46Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Jan 19, 2012 at 12:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> But I personally find your loop is even more disgusting\n\nYeah, I can't really argue with that.\n\nThe thing I was playing with was to move the entire loop content into\na helper function (which would return the new pp), and then the\n\"first-parent only\" case just wouldn't do a loop at all.\n\nBut I couldn't be bothered. Your patch certainly does have the\nadvantage of being minimally intrusive.\n\n                  Linus\n"}]}