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

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

[MichaelTiloDressel]

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

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

[Jeff King]

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

> is it a bug?

Sort of. More like a missing feature.

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

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

[PATCH 1/2] decorate: allow const objects to be decorated

[Jeff King]

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

diff --git a/decorate.c b/decorate.c
index d9668d2..82d9e22 100644
--- a/decorate.c
+++ b/decorate.c
@@ -6,13 +6,13 @@
 #include "object.h"
 #include "decorate.h"
 
-static unsigned int hash_obj(struct object *obj, unsigned int n)
+static unsigned int hash_obj(const struct object *obj, unsigned int n)
 {
 	unsigned int hash = *(unsigned int *)obj->sha1;
 	return hash % n;
 }
 
-static void *insert_decoration(struct decoration *n, struct object *base, void *decoration)
+static void *insert_decoration(struct decoration *n, const struct object *base, void *decoration)
 {
 	int size = n->size;
 	struct object_decoration *hash = n->hash;
@@ -44,7 +44,7 @@ static void grow_decoration(struct decoration *n)
 	n->nr = 0;
 
 	for (i = 0; i < old_size; i++) {
-		struct object *base = old_hash[i].base;
+		const struct object *base = old_hash[i].base;
 		void *decoration = old_hash[i].decoration;
 
 		if (!base)
@@ -55,7 +55,8 @@ static void grow_decoration(struct decoration *n)
 }
 
 /* Add a decoration pointer, return any old one */
-void *add_decoration(struct decoration *n, struct object *obj, void *decoration)
+void *add_decoration(struct decoration *n, const struct object *obj,
+		void *decoration)
 {
 	int nr = n->nr + 1;
 
@@ -65,7 +66,7 @@ void *add_decoration(struct decoration *n, struct object *obj, void *decoration)
 }
 
 /* Lookup a decoration pointer */
-void *lookup_decoration(struct decoration *n, struct object *obj)
+void *lookup_decoration(struct decoration *n, const struct object *obj)
 {
 	int j;
 
diff --git a/decorate.h b/decorate.h
index 1fa4ad9..e732804 100644
--- a/decorate.h
+++ b/decorate.h
@@ -2,7 +2,7 @@
 #define DECORATE_H
 
 struct object_decoration {
-	struct object *base;
+	const struct object *base;
 	void *decoration;
 };
 
@@ -12,7 +12,7 @@ struct decoration {
 	struct object_decoration *hash;
 };
 
-extern void *add_decoration(struct decoration *n, struct object *obj, void *decoration);
-extern void *lookup_decoration(struct decoration *n, struct object *obj);
+extern void *add_decoration(struct decoration *n, const struct object *obj, void *decoration);
+extern void *lookup_decoration(struct decoration *n, const struct object *obj);
 
 #endif
-- 
1.6.0.90.g00a5c.dirty

[PATCH 2/2] allow '%d' pretty format specifier to show decoration

[Jeff King]

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

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index c11d495..55a5954 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -116,6 +116,7 @@ The placeholders are:
 - '%cr': committer date, relative
 - '%ct': committer date, UNIX timestamp
 - '%ci': committer date, ISO 8601 format
+- '%d': decoration (if you specified --decorate)
 - '%e': encoding
 - '%s': subject
 - '%b': body
diff --git a/pretty.c b/pretty.c
index 33ef34a..00f19e1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -519,6 +519,21 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 			return 3;
 		} else
 			return 0;
+	case 'd':
+		{
+			struct name_decoration *d;
+			const char *prefix = "";
+			d = lookup_decoration(&name_decoration,
+					&commit->object);
+			while (d) {
+				strbuf_addstr(sb, prefix);
+				prefix = ", ";
+				strbuf_addstr(sb, d->name);
+				d = d->next;
+			}
+		}
+		return 1;
+
 	}
 
 	/* these depend on the commit */
-- 
1.6.0.90.g00a5c.dirty

Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>     Is it too loose? Perhaps the enclosing parentheses should be
>     automatic,...

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

>   - should this turn on --decorate automatically?

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.

Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration

[Johannes Schindelin]

Hi,

On Wed, 20 Aug 2008, Jeff King wrote:

> There is a lot of room for discussion here.

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

Ciao,
Dscho

Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration

[Junio C Hamano]

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

> On Wed, 20 Aug 2008, Jeff King wrote:
>
>> There is a lot of room for discussion here.
>
> Indeed.  When I posted a similar patch, there was some discussion, too.  
> But no resolution, as it seemed nobody was really interested.

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.

Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration

[Michael Dressel]

On Wed, 20 Aug 2008, Jeff King wrote:

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

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

Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration

[Johannes Schindelin]

Hi,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 20 Aug 2008, Jeff King wrote:
> >
> >> There is a lot of room for discussion here.
> >
> > Indeed.  When I posted a similar patch, there was some discussion, too.  
> > But no resolution, as it seemed nobody was really interested.
> 
> Why being so negative?  Got up on the wrong side of the bed?

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

Ciao,
Dscho

Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration

[Junio C Hamano]

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

> I use it in a script to extract the log title of commits between certain
> tags.

"git shortlog"?

Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration

[Michael Dressel]

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

> Michael Dressel <MichaelTiloDressel@t-online.de> writes:
>
>> I use it in a script to extract the log title of commits between certain
>> tags.
>
> "git shortlog"?
>

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

"log --pretty=format:" language

[Teemu Likonen]

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

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

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.

[...]

Re: "log --pretty=format:" language

[Jakub Narebski]

Teemu Likonen <tlikonen@iki.fi> writes:

> Jeff King wrote (2008-08-20 14:00 -0400):
> 
> > 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.
> 
> 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 [...]

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

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration

[Jeff King]

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

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

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