{"thread":{"id":"31503","subject":"[PATCH] clear_child_for_cleanup must correctly manage children_to_clean","startedAt":"2012-09-11T14:32:47Z","lastAt":"2012-09-11T20:32:50Z","messageCount":6,"participants":["David Gould","Jeff King","Erik Faye-Lund","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"198779","messageId":"1347373967-29248-1-git-send-email-david@optimisefitness.com","threadId":"31503","inReplyTo":"62cd8d4a1853cb6fe8fda9f534cc269c8b2e0f6c","subject":"[PATCH] clear_child_for_cleanup must correctly manage children_to_clean","fromName":"David Gould","fromEmail":"david@optimisefitness.com","sentAt":"2012-09-11T14:32:47Z","receivedAt":"2012-09-11T14:32:47Z","isPatch":true,"sender":{"key":"david@optimisefitness.com","avatar":"https://gravatar.com/avatar/55b32f8c7c7663aca403d7489f057d37a7b93a06af25da539aa6ef6ae984cb98?d=mp&s=160"},"body":"Iterate through children_to_clean using 'next' fields but with an\nextra level of indirection. This allows us to update the chain when\nwe remove a child and saves us managing several variables around\nthe loop mechanism.\n---\n run-command.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex f9922b9..c42d63b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -53,13 +53,13 @@ static void mark_child_for_cleanup(pid_t pid)\n \n static void clear_child_for_cleanup(pid_t pid)\n {\n-\tstruct child_to_clean **last, *p;\n+\tstruct child_to_clean **pp;\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+\tfor (pp = &children_to_clean; *pp; pp = &(*pp)->next) {\n+\t\tif ((*pp)->pid == pid) {\n+\t\t\tstruct child_to_clean *clean_me = *pp;\n+\t\t\t*pp = clean_me->next;\n+\t\t\tfree(clean_me);\n \t\t\treturn;\n \t\t}\n \t}\n-- \n1.7.9.5\n"},{"id":"198782","messageId":"20120911152041.GA11994@sigill.intra.peff.net","threadId":"31503","inReplyTo":"1347373967-29248-1-git-send-email-david@optimisefitness.com","subject":"Re: [PATCH] clear_child_for_cleanup must correctly manage children_to_clean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-11T15:20:41Z","receivedAt":"2012-09-11T15:20:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 11, 2012 at 03:32:47PM +0100, David Gould wrote:\n\n> Subject: Re: [PATCH] clear_child_for_cleanup must correctly manage\n> children_to_clean\n\nThanks for the patch. Overall it looks good, but let me nit-pick your\ncommit message a little (not because it is that horrible, but because\nyou are so close to perfect that I want to fix the minor things and then\nencourage you to submit more patches :) ).\n\nYour subject is a bit vague, and it is not clear if it is not correct\nnow, and this is a bugfix, or if it is a feature enhancement. I would\nhave said something like:\n\n Subject: fix broken list iteration in clear_child_for_cleanup\n\nwhich is _also_ vague about what exactly the breakage is, but is clear\nthat this is a bugfix. So then you can go on to describe the actual\nproblem:\n\n We iterate through the list of children to cleanup, but do not keep\n our \"last\" pointer up to date. As a result, we may end up cutting off\n part of the list instead of removing a single element.\n\nAnd then describe your fix:\n\n> Iterate through children_to_clean using 'next' fields but with an\n> extra level of indirection. This allows us to update the chain when\n> we remove a child and saves us managing several variables around\n> the loop mechanism.\n\nwhich I think is good.\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> +\tfor (pp = &children_to_clean; *pp; pp = &(*pp)->next) {\n> +\t\tif ((*pp)->pid == pid) {\n> +\t\t\tstruct child_to_clean *clean_me = *pp;\n> +\t\t\t*pp = clean_me->next;\n> +\t\t\tfree(clean_me);\n> \t\t\treturn;\n> \t\t}\n\nI think using the indirect pointer is a nice compromise; it makes it\nclear from just the for loop that this is not an ordinary for-each\ntraversal. You could hoist the extra pointer out of the conditional and\nsave one set of parentheses in the \"if\" statement, but I don't think it\nis a big deal either way.\n\nAcked-by: Jeff King \n\nThanks for the bug report and the patch.\n\n-Peff\n"},{"id":"198783","messageId":"CABPQNSYEiw2hw-S5x5YYJWrWYR6hNWBqudvHR=fxoHf0380jsA@mail.gmail.com","threadId":"31503","inReplyTo":"1347373967-29248-1-git-send-email-david@optimisefitness.com","subject":"Re: [PATCH] clear_child_for_cleanup must correctly manage children_to_clean","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-09-11T15:30:31Z","receivedAt":"2012-09-11T15:30:31Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Sep 11, 2012 at 4:32 PM, David Gould wrote:\n> Iterate through children_to_clean using 'next' fields but with an\n> extra level of indirection. This allows us to update the chain when\n> we remove a child and saves us managing several variables around\n> the loop mechanism.\n> ---\n> run-command.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index f9922b9..c42d63b 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -53,13 +53,13 @@ static void mark_child_for_cleanup(pid_t pid)\n>\n> static void clear_child_for_cleanup(pid_t pid)\n> {\n> - struct child_to_clean **last, *p;\n> + struct child_to_clean **pp;\n>\n> - last = &children_to_clean;\n> - for (p = children_to_clean; p; p = p->next) {\n> - if (p->pid == pid) {\n> - *last = p->next;\n> - free(p);\n> + for (pp = &children_to_clean; *pp; pp = &(*pp)->next) {\n> + if ((*pp)->pid == pid) {\n> + struct child_to_clean *clean_me = *pp;\n> + *pp = clean_me->next;\n> + free(clean_me);\n> return;\n> }\n> }\n> --\n> 1.7.9.5\n>\n\nLooks good to me. Thanks :)\n"},{"id":"198802","messageId":"7vpq5sh3dw.fsf@alter.siamese.dyndns.org","threadId":"31503","inReplyTo":"20120911152041.GA11994@sigill.intra.peff.net","subject":"Re: [PATCH] clear_child_for_cleanup must correctly manage children_to_clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-11T17:32:11Z","receivedAt":"2012-09-11T17:32:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Thanks for the patch. Overall it looks good, but let me nit-pick your\n> commit message a little (not because it is that horrible, but because\n> you are so close to perfect that I want to fix the minor things and then\n> encourage you to submit more patches :) ).\n> ...\n>\n> Acked-by: Jeff King \n\nThanks, both. I'd have to ask for a Sign-off, though.\n"},{"id":"198811","messageId":"1347394965-29906-1-git-send-email-david@optimisefitness.com","threadId":"31503","inReplyTo":"1347373967-29248-1-git-send-email-david@optimisefitness.com","subject":"[PATCHv2] fix broken list iteration in clear_child_for_cleanup","fromName":"David Gould","fromEmail":"david@optimisefitness.com","sentAt":"2012-09-11T20:22:45Z","receivedAt":"2012-09-11T20:22:45Z","isPatch":false,"sender":{"key":"david@optimisefitness.com","avatar":"https://gravatar.com/avatar/55b32f8c7c7663aca403d7489f057d37a7b93a06af25da539aa6ef6ae984cb98?d=mp&s=160"},"body":"We iterate through the list of children to cleanup, but do not keep\nour \"last\" pointer up to date. As a result, we may end up cutting off\npart of the list instead of removing a single element.\n\nIterate through children_to_clean using 'next' fields but with an\nextra level of indirection. This allows us to update the chain when\nwe remove a child and saves us managing several variables around\nthe loop mechanism.\n\nSigned-off-by: David Gould \nAcked-by: Jeff King \n---\nPATCHv2 updates PATCH only in the commit message: Peff suggested\nboth a helpful subject and a more-informative introduction.\n\n run-command.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex f9922b9..c42d63b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -53,13 +53,13 @@ static void mark_child_for_cleanup(pid_t pid)\n \n static void clear_child_for_cleanup(pid_t pid)\n {\n-\tstruct child_to_clean **last, *p;\n+\tstruct child_to_clean **pp;\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+\tfor (pp = &children_to_clean; *pp; pp = &(*pp)->next) {\n+\t\tif ((*pp)->pid == pid) {\n+\t\t\tstruct child_to_clean *clean_me = *pp;\n+\t\t\t*pp = clean_me->next;\n+\t\t\tfree(clean_me);\n \t\t\treturn;\n \t\t}\n \t}\n-- \n1.7.9.5\n"},{"id":"198813","messageId":"7vzk4wfggd.fsf@alter.siamese.dyndns.org","threadId":"31503","inReplyTo":"1347394965-29906-1-git-send-email-david@optimisefitness.com","subject":"Re: [PATCHv2] fix broken list iteration in clear_child_for_cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-11T20:32:50Z","receivedAt":"2012-09-11T20:32:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}