Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:
> On Thu, Jan 19, 2012 at 11:58 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> Comments?
>
> Looks conceptually right, but I do have to admit to hating that new variable.
>
> I don't see a better way to do it, though. Sure, you could do it with just
>
> if (revs->first_parent_only && pp != &commit->parents)
> break;
>
> and avoid the new variable that way, but that replaces the annoying
> variable with a pretty subtle thing.
>
> Or we could re-write that while() loop and move the 'parent' variable
> into it. Like the appended untested thing.
>
> But maybe your patch is better, and my dislike for that parent counter
> is just irrational.
I didn't like that parent counter that _only_ increments when we are
running under first-parent-only mode at the conceptual level. At the
implementation level, of course it is the right thing to do because
outside first-parent-only mode nobody cares about the parent counter,
so it is a valid but subtle optimization.
But I personally find your loop
do {
...
} while (!revs->first_parent_only);
is even more disgusting. It is misleading to have something that is not
supposed to change inside the loop as the terminating condition as if we
are saying "loop until somebody flips that bit" which is clearly not the
case.
So obviously I am saying that I do not think either patch is pretty
without offering a better alternative implementation, which is my usual
badness. As this is not an ultra urgent fix, I'll wait and see if somebody
else comes up with a more readable version.
Thanks for eyeballing the logic side of it, anyway. That was what I was
worried about the change the most.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html