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

Thanks for the feedback. Comments inline.

On Fri, Feb 20, 2009 at 6:27 AM, Jeff King [off-list ref] wrote:

On Fri, Feb 20, 2009 at 05:24:12AM -0800, eletuchy@gmail.com wrote:

quoted

 - git config value blame.date that expects one of the git log date
   formats ({relative,local,default,iso,rfc,short})

OK. I was concerned that this might muck with scripts, but it looks like
the --porcelain and --incremental codepaths are properly unaffected.
Good.

quoted

 - git blame command line option --date-format expects one of the git
   log date formats ({relative,local,default,iso,rfc,short})

Why not --date= ?

It is currently accepted by the revision option parsing, but not used;
you would just need to pull the value from revs.date_mode instead of
adding a new option.

Good call. I can change to using --date instead of --date-format. It
wasn't clear that this was an unused option.  For parity with
log.date, config blame.date still makes sense, right?

quoted

The tests pass. The mailmap test needed to be modified to expect iso
formatted blames rather than the new "default".

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?

All of that being said, I think there are two code issues to be dealt
with:

 1. There seems to be a bug. With your patch, running a simple test
    like:

      git blame --date-format=relative wt-status.c

    gives me relative output on some lines, and not on others. E.g.,
    the first 10 lines are:

85023577 (Junio C Hamano      Tue Dec 19 14:34:12 2006 -0800   1) #include "cache.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   2) #include "wt-status.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   3) #include "color.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   4) #include "object.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   5) #include "dir.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   6) #include "commit.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   7) #include "diff.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   8) #include "revision.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400   9) #include "diffcore.h"
a734d0b1 (Dmitry Potapov      12 months ago  10) #include "quote.h"
ac8d5afc (Ping Yin            10 months ago  11) #include "run-command.h"
b6975ab5 (Junio C Hamano      8 months ago  12) #include "remote.h"
c91f0d92 (Jeff King           Fri Sep 8 04:05:34 2006 -0400  13)

According to date.c comments, this is a "feature" of DATE_RELATIVE:
                /* 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?

 2. As you can see in the output above, there are potential alignment
    issues. The original date format had a fixed width, whereas
    arbitrary date formats can be variable. Obviously the mixture of
    relative and ISO dates makes it much worse, but even within an ISO
    date there are problems (e.g., "19" versus "8").

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?

The output is as follows:

./git blame --date=relative wt-status.c | head -10

85023577 (Junio C Hamano      2006-12-19       1) #include "cache.h"
c91f0d92 (Jeff King           2006-09-08       2) #include "wt-status.h"
c91f0d92 (Jeff King           2006-09-08       3) #include "color.h"
c91f0d92 (Jeff King           2006-09-08       4) #include "object.h"
c91f0d92 (Jeff King           2006-09-08       5) #include "dir.h"
c91f0d92 (Jeff King           2006-09-08       6) #include "commit.h"
c91f0d92 (Jeff King           2006-09-08       7) #include "diff.h"
c91f0d92 (Jeff King           2006-09-08       8) #include "revision.h"
c91f0d92 (Jeff King           2006-09-08       9) #include "diffcore.h"
a734d0b1 (Dmitry Potapov      12 months ago   10) #include "quote.h"

-Peff

-- 
Eugene