log-tree: make format_decorations more flexible

PATCH In review Harry Jeffery 2 participants 6 messages Jun 15, 2016 Documentation

Share & tools

Harry Jeffery

author

2016-06-15 23:02 UTC #0 lore raw

PATCH 1/2 log-tree: make format_decorations more flexible

The prefix, separator and suffix for decorations are hard-coded. Make format_decorations more flexible by having the caller specify the prefix, separator and suffix.

Signed-off-by Harry Jeffery <[email protected]>

---
 log-tree.c | 16 +++++++++-------
 log-tree.h |  2 +-
 pretty.c   |  2 +-
 3 files changed, 11 insertions(+), 9 deletions(-)

log-tree.c +9 -7

index 95e9b1d..860694c 100644
@@ -184,9 +184,11 @@ static void show_children(struct rev_info *opt, struct commit *commit, int abbre
  */
 void format_decorations(struct strbuf *sb,
 			const struct commit *commit,
-			int use_color)
+			int use_color,
+			const char* prefix,
+			const char* sep,
+			const char* suffix)
 {
-	const char *prefix;
 	struct name_decoration *decoration;
 	const char *color_commit =
 		diff_get_color(use_color, DIFF_COMMIT);

@@ -196,20 +198,20 @@ void format_decorations(struct strbuf *sb,
 	decoration = lookup_decoration(&name_decoration, &commit->object);
 	if (!decoration)
 		return;
-	prefix = " (";
+	strbuf_addstr(sb, prefix);
 	while (decoration) {
 		strbuf_addstr(sb, color_commit);
-		strbuf_addstr(sb, prefix);
 		strbuf_addstr(sb, decorate_get_color(use_color, decoration->type));
 		if (decoration->type == DECORATION_REF_TAG)
 			strbuf_addstr(sb, "tag: ");
 		strbuf_addstr(sb, decoration->name);
+		if(decoration->next)
+			strbuf_addstr(sb, sep);
 		strbuf_addstr(sb, color_reset);
-		prefix = ", ";
 		decoration = decoration->next;
 	}
 	strbuf_addstr(sb, color_commit);
-	strbuf_addch(sb, ')');
+	strbuf_addstr(sb, suffix);
 	strbuf_addstr(sb, color_reset);
 }

@@ -221,7 +223,7 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
 		printf("\t%s", (char *) commit->util);
 	if (!opt->show_decorations)
 		return;
-	format_decorations(&sb, commit, opt->diffopt.use_color);
+	format_decorations(&sb, commit, opt->diffopt.use_color, " (", ", ", ")");
 	fputs(sb.buf, stdout);
 	strbuf_release(&sb);
 }

log-tree.h +1 -1

index d6ecd4d..4816911 100644
@@ -13,7 +13,7 @@ int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
 int log_tree_opt_parse(struct rev_info *, const char **, int);
 void show_log(struct rev_info *opt);
-void format_decorations(struct strbuf *sb, const struct commit *commit, int use_color);
+void format_decorations(struct strbuf *sb, const struct commit *commit, int use_color, const char* prefix, const char* sep, const char* suffix);
 void show_decorations(struct rev_info *opt, struct commit *commit);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **subject_p,

pretty.c +1 -1

index 44b9f64..e4dc093 100644
@@ -1195,7 +1195,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return 1;
 	case 'd':
 		load_ref_decorations(DECORATE_SHORT_REFS);
