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

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

[Michael Dressel]

This is a variation of the patch provided by  Jeff King <peff@peff.net>:
allow '%d' pretty format specifier to show decoration

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'

to see them.

The difference is that you don't need --decorate when %d has been given
because this would be "doppeltgemoppelt" (redundant).

Signed-off-by: Michael Dressel <MichaelTiloDressel@t-online.de>
---
  Documentation/pretty-formats.txt |    1 +
  builtin-log.c                    |   24 +++++++++++++++++++++++-
  pretty.c                         |   15 +++++++++++++++
  3 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 388d492..2616d63 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
  - '%e': encoding
  - '%s': subject
  - '%b': body
diff --git a/builtin-log.c b/builtin-log.c
index 1d3c5cb..5424012 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -50,11 +50,29 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in
  	return 0;
  }

+static int deco_in_format(int argc, const char **argv)
+{
+	int i;
+	int deco=0;
+	for (i=0;i<argc;i++)
+	{
+		if ((strstr(argv[i], "pretty=format:") ||
+					strstr(argv[i], "pretty=tformat:")) &&
+				strstr(argv[i], "%d"))
+		{
+			deco=1;
+			break;
+		}
+
+	}
+	return deco;
+}
+
  static void cmd_log_init(int argc, const char **argv, const char *prefix,
  		      struct rev_info *rev)
  {
  	int i;
-	int decorate = 0;
+	int decorate = deco_in_format(argc, argv);

  	rev->abbrev = DEFAULT_ABBREV;
  	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -77,6 +95,10 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
  		if (rev->diffopt.nr_paths != 1)
  			usage("git logs can only follow renames on one pathname at a time");
  	}
