Re: [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors
From: Jeff King <hidden>
Date: 2016-06-15 22:53:46
On Fri, May 04, 2012 at 10:02:49AM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:quoted
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-basedThe 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'.
quoted hunk
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