-		format_decorations(sb, commit, c->auto_color);
+		format_decorations(sb, commit, c->auto_color, " (", ", ", ")");
 		return 1;
 	case 'g':		/* reflog info */
 		switch(placeholder[1]) {

Signature · 1 line

2.1.0

Harry Jeffery

author

2016-06-15 23:02 UTC #1 lore raw

PATCH 2/2 pretty: add %D format specifier

%d prints decorations wrapped by " (" and ")". %D provides the same output without the parenthesis, making " (%D)" and "%d" equivalent to one another.

Signed-off-by Harry Jeffery <[email protected]>

---
 Documentation/pretty-formats.txt | 6 ++++--
 pretty.c                         | 4 ++++
 2 files changed, 8 insertions(+), 2 deletions(-)

Documentation/pretty-formats.txt +4 -2

index eac7909..2632e1a 100644
@@ -128,6 +128,7 @@ The placeholders are:
 - '%ct': committer date, UNIX timestamp
 - '%ci': committer date, ISO 8601 format
 - '%d': ref names, like the --decorate option of linkgit:git-log[1]
+- '%D': ref names without the " (", ")" wrapping.
 - '%e': encoding
 - '%s': subject
 - '%f': sanitized subject line, suitable for a filename

@@ -182,8 +183,9 @@ The placeholders are:
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
 insert an empty string unless we are traversing reflog entries (e.g., by
-`git log -g`). The `%d` placeholder will use the "short" decoration
-format if `--decorate` was not already provided on the command line.
+`git log -g`). The `%d` and `%D` placeholders will use the "short"
+decoration format if `--decorate` was not already provided on the command
+line.
 
 If you add a `+` (plus sign) after '%' of a placeholder, a line-feed
 is inserted immediately before the expansion if and only if the

pretty.c +4 -0

index e4dc093..a75ad0d 100644
@@ -1197,6 +1197,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		load_ref_decorations(DECORATE_SHORT_REFS);
 		format_decorations(sb, commit, c->auto_color, " (", ", ", ")");
 		return 1;
+	case 'D':
+		load_ref_decorations(DECORATE_SHORT_REFS);
+		format_decorations(sb, commit, c->auto_color, "", ", ", "");
+		return 1;
 	case 'g':		/* reflog info */
 		switch(placeholder[1]) {
 		case 'd':	/* reflog selector */

Signature · 1 line

2.1.0

Junio C Hamano

2016-06-15 23:02 UTC #2 lore raw

Re: [PATCH 2/2] pretty: add %D format specifier

Because patch 1/2 alone does not make much sense without 2/2, it probably would have been better to do these as a single patch.

And of course a few additional tests to t4205 would not hurt ;-)

Thanks.

Harry Jeffery

author

2016-06-15 23:02 UTC #3 lore raw

Re: [PATCH 2/2] pretty: add %D format specifier

Quoted 2 lines · Junio C Hamano

Because patch 1/2 alone does not make much sense without 2/2, it probably would have been better to do these as a single patch.

Would you like me to resubmit it as a single patch, or are you applying them as is?

Quoted 1 line

And of course a few additional tests to t4205 would not hurt ;-)

Sure. Should the tests be in the same patch, or a subsequent one?

Junio C Hamano

2016-06-15 23:02 UTC #4 lore raw

Re: [PATCH 2/2] pretty: add %D format specifier

Quoted 10 lines · Harry Jeffery

Quoted 2 lines · Junio C Hamano
Because patch 1/2 alone does not make much sense without 2/2, it probably would have been better to do these as a single patch.
Would you like me to resubmit it as a single patch, or are you applying them as is?
Quoted 1 line
And of course a few additional tests to t4205 would not hurt ;-)
Sure. Should the tests be in the same patch, or a subsequent one?

All in one; that way, "git show $that_single_patch" later can become more useful by demonstrating expected uses in its test.

Junio C Hamano

2016-06-15 23:02 UTC #5 lore raw

In reply to Harry Jeffery

Quoted 19 lines · Harry Jeffery

The prefix, separator and suffix for decorations are hard-coded. Make format_decorations more flexible by having the caller specify the prefix, separator and suffix.
Signed-off-by Harry Jeffery <[email protected]>
---
 log-tree.c | 16 +++++++++-------
 log-tree.h |  2 +-
 pretty.c   |  2 +-
 3 files changed, 11 insertions(+), 9 deletions(-)
log-tree.c +0 -0
index 95e9b1d..860694c 100644
@@ -184,9 +184,11 @@ static void show_children(struct rev_info *opt,
struct commit *commit, int abbre
*/
void format_decorations(struct strbuf *sb,

log-tree.c static void show_children(struct rev_info *opt, struct commit *commit, int abbre · line 190

Quoted 6 lines

log-tree.c +4 -1

 			const struct commit *commit,
-			int use_color)
+			int use_color,
+			const char* prefix,
+			const char* sep,
+			const char* suffix)

In our codebase, please make asterisks stick to the variable not the type, i.e.

const char *prefix,
const char *separator,
const char *suffix)

Was there a reason why "sep" alone needed to be abbreviated?

