[BUG?] git log does not decorate when custom format is used

Hi,

is it a bug?

When I use something like: git log --pretty=format:'%H %s' --decorate I do not get the decoration. While git log --pretty=oneline --decorate does decorate.

I'm using: git version 1.5.6.1

Cheers, Michael

On Wed, Aug 20, 2008 at 02:25:30PM +0200, MichaelTiloDressel@t-online.de wrote:

Sort of. More like a missing feature.

The problem is where the decoration would go. I think it makes sense not to show the decoration automatically in that case, since its placement depends on the user formatting.

The right solution would be a '%d' placeholder to include the decoration. Patch series to follow:

  [1/2] decorate: allow const objects to be decorated
  [2/2] allow '%d' pretty format specifier to show decoration

-Peff

We don't actually modify the struct object, so there is no reason not to accept const versions (and this allows other callsites, like the next patch, to use the decoration machinery).

Signed-off-by: Jeff King <peff@peff.net>
---
This one is hopefully a no-brainer, and is required for the next patch.

 decorate.c |   11 ++++++-----
 decorate.h |    6 +++---
 2 files changed, 9 insertions(+), 8 deletions(-)

Previously, specifying

  git log --pretty=format:'%H %s' --decorate

would calculate decorations, but not show them. You can now do:

  git log --pretty=format:'%H (%d) %s' --decorate

to see them.

Signed-off-by: Jeff King <peff@peff.net>
---
There is a lot of room for discussion here.

For example:

  - what should %d show? Right now it shows each decoration, split by
    commas. It doesn't show the enclosing parentheses automatically.

    Is this too strict? Should there be some way of pulling out
    individual decorations from the list, or specifying a different
    delimiter? If so, probably that should be part of a general
    improvement in the format expansion macro language.

    Is it too loose? Perhaps the enclosing parentheses should be
    automatic, so that %d expands to nothing if there is no decoration,
    or the whole thing otherwise. Right now you are stuck with empty ()
    if there is no decoration. Alternatively, we could support some kind
    of conditional expansion in the formatting language (but I don't
    know how crazy we want to get wit new formatting features).

  - should this turn on --decorate automatically? If you use '%d'
    without --decorate, you will just get no decorations. I think that
    makes sense, though, since that opens room for specifying other
    types of decorations (e.g., there could be a --decorate-tags that
    only looks at tags).

 Documentation/pretty-formats.txt |    1 +
 pretty.c                         |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

Jeff King <peff@peff.net> writes:

I think so.  The default output should primarily be for humans.

Oh, absolutely. If you plan to have different kind of decorations in the future, then the logic can be "if there is explicit decoration specified, don't do anything, but if there isn't any, turn the default one on."

Also, if it doesn't already, "--pretty=[t]format:" should make sure parent rewriting is enabled when %p and %P is used. I didn't look.

Hi,

On Wed, 20 Aug 2008, Jeff King wrote:

Indeed. When I posted a similar patch, there was some discussion, too. But no resolution, as it seemed nobody was really interested.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Why being so negative?  Got up on the wrong side of the bed?

People drop off of a discussion for different reasons, and it is not necessarily from total lack of interest (unless nobody responded to the initial call-for-discussion, obviously). Perhaps there were more interesting and/or pressing thing for them to think about than interaction between decorate and pretty=format at the same time.

This time around, the original requestor may be more motivated and/or have time for this topic than you were motivated and/or you had time back then, and can carry the discussion forward. Let's see what happens.

Hi,

On Wed, 20 Aug 2008, Junio C Hamano wrote:

No. I just forgot to give a pointer, so that the arguments brought back then are not lost.

Ciao, Dscho

On Wed, Aug 20, 2008 at 08:43:29PM +0200, Johannes Schindelin wrote:

I know Junio called you negative, but I actually appreciate having you point out related work (even if I don't get around to reading it for 2 weeks!).

It is funny how similar our patches ended up, even though I didn't even realize yours existed. It looks like the comments were not all that different, either.

To summarize what was said then (for the benefit of Michael, who is moving this forward):

  - Junio didn't like the expansion to include a space and enclosing
    parentheses, because then we don't know where to put the space.
    I agree with that. Though note that you will still end up with an
    extra space for a blank decoration.

  - René suggested a comma delimeter, which Michael's patch does.

  - René suggested a foobar2000-inspired expansion construct for dealing
    with the extra parentheses and space when no decoration exists. If
    we are going to expand the expansion language I think I would prefer
    something more like what show-ref uses (as Jakub suggested), but
    with "tags" expanded so that they can contain arbitrary data. So
    something like:

      %(decorate:delim=, :prefix= (:suffix=\))

    or even:

      %(decorate:+ (%(decorate:delim=, ))

    to use the more shell-ish conditional.

    But I think it is not a problem to introduce '%d' _and_ make such
    improvements later.

  - René also suggested that he wanted a placeholder for git-describe
    output. Logically, this would also want %d. Maybe it is worth making
    this %decorate now to avoid confusion later.

-Peff

On Wed, 20 Aug 2008, Jeff King wrote:

Wow that was fast! Thanks for the help.

For those who care: I use it in a script to extract the log title of commits between certain tags. And to compile a simple log history of what has changed between tags. It was a bit more tricky than I initially thought it would be, because of merges. So what I do basically is to do a git log A..B where A and B are two of the tags I found using --decorate. My problem was that my script got confused when I had braces in the log title. That was why I wanted to use format in the first place. I know there is a web interface to git which probably does all that, but I wanted to compile an as simple as possible text file.

Cheers, Michael

Michael Dressel <MichaelTiloDressel@t-online.de> writes:

"git shortlog"?

On Wed, 20 Aug 2008, Junio C Hamano wrote:

I use my script to see all the changes (by log titles) between the commit (prev) I ran the script the last time and the current commit (curr). Now there may be uninteresting tags and some commits may have more than one tag. Merges made before tag B may introduce commits (say c1) that are made before say tag A but with git log prev..curr I may get a list like this

curr (current commit) . . B (latest tag) A (previous tag) c1 (commit merged some time between A and B. . . prev (commit when script was run the last time)

Actually I use "git rev-list prev..curr" to get a list of commits between the last time I ran the script and the current head of the branch. Then I need the git log --decorate only to get _all_ the tags assigned to a given commit. Git describe does not give all the tags, as discussed earlier. The resulting list of tags assigned to a commit is later searched for the tag names I'm interested in. And then I use git log again (git log A..B ) to extract the log titles between every two successive tags, at this point I may use git shortlog.

Cheers, Michael

Jeff King wrote (2008-08-20 14:00 -0400):

If such "general improvement" takes place I'd like to point out (most likely old news, but anyway) that %b can't be indented in practical sense. For example, the command

    git log --pretty=format:%x09%s%n%n%x09%b -1 3a634dc

prints this:

        Add hints to revert documentation about other ways to undo changes

        Based on its name, people may read the 'git revert' documentation when
they want to undo local changes, especially people who have used other
SCM's.  'git revert' may not be what they had in mind, but git
provides several other ways to undo changes to files.  We can help
them by pointing them towards the git commands that do what they might
want to do.

[...]

Teemu Likonen <tlikonen@iki.fi> writes:

We can take a look how rpm handling of --queryformat option handles it.

First, it uses %{NAME} notation instead of %X shorthand for writing single header (git-for-each-ref uses %(name) instead, so we might want to use %(...) instead of %{...}, or use both). It allows use of printf(3) type formatters, which include field width and align, for example "%-30{NAME} %10{SIZE}\n".

Second, for displaying arrays (like list of files, or list of dependencies) or multi line output like package description it prints each item in the array, or each line in multi-line field within qeuare brackets, e.g. "[%-50{FILENAMES} %10{FILESIZES}\n]". If one want to repeat single-valued field one should use %{=NAME} syntax (actually it simply takes first line/first element of array), e.g. "[%{=NAME}: %{FILENAMES}\n]"

Queryformat minilanguage is more reach, see /usr/share/doc/rpm-*/queryformat or http://rpm5.org/docs/api/queryformat.html