Re: [PATCH v2 0/5] Pretty formats for reflog data

From: Jeff King <peff@peff.net>
Date: Oct 16, 2009, 05:20 UTC
Message-ID: <20091016052003.GA10629@coredump.intra.peff.net>
In-Reply-To: <cover.1255645570.git.trast@student.ethz.ch>

On Fri, Oct 16, 2009 at 12:41:43AM +0200, Thomas Rast wrote:

Show 8 quoted lines

> Jeff King wrote:
> > Maybe a better solution would be a "short name" variant for pretty
> > format specifiers. We already have %(refname) and %(refname:short) [...]
> > The tricky part would be deciding on a syntax. This seems to come up a
> > fair bit.
> 
> Ok, I settled for %g[dDs] for now to save on letters.  I'm saving the
> syntax question for a later series while we discuss this one ;-)

:) That is probably sensible. Your %g[dD] doesn't support selecting between the numbered version and the "date" version, which is something we might want, but certainly it is no worse than the status quo (and doing something like that probably _would_ want an extended syntax, as you now have two orthogonal arguments: shorten and date/numbered).

Show 9 quoted lines

> I think going for %(...) wouldn't be too bad since we already have
> that in for-each-ref, and it can be backwards compatible.  So we would
> have different sets of short and long specifiers, e.g.
> 
>   %ae = %(authoremail)
>   %aE = %(authoremail:mailmap)
> 
> We can then pass arguments via some yet-to-be decided syntax, say,
> %(body:indent(10)).

That seems reasonable to me, though if we can limit ourselves to one argument per specifier (I suspect most specifiers would simply be boolean, but a few may take numbers or strings), then something like %(body:indent=10) might be a little more readable.

It would also be nice to have some sort of conditional inclusion, which could deal with your extra ": " in patch 3. Either something like:

  %(reflog:short)%(reflog:+: )

or even

  %(reflog:short:prefix=\: )

and note that allowing arbitrary arguments means we get to deal with quoting.

But that is all for another potential series.

> Other changes in this version include:
> 
> * I followed your struct suggestion, which is the new 1/5.

Thanks. It looks like it didn't turn out to be too invasive, and I think some of the callsites are a bit more readable.

> * I fixed the warning that Junio found (and finally found the right
>   combination of -W flags, though I cannot compile with -Werror myself
>   because of *other* warnings...)

I always compile with -Wall -Werror (including testing your series); what warnings are you getting?

Show 6 quoted lines

> Thomas Rast (5):
>   Refactor pretty_print_commit arguments into a struct
>   reflog-walk: refactor the branch@{num} formatting
>   Introduce new pretty formats %g[sdD] for reflog information
>   stash list: use new %g formats instead of sed
>   stash list: drop the default limit of 10 stashes

Thanks, this series looks really good to me. I have a few comments on patch 3 which I'll send separately.

-Peff