+	if (decorate)
+	{
+		for_each_ref(add_ref_decoration, NULL);
+	}
  	for (i = 1; i < argc; i++) {
  		const char *arg = argv[i];
  		if (!strcmp(arg, "--decorate")) {
diff --git a/pretty.c b/pretty.c
index a29c290..3430e4d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -520,8 +520,23 @@ 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 */
  	if (!commit->object.parsed)
  		parse_object(commit->object.sha1);
-- 
1.6.0.1

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

[Jeff King]

On Wed, Sep 03, 2008 at 08:40:08PM +0200, Michael Dressel wrote:

> This is a variation of the patch provided by  Jeff King <peff@peff.net>:
> allow '%d' pretty format specifier to show decoration

Thank you for picking this up. I had been meaning to look at it further
but hadn't gotten around to it yet.

> would calculate decorations, but not show them. You can now
> do:
>
>   git log --pretty=format:'%H (%d) %s'

I think Junio requested the enclosing parentheses be included. I am not
sure I agree, but we are already mandating the comma separator.
Personally, I think the right solution is more like

  %(decorate:delim=, )

but that is a much bigger change (and part of what was holding me up on
just improving this patch). I think adding %d in the meantime (with or
without the surrounding parentheses) is reasonable.

> The difference is that you don't need --decorate when %d has been given
> because this would be "doppeltgemoppelt" (redundant).

This is good, but I don't like the implementation:

> +static int deco_in_format(int argc, const char **argv)
> +{
> +	int i;
> +	int deco=0;
> +	for (i=0;i<argc;i++)
> +	{
> +		if ((strstr(argv[i], "pretty=format:") ||
> +					strstr(argv[i], "pretty=tformat:")) &&
> +				strstr(argv[i], "%d"))
> +		{
> +			deco=1;
> +			break;
> +		}
> +
> +	}
> +	return deco;
> +}

There are two bad things about this:

  1. It looks through argv. Partly this is a bit ugly, and we should
     just hook into the part where we have already parsed --pretty=.
     Which in this case really means looking at user_format after
     calling setup_revisions (which is sadly a static global; it really
     should be cleaned up as a member of struct rev_list). But worse
     here is that you don't even look through argv correctly. You really
     want prefixcmp(argv[i], "--pretty=format:"), since otherwise this
     would trigger on something like:

         git log ':/implement %d in pretty:format'

     which is unlikely, perhaps, but I don't think it's that hard to do
     it right in this case.

  2. It just looks for '%d' rather than using the same parser as the
     rest of the code. I _think_ this is actually correct now, because
     we don't even allow simple quoting like '%%d'. But it would be much
     cleaner, I think, to have a library call next to
     format_commit_message() that fills in a struct of "used" items.

So something like:

  static void cmd_log_init(...)
  {
          struct user_format_need ufneed;
  ...
          for (i = 1; i < argc; i++) {
            ...
                  if (!stcmp(arg, "--decorate"))
                          decorate = 1;
            ...
          }

          user_format_needs(&ufneed);
          if (decorate || ufneed.decorate)
            for_each_ref(add_ref_decoration, NULL);

  }

Make sense?

-Peff

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

[Jeff King]

On Wed, Sep 03, 2008 at 08:40:08PM +0200, Michael Dressel wrote:

> Signed-off-by: Michael Dressel <MichaelTiloDressel@t-online.de>

Also, I don't recall whether it was in the original message, but feel
free to add my

  Signed-off-by: Jeff King <peff@peff.net>

for my portions.

-Peff

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

[Michael Dressel]

On Wed, 3 Sep 2008, Jeff King wrote:

> On Wed, Sep 03, 2008 at 08:40:08PM +0200, Michael Dressel wrote:
>
>> Signed-off-by: Michael Dressel <MichaelTiloDressel@t-online.de>
>
> Also, I don't recall whether it was in the original message, but feel
> free to add my
>
>  Signed-off-by: Jeff King <peff@peff.net>
>
> for my portions.
>
I'm sorry for not properly citing you. I did mention you in the commit
message which is wrong. So what would be the right way? Just adding a
line 
Signed-off-by: Jeff King <peff@peff.net>
before the ---?
Or resending your original patch and than my patch on top of yours?

Cheers,
Michael

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>   git log --pretty=format:'%H (%d) %s'
>
> I think Junio requested the enclosing parentheses be included. I am not
> sure I agree, but we are already mandating the comma separator.
> Personally, I think the right solution is more like
>
>   %(decorate:delim=, )
>
> but that is a much bigger change (and part of what was holding me up on
> just improving this patch). I think adding %d in the meantime (with or
> without the surrounding parentheses) is reasonable.

I agree with the above reasoning.  If we go with surrounding parentheses,
it might even make sense to include a SP _before_ the opening parenthesis,
so that "%h %s%d" expands to either of these:

   $ git show -s --pretty=format:'%h %s%d' v1.6.0^0 v1.6.0^1
   ea02eef GIT 1.6.0 (refs/tags/v1.6.0)
   373a273 Merge git-gui 0.11.0

Also I did not like the bug to misidentify "git log --grep=pretty=format:"
you pointed out in your message.

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

[Jeff King]

On Wed, Sep 03, 2008 at 10:06:45PM +0200, Michael Dressel wrote:

> I'm sorry for not properly citing you. I did mention you in the commit
> message which is wrong. So what would be the right way? Just adding a
> line Signed-off-by: Jeff King <peff@peff.net>
> before the ---?
> Or resending your original patch and than my patch on top of yours?

Oh, you cited me just fine. The Signed-off-by line isn't for credit; it
indicates that you agree to the Developer's Certificate of Origin. IOW,
it says "I wrote this (or I know who did) and I know it can be licensed
for the project". So what I was saying was not "oh, you didn't credit me
properly" but rather "you may also add to the official record that the
parts I wrote are fine to be GPL'd for the project."

So in this case, I would simply add my Signed-off-by to the commit
message when you resend the patch.

-Peff

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

[Jeff King]

On Wed, Sep 03, 2008 at 01:10:12PM -0700, Junio C Hamano wrote:

> I agree with the above reasoning.  If we go with surrounding parentheses,
> it might even make sense to include a SP _before_ the opening parenthesis,
> so that "%h %s%d" expands to either of these:
> 
>    $ git show -s --pretty=format:'%h %s%d' v1.6.0^0 v1.6.0^1
>    ea02eef GIT 1.6.0 (refs/tags/v1.6.0)
>    373a273 Merge git-gui 0.11.0

Isn't that what you argued against in Dscho's version of the patch?

    http://mid.gmane.org/7v1w5exfwb.fsf@gitster.siamese.dyndns.org

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 03, 2008 at 01:10:12PM -0700, Junio C Hamano wrote:
>
>> I agree with the above reasoning.  If we go with surrounding parentheses,
>> it might even make sense to include a SP _before_ the opening parenthesis,
>> so that "%h %s%d" expands to either of these:
>> 
>>    $ git show -s --pretty=format:'%h %s%d' v1.6.0^0 v1.6.0^1
>>    ea02eef GIT 1.6.0 (refs/tags/v1.6.0)
>>    373a273 Merge git-gui 0.11.0
>
> Isn't that what you argued against in Dscho's version of the patch?
>
>     http://mid.gmane.org/7v1w5exfwb.fsf@gitster.siamese.dyndns.org

Yeah, but unlike Linus, I am not infallible.  Also, I can change my mind.

If the concensus is that we do the simpler variant _now_ and leave
extending it for later, I think it is Ok to pick any one reasonable
default/simple format, and including the parentheses (with leading SP)
would be one reasonable default, certainly more reasonable than not
including the parentheses at all.

The background is somewhat different, too.

Back then I didn't think anybody but Dscho was interested in including
decorate in --pretty=format in half-baked form.  René is comment was to do
more flexible but complex variant.  I didn't see many people wanted to
have it in "limited but usable for simpler needs" form.

It wasn't that urgent, as opposed to this round more people seem to be
interested in having it even in a limited form.  And I knew Dscho was very
much capable of doing the "going whole nine yards" version, instead of a
simple-limited one.  "If we can afford to spend time to get it right in
the first round why not?" was part of the reasons behind my objection.

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

[Jeff King]

On Wed, Sep 03, 2008 at 01:59:17PM -0700, Junio C Hamano wrote:

> > Isn't that what you argued against in Dscho's version of the patch?
> >
> >     http://mid.gmane.org/7v1w5exfwb.fsf@gitster.siamese.dyndns.org
> 
> Yeah, but unlike Linus, I am not infallible.  Also, I can change my mind.

Heh. OK, I am not against you changing your mind, but I like to at least
see "...and here is what is different." So thank you.

I think adding the space to the parentheses makes the most sense.
Without more advanced syntax, we have to make a choice between doing too
much (e.g., adding parentheses when the user doesn't want it) and too
little (the user has to put in parentheses manually, which means they
are there whether there is decoration or not). Doing parentheses
_without_ the space means we fail at both. So let us at least get it
half right. ;)

-Peff

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

[René Scharfe]

Junio C Hamano schrieb:
> If the concensus is that we do the simpler variant _now_ and leave
> extending it for later, I think it is Ok to pick any one reasonable
> default/simple format, and including the parentheses (with leading SP)
> would be one reasonable default, certainly more reasonable than not
> including the parentheses at all.

Yes, but can we please use the existing parser and not add another one
just to find out if some initialization is needed?  Also,
format_commit_message() is called from git archive.  In order to keep
all the placeholders working on each call site, it's better to control
everything from there.

The patch below does that by using a static variable to remember if the
decorations have already been loaded.  That's not too unreasonable, I
think, because we currently have no way of unloading them.

The patch should be split up and contains no documentation updates.  It

  - moves add_name_decoration() and add_ref_decoration() to log-tree.c,
    next to the variable name_decoration they are working with,

  - exports add_ref_decoration(),

  - adds load_ref_decorations(), which loads all refs as decorations,
    unless they have already been loaded,

  - adds the placeholder %d, with parentheses and leading space (or
    empty if no decoration is present).

Thanks,
René


 builtin-log.c |   25 -------------------------
 log-tree.c    |   36 ++++++++++++++++++++++++++++++++++++
 log-tree.h    |    3 +++
 pretty.c      |   25 +++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 25 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 1d3c5cb..ba1ef8d 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -25,31 +25,6 @@ static int default_show_root = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
-static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
-{
-	int plen = strlen(prefix);
-	int nlen = strlen(name);
-	struct name_decoration *res = xmalloc(sizeof(struct name_decoration) + plen + nlen);
-	memcpy(res->name, prefix, plen);
-	memcpy(res->name + plen, name, nlen + 1);
-	res->next = add_decoration(&name_decoration, obj, res);
-}
-
-static int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
-{
-	struct object *obj = parse_object(sha1);
-	if (!obj)
-		return 0;
-	add_name_decoration("", refname, obj);
-	while (obj->type == OBJ_TAG) {
-		obj = ((struct tag *)obj)->tagged;
-		if (!obj)
-			break;
-		add_name_decoration("tag: ", refname, obj);
-	}
-	return 0;
-}
-
 static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		      struct rev_info *rev)
 {
diff --git a/log-tree.c b/log-tree.c
index 30cd5bb..083d08a 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -1,12 +1,48 @@
 #include "cache.h"
 #include "diff.h"
 #include "commit.h"
+#include "tag.h"
 #include "graph.h"
 #include "log-tree.h"
 #include "reflog-walk.h"
+#include "refs.h"
 
 struct decoration name_decoration = { "object names" };
 
+static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
+{
+	int plen = strlen(prefix);
+	int nlen = strlen(name);
+	struct name_decoration *res = xmalloc(sizeof(struct name_decoration) + plen + nlen);
+	memcpy(res->name, prefix, plen);
+	memcpy(res->name + plen, name, nlen + 1);
+	res->next = add_decoration(&name_decoration, obj, res);
+}
+
+int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
+{
+	struct object *obj = parse_object(sha1);
+	if (!obj)
+		return 0;
+	add_name_decoration("", refname, obj);
+	while (obj->type == OBJ_TAG) {
+		obj = ((struct tag *)obj)->tagged;
+		if (!obj)
+			break;
+		add_name_decoration("tag: ", refname, obj);
+	}
+	return 0;
+}
+
+void load_ref_decorations(void)
+{
+	static int loaded;
+	if (!loaded) {
+		loaded = 1;
+		for_each_ref(add_ref_decoration, NULL);
+	}
+}
+
 static void show_parents(struct commit *commit, int abbrev)
 {
 	struct commit_list *p;
diff --git a/log-tree.h b/log-tree.h
index 59ba4c4..beeb86e 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -18,4 +18,7 @@ void log_write_email_headers(struct rev_info *opt, const char *name,
 			     const char **extra_headers_p,
 			     int *need_8bit_cte_p);
 
+int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data);
+void load_ref_decorations(void);
+
 #endif
diff --git a/pretty.c b/pretty.c
index a29c290..4a31e31 100644
--- a/pretty.c
+++ b/pretty.c
@@ -5,6 +5,7 @@
 #include "revision.h"
 #include "string-list.h"
 #include "mailmap.h"
+#include "log-tree.h"
 
 static char *user_format;
 
@@ -423,6 +424,7 @@ struct format_commit_context {
 	struct chunk abbrev_commit_hash;
 	struct chunk abbrev_tree_hash;
 	struct chunk abbrev_parent_hashes;
+	struct chunk decoration;
 };
 
 static int add_again(struct strbuf *sb, struct chunk *chunk)
@@ -481,6 +483,23 @@ static void parse_commit_header(struct format_commit_context *context)
 	context->commit_header_parsed = 1;
 }
 
+static void format_decoration(struct strbuf *sb, const struct commit *commit)
+{
+	struct name_decoration *d;
+	const char *prefix = " (";
+
+	load_ref_decorations();
+	d = lookup_decoration(&name_decoration, &commit->object);
+	while (d) {
+		strbuf_addstr(sb, prefix);
+		prefix = ", ";
+		strbuf_addstr(sb, d->name);
+		d = d->next;
+	}
+	if (prefix[0] == ',')
+		strbuf_addch(sb, ')');
+}
+
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
                                void *context)
 {
@@ -520,6 +539,12 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 			return 3;
 		} else
 			return 0;
