[PATCH] clear_child_for_cleanup must correctly manage children_to_clean

2012-09-11 Thread David Gould 
Iterate through children_to_clean using 'next' fields but with an
extra level of indirection. This allows us to update the chain when
we remove a child and saves us managing several variables around
the loop mechanism.
---
 run-command.c |   12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/run-command.c b/run-command.c
index f9922b9..c42d63b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -53,13 +53,13 @@ static void mark_child_for_cleanup(pid_t pid)
 
 static void clear_child_for_cleanup(pid_t pid)
 {
-   struct child_to_clean **last, *p;
+   struct child_to_clean **pp;
 
-   last = children_to_clean;
-   for (p = children_to_clean; p; p = p-next) {
-   if (p-pid == pid) {
-   *last = p-next;
-   free(p);
+   for (pp = children_to_clean; *pp; pp = (*pp)-next) {
+   if ((*pp)-pid == pid) {
+   struct child_to_clean *clean_me = *pp;
+   *pp = clean_me-next;
+   free(clean_me);
return;
}
}
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clear_child_for_cleanup must correctly manage children_to_clean

2012-09-11 Thread Jeff King 
On Tue, Sep 11, 2012 at 03:32:47PM +0100, David Gould wrote:

 Subject: Re: [PATCH] clear_child_for_cleanup must correctly manage
   children_to_clean

Thanks for the patch. Overall it looks good, but let me nit-pick your
commit message a little (not because it is that horrible, but because
you are so close to perfect that I want to fix the minor things and then
encourage you to submit more patches :) ).

Your subject is a bit vague, and it is not clear if it is not correct
now, and this is a bugfix, or if it is a feature enhancement. I would
have said something like:

  Subject: fix broken list iteration in clear_child_for_cleanup

which is _also_ vague about what exactly the breakage is, but is clear
that this is a bugfix. So then you can go on to describe the actual
problem:

  We iterate through the list of children to cleanup, but do not keep
  our last pointer up to date. As a result, we may end up cutting off
  part of the list instead of removing a single element.

And then describe your fix:

 Iterate through children_to_clean using 'next' fields but with an
 extra level of indirection. This allows us to update the chain when
 we remove a child and saves us managing several variables around
 the loop mechanism.

which I think is good.

 - last = children_to_clean;
 - for (p = children_to_clean; p; p = p-next) {
 - if (p-pid == pid) {
 - *last = p-next;
 - free(p);
 + for (pp = children_to_clean; *pp; pp = (*pp)-next) {
 + if ((*pp)-pid == pid) {
 + struct child_to_clean *clean_me = *pp;
 + *pp = clean_me-next;
 + free(clean_me);
   return;
   }

I think using the indirect pointer is a nice compromise; it makes it
clear from just the for loop that this is not an ordinary for-each
traversal. You could hoist the extra pointer out of the conditional and
save one set of parentheses in the if statement, but I don't think it
is a big deal either way.

Acked-by: Jeff King p...@peff.net

Thanks for the bug report and the patch.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clear_child_for_cleanup must correctly manage children_to_clean

2012-09-11 Thread Erik Faye-Lund 
On Tue, Sep 11, 2012 at 4:32 PM, David Gould da...@optimisefitness.com wrote:
 Iterate through children_to_clean using 'next' fields but with an
 extra level of indirection. This allows us to update the chain when
 we remove a child and saves us managing several variables around
 the loop mechanism.
 ---
  run-command.c |   12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

 diff --git a/run-command.c b/run-command.c
 index f9922b9..c42d63b 100644
 --- a/run-command.c
 +++ b/run-command.c
 @@ -53,13 +53,13 @@ static void mark_child_for_cleanup(pid_t pid)

  static void clear_child_for_cleanup(pid_t pid)
  {
 -   struct child_to_clean **last, *p;
 +   struct child_to_clean **pp;

 -   last = children_to_clean;
 -   for (p = children_to_clean; p; p = p-next) {
 -   if (p-pid == pid) {
 -   *last = p-next;
 -   free(p);
 +   for (pp = children_to_clean; *pp; pp = (*pp)-next) {
 +   if ((*pp)-pid == pid) {
 +   struct child_to_clean *clean_me = *pp;
 +   *pp = clean_me-next;
 +   free(clean_me);
 return;
 }
 }
 --
 1.7.9.5


Looks good to me. Thanks :)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clear_child_for_cleanup must correctly manage children_to_clean

2012-09-11 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 Thanks for the patch. Overall it looks good, but let me nit-pick your
 commit message a little (not because it is that horrible, but because
 you are so close to perfect that I want to fix the minor things and then
 encourage you to submit more patches :) ).
 ...

 Acked-by: Jeff King p...@peff.net

Thanks, both.  I'd have to ask for a Sign-off, though.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html