Re: [PATCH 1/3] Add 'human' date format

[Jeff King <peff@peff.net>]

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