+	case 'd':
+		if (add_again(sb, &c->decoration))
+			return 1;
+		format_decoration(sb, commit);
+		c->decoration.len = sb->len - c->decoration.off;
+		return 1;
 	}
 
 	/* these depend on the commit */

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

[Jeff King]

On Thu, Sep 04, 2008 at 12:06:18AM +0200, René Scharfe wrote:

> The patch below does that by using a static variable to remember if the
> decorations have already been loaded.  That's not too unreasonable, I
> think, because we currently have no way of unloading them.

Yes, I think lazy loading is a good solution here, since there is no
extra context that needs to be known to load the decorations.

> +	case 'd':
> +		if (add_again(sb, &c->decoration))
> +			return 1;
> +		format_decoration(sb, commit);
> +		c->decoration.len = sb->len - c->decoration.off;
> +		return 1;

The add_again chunk code is for things that take a long time (like
finding the shortest abbreviation for some hashes). I'm not sure this
counts as "long", as it is just doing a hash lookup on the decoration.
On the other hand, I doubt the extra couple of bytes used hurts much.

-Peff

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

[René Scharfe]

Jeff King schrieb:
> On Thu, Sep 04, 2008 at 12:06:18AM +0200, René Scharfe wrote:
>> +	case 'd':
>> +		if (add_again(sb, &c->decoration))
>> +			return 1;
>> +		format_decoration(sb, commit);
>> +		c->decoration.len = sb->len - c->decoration.off;
>> +		return 1;
> 
> The add_again chunk code is for things that take a long time (like
> finding the shortest abbreviation for some hashes). I'm not sure this
> counts as "long", as it is just doing a hash lookup on the decoration.
> On the other hand, I doubt the extra couple of bytes used hurts much.

