diff mbox series

[1/3] Add 'human' date format

Message ID 20181231003150.8031-2-ischis2@cox.net (mailing list archive)
State New, archived
Headers show
Series Add 'human' date format | expand

Commit Message

Stephen P. Smith Dec. 31, 2018, 12:31 a.m. UTC 
From: Linus Torvalds <torvalds@linux-foundation.org>

This adds --date=human, which skips the timezone if it matches the
current time-zone, and doesn't print the whole date if that matches (ie
skip printing year for dates that are "this year", but also skip the
whole date itself if it's in the last few days and we can just say what
weekday it was).

For really recent dates (same day), use the relative date stamp, while
for old dates (year doesn't match), don't bother with time and timezone.

Also add 'auto' date mode, which defaults to human if we're using the
pager.  So you can do

	git config --add log.date auto

and your "git log" commands will show the human-legible format unless
you're scripting things.

Note that this time format still shows the timezone for recent enough
events (but not so recent that they show up as relative dates).  You can
combine it with the "-local" suffix to never show timezones for an even
more simplified view.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 builtin/blame.c |   4 ++
 cache.h         |   1 +
 date.c          | 130 ++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 115 insertions(+), 20 deletions(-)

Comments

Jeff King Jan. 3, 2019, 7:37 a.m. UTC | #1 
On Sun, Dec 30, 2018 at 05:31:48PM -0700, Stephen P. Smith wrote:

> Also add 'auto' date mode, which defaults to human if we're using the
> pager.  So you can do
> 
> 	git config --add log.date auto
> 
> and your "git log" commands will show the human-legible format unless
> you're scripting things.

I like the idea of "human", and I like the idea of "auto", but it seems
to me that these are really two orthogonal things. E.g., might some
people not want to do something like:

  git config log.date auto:relative

?

I don't personally care about using this myself, but we already had to
deal with retrofitting "local" as a modifier. I'd prefer to avoid making
the same mistake again.

(I'd actually argue that "log.date" should basically _always_ have the
"auto" behavior, since it tends to get treated as plumbing anyway, and I
suspect that anybody who sets log.date now would see subtle breakage
from scripts. But maybe it's too late at this point?).

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 6d798f9939..f684e31d82 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -925,6 +925,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  		 */
>  		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
>  		break;
> +	case DATE_HUMAN:
> +		/* If the year is shown, no time is shown */
> +		blame_date_width = sizeof("Thu Oct 19 16:00");
> +		break;

OK, and we expect the year to be less than 5 characters. I briefly
wondered what would happen at Y100K (or somebody maliciously using a
bogus year), but it is not a buffer overflow. It is simply a mis-aligned
blame line (and actually, the same goes for the existing entries, which
use a 4-digit year).

-Peff
Stephen P. Smith Jan. 3, 2019, 1:19 p.m. UTC | #2 
On Thursday, January 3, 2019 12:37:35 AM MST Jeff King wrote:
> I like the idea of "human", and I like the idea of "auto", but it seems
> to me that these are really two orthogonal things. E.g., might some
> people not want to do something like:
> 
>   git config log.date auto:relative
I didn't see anything in the code which would prohibit setting something like 
that.  

> 
> I don't personally care about using this myself, but we already had to
> deal with retrofitting "local" as a modifier. I'd prefer to avoid making
> the same mistake again.
Since I wasn't involved could you summarize the you are referring to?

> 
> (I'd actually argue that "log.date" should basically _always_ have the
> "auto" behavior, since it tends to get treated as plumbing anyway, and I
> suspect that anybody who sets log.date now would see subtle breakage
> from scripts. But maybe it's too late at this point?).
If auto isn't added to the "log.date" file, then the date behaviour is not 
changed from is currently in the code base.   Therefore, there shouldn't be 
any breakage.
> 
> > diff --git a/builtin/blame.c b/builtin/blame.c
> > index 6d798f9939..f684e31d82 100644
> > --- a/builtin/blame.c
> > +++ b/builtin/blame.c
> > @@ -925,6 +925,10 @@ int cmd_blame(int argc, const char **argv, const char
> > *prefix)> 
> >  		 */
> >  		
> >  		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /*
> >  		add the null */ break;
> > 
> > +	case DATE_HUMAN:
> > +		/* If the year is shown, no time is shown */
> > +		blame_date_width = sizeof("Thu Oct 19 16:00");
> > +		break;
> 
> OK, and we expect the year to be less than 5 characters. I briefly
> wondered what would happen at Y100K (or somebody maliciously using a
> bogus year), but it is not a buffer overflow. It is simply a mis-aligned
> blame line (and actually, the same goes for the existing entries, which
> use a 4-digit year).
> 
> -Peff
Jeff King Jan. 4, 2019, 7:50 a.m. UTC | #3 
On Thu, Jan 03, 2019 at 06:19:56AM -0700, Stephen P. Smith wrote:

> On Thursday, January 3, 2019 12:37:35 AM MST Jeff King wrote:
> > I like the idea of "human", and I like the idea of "auto", but it seems
> > to me that these are really two orthogonal things. E.g., might some
> > people not want to do something like:
> > 
> >   git config log.date auto:relative
> I didn't see anything in the code which would prohibit setting something like 
> that.

Yeah, I don't think supporting that is too hard. I was thinking
something like this:

diff --git a/date.c b/date.c
index 4486c028ac..f731803872 100644
--- a/date.c
+++ b/date.c
@@ -883,11 +883,6 @@ int parse_date(const char *date, struct strbuf *result)
 	return 0;
 }
 
-static int auto_date_style(void)
-{
-	return (isatty(1) || pager_in_use()) ? DATE_HUMAN : DATE_NORMAL;
-}
-
 static enum date_mode_type parse_date_type(const char *format, const char **end)
 {
 	if (skip_prefix(format, "relative", end))
@@ -907,8 +902,6 @@ static enum date_mode_type parse_date_type(const char *format, const char **end)
 		return DATE_NORMAL;
 	if (skip_prefix(format, "human", end))
 		return DATE_HUMAN;
-	if (skip_prefix(format, "auto", end))
-		return auto_date_style();
 	if (skip_prefix(format, "raw", end))
 		return DATE_RAW;
 	if (skip_prefix(format, "unix", end))
@@ -923,6 +916,14 @@ void parse_date_format(const char *format, struct date_mode *mode)
 {
 	const char *p;
 
+	/* "auto:foo" is "if tty/pager, then foo, otherwise normal" */
+	if (skip_prefix(format, "auto:", &p)) {
+		if (isatty(1) || pager_in_use())
+			format = p;
+		else
+			format = "default";
+	}
+
 	/* historical alias */
 	if (!strcmp(format, "local"))
 		format = "default-local";

That removes "auto" completely. We could still support it as an alias
for "auto:human" with something like:

  if (!strcmp(format, "auto"))
	format = "auto:human";

but IMHO it is a simpler interface to just have the user be explicit
(this is meant to be set once in config, after all).

> > I don't personally care about using this myself, but we already had to
> > deal with retrofitting "local" as a modifier. I'd prefer to avoid making
> > the same mistake again.
> Since I wasn't involved could you summarize the you are referring to?

The format "local" was a variant of "default" that would use the local
timezone instead of the author's. But there was no way to format, say,
iso8601 in the local timezone. So we had to invent a new syntax that was
compatible ("iso8601-local"), and keep "local" around forever for
backwards compatibility. Not the end of the world, but we can avoid it
in this case with a little preparation.

> > (I'd actually argue that "log.date" should basically _always_ have the
> > "auto" behavior, since it tends to get treated as plumbing anyway, and I
> > suspect that anybody who sets log.date now would see subtle breakage
> > from scripts. But maybe it's too late at this point?).
> If auto isn't added to the "log.date" file, then the date behaviour is not 
> changed from is currently in the code base.   Therefore, there shouldn't be 
> any breakage.

Right, this isn't a problem with your patches. I mean that the existing
"log.date" is arguably mis-designed, and we ought to have had something
like "auto" from day one (or even made it the default for log.date).

-Peff
Stephen P. Smith Jan. 4, 2019, 1:03 p.m. UTC | #4 
On Friday, January 4, 2019 12:50:35 AM MST Jeff King wrote:
> On Thu, Jan 03, 2019 at 06:19:56AM -0700, Stephen P. Smith wrote:
> > 
> > I didn't see anything in the code which would prohibit setting something
> > like that.
> 
> Yeah, I don't think supporting that is too hard. I was thinking
> something like this:

I take it that if I update Linus's patch, I still keep Junio's and Linus' 
sign-off line for the purpose of the chain of custody?  Of should I use a 
second patch?

Just trying to follow the rules.
sps
Jeff King Jan. 6, 2019, 6:19 a.m. UTC | #5 
On Fri, Jan 04, 2019 at 06:03:18AM -0700, Stephen P Smith wrote:

> On Friday, January 4, 2019 12:50:35 AM MST Jeff King wrote:
> > On Thu, Jan 03, 2019 at 06:19:56AM -0700, Stephen P. Smith wrote:
> > > 
> > > I didn't see anything in the code which would prohibit setting something
> > > like that.
> > 
> > Yeah, I don't think supporting that is too hard. I was thinking
> > something like this:
> 
> I take it that if I update Linus's patch, I still keep Junio's and Linus' 
> sign-off line for the purpose of the chain of custody?  Of should I use a 
> second patch?

I think the most interesting question is the actual authorship (i.e.,
the "From:" field).  I think people are generally OK with having their
patches polished a bit to fix obvious bugs or short-comings. But at some
point if you make too many changes they or may not want to have the
result attributed to them. ;)

For the particular change I suggested, it's borderline to me on whether
it hits that case, so I'd probably err on the side of caution. And I'd
either expect Linus to say "yeah, that sounds like a good direction", or
I'd do it as a separate patch. And if a separate patch, I'd probably
tease Linus's patch out into two separate ones: one to add "human", and
one to implement "auto". And then drop the "auto" one in favor of your
new patch (with you as the author).

And I think that makes the signoff questions go away for this instance
(keep the signoffs for Linus's, and just signoff the new patch
yourself). But here's some general pontificating in that direction:

    Normally you can just drop Junio's signoff. The chain of custody is
    usually "author, then maintainer" and he'll re-add his maintainer
    signoff when he picks up your patch. In this case of this patch it's
    "author, then polisher, then maintainer", but Junio is still at the
    end.

    Now one can argue that Junio picked up Linus's patch, which you then
    picked up from Junio's repository and fed back to Junio. But you
    could just as well have picked Linus's patch up from the mailing
    list and then polished it. So I don't know that having Junio twice
    in the chain is really that interesting.

    Generally, yes, I'd keep Linus's signoff in a situation like this.
    He is asserting that the original work done meets the DCO
    requirements. You polishing the patch does not change that (of
    course you could introduce a bunch of new code that doesn't meet the
    DCO and sign it off anyway, but that's why there's ordering in the
    chain of custody. Somebody investigating would probably walk
    backwards up the chain).

-Peff
diff mbox series

Patch

diff --git a/builtin/blame.c b/builtin/blame.c
index 6d798f9939..f684e31d82 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -925,6 +925,10 @@  int cmd_blame(int argc, const char **argv, const char *prefix)
 		 */
 		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
 		break;
+	case DATE_HUMAN:
+		/* If the year is shown, no time is shown */
+		blame_date_width = sizeof("Thu Oct 19 16:00");
+		break;
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
 		break;
diff --git a/cache.h b/cache.h
index ca36b44ee0..c4396ebaa6 100644
--- a/cache.h
+++ b/cache.h
@@ -1439,6 +1439,7 @@  extern struct object *peel_to_type(const char *name, int namelen,
 
 enum date_mode_type {
 	DATE_NORMAL = 0,
+	DATE_HUMAN,
 	DATE_RELATIVE,
 	DATE_SHORT,
 	DATE_ISO8601,
diff --git a/date.c b/date.c
index 9bc15df6f9..a8d50eb206 100644
--- a/date.c
+++ b/date.c
@@ -77,22 +77,16 @@  static struct tm *time_to_tm_local(timestamp_t time)
 }
 
 /*
- * What value of "tz" was in effect back then at "time" in the
- * local timezone?
+ * Fill in the localtime 'struct tm' for the supplied time,
+ * and return the local tz.
  */
-static int local_tzoffset(timestamp_t time)
+static int local_time_tzoffset(time_t t, struct tm *tm)
 {
-	time_t t, t_local;
-	struct tm tm;
+	time_t t_local;
 	int offset, eastwest;
 
-	if (date_overflows(time))
-		die("Timestamp too large for this system: %"PRItime, time);
-
-	t = (time_t)time;
-	localtime_r(&t, &tm);
-	t_local = tm_to_time_t(&tm);
-
+	localtime_r(&t, tm);
+	t_local = tm_to_time_t(tm);
 	if (t_local == -1)
 		return 0; /* error; just use +0000 */
 	if (t_local < t) {
@@ -107,6 +101,20 @@  static int local_tzoffset(timestamp_t time)
 	return offset * eastwest;
 }
 
+/*
+ * What value of "tz" was in effect back then at "time" in the
+ * local timezone?
+ */
+static int local_tzoffset(timestamp_t time)
+{
+	struct tm tm;
+
+	if (date_overflows(time))
+		die("Timestamp too large for this system: %"PRItime, time);
+
+	return local_time_tzoffset((time_t)time, &tm);
+}
+
 void show_date_relative(timestamp_t time, int tz,
 			       const struct timeval *now,
 			       struct strbuf *timebuf)
@@ -191,9 +199,80 @@  struct date_mode *date_mode_from_type(enum date_mode_type type)
 	return &mode;
 }
 
+static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, struct tm *human_tm, int human_tz, int local)
+{
+	struct {
+		unsigned int	year:1,
+				date:1,
+				wday:1,
+				time:1,
+				seconds:1,
+				tz:1;
+	} hide = { 0 };
+
+	hide.tz = local || tz == human_tz;
+	hide.year = tm->tm_year == human_tm->tm_year;
+	if (hide.year) {
+		if (tm->tm_mon == human_tm->tm_mon) {
+			if (tm->tm_mday > human_tm->tm_mday) {
+				/* Future date: think timezones */
+			} else if (tm->tm_mday == human_tm->tm_mday) {
+				hide.date = hide.wday = 1;
+			} else if (tm->tm_mday + 5 > human_tm->tm_mday) {
+				/* Leave just weekday if it was a few days ago */
+				hide.date = 1;
+			}
+		}
+	}
+
+	/* Show "today" times as just relative times */
+	if (hide.wday) {
+		struct timeval now;
+		gettimeofday(&now, NULL);
+		show_date_relative(time, tz, &now, buf);
+		return;
+	}
+
+	/*
+	 * Always hide seconds for human-readable.
+	 * Hide timezone if showing date.
+	 * Hide weekday and time if showing year.
+	 *
+	 * The logic here is two-fold:
+	 *  (a) only show details when recent enough to matter
+	 *  (b) keep the maximum length "similar", and in check
+	 */
+	if (human_tm->tm_year) {
+		hide.seconds = 1;
+		hide.tz |= !hide.date;
+		hide.wday = hide.time = !hide.year;
+	}
+
+	if (!hide.wday)
+		strbuf_addf(buf, "%.3s ", weekday_names[tm->tm_wday]);
+	if (!hide.date)
+		strbuf_addf(buf, "%.3s %d ", month_names[tm->tm_mon], tm->tm_mday);
+
+	/* Do we want AM/PM depending on locale? */
+	if (!hide.time) {
+		strbuf_addf(buf, "%02d:%02d", tm->tm_hour, tm->tm_min);
+		if (!hide.seconds)
+			strbuf_addf(buf, ":%02d", tm->tm_sec);
+	} else
+		strbuf_rtrim(buf);
+
+	if (!hide.year)
+		strbuf_addf(buf, " %d", tm->tm_year + 1900);
+
+	if (!hide.tz)
+		strbuf_addf(buf, " %+05d", tz);
+}
+
 const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 {
 	struct tm *tm;
+	struct tm human_tm = { 0 };
+	int human_tz = -1;
 	static struct strbuf timebuf = STRBUF_INIT;
 
 	if (mode->type == DATE_UNIX) {
@@ -202,6 +281,15 @@  const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 		return timebuf.buf;
 	}
 
+	if (mode->type == DATE_HUMAN) {
+		struct timeval now;
+
+		gettimeofday(&now, NULL);
+
+		/* Fill in the data for "current time" in human_tz and human_tm */
+		human_tz = local_time_tzoffset(now.tv_sec, &human_tm);
+	}
+
 	if (mode->local)
 		tz = local_tzoffset(time);
 
@@ -258,14 +346,7 @@  const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
 				!mode->local);
 	else
-		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
-				weekday_names[tm->tm_wday],
-				month_names[tm->tm_mon],
-				tm->tm_mday,
-				tm->tm_hour, tm->tm_min, tm->tm_sec,
-				tm->tm_year + 1900,
-				mode->local ? 0 : ' ',
-				tz);
+		show_date_normal(&timebuf, time, tm, tz, &human_tm, human_tz, mode->local);
 	return timebuf.buf;
 }
 
@@ -802,6 +883,11 @@  int parse_date(const char *date, struct strbuf *result)
 	return 0;
 }
 
+static int auto_date_style(void)
+{
+	return (isatty(1) || pager_in_use()) ? DATE_HUMAN : DATE_NORMAL;
+}
+
 static enum date_mode_type parse_date_type(const char *format, const char **end)
 {
 	if (skip_prefix(format, "relative", end))
@@ -819,6 +905,10 @@  static enum date_mode_type parse_date_type(const char *format, const char **end)
 		return DATE_SHORT;
 	if (skip_prefix(format, "default", end))
 		return DATE_NORMAL;
+	if (skip_prefix(format, "human", end))
+		return DATE_HUMAN;
+	if (skip_prefix(format, "auto", end))
+		return auto_date_style();
 	if (skip_prefix(format, "raw", end))
 		return DATE_RAW;
 	if (skip_prefix(format, "unix", end))