{"thread":{"id":"13473","subject":"\"git log --first-parent\" shows parents that are not first","startedAt":"2008-05-11T07:03:20Z","lastAt":"2008-05-14T11:10:34Z","messageCount":10,"participants":["しらいしななこ","Junio C Hamano","Lars Hjemli","Stephen R. van den Berg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"76587","messageId":"200805110706.m4B76eLE006432@mi0.bluebottle.com","threadId":"13473","inReplyTo":null,"subject":"\"git log --first-parent\" shows parents that are not first","fromName":"しらいしななこ","fromEmail":"nanako3@bluebottle.com","sentAt":"2008-05-11T07:03:20Z","receivedAt":"2008-05-11T07:03:20Z","isPatch":false,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting ������� :\n\n> The result given by \"git log --first-parent\" ('next' version) is\n> unexpected to me.\n>\n> % git rev-parse origin/next\n> 4eddac518225621c3e4f7285beb879d2b4bad38a\n> % git log --abbrev-commit --pretty=oneline --first-parent origin/next^..origin/next\n> 4eddac5... Merge branch 'master' into next\n> 1f8115b... Merge branch 'maint'\n> ca1c991... Merge branch 'sg/merge-options' (early part)\n> 31a3c6b... Merge branch 'db/learn-HEAD'\n> a064ac1... Merge branch 'jn/webfeed'\n> d576c45... Merge branch 'cc/help'\n> ca1a5ee... Merge branch 'dm/cherry-pick-s'\n> 4c4d3ac... Merge branch 'lt/dirmatch-optim'\n> c5445fe... compat-util: avoid macro redefinition warning\n> eb120e6... compat/fopen.c: avoid clobbering the system defined fopen macro\n> bac59f1... Documentation: bisect: add a few \"git bisect run\" examples\n> d84ae0d... Documentation/config.txt: Add git-gui options\n> 921177f... Documentation: improve \"add\", \"pull\" and \"format-patch\" examples\n> c904bf3... Be more careful with objects directory permissions on clone\n>\n> I asked for the log between one commit before the tip of \"origin/next\" and the tip of the branch, following only the first-parent links. v1.5.5 is not broken and shows the expected result:\n>\n> % ~/git-v1.5.5/bin/git log --abbrev-commit --pretty=oneline --first-parent origin/next^..origin/next\n> 4eddac5... Merge branch 'master' into next\n\nCould you please revert d9c292e8bbd51c84cb9ecd86cb89b8a1b35a2a82? With\nthat patch reverted from 'next', the problem disappears.\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n\n----------------------------------------------------------------------\nFree pop3 email with a spam filter.\nhttp://www.bluebottle.com/tag/5\n"},{"id":"76656","messageId":"7vd4ns3cll.fsf@gitster.siamese.dyndns.org","threadId":"13473","inReplyTo":"200805110706.m4B76eLE006432@mi0.bluebottle.com","subject":"Re: \"git log --first-parent\" shows parents that are not first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-11T18:44:06Z","receivedAt":"2008-05-11T18:44:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"しらいしななこ writes:\n\n>> The result given by \"git log --first-parent\" ('next' version) is\n>> unexpected to me.\n>>\n>> % git rev-parse origin/next\n>> 4eddac518225621c3e4f7285beb879d2b4bad38a\n>> % git log --abbrev-commit --pretty=oneline --first-parent origin/next^..origin/next\n>> 4eddac5... Merge branch 'master' into next\n>> 1f8115b... Merge branch 'maint'\n>> ...\n>> 921177f... Documentation: improve \"add\", \"pull\" and \"format-patch\" examples\n>> c904bf3... Be more careful with objects directory permissions on clone\n>>\n>> I asked for the log between one commit before the tip of \"origin/next\"\n>> and the tip of the branch, following only the first-parent links.\n>> v1.5.5 is not broken and shows the expected result:\n>>\n>> % ~/git-v1.5.5/bin/git log --abbrev-commit --pretty=oneline --first-parent origin/next^..origin/next\n>> 4eddac5... Merge branch 'master' into next\n>\n> Could you please revert d9c292e8bbd51c84cb9ecd86cb89b8a1b35a2a82? With\n> that patch reverted from 'next', the problem disappears.\n\nThat's d9c292e (Simplify and fix --first-parent implementation,\n2008-04-27) by Stephen.\n\nI know that the alleged \"fix\" works around a corner-case, a fast-forward\nsituation that was artificually recorded as a merge, but if the \"cure\"\nbreaks a normal case like this, it is worse than the disease.\n\nStephen, do you have a fix?\n"},{"id":"76666","messageId":"1210547651-32510-1-git-send-email-hjemli@gmail.com","threadId":"13473","inReplyTo":"7vd4ns3cll.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] revision.c: really honor --first-parent","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2008-05-11T23:14:11Z","receivedAt":"2008-05-11T23:14:11Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"In add_parents_to_list, if any parent of a revision had already been\nSEEN, the current code would continue with the next parent. But if the\nfirst parent has been SEEN and --first-parent has been specified we need\nto break, not continue.\n\nSigned-off-by: Lars Hjemli \n---\n revision.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 44d780b..974ad10 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -468,8 +468,11 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str\n \t\tif (parse_commit(p) < 0)\n \t\t\treturn -1;\n \t\tp->object.flags |= left_flag;\n-\t\tif (p->object.flags & SEEN)\n+\t\tif (p->object.flags & SEEN) {\n+\t\t\tif (revs->first_parent_only)\n+\t\t\t\tbreak;\n \t\t\tcontinue;\n+\t\t}\n \t\tp->object.flags |= SEEN;\n \t\tinsert_by_date(p, list);\n \t\tif(revs->first_parent_only)\n-- \n1.5.5.1.148.g8ee22.dirty\n"},{"id":"76716","messageId":"1210605156-22926-1-git-send-email-hjemli@gmail.com","threadId":"13473","inReplyTo":"1210547651-32510-1-git-send-email-hjemli@gmail.com","subject":"[PATCH v2] revision.c: really honor --first-parent","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2008-05-12T15:12:36Z","receivedAt":"2008-05-12T15:12:36Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"In add_parents_to_list, if any parent of a revision had already been\nSEEN, the current code would continue with the next parent, skipping\nthe test for --first-parent. This patch inverts the test for SEEN so\nthat the test for --first-parent is always performed.\n\nSigned-off-by: Lars Hjemli \n---\nThis is a slightly different approach which I think is less ugly.\n\n revision.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 44d780b..2fc26b8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -468,10 +468,10 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str\n \t\tif (parse_commit(p) < 0)\n \t\t\treturn -1;\n \t\tp->object.flags |= left_flag;\n-\t\tif (p->object.flags & SEEN)\n-\t\t\tcontinue;\n-\t\tp->object.flags |= SEEN;\n-\t\tinsert_by_date(p, list);\n+\t\tif (!(p->object.flags & SEEN)) {\n+\t\t\tp->object.flags |= SEEN;\n+\t\t\tinsert_by_date(p, list);\n+\t\t}\n \t\tif(revs->first_parent_only)\n \t\t\tbreak;\n \t}\n-- \n1.5.5.1.148.g8ee22.dirty\n"},{"id":"76882","messageId":"20080513201522.GA11485@cuci.nl","threadId":"13473","inReplyTo":"1210605156-22926-1-git-send-email-hjemli@gmail.com","subject":"Re: [PATCH v2] revision.c: really honor --first-parent","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-05-13T20:15:22Z","receivedAt":"2008-05-13T20:15:22Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Lars Hjemli wrote:\n>In add_parents_to_list, if any parent of a revision had already been\n>SEEN, the current code would continue with the next parent, skipping\n>the test for --first-parent. This patch inverts the test for SEEN so\n>that the test for --first-parent is always performed.\n\nLet's put it this way:\n- If there would have been only one path to any particular point in the\n tree, then the --first-parent flag makes no differences, because the\n tree wouldn't contain any merges to begin with.\n- If a tree contains *any* merges (i.e. a commit with multiple parents),\n then there are always multiple paths to some common ancestor, and\n therefore depending on which path you travel up first, you sometimes get\n clashes with the SEEN flag (unpredictable by definition).\n- It would seem logical and sufficient to avoid this unpredictability by\n utilising the --first-parent flag to present and walk a tree of commits\n AS IF there were no merges.\n- My original patch did just that, it simplified the code to make sure\n that all other parents beside the first parent are ignored when\n walking the tree.\n- Your code now doesn't simplify the (IMO) convoluted walk, and still\n marks things as seen, even though in the first-parent case, these\n commits are not really seen at all. It implies that your code\n generates differing output, depending on the merges present.\n- The question now is, do we want the output of --first-parent to be\n immutable with respect to merges being present (but hidden from sight\n during a --first-parent run), or do we want the output of\n --first-parent to actually change depending on variations in parents\n other than the first parent?\n\nI'd say it's better to keep the code simpler, and to make sure the\noutput does *not* depend on any parents other than the first (as\nimplemented in my original patch).\n\n>This is a slightly different approach which I think is less ugly.\n\nYour patch is smaller, and therefore (perhaps) less ugly; the resulting\ncode and logic of my original patch is simpler (IMHO), and therefore\ncleaner (but it all depends on (the lack of) consensus over the points above).\n-- \nSincerely, srb@cuci.nl\n Stephen R. van den Berg.\n\n\"If I had to live my life again, I'd make the same mistakes, only sooner.\"\n"},{"id":"76884","messageId":"8c5c35580805131343kc115df6yd7ce3281fb3e6171@mail.gmail.com","threadId":"13473","inReplyTo":"20080513201522.GA11485@cuci.nl","subject":"Re: [PATCH v2] revision.c: really honor --first-parent","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2008-05-13T20:43:52Z","receivedAt":"2008-05-13T20:43:52Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Tue, May 13, 2008 at 10:15 PM, Stephen R. van den Berg wrote:\n> Lars Hjemli wrote:\n> >In add_parents_to_list, if any parent of a revision had already been\n> >SEEN, the current code would continue with the next parent, skipping\n> >the test for --first-parent. This patch inverts the test for SEEN so\n> >that the test for --first-parent is always performed.\n>\n> Let's put it this way:\n> - If there would have been only one path to any particular point in the\n> tree, then the --first-parent flag makes no differences, because the\n> tree wouldn't contain any merges to begin with.\n\nTrue\n\n> - If a tree contains *any* merges (i.e. a commit with multiple parents),\n> then there are always multiple paths to some common ancestor, and\n> therefore depending on which path you travel up first, you sometimes get\n> clashes with the SEEN flag (unpredictable by definition).\n\nTrue\n\n> - It would seem logical and sufficient to avoid this unpredictability by\n> utilising the --first-parent flag to present and walk a tree of commits\n> AS IF there were no merges.\n\nTrue\n\n> - My original patch did just that, it simplified the code to make sure\n> that all other parents beside the first parent are ignored when\n> walking the tree.\n\nExcept for the case where the first parent had been already SEEN; then\nit would continue to test the next parents until one was found which\nwas not already SEEN and _that_ parent would be treated as if it was\nfirst. And as Nanako showed, a simple `git rev-list HEAD^..HEAD` marks\nboth HEAD and HEAD^ as seen. When combined with --first-parent, the\nresult (with your patch) is that HEAD^2 is treated as the first\nparent. With my patch on top of yours, the walk stops as HEAD^, which\nis what we probably both want.\n\n> - Your code now doesn't simplify the (IMO) convoluted walk, and still\n> marks things as seen, even though in the first-parent case, these\n> commits are not really seen at all. It implies that your code\n> generates differing output, depending on the merges present.\n\nI don't think so. My code should neither follow nor mark as SEEN any\nparent but the first (but I could obviously be wrong).\n\n\n> - The question now is, do we want the output of --first-parent to be\n> immutable with respect to merges being present (but hidden from sight\n> during a --first-parent run), or do we want the output of\n> --first-parent to actually change depending on variations in parents\n> other than the first parent?\n>\n> I'd say it's better to keep the code simpler, and to make sure the\n> output does *not* depend on any parents other than the first (as\n> implemented in my original patch).\n\nI agree with your reasoning, and your patch with mine on top seems to\nachieve that goal.\n\n--\nlarsh\n"},{"id":"76893","messageId":"7vej85suc2.fsf@gitster.siamese.dyndns.org","threadId":"13473","inReplyTo":"8c5c35580805131343kc115df6yd7ce3281fb3e6171@mail.gmail.com","subject":"Re: [PATCH v2] revision.c: really honor --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-13T22:38:37Z","receivedAt":"2008-05-13T22:38:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lars Hjemli\" writes:\n\n>> - My original patch did just that, it simplified the code to make sure\n>> that all other parents beside the first parent are ignored when\n>> walking the tree.\n>\n> Except for the case where the first parent had been already SEEN; then\n> it would continue to test the next parents until one was found which\n> was not already SEEN and _that_ parent would be treated as if it was\n> first. And as Nanako showed, a simple `git rev-list HEAD^..HEAD` marks\n> both HEAD and HEAD^ as seen. When combined with --first-parent, the\n> result (with your patch) is that HEAD^2 is treated as the first\n> parent. With my patch on top of yours, the walk stops as HEAD^, which\n> is what we probably both want.\n>\n>> - Your code now doesn't simplify the (IMO) convoluted walk, and still\n>> marks things as seen, even though in the first-parent case, these\n>> commits are not really seen at all. It implies that your code\n>> generates differing output, depending on the merges present.\n>\n> I don't think so. My code should neither follow nor mark as SEEN any\n> parent but the first (but I could obviously be wrong).\n\nA major part of the \"convoluted walk\" is the (il-)logic that skipped\nearlier SEEN parents and treated the first unseen one as if it was the\nfirst parent, which is not exactly Stephen's fault. It was placed by\nyours truly in the very original code but it was done without much\nthought.\n\nI think your patch is the correct fix for that convolution, regardless of\nthe traversal order stability issue Stephen mentions.\n"},{"id":"76936","messageId":"20080514103454.GA28610@cuci.nl","threadId":"13473","inReplyTo":"7vej85suc2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] revision.c: really honor --first-parent","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-05-14T10:34:54Z","receivedAt":"2008-05-14T10:34:54Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>\"Lars Hjemli\" writes:\n>>> - Your code now doesn't simplify the (IMO) convoluted walk, and still\n>>> marks things as seen, even though in the first-parent case, these\n>>> commits are not really seen at all. It implies that your code\n>>> generates differing output, depending on the merges present.\n\n>> I don't think so. My code should neither follow nor mark as SEEN any\n>> parent but the first (but I could obviously be wrong).\n\n>A major part of the \"convoluted walk\" is the (il-)logic that skipped\n>earlier SEEN parents and treated the first unseen one as if it was the\n>first parent, which is not exactly Stephen's fault. It was placed by\n>yours truly in the very original code but it was done without much\n>thought.\n\n>I think your patch is the correct fix for that convolution, regardless of\n>the traversal order stability issue Stephen mentions.\n\nI agree that Lars' patch prevents parts of the tree to go \"dark\" (so did\nmy patch).\nHowever, without either patch, that implies that the current --first-parent\ncode has a high probability of obscuring parts of the tree depending on\ntraversing order (in any tree which contains at least one merge).\n\nSo, I'd say, since the current code does not and cannot work reliably\nfor anyone specifically using --first-parent (with every merge\nencountered, the probability of correctness is multiplied by 0.5 at\nmost/least), you are going to do them a favour anyway by fixing the code,\nthen why not simplify the convolution and make the code rock-steady (and\nimplement my patch)?\n\nAnyone using --first-parent in production now has an embarrassingly high\nprobability of missing commits in his generated lists (I know that I\nnoticed the problem within 5 minutes from actually trying to use the\nflag to get meaningful output). So fixing and simplifying the code now\nis rather unlikely to create any more surprises than the current code\nalready presents to existing users (if any).\n-- \nSincerely, srb@cuci.nl\n Stephen R. van den Berg.\n\nWhat if there were no hypothetical questions?\n"},{"id":"76937","messageId":"8c5c35580805140354s62301343n62f8319b1853bfbd@mail.gmail.com","threadId":"13473","inReplyTo":"20080514103454.GA28610@cuci.nl","subject":"Re: [PATCH v2] revision.c: really honor --first-parent","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2008-05-14T10:54:32Z","receivedAt":"2008-05-14T10:54:32Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Wed, May 14, 2008 at 12:34 PM, Stephen R. van den Berg wrote:\n> So, I'd say, since the current code does not and cannot work reliably\n> for anyone specifically using --first-parent (with every merge\n> encountered, the probability of correctness is multiplied by 0.5 at\n> most/least), you are going to do them a favour anyway by fixing the code,\n> then why not simplify the convolution and make the code rock-steady (and\n> implement my patch)?\n\nThe current 'next' branch in git.git contains your patch with my fixup\non top and I believe this fixes _both_ the original issue with\nfirst-parent (thanks to your patch) and the issue Nanako discovered\n(thanks to my patch). Am I missing something?\n\n-- \nlarsh\n"},{"id":"76938","messageId":"20080514111034.GA29387@cuci.nl","threadId":"13473","inReplyTo":"8c5c35580805140354s62301343n62f8319b1853bfbd@mail.gmail.com","subject":"Re: [PATCH v2] revision.c: really honor --first-parent","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-05-14T11:10:34Z","receivedAt":"2008-05-14T11:10:34Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Lars Hjemli wrote:\n>On Wed, May 14, 2008 at 12:34 PM, Stephen R. van den Berg wrote:\n>> So, I'd say, since the current code does not and cannot work reliably\n>> for anyone specifically using --first-parent (with every merge\n>> encountered, the probability of correctness is multiplied by 0.5 at\n>> most/least), you are going to do them a favour anyway by fixing the code,\n>> then why not simplify the convolution and make the code rock-steady (and\n>> implement my patch)?\n\n>The current 'next' branch in git.git contains your patch with my fixup\n>on top and I believe this fixes _both_ the original issue with\n>first-parent (thanks to your patch) and the issue Nanako discovered\n>(thanks to my patch). Am I missing something?\n\nProbably not. I didn't check 'next' yet, since neither mine nor your\npatch had been Acked on the list (I guess it shows that I don't know the\nprocedures here all too well yet).\n-- \nSincerely, srb@cuci.nl\n Stephen R. van den Berg.\n\nWhat if there were no hypothetical questions?\n"}]}