Re: [PATCH/RFC 0/9] add long forms for format placeholders

[Will Palmer <wmpalmer@gmail.com>]

On Mon, 2011-03-28 at 17:28 -0700, Junio C Hamano wrote:
> Will Palmer <wmpalmer@gmail.com> writes:
> 
> > I've been kicking around this series for a while now as part of a larger
> > effort of refactoring the pretty formats. A recent discussion on the
> > list has lead me to believe that this smaller subset may be of use
> > sooner, rather than later.
> >
> > This series attempts to add "long forms" for common format placeholders
> > in the "git log" family of commands, making the way for yet more
> > placeholders to be added without needing to worry too much about the
> > increasingly limited set of available one-letter mnemonics. It also
> > moves towards the possibility of eventual unification with the format
> > options in for-each-ref.
> 
> I don't claim that I read 1300+ long [PATCH 5/9] carefully, but I like the
> direction in which this topic is going very much.
> 
> Except that [PATCH 2/9] looked quite out of place---more like "I wanted to
> sneak this feature in" than "this was needed to keep the resulting code
> backward compatible" or anything like that.

just for context, we're talking about:
 [PATCH/RFC 2/9] add support for --date=unix to complement %at

I was warned that I should tweak that message!

This one is actually in there to make the later [PATCH 5/9] more
consistent in how it handled dates, as: {AUTHOR,COMMITTER}_DATE: <TYPE>,
rather than having a special case just for _UNIX.

When I added DATE_UNIX to the enum, gcc started complaining about
unhandled enum values in switch()es. To get around those (and noticing
that %at was the only format that wasn't available as a --date= switch)
--date=unix was added. It seemed like a good idea to move that change to
earlier in the series, rather than "sneaking it in" as part of [PATCH
5/9]

Of course, [PATCH 1/9] is only in there to make the documentation tweaks
in [PATCH 2/9] more readable.

> 
> Off the top of my head, I don't think of a reason to say that [PATCH 3/9]
> is going in a wrong direction---is there a reason to make you worried in
> the particular change?

 [PATCH/RFC 3/9] interpret %C(invalid) as we would %%C(invalid)

This one I was iffy on. On the one hand, it's inconsistent to treat
%C(invalid) any differently from %Z(doesn't even exist), but on the
other hand we lose feedback of telling the user why it's actually not
working as intended.

The real purpose of it was to prevent strange messages later on in the
larger series, which adds support for %(alias:<aliasname>). Seeing the
message "bad color value 'bkue' for variable '--pretty format'" when
what you actually typed was:
    commit %h%+(alias:mergeline)%+(alias:message)
could be confusing.

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

-- 
-- Will