Re: Probable bug in file run-command.c function clear_child_for_cleanup

2012-09-11 Thread David Gould 

Hi guys,

Sorry for the delayed reply - what passes for my real life intruded 
somewhat.


I'll get on to it today, but please be aware this will be my first-ever 
patch for ANY project, so am likely to foul up the process.


I am reading the How To Submit Patches document even now

Cheers,
David

On 10/09/12 21:12, Junio C Hamano wrote:

Jeff King p...@peff.net writes:


On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote:


And to conclude my bikeshedding for the day: Shouldn't last ideally
be called something like prev instead? It's the previously visited
element, not the last element in the list.


It is the last element visited (just as last week is not the end of
the world), but yes, it is ambiguous, and prev is not. Either is fine
by me.


OK, so who's gonna do the honors?


I was hoping to give David a chance to submit his first-ever patch to
git.


OK. David, is it OK for us to expect a patch from you sometime not
in distant future (it is an old bug we survived for a long time and
nothing ultra-urgent)?



--
David Gould, Personal Trainer
Register of Kettlebell Professionals
INWA Nordic Walking Instructor
Optimise Fitness Ltd -- fit for life
01264 720709
www.optimisefitness.com
--
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: Probable bug in file run-command.c function clear_child_for_cleanup

2012-09-10 Thread Jeff King 
On Sun, Sep 09, 2012 at 03:44:54PM +0100, David Gould wrote:

 static void clear_child_for_cleanup(pid_t pid)
 {
   struct child_to_clean **last, *p;
 
   last = children_to_clean;
   for (p = children_to_clean; p; p = p-next) {
   if (p-pid == pid) {
   *last = p-next;
   free(p);
   return;
   }
   }
 }
 
 It appears that last is intended to point to the next field that's
 being updated, but fails to follow the p pointer along the chain.
 The result is that children_to_clean will end up pointing to the
 entry after the deleted one, and all the entries before it will be
 lost. It'll only be fine in the case that the pid is that of the
 first entry in the chain.