Yes, you're right -- let's do without caching at first.  If it turns out
that we're too slow we can easily add it later.

Thanks,
René

[PATCH 1/3] log: add load_ref_decorations()

[René Scharfe]

Move the loading of all ref names for decoration into its own function.
A static variable prevents loading twice, because it's quite expensive.
We can do it this way because we currently never unload decorations.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin-log.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 1d3c5cb..0f16462 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -50,6 +50,15 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in
 	return 0;
 }
 
+void load_ref_decorations(void)
+{
+	static int loaded;
+	if (!loaded) {
+		loaded = 1;
+		for_each_ref(add_ref_decoration, NULL);
+	}
+}
+
 static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		      struct rev_info *rev)
 {
@@ -80,8 +89,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (!strcmp(arg, "--decorate")) {
-			if (!decorate)
-				for_each_ref(add_ref_decoration, NULL);
+			load_ref_decorations();
 			decorate = 1;
 		} else
 			die("unrecognized argument: %s", arg);
-- 
1.6.0.1.161.g7f314

[PATCH 2/3] move load_ref_decorations() to log-tree.c and export it

[René Scharfe]

log-tree.c is the ideal place for load_ref_decorations() and its
helper functions to live in, because the variable name_decoration
they're operating on is already located there, so move them thither.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin-log.c |   35 -----------------------------------
 log-tree.c    |   36 ++++++++++++++++++++++++++++++++++++
 log-tree.h    |    1 +
 3 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 0f16462..081e660 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -14,7 +14,6 @@
 #include "tag.h"
 #include "reflog-walk.h"
 #include "patch-ids.h"
-#include "refs.h"
 #include "run-command.h"
 #include "shortlog.h"
 
@@ -25,40 +24,6 @@ static int default_show_root = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
-static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
-{
-	int plen = strlen(prefix);
-	int nlen = strlen(name);
-	struct name_decoration *res = xmalloc(sizeof(struct name_decoration) + plen + nlen);
-	memcpy(res->name, prefix, plen);
-	memcpy(res->name + plen, name, nlen + 1);
-	res->next = add_decoration(&name_decoration, obj, res);
-}
-
-static int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
-{
-	struct object *obj = parse_object(sha1);
-	if (!obj)
-		return 0;
-	add_name_decoration("", refname, obj);
-	while (obj->type == OBJ_TAG) {
-		obj = ((struct tag *)obj)->tagged;
-		if (!obj)
-			break;
-		add_name_decoration("tag: ", refname, obj);
-	}
-	return 0;
-}
-
-void load_ref_decorations(void)
-{
-	static int loaded;
-	if (!loaded) {
-		loaded = 1;
-		for_each_ref(add_ref_decoration, NULL);
-	}
-}
-
 static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		      struct rev_info *rev)
 {
diff --git a/log-tree.c b/log-tree.c
index 30cd5bb..2c1f3e6 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -1,12 +1,48 @@
 #include "cache.h"
 #include "diff.h"
 #include "commit.h"
+#include "tag.h"
 #include "graph.h"
 #include "log-tree.h"
 #include "reflog-walk.h"
+#include "refs.h"
 
 struct decoration name_decoration = { "object names" };
 
+static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
+{
+	int plen = strlen(prefix);
+	int nlen = strlen(name);
+	struct name_decoration *res = xmalloc(sizeof(struct name_decoration) + plen + nlen);
+	memcpy(res->name, prefix, plen);
+	memcpy(res->name + plen, name, nlen + 1);
+	res->next = add_decoration(&name_decoration, obj, res);
+}
+
+static int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
+{
+	struct object *obj = parse_object(sha1);
+	if (!obj)
+		return 0;
+	add_name_decoration("", refname, obj);
+	while (obj->type == OBJ_TAG) {
+		obj = ((struct tag *)obj)->tagged;
+		if (!obj)
+			break;
+		add_name_decoration("tag: ", refname, obj);
+	}
+	return 0;
+}
+
+void load_ref_decorations(void)
+{
+	static int loaded;
+	if (!loaded) {
+		loaded = 1;
+		for_each_ref(add_ref_decoration, NULL);
+	}
+}
+
 static void show_parents(struct commit *commit, int abbrev)
 {
 	struct commit_list *p;
diff --git a/log-tree.h b/log-tree.h
index 59ba4c4..3c8127b 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -17,5 +17,6 @@ void log_write_email_headers(struct rev_info *opt, const char *name,
 			     const char **subject_p,
 			     const char **extra_headers_p,
 			     int *need_8bit_cte_p);
+void load_ref_decorations(void);
 
 #endif
-- 
1.6.0.1.161.g7f314

[PATCH 3/3] add '%d' pretty format specifier to show decoration

[René Scharfe]

Add a new format placeholder, %d, which expands to a ref name decoration
(think git log --decorate).  It expands to an empty string if the commit
has no decoration, or otherwise to a comma (and space) separated list of
decorations, surrounded by parentheses and a leading space.

Michael Dressel implemented an initial version and chose the letter d,
Junio suggested to add a leading space and parentheses.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Documentation/pretty-formats.txt |    1 +
 pretty.c                         |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 388d492..f18d33e 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': ref names, like the --decorate option of linkgit:git-log[1]
 - '%e': encoding
 - '%s': subject
 - '%b': body
diff --git a/pretty.c b/pretty.c
index a29c290..8beafa0 100644
--- a/pretty.c
+++ b/pretty.c
@@ -5,6 +5,7 @@
 #include "revision.h"
 #include "string-list.h"
 #include "mailmap.h"
+#include "log-tree.h"
 
 static char *user_format;
 
@@ -481,6 +482,23 @@ static void parse_commit_header(struct format_commit_context *context)
 	context->commit_header_parsed = 1;
 }
 
+static void format_decoration(struct strbuf *sb, const struct commit *commit)
+{
+	struct name_decoration *d;
+	const char *prefix = " (";
+
+	load_ref_decorations();
+	d = lookup_decoration(&name_decoration, &commit->object);
+	while (d) {
+		strbuf_addstr(sb, prefix);
+		prefix = ", ";
+		strbuf_addstr(sb, d->name);
+		d = d->next;
+	}
+	if (prefix[0] == ',')
+		strbuf_addch(sb, ')');
+}
+
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
                                void *context)
 {
@@ -573,6 +591,9 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 		                 ? '<'
 		                 : '>');
 		return 1;
