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

From: Eugene Letuchy <eletuchy@gmail.com>
Date: Feb 20, 2009, 16:13 UTC
Message-ID: <fbb390660902200813h2455eak4e72144c7c491ef9@mail.gmail.com>
In-Reply-To: <20090220142730.GA32751@coredump.intra.peff.net>

Thanks for the feedback. Comments inline.

On Fri, Feb 20, 2009 at 6:27 AM, Jeff King <peff@peff.net> wrote:

Show 18 quoted lines

> On Fri, Feb 20, 2009 at 05:24:12AM -0800, eletuchy@gmail.com wrote:
>
>>  - 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.
>
>>  - 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?

Show 13 quoted lines

>> 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?

Show 26 quoted lines

>
> 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?

Show 6 quoted lines

>  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