Re: [PATCH] Make git blame date output format configurable, a la git log

On Fri, Feb 20, 2009 at 08:13:34AM -0800, Eugene Letuchy wrote:

Good call. I can change to using --date instead of --date-format. It
wasn't clear that this was an unused option.

Yeah, it is a slight confusion both to developers and to users that
programs which take revision arguments sometimes accept but ignore them.

But the revs.date_mode set by the revision library is basically just
used by log-tree, which is not used by blame. So it is safe to reuse,
and doing so actually reduces confusion.

For parity with log.date, config blame.date still makes sense, right?

Sure. It might even make sense to have an unset blame.date default to
the value of log.date. But I don't use log.date, nor do I directly use
blame (I use tig's blame mode). So I don't know what people expect or
would find useful.

quoted

So there are actually two changes here:

 1. support specifying date format

 2. changing the default date format

I think (1) is a good change, but it should definitely not be lumped in
with (2), as people might like one and not the other (and I happen not
to like (2)).

What about consistency with all git-rev-list clients?

I think blame is a bit different than other clients because it is
showing the date on a line with a bunch of other stuff, whereas most
clients use "Date: <whatever>" on a separate line. So it has to be a bit
more careful about how much space is used.

That being said, I think this discussion proves my main point, which is
that it should be split into two patches. Then discussion over the
default format will not hold up the --date support.

quoted

    gives me relative output on some lines, and not on others. E.g.,

[...]
According to date.c comments, this is a "feature" of DATE_RELATIVE:

Oh, right. Sorry for the noise, I totally forgot about that that feature
(which I now remember annoying me in the past, too).

                /* Say months for the past 12 months or so */
                if (diff < 360) {
                        snprintf(timebuf, sizeof(timebuf), "%lu months
ago", (diff + 15) / 30);
                        return timebuf;
                }
                /* Else fall back on absolute format.. */

A single line fixes that to be a bit more logical:
-               /* Else fall back on absolute format.. */
+               /* Else fall back to the short format */
+                mode = DATE_SHORT;

but i think that's a separate commit, no?

I do think that's a reasonable change; there's no point in giving a very
precise date for things more than a year past when we have already
dropped precision to "month" for everything else. But definitely a
separate commit.

Personally, I think I would rather see "months" up until about 2-3
years, and then simply "N years ago" after that.

I have a patch to fix the alignment issues: it figures out the max
width of each date format and memsets in that number of spaces in
format_time. Is it better to submit that as a separate commit, or send
a revised patch?

I think it makes sense to send a revised patch with all of the changes
we've discussed (please mark it as v2 and give a brief summary of what's
changed below the "---" marker to help out other reviewers).

-Peff