+	case 'd':
+		format_decoration(sb, commit);
+		return 1;
 	}
 
 	/* For the rest we have to parse the commit header. */
-- 
1.6.0.1.161.g7f314

Re: [PATCH 3/3] add '%d' pretty format specifier to show decoration

[Jeff King]

On Thu, Sep 04, 2008 at 11:40:03PM +0200, René Scharfe wrote:

> Michael Dressel implemented an initial version and chose the letter d,
> Junio suggested to add a leading space and parentheses.

The whole series looks good to me, and I am happy if it is applied
as-is. The only question I might raise is whether we want to use "%d"
for this, or use something longer to anticipate a collision with other
"d" words (I think you mentioned "describe" at one point).

-Peff

Re: [PATCH 3/3] add '%d' pretty format specifier to show decoration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 04, 2008 at 11:40:03PM +0200, René Scharfe wrote:
>
>> Michael Dressel implemented an initial version and chose the letter d,
>> Junio suggested to add a leading space and parentheses.
>
> The whole series looks good to me, and I am happy if it is applied
> as-is. The only question I might raise is whether we want to use "%d"
> for this, or use something longer to anticipate a collision with other
> "d" words (I think you mentioned "describe" at one point).

How about using "%d()" for this one, so that later enhancements can
specify their features inside parentheses?

