Jeff King <peff@xxxxxxxx> writes: > On Sun, Nov 21, 2010 at 12:36:21PM +0100, Johannes Schindelin wrote: > >> On Sun, 21 Nov 2010, Jeff King wrote: >> >> > 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. -- 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