{"thread":{"id":"31485","subject":"Probable bug in file run-command.c function clear_child_for_cleanup","startedAt":"2012-09-09T14:44:54Z","lastAt":"2012-09-11T08:40:51Z","messageCount":8,"participants":["David Gould","Jeff King","Erik Faye-Lund","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"198631","messageId":"504CAB66.1050003@optimisefitness.com","threadId":"31485","inReplyTo":null,"subject":"Probable bug in file run-command.c function clear_child_for_cleanup","fromName":"David Gould","fromEmail":"david@optimisefitness.com","sentAt":"2012-09-09T14:44:54Z","receivedAt":"2012-09-09T14:44:54Z","isPatch":false,"sender":{"key":"david@optimisefitness.com","avatar":"https://gravatar.com/avatar/55b32f8c7c7663aca403d7489f057d37a7b93a06af25da539aa6ef6ae984cb98?d=mp&s=160"},"body":"Hi,\n\nThis is probably the wrong way to do this, and I'm sorry if I end up \nwasting someone's time. That said, here goes....\n\nWhile idly browsing through the git source (as you do on a sunny Sunday \nafternoon), I spotted the following code (that appears to be wrong) in \nthe file https://github.com/git/git/blob/master/run-command.c It's the \nsame in branches maint, next and pu. The branch todo gives me a 404.\n\n(line 53 is here)\nstatic void clear_child_for_cleanup(pid_t pid)\n{\n\tstruct child_to_clean **last, *p;\n\n\tlast = &children_to_clean;\n\tfor (p = children_to_clean; p; p = p->next) {\n\t\tif (p->pid == pid) {\n\t\t\t*last = p->next;\n\t\t\tfree(p);\n\t\t\treturn;\n\t\t}\n\t}\n}\n(line 67 is here)\n\nIt appears that last is intended to point to the next field that's being \nupdated, but fails to \"follow\" the p pointer along the chain. The result \nis that children_to_clean will end up pointing to the entry after the \ndeleted one, and all the entries before it will be lost. It'll only be \nfine in the case that the pid is that of the first entry in the chain.\n\nYou want something like:\n\nfor (... {\n\tif (... {\n\t\t...\n\t}\n\tlast = &p->next;\n}\n\nor (probably clearer, but I haven't read your coding style guide, if \nthere is one, and some people don't like this approach)\n\nfor (p = children_to_clean; p; last = &p->next, p = p->next) {\n\t...\n\nCheers,\nDavid\n\n-- \nDavid Gould, Personal Trainer\n\tRegister of Kettlebell Professionals\n\tINWA Nordic Walking Instructor\nOptimise Fitness Ltd -- fit for life\n01264 720709\nwww.optimisefitness.com\n"},{"id":"198672","messageId":"20120910134406.GA7496@sigill.intra.peff.net","threadId":"31485","inReplyTo":"504CAB66.1050003@optimisefitness.com","subject":"Re: Probable bug in file run-command.c function clear_child_for_cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T13:44:06Z","receivedAt":"2012-09-10T13:44:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 09, 2012 at 03:44:54PM +0100, David Gould wrote:\n\n> static void clear_child_for_cleanup(pid_t pid)\n> {\n> \tstruct child_to_clean **last, *p;\n> \n> \tlast = &children_to_clean;\n> \tfor (p = children_to_clean; p; p = p->next) {\n> \t\tif (p->pid == pid) {\n> \t\t\t*last = p->next;\n> \t\t\tfree(p);\n> \t\t\treturn;\n> \t\t}\n> \t}\n> }\n> \n> It appears that last is intended to point to the next field that's\n> being updated, but fails to \"follow\" the p pointer along the chain.\n> The result is that children_to_clean will end up pointing to the\n> entry after the deleted one, and all the entries before it will be\n> lost. It'll only be fine in the case that the pid is that of the\n> first entry in the chain.\n\nYes, it's a bug. We should update \"last\" on each iteration.\n\n> You want something like:\n> \n> for (... {\n> \tif (... {\n> \t\t...\n> \t}\n> \tlast = &p->next;\n> }\n> \n> or (probably clearer, but I haven't read your coding style guide, if\n> there is one, and some people don't like this approach)\n\nYes, that's the correct fix. Care to submit a patch?\n\n> for (p = children_to_clean; p; last = &p->next, p = p->next) {\n> \t...\n\nThat is OK, too, but I think I prefer the first one.\n\n-Peff\n"},{"id":"198673","messageId":"CABPQNSaufY9vUAFp9VL2i=6bNmO5_2+anzX6+omfq+UgP03kXg@mail.gmail.com","threadId":"31485","inReplyTo":"20120910134406.GA7496@sigill.intra.peff.net","subject":"Re: Probable bug in file run-command.c function clear_child_for_cleanup","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-09-10T13:58:40Z","receivedAt":"2012-09-10T13:58:40Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Sep 10, 2012 at 3:44 PM, Jeff King wrote:\n> On Sun, Sep 09, 2012 at 03:44:54PM +0100, David Gould wrote:\n>> You want something like:\n>>\n>> for (... {\n>> if (... {\n>> ...\n>> }\n>> last = &p->next;\n>> }\n>>\n>> or (probably clearer, but I haven't read your coding style guide, if\n>> there is one, and some people don't like this approach)\n>\n> Yes, that's the correct fix. Care to submit a patch?\n>\n>> for (p = children_to_clean; p; last = &p->next, p = p->next) {\n>> ...\n>\n> That is OK, too, but I think I prefer the first one.\n>\n\nI feel like bikeshedding a bit today!\n\nI tend to either prefer either the latter or something like this:\n\nwhile (p) {\n\t...\n\n\tlast = p;\n\tp = p->next;\n}\n\nbecause those approaches put all the iteration logic in the same\nplace. The in-body traversal approach is a bit more explicit about the\ntraversal details.\n\nAnd to conclude my bikeshedding for the day: Shouldn't \"last\" ideally\nbe called something like \"prev\" instead? It's the previously visited\nelement, not the last element in the list.\n"},{"id":"198675","messageId":"20120910141027.GA8284@sigill.intra.peff.net","threadId":"31485","inReplyTo":"CABPQNSaufY9vUAFp9VL2i=6bNmO5_2+anzX6+omfq+UgP03kXg@mail.gmail.com","subject":"Re: Probable bug in file run-command.c function clear_child_for_cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T14:10:27Z","receivedAt":"2012-09-10T14:10:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2012 at 03:58:40PM +0200, Erik Faye-Lund wrote:\n\n> >> for (... {\n> >> if (... {\n> >> ...\n> >> }\n> >> last = &p->next;\n> >> }\n> [...]\n> I feel like bikeshedding a bit today!\n> \n> I tend to either prefer either the latter or something like this:\n> \n> while (p) {\n> \t...\n> \n> \tlast = p;\n> \tp = p->next;\n> }\n> \n> because those approaches put all the iteration logic in the same\n> place. The in-body traversal approach is a bit more explicit about the\n> traversal details.\n\nAlso fine by me.\n\n> And to conclude my bikeshedding for the day: Shouldn't \"last\" ideally\n> be called something like \"prev\" instead? It's the previously visited\n> element, not the last element in the list.\n\nIt is the \"last\" element visited (just as \"last week\" is not the end of\nthe world), but yes, it is ambiguous, and \"prev\" is not. Either is fine\nby me.\n\n-Peff\n"},{"id":"198716","messageId":"7v8vchodgc.fsf@alter.siamese.dyndns.org","threadId":"31485","inReplyTo":"20120910141027.GA8284@sigill.intra.peff.net","subject":"Re: Probable bug in file run-command.c function clear_child_for_cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T20:00:35Z","receivedAt":"2012-09-10T20:00:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Sep 10, 2012 at 03:58:40PM +0200, Erik Faye-Lund wrote:\n>\n>> >> for (... {\n>> >> if (... {\n>> >> ...\n>> >> }\n>> >> last = &p->next;\n>> >> }\n>> [...]\n>> I feel like bikeshedding a bit today!\n>> \n>> I tend to either prefer either the latter or something like this:\n>> \n>> while (p) {\n>> \t...\n>> \n>> \tlast = p;\n>> \tp = p->next;\n>> }\n>> \n>> because those approaches put all the iteration logic in the same\n>> place. The in-body traversal approach is a bit more explicit about the\n>> traversal details.\n>\n> Also fine by me.\n>\n>> And to conclude my bikeshedding for the day: Shouldn't \"last\" ideally\n>> be called something like \"prev\" instead? It's the previously visited\n>> element, not the last element in the list.\n>\n> It is the \"last\" element visited (just as \"last week\" is not the end of\n> the world), but yes, it is ambiguous, and \"prev\" is not. Either is fine\n> by me.\n\nOK, so who's gonna do the honors?\n"},{"id":"198717","messageId":"20120910200110.GA32437@sigill.intra.peff.net","threadId":"31485","inReplyTo":"7v8vchodgc.fsf@alter.siamese.dyndns.org","subject":"Re: Probable bug in file run-command.c function clear_child_for_cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T20:01:10Z","receivedAt":"2012-09-10T20:01:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote:\n\n> >> And to conclude my bikeshedding for the day: Shouldn't \"last\" ideally\n> >> be called something like \"prev\" instead? It's the previously visited\n> >> element, not the last element in the list.\n> >\n> > It is the \"last\" element visited (just as \"last week\" is not the end of\n> > the world), but yes, it is ambiguous, and \"prev\" is not. Either is fine\n> > by me.\n> \n> OK, so who's gonna do the honors?\n\nI was hoping to give David a chance to submit his first-ever patch to\ngit.\n\n-Peff\n"},{"id":"198719","messageId":"7v4nn5ocvv.fsf@alter.siamese.dyndns.org","threadId":"31485","inReplyTo":"20120910200110.GA32437@sigill.intra.peff.net","subject":"Re: Probable bug in file run-command.c function clear_child_for_cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-10T20:12:52Z","receivedAt":"2012-09-10T20:12:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote:\n>\n>> >> And to conclude my bikeshedding for the day: Shouldn't \"last\" ideally\n>> >> be called something like \"prev\" instead? It's the previously visited\n>> >> element, not the last element in the list.\n>> >\n>> > It is the \"last\" element visited (just as \"last week\" is not the end of\n>> > the world), but yes, it is ambiguous, and \"prev\" is not. Either is fine\n>> > by me.\n>> \n>> OK, so who's gonna do the honors?\n>\n> I was hoping to give David a chance to submit his first-ever patch to\n> git.\n\nOK. David, is it OK for us to expect a patch from you sometime not\nin distant future (it is an old bug we survived for a long time and\nnothing ultra-urgent)?\n"},{"id":"198756","messageId":"504EF913.2050202@optimisefitness.com","threadId":"31485","inReplyTo":"7v4nn5ocvv.fsf@alter.siamese.dyndns.org","subject":"Re: Probable bug in file run-command.c function clear_child_for_cleanup","fromName":"David Gould","fromEmail":"david@optimisefitness.com","sentAt":"2012-09-11T08:40:51Z","receivedAt":"2012-09-11T08:40:51Z","isPatch":false,"sender":{"key":"david@optimisefitness.com","avatar":"https://gravatar.com/avatar/55b32f8c7c7663aca403d7489f057d37a7b93a06af25da539aa6ef6ae984cb98?d=mp&s=160"},"body":"Hi guys,\n\nSorry for the delayed reply - what passes for my real life intruded \nsomewhat.\n\nI'll get on to it today, but please be aware this will be my first-ever \npatch for ANY project, so am likely to foul up the process.\n\nI am reading the How To Submit Patches document even now....\n\nCheers,\nDavid\n\nOn 10/09/12 21:12, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote:\n>>\n>>>>> And to conclude my bikeshedding for the day: Shouldn't \"last\" ideally\n>>>>> be called something like \"prev\" instead? It's the previously visited\n>>>>> element, not the last element in the list.\n>>>>\n>>>> It is the \"last\" element visited (just as \"last week\" is not the end of\n>>>> the world), but yes, it is ambiguous, and \"prev\" is not. Either is fine\n>>>> by me.\n>>>\n>>> OK, so who's gonna do the honors?\n>>\n>> I was hoping to give David a chance to submit his first-ever patch to\n>> git.\n>\n> OK. David, is it OK for us to expect a patch from you sometime not\n> in distant future (it is an old bug we survived for a long time and\n> nothing ultra-urgent)?\n>\n\n-- \nDavid Gould, Personal Trainer\n\tRegister of Kettlebell Professionals\n\tINWA Nordic Walking Instructor\nOptimise Fitness Ltd -- fit for life\n01264 720709\nwww.optimisefitness.com\n"}]}