Re: First/oldest entry in reflog dropped

Jeff King [off-list ref] writes:

On Sun, Nov 21, 2010 at 12:36:21PM +0100, Johannes Schindelin wrote:

quoted

On Sun, 21 Nov 2010, Jeff King wrote:

quoted

This patch clears up your bug, and doesn't break any tests. But I'd 
really like to get a second opinion on the significance of those other 
flags, or why the flag clearing was at the bottom of the function in the 
first place.

The flag clearing was at the bottom because I had the impression that 
something function one might want to call in that function in the future 
could set the flags again. Maybe a goto would be appropriate here instead 
of the early returns?

That makes sense. I did a quick skim of the called code and I'm not sure
any flags could be set, but even if I am right, I think it is better to
be defensive.

So let's do this, which is the equivalent behavior to your gotos, but
this structure makes more sense to me as a reader (and it doesn't
involve goto :) ).

I have a feeling that we probably should have structured the code a bit
more modularly, placing some info in "rev" that tells us how to "pop" a
commit from the rev->list (typically "not the ones that we have shown"),
what other commits to push back into the queue (typically, "all the
parents that are not interesting"), and what side effects we should cause
when we do so (typically, "mark uninteresting parents"), etc., instead of
the current "if we are walking reflog, here is the special codepath we
take", so that the walking is more generalized when we did the reflog
walking (in fact, if we did this properly we probably wouldn't be calling
it "bolted on").  But for now let's refrain from doing such a rewrite.

Thanks, both.

On Tue, Nov 23, 2010 at 04:35:52PM -0800, Junio C Hamano wrote:

quoted

So let's do this, which is the equivalent behavior to your gotos, but
this structure makes more sense to me as a reader (and it doesn't
involve goto :) ).

I have a feeling that we probably should have structured the code a bit
more modularly, placing some info in "rev" that tells us how to "pop" a
commit from the rev->list (typically "not the ones that we have shown"),
what other commits to push back into the queue (typically, "all the
parents that are not interesting"), and what side effects we should cause
when we do so (typically, "mark uninteresting parents"), etc., instead of
the current "if we are walking reflog, here is the special codepath we
take", so that the walking is more generalized when we did the reflog
walking (in fact, if we did this properly we probably wouldn't be calling
it "bolted on").  But for now let's refrain from doing such a rewrite.

FWIW, I agree with all of that. I don't think it's worth fixing now, but
perhaps in libgit2. :)

-Peff