Re: [PATCH 3/3] add '%d' pretty format specifier to show decoration

[Jeff King]

On Thu, Sep 04, 2008 at 05:28:13PM -0700, Junio C Hamano wrote:

> > The whole series looks good to me, and I am happy if it is applied
> > as-is. The only question I might raise is whether we want to use "%d"
> > for this, or use something longer to anticipate a collision with other
> > "d" words (I think you mentioned "describe" at one point).
> 
> How about using "%d()" for this one, so that later enhancements can
> specify their features inside parentheses?

I am slightly opposed to that, just because it then is very inconsistent
with the other formatting specifiers. I think it is worth introducing a
new, consistent syntax, providing that syntax for all specifiers (e.g.,
%(body), %(decorate)), and then saying "the existing %b, %d, et al are
still available and will be available forever. BUT they will never grow
the more interesting features like %(body:wrap=80) or
%(decorate:delim=, ).

-Peff

Re: [PATCH 3/3] add '%d' pretty format specifier to show decoration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 04, 2008 at 05:28:13PM -0700, Junio C Hamano wrote:
>
>> > The whole series looks good to me, and I am happy if it is applied
>> > as-is. The only question I might raise is whether we want to use "%d"
>> > for this, or use something longer to anticipate a collision with other
>> > "d" words (I think you mentioned "describe" at one point).
>> 
>> How about using "%d()" for this one, so that later enhancements can
>> specify their features inside parentheses?
>
> I am slightly opposed to that, just because it then is very inconsistent
> with the other formatting specifiers. I think it is worth introducing a
> new, consistent syntax, providing that syntax for all specifiers (e.g.,
> %(body), %(decorate)), and then saying "the existing %b, %d, et al are
> still available and will be available forever. BUT they will never grow
> the more interesting features like %(body:wrap=80) or
> %(decorate:delim=, ).

