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