log-tree.c void format_decorations(struct strbuf *sb, · line 208

Quoted 12 lines

log-tree.c +2 -2

 	if (!decoration)
 		return;
-	prefix = " (";
+	strbuf_addstr(sb, prefix);
 	while (decoration) {
 		strbuf_addstr(sb, color_commit);
-		strbuf_addstr(sb, prefix);
 		strbuf_addstr(sb, decorate_get_color(use_color, decoration->type));
 		if (decoration->type == DECORATION_REF_TAG)
 			strbuf_addstr(sb, "tag: ");
 		strbuf_addstr(sb, decoration->name);
+		if(decoration->next)

Have SP between the control statement (i.e. not a function name) and its opening parenthesis, i.e.

		if (decoration->next)

log-tree.c void format_decorations(struct strbuf *sb, · line 212

Quoted 5 lines

log-tree.c +1 -1

+			strbuf_addstr(sb, sep);
 		strbuf_addstr(sb, color_reset);
-		prefix = ", ";
 		decoration = decoration->next;
 	}

Hmph. I was kind of found of the nice trick to use a punctuation, which first points at the prefix " (" and then later points at the separator ", ", to allow the code that prefixes the punctuation before showing a new item. It is now lost.

Quoted 21 lines

log-tree.c +2 -2

 	strbuf_addstr(sb, color_commit);
-	strbuf_addch(sb, ')');
+	strbuf_addstr(sb, suffix);
 	strbuf_addstr(sb, color_reset);
 }

@@ -221,7 +223,7 @@ void show_decorations(struct rev_info *opt, struct
 commit *commit)
 		printf("\t%s", (char *) commit->util);
 	if (!opt->show_decorations)
 		return;
-	format_decorations(&sb, commit, opt->diffopt.use_color);
+	format_decorations(&sb, commit, opt->diffopt.use_color, " (", ", ", ")");
 	fputs(sb.buf, stdout);
 	strbuf_release(&sb);

}

log-tree.h +0 -0

index d6ecd4d..4816911 100644

log-tree.h int log_tree_diff_flush(struct rev_info *); · line 18

Quoted 7 lines

log-tree.h +1 -1

@@ -13,7 +13,7 @@ int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
 int log_tree_opt_parse(struct rev_info *, const char **, int);
 void show_log(struct rev_info *opt);
-void format_decorations(struct strbuf *sb, const struct commit
 *commit, int use_color);
+void format_decorations(struct strbuf *sb, const struct commit
 *commit, int use_color, const char* prefix, const char* sep, const

Linewrapped by your MUA, perhaps?

Again, please check where your asterisks are.

Quoted 9 lines

log-tree.h +0 -0
 char* suffix);
void show_decorations(struct rev_info *opt, struct commit *commit); void log_write_email_headers(struct rev_info *opt, struct commit *commit,
log-tree.h +0 -0
 			     const char **subject_p,
pretty.c +0 -0
index 44b9f64..e4dc093 100644

pretty.c static size_t format_commit_one(struct strbuf · line 1199

Quoted 6 lines

pretty.c +1 -1

@@ -1195,7 +1195,7 @@ static size_t format_commit_one(struct strbuf
 *sb, /* in UTF-8 */
 		return 1;
 	case 'd':
 		load_ref_decorations(DECORATE_SHORT_REFS);
-		format_decorations(sb, commit, c->auto_color);
+		format_decorations(sb, commit, c->auto_color, " (", ", ", ")");

My eyes hurt staring at this line and the same one in the other file, trying to see which comma is which.

I wonder if doing something like this once at a single place:

#define format_decorations_std(strbuf, commit, color) \
		format_decorations((strbuf), (commit), (color), " (", ", ", ")")

and using

	format_decorations_std(sb, commit, opt->diffopt.use_color);
	format_decorations_std(sb, commit, c->auto_color);

or even better, name the one that takes three extra parameters as format_decorations_extended(), and keep the behaviour of the original one the same, i.e.

	#define format_decorations(strbuf, commit, color) \
		format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")

That way you do not have to touch the original callers, nor you would have to worry about breaking any topic that somebody else may be preparing that adds new calls to format_decorations().

Quoted 3 lines

pretty.c +0 -0
 		return 1;
 	case 'g':		/* reflog info */
switch(placeholder[1]) {