Yes, it's a bug. We should update last on each iteration.

 You want something like:
 
 for (... {
   if (... {
   ...
   }
   last = p-next;
 }
 
 or (probably clearer, but I haven't read your coding style guide, if
 there is one, and some people don't like this approach)

Yes, that's the correct fix. Care to submit a patch?

 for (p = children_to_clean; p; last = p-next, p = p-next) {
   ...

That is OK, too, but I think I prefer the first one.

-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: Probable bug in file run-command.c function clear_child_for_cleanup

2012-09-10 Thread Erik Faye-Lund 
On Mon, Sep 10, 2012 at 3:44 PM, Jeff King p...@peff.net wrote:
 On Sun, Sep 09, 2012 at 03:44:54PM +0100, David Gould wrote:
 You want something like:

 for (... {
   if (... {
   ...
   }
   last = p-next;
 }

 or (probably clearer, but I haven't read your coding style guide, if
 there is one, and some people don't like this approach)

 Yes, that's the correct fix. Care to submit a patch?

 for (p = children_to_clean; p; last = p-next, p = p-next) {
   ...

 That is OK, too, but I think I prefer the first one.


I feel like bikeshedding a bit today!

I tend to either prefer either the latter or something like this:

while (p) {
...

last = p;
p = p-next;
}

because those approaches put all the iteration logic in the same
place. The in-body traversal approach is a bit more explicit about the
traversal details.

And to conclude my bikeshedding for the day: Shouldn't last ideally
be called something like prev instead? It's the previously visited
element, not the last element in the list.
--
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: Probable bug in file run-command.c function clear_child_for_cleanup

2012-09-10 Thread Jeff King 
On Mon, Sep 10, 2012 at 03:58:40PM +0200, Erik Faye-Lund wrote:

  for (... {
if (... {
...
}
last = p-next;
  }
 [...]
 I feel like bikeshedding a bit today!
 
 I tend to either prefer either the latter or something like this:
 
 while (p) {
   ...
 
   last = p;
   p = p-next;
 }
 
 because those approaches put all the iteration logic in the same
 place. The in-body traversal approach is a bit more explicit about the
 traversal details.

Also fine by me.

 And to conclude my bikeshedding for the day: Shouldn't last ideally
 be called something like prev instead? It's the previously visited
 element, not the last element in the list.

It is the last element visited (just as last week is not the end of
the world), but yes, it is ambiguous, and prev is not. Either is fine
by me.

-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: Probable bug in file run-command.c function clear_child_for_cleanup

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

 On Mon, Sep 10, 2012 at 03:58:40PM +0200, Erik Faye-Lund wrote:

  for (... {
if (... {
...
}
last = p-next;
  }
 [...]
 I feel like bikeshedding a bit today!
 
 I tend to either prefer either the latter or something like this:
 
 while (p) {
  ...
 
  last = p;
  p = p-next;
 }
 
 because those approaches put all the iteration logic in the same
 place. The in-body traversal approach is a bit more explicit about the
 traversal details.

 Also fine by me.

 And to conclude my bikeshedding for the day: Shouldn't last ideally
 be called something like prev instead? It's the previously visited
 element, not the last element in the list.

 It is the last element visited (just as last week is not the end of
 the world), but yes, it is ambiguous, and prev is not. Either is fine
 by me.

OK, so who's gonna do the honors?
--
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: Probable bug in file run-command.c function clear_child_for_cleanup

2012-09-10 Thread Jeff King 
On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote:

  And to conclude my bikeshedding for the day: Shouldn't last ideally
  be called something like prev instead? It's the previously visited
  element, not the last element in the list.
 
  It is the last element visited (just as last week is not the end of
  the world), but yes, it is ambiguous, and prev is not. Either is fine
  by me.
 
 OK, so who's gonna do the honors?

I was hoping to give David a chance to submit his first-ever patch to
git.

-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: Probable bug in file run-command.c function clear_child_for_cleanup

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

 On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote:

  And to conclude my bikeshedding for the day: Shouldn't last ideally
  be called something like prev instead? It's the previously visited
  element, not the last element in the list.
 
  It is the last element visited (just as last week is not the end of
  the world), but yes, it is ambiguous, and prev is not. Either is fine
  by me.
 
 OK, so who's gonna do the honors?

 I was hoping to give David a chance to submit his first-ever patch to
 git.

OK. David, is it OK for us to expect a patch from you sometime not
in distant future (it is an old bug we survived for a long time and
nothing ultra-urgent)?

--
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

Probable bug in file run-command.c function clear_child_for_cleanup

2012-09-09 Thread David Gould 

Hi,

This is probably the wrong way to do this, and I'm sorry if I end up 
wasting someone's time. That said, here goes


While idly browsing through the git source (as you do on a sunny Sunday 
afternoon), I spotted the following code (that appears to be wrong) in 
the file  https://github.com/git/git/blob/master/run-command.c It's the 
same in branches maint, next and pu. The branch todo gives me a 404.


(line 53 is here)
static void clear_child_for_cleanup(pid_t pid)
{
struct child_to_clean **last, *p;

last = children_to_clean;
for (p = children_to_clean; p; p = p-next) {
if (p-pid == pid) {
*last = p-next;
free(p);
return;
}
}
}
(line 67 is here)

It appears that last is intended to point to the next field that's being 
updated, but fails to follow the p pointer along the chain. The result 
is that children_to_clean will end up pointing to the entry after the 
deleted one, and all the entries before it will be lost. It'll only be 
fine in the case that the pid is that of the first entry in the chain.


You want something like:

for (... {
if (... {
...
}
last = p-next;
}

or (probably clearer, but I haven't read your coding style guide, if 
there is one, and some people don't like this approach)


for (p = children_to_clean; p; last = p-next, p = p-next) {
...

Cheers,
David

--
David Gould, Personal Trainer
Register of Kettlebell Professionals
INWA Nordic Walking Instructor
Optimise Fitness Ltd -- fit for life
01264 720709
www.optimisefitness.com
--
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