Ok, fair enough.  Then it will be between "%d" vs "%(decorate)" for
today's three patches.  If we do the former, then we will have one more in
the "existing %b %d et al" set when we do start working on extended
formats; if we do the latter, "existing" will have one less format, but we
have many "existing" ones already, and one more will not hurt that much in
the long run.

So let's take these patches as-is, unless somebody else have better ideas?

Re: [PATCH 3/3] add '%d' pretty format specifier to show decoration

[René Scharfe]

Jeff King schrieb:
> On Thu, Sep 04, 2008 at 11:40:03PM +0200, René Scharfe wrote:
> 
>> Michael Dressel implemented an initial version and chose the letter d,
>> Junio suggested to add a leading space and parentheses.
> 
> The whole series looks good to me, and I am happy if it is applied
> as-is. The only question I might raise is whether we want to use "%d"
> for this, or use something longer to anticipate a collision with other
> "d" words (I think you mentioned "describe" at one point).

%d for decorations is OK, I think -- we still can use %D for describe.
When someone implements it eventually. *cough* ;-)

Thanks,
René

Re: [PATCH 3/3] add '%d' pretty format specifier to show decoration

[Michael Dressel]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 388 bytes --]



On Thu, 4 Sep 2008, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
>> On Thu, Sep 04, 2008 at 11:40:03PM +0200, René Scharfe wrote:
>>
>>> Michael Dressel implemented an initial version and chose the letter d,
>>> Junio suggested to add a leading space and parentheses.
>>

