Thanks for the feedback. Comments inline.
On Fri, Feb 20, 2009 at 6:27 AM, Jeff King <peff@xxxxxxxx> wrote:
> On Fri, Feb 20, 2009 at 05:24:12AM -0800, eletuchy@xxxxxxxxx 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?
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
