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

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

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

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

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

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

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.

[...]

Teemu Likonen [off-list ref] writes:

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

quoted

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

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