Very nice.
Only that I did not do the very first implementation it was Jeff King.

Cheers,
Michael

Re: [PATCH 3/3] add '%d' pretty format specifier to show decoration

[Michael Dressel]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 661 bytes --]

On Fri, 5 Sep 2008, Michael Dressel wrote:

>
>
> On Thu, 4 Sep 2008, Junio C Hamano wrote:
>
>>  Jeff King <peff@peff.net> writes:
>> 
>> >  On Thu, Sep 04, 2008 at 11:40:03PM +0200, René Scharfe wrote:
>> > 
>> > >  Michael Dressel implemented an initial version and chose the letter d,
>> > >  Junio suggested to add a leading space and parentheses.
>> > 
>
> Very nice.
> Only that I did not do the very first implementation it was Jeff King.

I made a mistake. My last comment is wrong, There is an even  earlier 
patch from Johannes Schindelin. Second there is no contribution  left from 
me. I'm merely a happy user of the new feature.

Cheers,
Michael

Re: [PATCH 3/3] add '%d' pretty format specifier to show decoration

[René Scharfe]

Michael Dressel schrieb:
> On Fri, 5 Sep 2008, Michael Dressel wrote:
>> On Thu, 4 Sep 2008, Junio C Hamano wrote:
>>> Jeff King <peff@peff.net> writes:
>>>> On Thu, Sep 04, 2008 at 11:40:03PM +0200, René Scharfe wrote:
>>>>> Michael Dressel implemented an initial version and chose the 
>>>>> letter d, Junio suggested to add a leading space and 
>>>>> parentheses.
>> 
>> Very nice. Only that I did not do the very first implementation it 
>> was Jeff King.
> 
> I made a mistake. My last comment is wrong, There is an even  earlier
> patch from Johannes Schindelin. Second there is no contribution left
> from me. I'm merely a happy user of the new feature.

Hmm, when I wrote this commit message I didn't reread yours that
mentioned Jeff.  The patch is already in next; I'm not sure if changing
its commit message is a sensible option.  I remember that Dscho
suggested multiple times to steal his ideas, implement them and then
reap all the credit for them, though. ;-)

René