Re: [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors

From: Jeff King <peff-adepduraxsq@public.gmane.org>
Date: May 10, 2012, 15:37 UTC
Message-ID: <20120510153754.GA23941@sigill.intra.peff.net>
In-Reply-To: <20120507213752.GA19911-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>

On Mon, May 07, 2012 at 05:37:52PM -0400, Jeff King wrote:

Show 12 quoted lines

> >  	strbuf_addf(sb, "%s@{", printed_ref);
> >  	if (commit_reflog->selector == SELECTOR_DATE ||
> > -	    (commit_reflog->selector == SELECTOR_NONE && dmode)) {
> > +	    (commit_reflog->selector == SELECTOR_NONE && (dmode != DATE_DEFAULT))) {
> >  		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
> >  		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));
> >  	} else {
> 
> I think some of the callers set dmode to DATE_NORMAL explicitly. So this
> code would be confused into thinking that the user had asked for it
> explicitly. Or maybe it happens before the date_mode_explicit check, and
> it would be OK. I'd have to do audit the code.

I just took a look at what you built on top of this topic (55ccf85) instead of the bit quoted above. I also found it ugly not to pass the explicit flag all the way down to the point-of-use. I had a nagging feeling that the original did not do it that way for some good reason, but looking at your patch, I cannot fathom what that reason could possibly be. So it looks good to me.

-Peff

PS It would have been nice to see the patch on the list for review. I
   only noticed it because it hit 'next', and had a minor conflict with
   my patches in the area.