# [BUG?] git log does not decorate when custom format is used 14 messages from 2008-08-20 to 2008-09-03. Participants: MichaelTiloDressel@t-online.de, Jeff King, Junio C Hamano, Johannes Schindelin, Michael Dressel, Teemu Likonen, Jakub Narebski. Thread: https://gitlist.dev/t/15107 ## MichaelTiloDressel@t-online.de, 2008-08-20 12:25 Subject: [BUG?] git log does not decorate when custom format is used Message-ID: <1KVmkk-2IOrGi0@fwd26.aul.t-online.de> URL: https://gitlist.dev/e/1KVmkk-2IOrGi0%40fwd26.aul.t-online.de ``` 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 ``` ## Jeff King, 2008-08-20 17:53 Subject: Re: [BUG?] git log does not decorate when custom format is used Message-ID: <20080820175325.GD27773@sigill.intra.peff.net> URL: https://gitlist.dev/e/20080820175325.GD27773%40sigill.intra.peff.net In-Reply-To: <1KVmkk-2IOrGi0@fwd26.aul.t-online.de> ``` 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 ``` ## Jeff King, 2008-08-20 17:55 Subject: [PATCH 1/2] decorate: allow const objects to be decorated Message-ID: <20080820175532.GA32005@sigill.intra.peff.net> URL: https://gitlist.dev/e/20080820175532.GA32005%40sigill.intra.peff.net In-Reply-To: <20080820175325.GD27773@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Jeff King, 2008-08-20 18:00 Subject: [PATCH 2/2] allow '%d' pretty format specifier to show decoration Message-ID: <20080820180034.GB32005@sigill.intra.peff.net> URL: https://gitlist.dev/e/20080820180034.GB32005%40sigill.intra.peff.net In-Reply-To: <20080820175325.GD27773@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Junio C Hamano, 2008-08-20 18:32 Subject: Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration Message-ID: <7vwsibr0zz.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwsibr0zz.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080820180034.GB32005@sigill.intra.peff.net> ``` Jeff King 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. ``` ## Johannes Schindelin, 2008-08-20 18:43 Subject: Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0808202042260.24820%40pacific.mpi-cbg.de.mpi-cbg.de In-Reply-To: <20080820180034.GB32005@sigill.intra.peff.net> ``` 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 ``` ## Junio C Hamano, 2008-08-20 19:01 Subject: Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration Message-ID: <7vljyrqzn0.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vljyrqzn0.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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. ``` ## Michael Dressel, 2008-08-20 19:51 Subject: Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.10.0808202138340.4070%40pollux In-Reply-To: <20080820180034.GB32005@sigill.intra.peff.net> ``` 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 ``` ## Johannes Schindelin, 2008-08-20 19:53 Subject: Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0808202153140.24820%40pacific.mpi-cbg.de.mpi-cbg.de In-Reply-To: <7vljyrqzn0.fsf@gitster.siamese.dyndns.org> ``` Hi, On Wed, 20 Aug 2008, Junio C Hamano wrote: > Johannes Schindelin 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 ``` ## Junio C Hamano, 2008-08-20 20:21 Subject: Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration Message-ID: <7vzln7phe1.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzln7phe1.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Michael Dressel writes: > I use it in a script to extract the log title of commits between certain > tags. "git shortlog"? ``` ## Michael Dressel, 2008-08-20 21:05 Subject: Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.10.0808202305230.4170%40pollux In-Reply-To: <7vzln7phe1.fsf@gitster.siamese.dyndns.org> ``` On Wed, 20 Aug 2008, Junio C Hamano wrote: > Michael Dressel 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 ``` ## Teemu Likonen, 2008-08-21 05:02 Subject: "log --pretty=format:" language Message-ID: <20080821050223.GA3295@mithlond.arda.local> URL: https://gitlist.dev/e/20080821050223.GA3295%40mithlond.arda.local In-Reply-To: <20080820180034.GB32005@sigill.intra.peff.net> ``` 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. [...] ``` ## Jakub Narebski, 2008-08-24 18:30 Subject: Re: "log --pretty=format:" language Message-ID: URL: https://gitlist.dev/e/m33akus1tj.fsf%40localhost.localdomain In-Reply-To: <20080821050223.GA3295@mithlond.arda.local> ``` Teemu Likonen 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 ``` ## Jeff King, 2008-09-03 19:36 Subject: Re: [PATCH 2/2] allow '%d' pretty format specifier to show decoration Message-ID: <20080903193615.GA31225@coredump.intra.peff.net> URL: https://gitlist.dev/e/20080903193615.GA31225%40coredump.intra.peff.net In-Reply-To: ``` 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 ```