{"thread":{"id":"13258","subject":"[PATCH] Simplify and fix --first-parent implementation","startedAt":"2008-04-25T18:10:46Z","lastAt":"2008-04-26T19:13:38Z","messageCount":5,"participants":["Stephen R. van den Berg","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"75199","messageId":"20080425234556.D60FD5461@aristoteles.cuci.nl","threadId":"13258","inReplyTo":null,"subject":"[PATCH] Simplify and fix --first-parent implementation","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-04-25T18:10:46Z","receivedAt":"2008-04-25T18:10:46Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"---\n revision.c |   10 ++++------\n 1 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 4231ea2..bcfcd2a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -415,7 +415,6 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str\n {\n \tstruct commit_list *parent = commit->parents;\n \tunsigned left_flag;\n-\tint add, rest;\n \n \tif (commit->object.flags & ADDED)\n \t\treturn 0;\n@@ -462,19 +461,18 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str\n \n \tleft_flag = (commit->object.flags & SYMMETRIC_LEFT);\n \n-\trest = !revs->first_parent_only;\n-\tfor (parent = commit->parents, add = 1; parent; add = rest) {\n+\tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n \n-\t\tparent = parent->next;\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\tif (add)\n-\t\t\tinsert_by_date(p, list);\n+\t\tinsert_by_date(p, list);\n+\t\tif(revs->first_parent_only)\n+\t\t\tbreak;\n \t}\n \treturn 0;\n }\n-- \n1.5.5.1.83.ge77a4.dirty\n"},{"id":"75206","messageId":"7viqy5o4om.fsf@gitster.siamese.dyndns.org","threadId":"13258","inReplyTo":"20080425234556.D60FD5461@aristoteles.cuci.nl","subject":"Re: [PATCH] Simplify and fix --first-parent implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-26T00:11:53Z","receivedAt":"2008-04-26T00:11:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n\n> ---\n\nNo explanation of what the patch does, nor justification of why it is a\ngood change?\n\nPlease also sign your patch.\n\n> @@ -462,19 +461,18 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str\n>  \n>  \tleft_flag = (commit->object.flags & SYMMETRIC_LEFT);\n>  \n> -\trest = !revs->first_parent_only;\n> -\tfor (parent = commit->parents, add = 1; parent; add = rest) {\n> +\tfor (parent = commit->parents; parent; parent = parent->next) {\n>  \t\tstruct commit *p = parent->item;\n>  \n> -\t\tparent = parent->next;\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\tif (add)\n> -\t\t\tinsert_by_date(p, list);\n> +\t\tinsert_by_date(p, list);\n> +\t\tif(revs->first_parent_only)\n> +\t\t\tbreak;\n>  \t}\n\nThe original code makes sure all the parents of the given commits are\nmarked as SEEN (and SYMMETRIC_LEFT if needed), even when only it traverses\nthe first parent.  By leaving the loop early, you are changing the\nsemantics of the code.  Other parents, when reached from other paths while\ntraversing the commit ancestry chain, will behave differently between the\nversion with your patch and without.\n\nYou would need to explain how and why that behaviour change is a \"fix\" in\nthe proposed commit log message.  \"Before the change, traversing a history\nof this shape behaves like this, but that is wrong for such and such\nreasons, and I identified the culprit to be this code.  With this patch,\nthe same traversal will instead behave this way, which is what I expect to\nbe the right behaviour\".\n"},{"id":"75215","messageId":"20080426115956.GB19558@cuci.nl","threadId":"13258","inReplyTo":"7viqy5o4om.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Simplify and fix --first-parent implementation","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-04-26T11:59:56Z","receivedAt":"2008-04-26T11:59:56Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n>> ---\n>No explanation of what the patch does, nor justification of why it is a\n>good change?\n\nSorry, thought that one-line description was enough.  I'll resubmit this\none more verbosely.\n\n>Please also sign your patch.\n\nJust with git-commit -s ?  Or do I need to get gpg involved?\n\n>The original code makes sure all the parents of the given commits are\n>marked as SEEN (and SYMMETRIC_LEFT if needed), even when only it traverses\n>the first parent.  By leaving the loop early, you are changing the\n>semantics of the code.  Other parents, when reached from other paths while\n>traversing the commit ancestry chain, will behave differently between the\n>version with your patch and without.\n\nYes, I was and am aware of that.  The previous behaviour appears to be bogus.\nI'll elaborate on resubmission.\n\nP.S. No reaction on the other patches means that they're accepted, or do\nI need to resubmit them as well (signed)?\n-- \nSincerely,                                                          srb@cuci.nl\n           Stephen R. van den Berg.\n"},{"id":"75216","messageId":"20080426122741.GA32457@sigill.intra.peff.net","threadId":"13258","inReplyTo":"20080426115956.GB19558@cuci.nl","subject":"Re: [PATCH] Simplify and fix --first-parent implementation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-04-26T12:27:42Z","receivedAt":"2008-04-26T12:27:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 26, 2008 at 01:59:56PM +0200, Stephen R. van den Berg wrote:\n\n> >Please also sign your patch.\n> \n> Just with git-commit -s ?  Or do I need to get gpg involved?\n\nWith git-commit -s; check out Documentation/SubmittingPatches.\n\n> P.S. No reaction on the other patches means that they're accepted, or do\n> I need to resubmit them as well (signed)?\n\nI for one would have liked to see more explanation on your \"check for\ncircular references\" patch. I am not clear on what circumstances create\nsuch circular references.\n\n-Peff\n"},{"id":"75232","messageId":"7vfxt8mntp.fsf@gitster.siamese.dyndns.org","threadId":"13258","inReplyTo":"20080426115956.GB19558@cuci.nl","subject":"Re: [PATCH] Simplify and fix --first-parent implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-26T19:13:38Z","receivedAt":"2008-04-26T19:13:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n\n> P.S. No reaction on the other patches means that they're accepted, or do\n> I need to resubmit them as well (signed)?\n\nNo reaction means just that.  I either haven't looked at them, or after\nhaving looked at them I did not find them interesting enough to comment\non.\n\nThe latter does not mean they are rejected, though.  Remember, my review\nis NOT the only thing that counts here, other people's review and\ncomment too here, and it tends to take time.\n\nThe proposed commit log message would express why the change was needed.\nIt would present a use case that is useful (and argue why that use case is\nworth supporting), that is not easily supported with the existing code,\nand how the patch makes it so.  When I review a patch posted on the list,\nhere are the things I consider.  This is pretty much personal, and other\npeople may do things in different order:\n\n (0) The merit of the patch itself is not obvious from the diff, but there\n     is no explanation; or\n\n (1) The argument to support the use case may not be convincing or may be\n     outright wrong; or\n\n (2) Even if the argument is convincing, the claim that the current code\n     does not support it may be false; or\n\n (3) The patch may not be the right way to support it and there may be\n     better ways; or\n\n (4) The patch may make the new use case supported, but breaks existing\n     use cases.\n\nI look at (0) to _guess_ why the submitter thought the patch was a good\nidea when I have nothing better to do (but that seldom happens these days)\nor the submitter is new to the list.\n\nPatches in (1) and (2) categories may get comments on what the patch tries\nto achieve, and for that I do not have to look at the diff.  I tend start\nto look at the diff for patches in categories (3) and (4).\n\nThe ideal case is obviously:\n\n (5) The merit of the patch itself is very clear and there is a good\n     explanation in the commit log message.\n"}]}