Probable bug in file run-command.c function clear_child_for_cleanup

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