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

From: Jeff King <peff-adepduraxsq@public.gmane.org>
Date: May 7, 2012, 21:37 UTC
Message-ID: <20120507213752.GA19911@sigill.intra.peff.net>
In-Reply-To: <7v7gwrc212.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>

On Fri, May 04, 2012 at 10:02:49AM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Jeff King <peff-AdEPDUrAXsQ@public.gmane.org> writes:
> 
> > This patch flips the rules to:
> >
> >   1. if the user asked for ref@{0}, always show the index
> >
> >   2. if the user asked for ref@{now}, always show the date
> >
> >   3. otherwise, we have just "ref"; show them counted by
> >      default, but respect the presence of "--date" as a clue
> >      that the user wanted them date-based
> 
> The revision.c parser for "git log --date=default -g master" would flip
> the "explicit" bit, revs->date_mode is set to DATE_NORMAL, and that value
> will eventually come as dmode here.
> [...]
> But DATE_NORMAL happens to be zero ;-) "git log --date=default -g master"
> would still show the counted version.

Yeah, I noticed that, but decided not to tackle it, as nobody had really complained (and you can get the behavior you want with master@{now}). However, I agree it would be better for "--date=default" to trigger the date-based selector.

> I personally do not care about that behaviour, but I know that I will
> later later have to deal with people who do care, which is annoying.

Maybe. It has been that way for years and nobody has yet complained. :)

> Probably we would internally need to define two values to ask for the
> DATE_NORMAL output.  Move DATE_NORMAL to non-zero value, introduce a new
> DATE_DEFAULT that is zero, and make their output identical, perhaps
> something like the attached (not even compile tested).

I think that is the right way forward. I am worried that we will end up with parts of the code that do not handle the distinction properly (see below). But maybe it is best to try it and shake the bugs out.

> The implicit comparison to zero in the above is a bad code (but that is
> a problem from the very old days).

It is. The enum at least explicitly starts at 0 for this reason, but I don't mind at all if it is updated to an explicit '!= DATE_NORMAL'.

Show 27 quoted lines

> diff --git a/cache.h b/cache.h
> index 58ff054..fe42e80 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -876,7 +876,8 @@ extern struct object *peel_to_type(const char *name, int namelen,
>  				   struct object *o, enum object_type);
>  
>  enum date_mode {
> -	DATE_NORMAL = 0,
> +	DATE_DEFAULT = 0,
> +	DATE_NORMAL,
>  	DATE_RELATIVE,
>  	DATE_SHORT,
>  	DATE_LOCAL,
> diff --git a/reflog-walk.c b/reflog-walk.c
> index b974258..d002516 100644
> --- a/reflog-walk.c
> +++ b/reflog-walk.c
> @@ -277,7 +277,7 @@ void get_reflog_selector(struct strbuf *sb,
>  
>  	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.

-Peff