Thanks for the feedback. Comments inline.

On Fri, Feb 20, 2009 at 6:27 AM, Jeff King <peff@peff.net> wrote:
quoted text
> 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?

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

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

quoted text
>  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:

quoted text
> ./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"

quoted text
> -Peff
>



-- 
Eugene
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html