[PATCH v2 0/3] pretty: format aliases

The following patch series adds the ability to configure aliases for
user-defined formats. The first two patches define new placeholders and
modify the output of existing placeholders to allow aliases to be more
consistent with the way builtin formats are handled. The final patch
adds support for the aliases themselves.

There were a couple of places where I wasn't entirely sure about which
color setting I should be following, but I've tried to be consistent
throughout. It may be that I could have simply followed diffopt's color
option in all cases, in which case various modifications to show_log()
were entirely unnecessary. I'll await judgement at the hands of one who
groks those sections more than I do, but I think what I've done feels
correct.

My original goal was to make it possible to define all of the builtin
formats as builtin aliases to format strings, but complications
regarding how --parents and --decorate would be handled require further
thought and discussion. For example, we could simply make
"--format=%H --decorate" synonymous with "--format=%H%d", but I'm not
sure if that feels clean enough.

For now, I think this is at a point where its good-enough to submit, if
only as a starting point for some discussion as to where to head next.

This is the second version of the patch. Following feedback from Jeff
King [off-list ref], I realized that the modification to the arguments
of show_log() were unnecessary, as they only made a difference within
show-branch.c, which does not accept a --format option in any case.

Will Palmer (3):
  pretty: add conditional %C?colorname placeholders
  pretty: make %H/%h dependent on --abbrev[-commit]
  pretty: add aliases for pretty formats

 Documentation/config.txt         |    8 ++
 Documentation/pretty-formats.txt |    1 +
 builtin/log.c                    |    2 +-
 builtin/rev-list.c               |    2 +
 builtin/shortlog.c               |    7 +-
 commit.h                         |    2 +
 log-tree.c                       |    3 +
 pretty.c                         |  248 ++++++++++++++++++++++++++++++--------
 shortlog.h                       |    2 +-
 t/t4205-log-pretty-formats.sh    |   87 +++++++++++++
 10 files changed, 307 insertions(+), 55 deletions(-)
 create mode 100755 t/t4205-log-pretty-formats.sh

Many commands are able to colorize, or not, depending on a user's
configuration and whether output is being sent to a terminal. However,
if a user explicitly specifies a --pretty format, color will always be
output, regardless of the destination. This would generally be okay, in
a "do what I tell you, whether or not I should tell you to" sense, but
the assumption fell apart when an alias was defined which may be run in
various contexts: there was no way to specify "use this color, but only
if you normally would display color at all"

Here we add the %C?colorname placeholders which act just as the
%Ccolorname placeholders, with the exception that the pretty_context is
checked to see if color should be used according to configuration

Signed-off-by: Will Palmer <redacted>
---
 Documentation/pretty-formats.txt |    1 +
 builtin/log.c                    |    2 +-
 builtin/rev-list.c               |    1 +
 builtin/shortlog.c               |    5 ++-
 commit.h                         |    1 +
 log-tree.c                       |    1 +
 pretty.c                         |   49 ++++++++++++++++++++++++-------------
 shortlog.h                       |    2 +-
 t/t4205-log-pretty-formats.sh    |   44 ++++++++++++++++++++++++++++++++++
 9 files changed, 85 insertions(+), 21 deletions(-)
 create mode 100755 t/t4205-log-pretty-formats.sh

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 1686a54..53eb903 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -132,6 +132,7 @@ The placeholders are:
 - '%Cblue': switch color to blue
 - '%Creset': reset color
 - '%C(...)': color specification, as described in color.branch.* config option
+- '%C?...: switch to specified color, if relevant color.* config option specifies that color is ok
 - '%m': left, right or boundary mark
 - '%n': newline
 - '%%': a raw '%'

diff --git a/builtin/log.c b/builtin/log.c
index 6208703..a9fc6ea 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -740,7 +740,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	log.in1 = 2;
 	log.in2 = 4;
 	for (i = 0; i < nr; i++)
-		shortlog_add_commit(&log, list[i]);
+		shortlog_add_commit(&log, list[i], rev);
 
 	shortlog_output(&log);
 

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 51ceb19..5a53862 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c

@@ -99,6 +99,7 @@ static void show_commit(struct commit *commit, void *data)
 		struct pretty_print_context ctx = {0};
 		ctx.abbrev = revs->abbrev;
 		ctx.date_mode = revs->date_mode;
+		ctx.use_color = DIFF_OPT_TST(&revs->diffopt, COLOR_DIFF);
 		pretty_print_commit(revs->commit_format, commit, &buf, &ctx);
 		if (revs->graph) {
 			if (buf.len) {

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 06320f5..7aee491 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c

@@ -136,13 +136,14 @@ static void read_from_stdin(struct shortlog *log)
 	}
 }
 
-void shortlog_add_commit(struct shortlog *log, struct commit *commit)
+void shortlog_add_commit(struct shortlog *log, struct commit *commit, struct rev_info *rev)
 {
 	const char *author = NULL, *buffer;
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ufbuf = STRBUF_INIT;
 	struct pretty_print_context ctx = {0};
 
+	ctx.use_color = DIFF_OPT_TST(&rev->diffopt, COLOR_DIFF);
 	pretty_print_commit(CMIT_FMT_RAW, commit, &buf, &ctx);
 	buffer = buf.buf;
 	while (*buffer && *buffer != '\n') {

@@ -183,7 +184,7 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)
 	if (prepare_revision_walk(rev))
 		die("revision walk setup failed");
 	while ((commit = get_revision(rev)) != NULL)
-		shortlog_add_commit(log, commit);
+		shortlog_add_commit(log, commit, rev);
 }
 
 static int parse_uint(char const **arg, int comma, int defval)

diff --git a/commit.h b/commit.h
index 26ec8c0..b6caf91 100644
--- a/commit.h
+++ b/commit.h

@@ -71,6 +71,7 @@ struct pretty_print_context
 	enum date_mode date_mode;
 	int need_8bit_cte;
 	int show_notes;
+	int use_color;
 	struct reflog_walk_info *reflog_info;
 };
 

diff --git a/log-tree.c b/log-tree.c
index d3ae969..6bb4748 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -282,6 +282,7 @@ void show_log(struct rev_info *opt)
 	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
 	const char *extra_headers = opt->extra_headers;
 	struct pretty_print_context ctx = {0};
+	ctx.use_color = DIFF_OPT_TST(&opt->diffopt, COLOR_DIFF);
 
 	opt->loginfo = NULL;
 	ctx.show_notes = opt->show_notes;

diff --git a/pretty.c b/pretty.c
index 7cb3a2a..fdb5e16 100644
--- a/pretty.c
+++ b/pretty.c

@@ -634,35 +634,50 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 	struct format_commit_context *c = context;
 	const struct commit *commit = c->commit;
 	const char *msg = commit->buffer;
+	const char *color_start;
 	struct commit_list *p;
+	int use_color = c->pretty_ctx->use_color;
+	int conditional_color = 0;
 	int h1, h2;
 
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
 	case 'C':
-		if (placeholder[1] == '(') {
+		color_start = placeholder;
+		if (placeholder[1] == '?') {
+			color_start++;
+			conditional_color = 1;
+		}
+		if (color_start[1] == '(') {
 			const char *end = strchr(placeholder + 2, ')');
 			char color[COLOR_MAXLEN];
 			if (!end)
 				return 0;
-			color_parse_mem(placeholder + 2,
-					end - (placeholder + 2),
-					"--pretty format", color);
-			strbuf_addstr(sb, color);
+			if ( !conditional_color || use_color ) {
+				color_parse_mem(color_start + 2,
+						end - (color_start + 2),
+						"--pretty format", color);
+				strbuf_addstr(sb, color);
+			}
 			return end - placeholder + 1;
 		}
-		if (!prefixcmp(placeholder + 1, "red")) {
-			strbuf_addstr(sb, GIT_COLOR_RED);
-			return 4;
-		} else if (!prefixcmp(placeholder + 1, "green")) {
-			strbuf_addstr(sb, GIT_COLOR_GREEN);
-			return 6;
-		} else if (!prefixcmp(placeholder + 1, "blue")) {
-			strbuf_addstr(sb, GIT_COLOR_BLUE);
-			return 5;
-		} else if (!prefixcmp(placeholder + 1, "reset")) {
-			strbuf_addstr(sb, GIT_COLOR_RESET);
-			return 6;
+
+		if (!prefixcmp(color_start + 1, "red")) {
+			if ( !conditional_color || use_color )
+				strbuf_addstr(sb, GIT_COLOR_RED);
+			return 4 + (conditional_color ? 1 : 0);
+		} else if (!prefixcmp(color_start + 1, "green")) {
+			if ( !conditional_color || use_color )
+				strbuf_addstr(sb, GIT_COLOR_GREEN);
+			return 6 + (conditional_color ? 1 : 0);
+		} else if (!prefixcmp(color_start + 1, "blue")) {
+			if ( !conditional_color || use_color )
+				strbuf_addstr(sb, GIT_COLOR_BLUE);
+			return 5 + (conditional_color ? 1 : 0);
+		} else if (!prefixcmp(color_start + 1, "reset")) {
+			if ( !conditional_color || use_color )
+				strbuf_addstr(sb, GIT_COLOR_RESET);
+			return 6 + (conditional_color ? 1 : 0);
 		} else
 			return 0;
 	case 'n':		/* newline */

diff --git a/shortlog.h b/shortlog.h
index bc02cc2..43498a0 100644
--- a/shortlog.h
+++ b/shortlog.h

@@ -20,7 +20,7 @@ struct shortlog {
 
 void shortlog_init(struct shortlog *log);
 
-void shortlog_add_commit(struct shortlog *log, struct commit *commit);
+void shortlog_add_commit(struct shortlog *log, struct commit *commit, struct rev_info *rev);
 
 void shortlog_output(struct shortlog *log);
 

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
new file mode 100755
index 0000000..28d2948
--- /dev/null
+++ b/t/t4205-log-pretty-formats.sh

@@ -0,0 +1,44 @@
+#!/bin/sh
+#
+# Released into Public Domain by Will Palmer 2010
+#
+
+test_description='Test pretty formats'
+. ./test-lib.sh
+
+test_expect_success "set up basic repos" ">foo && git add foo && git commit -m initial"
+
+for flag in false true always; do
+for color in red green blue reset; do
+
+	make_expected="git config --get-color no.such.slot $color >expected"
+	test_expect_success "%C$color with color.ui $flag" \
+		"$make_expected &&
+		git config color.ui $flag &&
+		git log -1 --pretty=format:'%C$color' > actual &&
+		cmp expected actual"
+
+
+	test_expect_success "%C($color) with color.ui $flag" \
+		"$make_expected &&
+		git config color.ui $flag &&
+		git log -1 --pretty=format:'%C($color)' > actual &&
+		cmp expected actual"
+
+	[ ! "$flag" = "always" ] && make_expected=">expected"
+	test_expect_success "%C?$color with color.ui $flag" \
+		"$make_expected &&
+		git config color.ui $flag &&
+		git log -1 --pretty=format:'%C?$color' > actual &&
+		cmp expected actual"
+
+	test_expect_success "%C?($color) with color.ui $flag" \
+		"$make_expected &&
+		git config color.ui $flag &&
+		git log -1 --pretty=format:'%C?($color)' > actual &&
+		cmp expected actual"
+
+done
+done
+
+test_done

-- 
1.7.1.rc1.13.gbb0a0a.dirty

Prior to this, the output of %H was always 40 characters long, and the
output of %h always DEFAULT_ABBREV characters long, without regard to
whether --abbrev-commit or --abbrev had been passed.

Here we make "git log --pretty=%H --abbrev-commit" synonymous with
"git log --pretty=%h", and make %h/abbreviated-%H respect the length
specified for --abbrev.

The same is applied to other commit-placeholders %P and %p, and
--abbrev is respected for %t, though %T is not changed.

Signed-off-by: Will Palmer <redacted>
---
 builtin/rev-list.c |    1 +
 builtin/shortlog.c |    2 ++
 commit.h           |    1 +
 log-tree.c         |    2 ++
 pretty.c           |   30 +++++++++++++++++++-----------
 5 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 5a53862..1d1e59c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c

@@ -98,6 +98,7 @@ static void show_commit(struct commit *commit, void *data)
 		struct strbuf buf = STRBUF_INIT;
 		struct pretty_print_context ctx = {0};
 		ctx.abbrev = revs->abbrev;
+		ctx.abbrev_commit = revs->abbrev_commit;
 		ctx.date_mode = revs->date_mode;
 		ctx.use_color = DIFF_OPT_TST(&revs->diffopt, COLOR_DIFF);
 		pretty_print_commit(revs->commit_format, commit, &buf, &ctx);

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 7aee491..5c0721c 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c

@@ -143,6 +143,8 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit, struct rev
 	struct strbuf ufbuf = STRBUF_INIT;
 	struct pretty_print_context ctx = {0};
 
+	ctx.abbrev = rev->abbrev;
+	ctx.abbrev_commit = rev->abbrev_commit;
 	ctx.use_color = DIFF_OPT_TST(&rev->diffopt, COLOR_DIFF);
 	pretty_print_commit(CMIT_FMT_RAW, commit, &buf, &ctx);
 	buffer = buf.buf;

diff --git a/commit.h b/commit.h
index b6caf91..7a476a0 100644
--- a/commit.h
+++ b/commit.h

@@ -72,6 +72,7 @@ struct pretty_print_context
 	int need_8bit_cte;
 	int show_notes;
 	int use_color;
+	int abbrev_commit;
 	struct reflog_walk_info *reflog_info;
 };
 

diff --git a/log-tree.c b/log-tree.c
index 6bb4748..0a2309c 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -282,6 +282,8 @@ void show_log(struct rev_info *opt)
 	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
 	const char *extra_headers = opt->extra_headers;
 	struct pretty_print_context ctx = {0};
+	ctx.abbrev = opt->abbrev;
+	ctx.abbrev_commit = opt->abbrev_commit;
 	ctx.use_color = DIFF_OPT_TST(&opt->diffopt, COLOR_DIFF);
 
 	opt->loginfo = NULL;

diff --git a/pretty.c b/pretty.c
index fdb5e16..f884f48 100644
--- a/pretty.c
+++ b/pretty.c

@@ -725,13 +725,16 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 
 	switch (placeholder[0]) {
 	case 'H':		/* commit hash */
-		strbuf_addstr(sb, sha1_to_hex(commit->object.sha1));
-		return 1;
 	case 'h':		/* abbreviated commit hash */
+		if (placeholder[0] != 'h' && !c->pretty_ctx->abbrev_commit) {
+			strbuf_addstr(sb, sha1_to_hex(commit->object.sha1));
+			return 1;
+		}
+
 		if (add_again(sb, &c->abbrev_commit_hash))
 			return 1;
 		strbuf_addstr(sb, find_unique_abbrev(commit->object.sha1,
-		                                     DEFAULT_ABBREV));
+						     c->pretty_ctx->abbrev));
 		c->abbrev_commit_hash.len = sb->len - c->abbrev_commit_hash.off;
 		return 1;
 	case 'T':		/* tree hash */

@@ -741,24 +744,29 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 		if (add_again(sb, &c->abbrev_tree_hash))
 			return 1;
 		strbuf_addstr(sb, find_unique_abbrev(commit->tree->object.sha1,
-		                                     DEFAULT_ABBREV));
+						     c->pretty_ctx->abbrev));
 		c->abbrev_tree_hash.len = sb->len - c->abbrev_tree_hash.off;
 		return 1;
 	case 'P':		/* parent hashes */
-		for (p = commit->parents; p; p = p->next) {
-			if (p != commit->parents)
-				strbuf_addch(sb, ' ');
-			strbuf_addstr(sb, sha1_to_hex(p->item->object.sha1));
-		}
-		return 1;
 	case 'p':		/* abbreviated parent hashes */
+		if (placeholder[0] != 'p' && !c->pretty_ctx->abbrev_commit) {
+			for (p = commit->parents; p; p = p->next) {
+				if (p != commit->parents)
+					strbuf_addch(sb, ' ');
+				strbuf_addstr(sb,
+					      sha1_to_hex(p->item->object.sha1));
+			}
+			return 1;
+		}
+
 		if (add_again(sb, &c->abbrev_parent_hashes))
 			return 1;
 		for (p = commit->parents; p; p = p->next) {
 			if (p != commit->parents)
 				strbuf_addch(sb, ' ');
 			strbuf_addstr(sb, find_unique_abbrev(
-					p->item->object.sha1, DEFAULT_ABBREV));
+				      p->item->object.sha1,
+				      c->pretty_ctx->abbrev));
 		}
 		c->abbrev_parent_hashes.len = sb->len -
 		                              c->abbrev_parent_hashes.off;

-- 
1.7.1.rc1.13.gbb0a0a.dirty

previously the only ways to alias a --pretty format within git were
either to set the format as your default format (via the format.pretty
configuration variable), or by using a regular git alias. This left the
definition of more complicated formats to the realm of "builtin or
nothing", with user-defined formats usually being reserved for quick
one-offs.

Here we allow user-defined formats to enjoy more or less the same
benefits of builtins. By defining format.pretty.myalias, "myalias" can
be used in place of whatever would normally come after --pretty=. This
can be a format:, tformat:, raw (ie, defaulting to tformat), or the name
of another builtin or user-defined pretty format.

Signed-off-by: Will Palmer <redacted>
---
 Documentation/config.txt      |    8 ++
 pretty.c                      |  169 +++++++++++++++++++++++++++++++++++------
 t/t4205-log-pretty-formats.sh |   53 ++++++++++++-
 3 files changed, 202 insertions(+), 28 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 92f851e..6573d18 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -894,6 +894,14 @@ format.pretty::
 	See linkgit:git-log[1], linkgit:git-show[1],
 	linkgit:git-whatchanged[1].
 
+format.pretty.<name>::
+	Alias for a --pretty= format string, as specified in
+	linkgit:git-log[1]. Any aliases defined here can be used just
+	as the builtin pretty formats could. For example, defining
+	"format.pretty.hash = format:%H" would cause the invocation
+	"git log --pretty=hash" to be equivalent to running
+	"git log --pretty=format:%H".
+
 format.thread::
 	The default threading style for 'git format-patch'.  Can be
 	a boolean value, or `shallow` or `deep`.  `shallow` threading

diff --git a/pretty.c b/pretty.c
index f884f48..d49fec7 100644
--- a/pretty.c
+++ b/pretty.c

@@ -11,6 +11,16 @@
 #include "reflog-walk.h"
 
 static char *user_format;
+static struct cmt_fmt_map {
+	const char *name;
+	enum cmit_fmt format;
+	const char *user_format;
+	int is_tformat;
+	int is_alias;
+} *commit_formats = NULL;
+static size_t commit_formats_len = 0;
+static size_t commit_formats_alloc = 0;
+static struct cmt_fmt_map *find_commit_format(const char *sought);
 
 static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)
 {

@@ -21,22 +31,134 @@ static void save_user_format(struct rev_info *rev, const char *cp, int is_tforma
 	rev->commit_format = CMIT_FMT_USERFORMAT;
 }
 
-void get_commit_format(const char *arg, struct rev_info *rev)
+static int git_pretty_formats_config(const char *var, const char *value, void *cb)
+{
+	if (!prefixcmp(var, "format.pretty.")) {
+		struct cmt_fmt_map user_format = {0};
+		const char *fmt;
+
+		user_format.name = xstrdup(&var[14]);
+		user_format.format = CMIT_FMT_USERFORMAT;
+		git_config_string(&fmt, var, value);
+		if (!prefixcmp(fmt, "format:") || !prefixcmp(fmt, "tformat:")) {
+			user_format.is_tformat = fmt[0] == 't';
+			fmt = strchr(fmt, ':') + 1;
+		} else if (strchr(fmt, '%'))
+			user_format.is_tformat = 1;
+		else
+			user_format.is_alias = 1;
+		user_format.user_format = fmt;
+
+		ALLOC_GROW(commit_formats, commit_formats_len+1,
+			   commit_formats_alloc);
+		memcpy(&commit_formats[ commit_formats_len ], &user_format,
+		       sizeof(user_format));
+		commit_formats_len++;
+	}
+	return 0;
+}
+
+static void setup_commit_formats(void)
 {
 	int i;
-	static struct cmt_fmt_map {
-		const char *n;
-		size_t cmp_len;
-		enum cmit_fmt v;
-	} cmt_fmts[] = {
-		{ "raw",	1,	CMIT_FMT_RAW },
-		{ "medium",	1,	CMIT_FMT_MEDIUM },
-		{ "short",	1,	CMIT_FMT_SHORT },
-		{ "email",	1,	CMIT_FMT_EMAIL },
-		{ "full",	5,	CMIT_FMT_FULL },
-		{ "fuller",	5,	CMIT_FMT_FULLER },
-		{ "oneline",	1,	CMIT_FMT_ONELINE },
+	const char **attempted_aliases = NULL;
+	size_t attempted_aliases_alloc = 0;
+	size_t attempted_aliases_len;
+	struct cmt_fmt_map builtin_formats[] = {
+		{ "raw",	CMIT_FMT_RAW,		NULL,	0 },
+		{ "medium",	CMIT_FMT_MEDIUM,	NULL,	0 },
+		{ "short",	CMIT_FMT_SHORT,		NULL,	0 },
+		{ "email",	CMIT_FMT_EMAIL,		NULL,	0 },
+		{ "full",	CMIT_FMT_FULL,		NULL,	0 },
+		{ "fuller",	CMIT_FMT_FULLER,	NULL,	0 },
+		{ "oneline",	CMIT_FMT_ONELINE,	NULL,	1 }
 	};
+	commit_formats_len = ARRAY_SIZE(builtin_formats);
+	ALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);
+	memcpy(commit_formats, builtin_formats,
+	       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));
+
+	git_config(git_pretty_formats_config, NULL);
+
+	for (i = ARRAY_SIZE(builtin_formats); i < commit_formats_len; i++) {
+		attempted_aliases_len = 0;
+		struct cmt_fmt_map *aliased_format = &commit_formats[i];
+		const char *fmt = commit_formats[i].user_format;
+		int j;
+
+		if (!commit_formats[i].is_alias)
+			continue;
+
+		while ((aliased_format = find_commit_format(fmt))) {
+			if (!aliased_format->is_alias)
+				break;
+
+			fmt = aliased_format->user_format;
+			for (j=0; j<attempted_aliases_len; j++) {
+				if (!strcmp(fmt, attempted_aliases[j])) {
+					aliased_format = NULL;
+					break;
+				}
+			}
+			if (!aliased_format)
+				break;
+
+			ALLOC_GROW(attempted_aliases, attempted_aliases_len+1,
+				   attempted_aliases_alloc);
+			attempted_aliases[attempted_aliases_len] = fmt;
+			attempted_aliases_len++;
+		}
+		if (aliased_format) {
+			commit_formats[i].format = aliased_format->format;
+			commit_formats[i].user_format = aliased_format->user_format;
+			commit_formats[i].is_tformat = aliased_format->is_tformat;
+			commit_formats[i].is_alias = 0;
+		} else
+			commit_formats[i].format = CMIT_FMT_UNSPECIFIED;
+	}
+}
+
+static struct cmt_fmt_map *find_commit_format(const char *sought)
+{
+	struct cmt_fmt_map *found = NULL;
+	size_t found_match_len = 0;
+
+	if (!commit_formats)
+		setup_commit_formats();
+
+	int i;
+	for (i = 0; i < commit_formats_len; i++) {
+		const char *candidate = commit_formats[i].name;
+		const char *s = sought;
+		size_t match_len = 0;
+
+		if (commit_formats[i].format == CMIT_FMT_UNSPECIFIED)
+			continue;
+
+		for (; s++, candidate++;) {
+			if (!*candidate && *s) {
+				match_len = 0;
+				break;
+			}
+			if (!*candidate || !*s) {
+				match_len = s - sought;
+				break;
+			}
+			if (*s != *candidate) {
+				match_len = 0;
+				break;
+			}
+		}
+		if (match_len > found_match_len) {
+			found = &commit_formats[i];
+		}
+	}
+	return found;
+}
+
+void get_commit_format(const char *arg, struct rev_info *rev)
+{
+	struct cmt_fmt_map *commit_format;
 
 	rev->use_terminator = 0;
 	if (!arg || !*arg) {

@@ -47,21 +169,22 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 		save_user_format(rev, strchr(arg, ':') + 1, arg[0] == 't');
 		return;
 	}
-	for (i = 0; i < ARRAY_SIZE(cmt_fmts); i++) {
-		if (!strncmp(arg, cmt_fmts[i].n, cmt_fmts[i].cmp_len) &&
-		    !strncmp(arg, cmt_fmts[i].n, strlen(arg))) {
-			if (cmt_fmts[i].v == CMIT_FMT_ONELINE)
-				rev->use_terminator = 1;
-			rev->commit_format = cmt_fmts[i].v;
-			return;
-		}
-	}
+
 	if (strchr(arg, '%')) {
 		save_user_format(rev, arg, 1);
 		return;
 	}
 
-	die("invalid --pretty format: %s", arg);
+	commit_format = find_commit_format(arg);
+	if( !commit_format )
+		die("invalid --pretty format: %s", arg);
+
+	rev->commit_format = commit_format->format;
+	rev->use_terminator = commit_format->is_tformat;
+	if( commit_format->format == CMIT_FMT_USERFORMAT ){
+		save_user_format(rev, commit_format->user_format,
+				 commit_format->is_tformat);
+	}
 }
 
 /*

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 28d2948..ee3e934 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh

@@ -6,7 +6,15 @@
 test_description='Test pretty formats'
 . ./test-lib.sh
 
-test_expect_success "set up basic repos" ">foo && git add foo && git commit -m initial"
+test_expect_success "set up basic repos" \
+	">foo &&
+	>bar &&
+	git add foo &&
+	test_tick &&
+	git commit -m initial &&
+	git add bar &&
+	test_tick &&
+	git commit -m 'add bar'"
 
 for flag in false true always; do
 for color in red green blue reset; do

@@ -16,29 +24,64 @@ for color in red green blue reset; do
 		"$make_expected &&
 		git config color.ui $flag &&
 		git log -1 --pretty=format:'%C$color' > actual &&
-		cmp expected actual"
+		test_cmp expected actual"
 
 
 	test_expect_success "%C($color) with color.ui $flag" \
 		"$make_expected &&
 		git config color.ui $flag &&
 		git log -1 --pretty=format:'%C($color)' > actual &&
-		cmp expected actual"
+		test_cmp expected actual"
 
 	[ ! "$flag" = "always" ] && make_expected=">expected"
 	test_expect_success "%C?$color with color.ui $flag" \
 		"$make_expected &&
 		git config color.ui $flag &&
 		git log -1 --pretty=format:'%C?$color' > actual &&
-		cmp expected actual"
+		test_cmp expected actual"
 
 	test_expect_success "%C?($color) with color.ui $flag" \
 		"$make_expected &&
 		git config color.ui $flag &&
 		git log -1 --pretty=format:'%C?($color)' > actual &&
-		cmp expected actual"
+		test_cmp expected actual"
 
 done
 done
+test_expect_success "reset color flags" "git config --unset color.ui"
+
+test_expect_success "alias builtin format" \
+	"git log --pretty=oneline >expected &&
+	git config format.pretty.test-alias oneline &&
+	git log --pretty=test-alias >actual &&
+	test_cmp expected actual"
+
+test_expect_success "alias user-defined format" \
+	"git log --pretty='format:%h' >expected &&
+	git config format.pretty.test-alias 'format:%h' &&
+	git log --pretty=test-alias >actual &&
+	test_cmp expected actual"
+
+test_expect_success "alias user-defined tformat" \
+	"git log --pretty='tformat:%h' >expected &&
+	git config format.pretty.test-alias 'tformat:%h' &&
+	git log --pretty=test-alias >actual &&
+	test_cmp expected actual"
+
+test_expect_code 128 "alias non-existant format" \
+	"git config format.pretty.test-alias format-that-will-never-exist &&
+	git log --pretty=test-alias"
+
+test_expect_success "alias of an alias" \
+	"git log --pretty='tformat:%h' >expected &&
+	git config format.pretty.test-foo 'tformat:%h' &&
+	git config format.pretty.test-bar test-foo &&
+	git log --pretty=test-bar >actual &&
+	test_cmp expected actual"
+
+test_expect_code 128 "alias loop" \
+	"git config format.pretty.test-foo test-bar &&
+	git config format.pretty.test-bar test-foo &&
+	git log --pretty=test-foo"
 
 test_done

-- 
1.7.1.rc1.13.gbb0a0a.dirty

Hi Will,

Will Palmer wrote:

Here we add the %C?colorname placeholders which act just as the
%Ccolorname placeholders, with the exception that the pretty_context is
checked to see if color should be used according to configuration

Thanks for tackling this.

I have thought a little about a related problem: some commands have
configuration for the colors they use, like:

    color.grep.<slot>
        Use customized color for grep colorization.  <slot> specifies which
        part of the line to use the specified color, and is one of

        context
            non-matching text in context lines (when using -A, -B, or -C)

        filename
            filename prefix (when not using -h)

This is nice because in certain situations (e.g. different background
colors), the default colors might not be suitable.  As an example of
this, the ‘commit ’ line of ‘git log’ output uses color.diff.commit.

So it would be nice to be able to use %C(diff.commit) and
automatically use the right color, if color is enabled.

Why not make %C always check?  I can understand that it would be
annoying when first trying to use %C.  On the other hand, it would be
more convenient for writing format.pretty configuration that should be
shared with old git, and I assume anyone using %C for the first time
would be looking at the manual, which could warn her.

Cheers,
Jonathan

Will Palmer wrote:

Here we make "git log --pretty=%H --abbrev-commit" synonymous with
"git log --pretty=%h", and make %h/abbreviated-%H respect the length
specified for --abbrev.

The same is applied to other commit-placeholders %P and %p, and
--abbrev is respected for %t, though %T is not changed.

Signed-off-by: Will Palmer <redacted>
---
 builtin/rev-list.c |    1 +
 builtin/shortlog.c |    2 ++
 commit.h           |    1 +
 log-tree.c         |    2 ++
 pretty.c           |   30 +++++++++++++++++++-----------
 5 files changed, 25 insertions(+), 11 deletions(-)

I agree that this is the right to do, since this is how the built-in
formats work (the ‘commit ...’ line follows the semantics of your %H,
and ‘Merge: ...’ line your %p, for example).

Documentation and tests?

Makes sense.

Shortlog doesn’t print commit hashes, does it?

nitpick: I’d stick this up with abbrev and maybe add a comment to
explain their distinct uses.

There is a

ctx.abbrev = opt->diffopt.abbrev;

later in the same function; how do these interact?

[...]

 			strbuf_addstr(sb, find_unique_abbrev(
-					p->item->object.sha1, DEFAULT_ABBREV));
+				      p->item->object.sha1,
+				      c->pretty_ctx->abbrev));

nitpick: the new indentation makes these look like parameters to
strbuf_addstr.

Here’s an alternative implementation of the more controversial half of
your patch, for your amusement.  The big downside is that it requires
one to specify --abbrev-commit before the --format option.

Thanks for the pleasant read.

Jonathan

diff --git a/pretty.c b/pretty.c
index 7cb3a2a..1008a41 100644
--- a/pretty.c
+++ b/pretty.c

@@ -12,10 +12,31 @@
 
 static char *user_format;
 
+static void abbreviate_commit_hashes(char *fmt)
+{
+	char *p;
+	for (p = fmt; p != NULL; p = strchr(p + 1, '%')) {
+		p++;
+		switch (*p) {
+		case 'H':
+			*p = 'h';
+			break;
+		case 'P':
+			*p = 'p';
+			break;
+		case 'T':
+		default:
+			break;
+		}
+	}
+}
+
 static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)
 {
 	free(user_format);
 	user_format = xstrdup(cp);
+	if (rev->abbrev_commit)
+		abbreviate_commit_hashes(user_format);
 	if (is_tformat)
 		rev->use_terminator = 1;
 	rev->commit_format = CMIT_FMT_USERFORMAT;

On Sun, Apr 25, 2010 at 09:13:47PM -0500, Jonathan Nieder wrote:

This is nice because in certain situations (e.g. different background
colors), the default colors might not be suitable.  As an example of
this, the ‘commit ’ line of ‘git log’ output uses color.diff.commit.

So it would be nice to be able to use %C(diff.commit) and
automatically use the right color, if color is enabled.

FWIW, I like this idea very much. And it would be a bit more natural for
%C(diff.commit) to respect diff.color, whereas we could perhaps keep
%Cred as "always on" for backwards compatibility.

However:

Why not make %C always check?  I can understand that it would be
annoying when first trying to use %C.  On the other hand, it would be

I am a little nervous that we would be breaking scripts that pipe the
colorized output for later display to the user. Right now doing
something like[1]:

  log=`git log --format=%Cred%h`
  test "x$log" != x && echo "foo: $log"

colorizes, but we would be breaking it. And for new values like
%C(diff.commit), we are not breaking anything (because it is a new
syntax), but a script like the one above may want to convert, and there
is no way to say "respect the color _config_, but don't respect
isatty(1)". Saying "--color" doesn't work, because it overrides the
color config. We can cheat a little with GIT_PAGER_IN_USE, but that will
have funny interactions with pager.color.

[1] Yes, I know this snippet is contrived, but it seems within the realm
of possibility. git-add--interactive reads the diff output in both
regular and color forms, though it takes some care to look at the user's
config and decide whether to show the color.

-Peff

On Sun, Apr 25, 2010 at 10:11:37PM -0500, Jonathan Nieder wrote:

Here’s an alternative implementation of the more controversial half of
your patch, for your amusement.  The big downside is that it requires
one to specify --abbrev-commit before the --format option.

That is not insurmountable, as we could just check after the parsing
stage. But there is a worse problem:

+static void abbreviate_commit_hashes(char *fmt)
+{
+	char *p;
+	for (p = fmt; p != NULL; p = strchr(p + 1, '%')) {
+		p++;
+		switch (*p) {
+		case 'H':
+			*p = 'h';
+			break;
+		case 'P':
+			*p = 'p';
+			break;
+		case 'T':
+		default:
+			break;
+		}
+	}
+}

You parse '%%H' incorrectly. I would really rather not see ad-hoc
parsers for the format like this, but rather use or extend strbuf_expand
as appropriate. That would make things less painful if and when we
decide to tweak the syntax.

I think a lot of this might be more pleasant if we had some extensible
and backwards compatible syntax like %(placeholder, arg, ...) and could
do "%(H, abbrev=always)" or "%(H, abbrev=config)". Yeah, it's long, but
it is readable and explicit.

-Peff

Jeff King wrote:

On Sun, Apr 25, 2010 at 10:11:37PM -0500, Jonathan Nieder wrote:

quoted

Here’s an alternative implementation of the more controversial half of
your patch, for your amusement.  The big downside is that it requires
one to specify --abbrev-commit before the --format option.

That is not insurmountable, as we could just check after the parsing
stage. But there is a worse problem:

quoted

+static void abbreviate_commit_hashes(char *fmt)
+{
+	char *p;
+	for (p = fmt; p != NULL; p = strchr(p + 1, '%')) {
+		p++;
+		switch (*p) {
+		case 'H':
+			*p = 'h';
+			break;
+		case 'P':
+			*p = 'p';
+			break;
+		case 'T':
+		default:
+			break;
+		}
+	}
+}

You parse '%%H' incorrectly.

I’m pretty sure I don’t.

I would really rather not see ad-hoc
parsers for the format like this, but rather use or extend strbuf_expand
as appropriate. That would make things less painful if and when we
decide to tweak the syntax.

However, this point still applies.

Jonathan

Jonathan Nieder wrote:

Jeff King wrote:

quoted

On Sun, Apr 25, 2010 at 10:11:37PM -0500, Jonathan Nieder wrote:

quoted

quoted

+static void abbreviate_commit_hashes(char *fmt)
+{
+	char *p;
+	for (p = fmt; p != NULL; p = strchr(p + 1, '%')) {
+		p++;
+		switch (*p) {
+		case 'H':
+			*p = 'h';
+			break;
+		case 'P':
+			*p = 'p';
+			break;
+		case 'T':
+		default:
+			break;
+		}
+	}
+}

You parse '%%H' incorrectly.

I’m pretty sure I don’t.

Aggh, I see it now.  The first line should be

 for (p = strchr(fmt, '%'); ...

as I would have noticed with even a little testing.

Sorry for the nonsense.
Jonathan

On Sun, Apr 25, 2010 at 10:38:13PM -0500, Jonathan Nieder wrote:

quoted

You parse '%%H' incorrectly.

I’m pretty sure I don’t.

Sorry, you're right. I should read more carefully.

-Peff

On Sun, Apr 25, 2010 at 10:41:35PM -0500, Jonathan Nieder wrote:

Jonathan Nieder wrote:

quoted

Jeff King wrote:

quoted

On Sun, Apr 25, 2010 at 10:11:37PM -0500, Jonathan Nieder wrote:

quoted

quoted

quoted

+static void abbreviate_commit_hashes(char *fmt)
+{
+	char *p;
+	for (p = fmt; p != NULL; p = strchr(p + 1, '%')) {
+		p++;
+		switch (*p) {
+		case 'H':
+			*p = 'h';
+			break;
+		case 'P':
+			*p = 'p';
+			break;
+		case 'T':
+		default:
+			break;
+		}
+	}
+}

You parse '%%H' incorrectly.

I’m pretty sure I don’t.

Aggh, I see it now.  The first line should be

 for (p = strchr(fmt, '%'); ...

as I would have noticed with even a little testing.

Actually, we are both failing. You _do_ parse %%H right, but you don't
parse "WHO" correctly. So yours is a different bug than what I thought.
:)

-Peff

Jeff King wrote:

I am a little nervous that we would be breaking scripts that pipe the
colorized output for later display to the user. Right now doing
something like[1]:

  log=`git log --format=%Cred%h`
  test "x$log" != x && echo "foo: $log"

colorizes, but we would be breaking it. And for new values like
%C(diff.commit), we are not breaking anything (because it is a new
syntax), but a script like the one above may want to convert, and there
is no way to say "respect the color _config_, but don't respect
isatty(1)". Saying "--color" doesn't work, because it overrides the
color config. We can cheat a little with GIT_PAGER_IN_USE, but that will
have funny interactions with pager.color.

Very interesting.  I think the natural solution (for new colors) would
be a --color=config option, which would require parsing options before
checking configuration.

Does this sound sane?

Jonathan

Will Palmer wrote:

format.pretty.<name>::
       Alias for a --pretty= format string, as specified in
       linkgit:git-log[1]. Any aliases defined here can be used just
       as the builtin pretty formats could. For example, defining
       "format.pretty.hash = format:%H" would cause the invocation
       "git log --pretty=hash" to be equivalent to running
       "git log --pretty=format:%H".

Ah, so I could use 

	[format "pretty"]
		wrapped = "format:\
	%C(yellow)commit %H%n\
	Merge: %p%n\
	Author:	%aN <%aE>%n\
	Date:	%ad%n%n%w(80,4,4)%s%n\
	%+b"

and then by default I get the standard medium, but with --format=wrapped,
I get my imitation of it.  Sounds very useful, thanks.

Simpler to use

	if (prefixcmp(var, "format.pretty."))
		return 0;

to avoid keeping the reader in suspense.

+		struct cmt_fmt_map user_format = {0};
+		const char *fmt;
+
+		user_format.name = xstrdup(&var[14]);
+		user_format.format = CMIT_FMT_USERFORMAT;
+		git_config_string(&fmt, var, value);

git_config_string() does a strdup(), but we were about to either
discard the value or do that ourselves anyway...

+		if (!prefixcmp(fmt, "format:") || !prefixcmp(fmt, "tformat:")) {
+			user_format.is_tformat = fmt[0] == 't';
+			fmt = strchr(fmt, ':') + 1;
+		} else if (strchr(fmt, '%'))
+			user_format.is_tformat = 1;
+		else
+			user_format.is_alias = 1;
+		user_format.user_format = fmt;

... or rather we would be, if this reused code from get_commit_format/
save_user_format.

+
+		ALLOC_GROW(commit_formats, commit_formats_len+1,
+			   commit_formats_alloc);
+		memcpy(&commit_formats[ commit_formats_len ], &user_format,
+		       sizeof(user_format));
+		commit_formats_len++;

Why not build it in place?  Not for performance reasons (that could go
either way); it is just that that would seem simpler to me.

+	}
+	return 0;
+}

Regarding the next piece: I suspect the review would be easier if
it had been more than one patch.  Maybe three patches:

 1 restructure get_commit_format to read from a (dynamic) list of
   supported formats that is not its responsibility

 2 infrastructure for format aliases (this is not needed for
   --format=datelist where datelist = "tformat:%h %cd")

 3 new configuration for user-defined formats and format aliases

Maybe 3 could come before 2, since it seems like complicated.

The new call graph looks like this:

 setup_revisions() ->
   handle_revision_opt() ->
   get_commit_format() ->
   find_commit_format() ->
   setup_commit_formats() ->
   git_config() ->
   git_pretty_formats_config()

This means we have to have searched for a repository before parsing
these arguments; this constraint already exists for parsing the actual
revision arguments (maybe some day we will defer handling those
arguments for some reason).

I would have put the setup_commit_formats() call in setup_revisions()
to make this more obvious, but I suppose this way you save time if no
--format option is used.

+
+static void setup_commit_formats(void)
 {
 	int i;
-	static struct cmt_fmt_map {
-		const char *n;
-		size_t cmp_len;
-		enum cmit_fmt v;
-	} cmt_fmts[] = {
-		{ "raw",	1,	CMIT_FMT_RAW },
-		{ "medium",	1,	CMIT_FMT_MEDIUM },
-		{ "short",	1,	CMIT_FMT_SHORT },
-		{ "email",	1,	CMIT_FMT_EMAIL },
-		{ "full",	5,	CMIT_FMT_FULL },
-		{ "fuller",	5,	CMIT_FMT_FULLER },
-		{ "oneline",	1,	CMIT_FMT_ONELINE },
+	const char **attempted_aliases = NULL;
+	size_t attempted_aliases_alloc = 0;
+	size_t attempted_aliases_len;
+	struct cmt_fmt_map builtin_formats[] = {
+		{ "raw",	CMIT_FMT_RAW,		NULL,	0 },
+		{ "medium",	CMIT_FMT_MEDIUM,	NULL,	0 },
+		{ "short",	CMIT_FMT_SHORT,		NULL,	0 },
+		{ "email",	CMIT_FMT_EMAIL,		NULL,	0 },
+		{ "full",	CMIT_FMT_FULL,		NULL,	0 },
+		{ "fuller",	CMIT_FMT_FULLER,	NULL,	0 },
+		{ "oneline",	CMIT_FMT_ONELINE,	NULL,	1 }

nitpick: Might be less noisy if the null format string field were the
last field.

 	};
+	commit_formats_len = ARRAY_SIZE(builtin_formats);
+	ALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);
+	memcpy(commit_formats, builtin_formats,
+	       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));
+
+	git_config(git_pretty_formats_config, NULL);

I suspect the body of the next loop should be its own function to keep
the reader’s attention.

+
+	for (i = ARRAY_SIZE(builtin_formats); i < commit_formats_len; i++) {
+		attempted_aliases_len = 0;
+		struct cmt_fmt_map *aliased_format = &commit_formats[i];
+		const char *fmt = commit_formats[i].user_format;
+		int j;

declaration after statement

+
+		if (!commit_formats[i].is_alias)
+			continue;
+
+		while ((aliased_format = find_commit_format(fmt))) {

[...]

Is this the right time to do this check?  Maybe we could check lazily
when the format is used.

+			if (!aliased_format->is_alias)
+				break;
+
+			fmt = aliased_format->user_format;
+			for (j=0; j<attempted_aliases_len; j++) {
+				if (!strcmp(fmt, attempted_aliases[j])) {
+					aliased_format = NULL;
+					break;
+				}
+			}

Example:

	[format "pretty"]
		foo = nonsense
		one = foo
		two = foo

one is treated as an alias, but two is not.  Why?

+			if (!aliased_format)
+				break;

Is to escape from the wider loop?  I think this is a valid use for goto.

+
+			ALLOC_GROW(attempted_aliases, attempted_aliases_len+1,
+				   attempted_aliases_alloc);
+			attempted_aliases[attempted_aliases_len] = fmt;
+			attempted_aliases_len++;
+		}

Example:

	[format "pretty"]
		foo = medium
		xyzzy = one
		one = foo
		two = foo
		frotz = two

At the end of this loop, attempted_aliases contains:

	one
	foo
	two

Every alias which is itself referred to by an alias is listed.

+		if (aliased_format) {
+			commit_formats[i].format = aliased_format->format;
+			commit_formats[i].user_format = aliased_format->user_format;
+			commit_formats[i].is_tformat = aliased_format->is_tformat;
+			commit_formats[i].is_alias = 0;
+		} else
+			commit_formats[i].format = CMIT_FMT_UNSPECIFIED;
+	}
+}

Why go to the trouble to build attempted_aliases when it is never used?

I suspect I’ve completely misunderstood, so I’m stopping here.  Maybe
someone else can clear it up or take over.

Jonathan

I agree that this is the right to do, since this is how the built-in
formats work (the ‘commit ...’ line follows the semantics of your %H,
and ‘Merge: ...’ line your %p, for example).

Documentation and tests?

Tests, noted. I'll include some in the next version of the patch.
As for documentation, I'm not entirely sure what to add, as it seemed
like the change
merely implements what I would expect when reading the docs for
git-log. Still, noted.
I'll stick a short note in each of %H, %h, %P, %p, and %t

Shortlog doesn’t print commit hashes, does it?

Shortlog accepts --format, though this doesn't seem to be documented
(if I type "man" and search
for "format"), so perhaps it should be.

quoted

diff --git a/commit.h b/commit.h
index b6caf91..7a476a0 100644
--- a/commit.h
+++ b/commit.h

@@ -72,6 +72,7 @@ struct pretty_print_context

      int need_8bit_cte;
      int show_notes;
      int use_color;
+     int abbrev_commit;
      struct reflog_walk_info *reflog_info;
 };

nitpick: I’d stick this up with abbrev and maybe add a comment to
explain their distinct uses.

Noted. Thanks

quoted

diff --git a/log-tree.c b/log-tree.c
index 6bb4748..0a2309c 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -282,6 +282,8 @@ void show_log(struct rev_info *opt)

      int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
      const char *extra_headers = opt->extra_headers;
      struct pretty_print_context ctx = {0};
+     ctx.abbrev = opt->abbrev;
+     ctx.abbrev_commit = opt->abbrev_commit;
      ctx.use_color = DIFF_OPT_TST(&opt->diffopt, COLOR_DIFF);

      opt->loginfo = NULL;

There is a

ctx.abbrev = opt->diffopt.abbrev;

later in the same function; how do these interact?

I hadn't caught that. My guess: stupidly doing the same thing twice.
I'll double-check,
and take out one of them if that's the case.

What all this
has shown me is that there are really too many ways to specify "context when
printing information about a commit". I don't like it, and the lot of
them can probably
be re-factored, perhaps by getting rid of pretty_context and passing
rev_info around
everywhere. I don't know the full implications of that and it seems
outside the scope
of this change.

quoted

@@ -741,24 +744,29 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,

[...]

quoted

                      strbuf_addstr(sb, find_unique_abbrev(
-                                     p->item->object.sha1, DEFAULT_ABBREV));
+                                   p->item->object.sha1,
+                                   c->pretty_ctx->abbrev));

nitpick: the new indentation makes these look like parameters to
strbuf_addstr.

Noted. I'll restore the extra indent, which I had assumed was a typo.

I had been thinking that this wouldn't be safe, but then that was my
being overly-cautious:
it's just been xstrdup()ed, so what we're parsing is ours, no real
reason not to do it.
I think the "must be specified after --abbrev-commit" is a rather
large nail, though. If you work
out the bugs mentioned by Jeff King, and it works, I'll stick it in
there, as I don't like
falling through case statements any more than the next guy. (well,
maybe a little more than
the next guy).

Thanks for the review!
-- Will Palmer

I don't have time right now to reply line-by-line to this, so this is
just an ACK that it was
received, and I'll reply later today.


On Mon, Apr 26, 2010 at 8:25 AM, Jonathan Nieder [off-list ref] wrote:

Will Palmer wrote:

quoted

format.pretty.<name>::
       Alias for a --pretty= format string, as specified in
       linkgit:git-log[1]. Any aliases defined here can be used just
       as the builtin pretty formats could. For example, defining
       "format.pretty.hash = format:%H" would cause the invocation
       "git log --pretty=hash" to be equivalent to running
       "git log --pretty=format:%H".

Ah, so I could use

       [format "pretty"]
               wrapped = "format:\
       %C(yellow)commit %H%n\
       Merge: %p%n\
       Author: %aN <%aE>%n\
       Date:   %ad%n%n%w(80,4,4)%s%n\
       %+b"

and then by default I get the standard medium, but with --format=wrapped,
I get my imitation of it.  Sounds very useful, thanks.

quoted

diff --git a/pretty.c b/pretty.c
index f884f48..d49fec7 100644
--- a/pretty.c
+++ b/pretty.c

@@ -11,6 +11,16 @@

 #include "reflog-walk.h"

 static char *user_format;
+static struct cmt_fmt_map {
+     const char *name;
+     enum cmit_fmt format;
+     const char *user_format;
+     int is_tformat;
+     int is_alias;
+} *commit_formats = NULL;
+static size_t commit_formats_len = 0;
+static size_t commit_formats_alloc = 0;
+static struct cmt_fmt_map *find_commit_format(const char *sought);

 static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)
 {

@@ -21,22 +31,134 @@ static void save_user_format(struct rev_info *rev, const char *cp, int is_tforma

      rev->commit_format = CMIT_FMT_USERFORMAT;
 }

-void get_commit_format(const char *arg, struct rev_info *rev)
+static int git_pretty_formats_config(const char *var, const char *value, void *cb)
+{
+     if (!prefixcmp(var, "format.pretty.")) {

Simpler to use

       if (prefixcmp(var, "format.pretty."))
               return 0;

to avoid keeping the reader in suspense.

quoted

+             struct cmt_fmt_map user_format = {0};
+             const char *fmt;
+
+             user_format.name = xstrdup(&var[14]);
+             user_format.format = CMIT_FMT_USERFORMAT;
+             git_config_string(&fmt, var, value);

git_config_string() does a strdup(), but we were about to either
discard the value or do that ourselves anyway...

quoted

+             if (!prefixcmp(fmt, "format:") || !prefixcmp(fmt, "tformat:")) {
+                     user_format.is_tformat = fmt[0] == 't';
+                     fmt = strchr(fmt, ':') + 1;
+             } else if (strchr(fmt, '%'))
+                     user_format.is_tformat = 1;
+             else
+                     user_format.is_alias = 1;
+             user_format.user_format = fmt;

... or rather we would be, if this reused code from get_commit_format/
save_user_format.

quoted

+
+             ALLOC_GROW(commit_formats, commit_formats_len+1,
+                        commit_formats_alloc);
+             memcpy(&commit_formats[ commit_formats_len ], &user_format,
+                    sizeof(user_format));
+             commit_formats_len++;

Why not build it in place?  Not for performance reasons (that could go
either way); it is just that that would seem simpler to me.

quoted

+     }
+     return 0;
+}

Regarding the next piece: I suspect the review would be easier if
it had been more than one patch.  Maybe three patches:

 1 restructure get_commit_format to read from a (dynamic) list of
  supported formats that is not its responsibility

 2 infrastructure for format aliases (this is not needed for
  --format=datelist where datelist = "tformat:%h %cd")

 3 new configuration for user-defined formats and format aliases

Maybe 3 could come before 2, since it seems like complicated.

The new call graph looks like this:

 setup_revisions() ->
  handle_revision_opt() ->
  get_commit_format() ->
  find_commit_format() ->
  setup_commit_formats() ->
  git_config() ->
  git_pretty_formats_config()

This means we have to have searched for a repository before parsing
these arguments; this constraint already exists for parsing the actual
revision arguments (maybe some day we will defer handling those
arguments for some reason).

I would have put the setup_commit_formats() call in setup_revisions()
to make this more obvious, but I suppose this way you save time if no
--format option is used.

quoted

+
+static void setup_commit_formats(void)
 {
      int i;
-     static struct cmt_fmt_map {
-             const char *n;
-             size_t cmp_len;
-             enum cmit_fmt v;
-     } cmt_fmts[] = {
-             { "raw",        1,      CMIT_FMT_RAW },
-             { "medium",     1,      CMIT_FMT_MEDIUM },
-             { "short",      1,      CMIT_FMT_SHORT },
-             { "email",      1,      CMIT_FMT_EMAIL },
-             { "full",       5,      CMIT_FMT_FULL },
-             { "fuller",     5,      CMIT_FMT_FULLER },
-             { "oneline",    1,      CMIT_FMT_ONELINE },
+     const char **attempted_aliases = NULL;
+     size_t attempted_aliases_alloc = 0;
+     size_t attempted_aliases_len;
+     struct cmt_fmt_map builtin_formats[] = {
+             { "raw",        CMIT_FMT_RAW,           NULL,   0 },
+             { "medium",     CMIT_FMT_MEDIUM,        NULL,   0 },
+             { "short",      CMIT_FMT_SHORT,         NULL,   0 },
+             { "email",      CMIT_FMT_EMAIL,         NULL,   0 },
+             { "full",       CMIT_FMT_FULL,          NULL,   0 },
+             { "fuller",     CMIT_FMT_FULLER,        NULL,   0 },
+             { "oneline",    CMIT_FMT_ONELINE,       NULL,   1 }

nitpick: Might be less noisy if the null format string field were the
last field.

quoted

      };
+     commit_formats_len = ARRAY_SIZE(builtin_formats);
+     ALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);
+     memcpy(commit_formats, builtin_formats,
+            sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));
+
+     git_config(git_pretty_formats_config, NULL);

I suspect the body of the next loop should be its own function to keep
the reader’s attention.

quoted

+
+     for (i = ARRAY_SIZE(builtin_formats); i < commit_formats_len; i++) {
+             attempted_aliases_len = 0;
+             struct cmt_fmt_map *aliased_format = &commit_formats[i];
+             const char *fmt = commit_formats[i].user_format;
+             int j;

declaration after statement

quoted

+
+             if (!commit_formats[i].is_alias)
+                     continue;
+
+             while ((aliased_format = find_commit_format(fmt))) {

[...]

Is this the right time to do this check?  Maybe we could check lazily
when the format is used.

quoted

+                     if (!aliased_format->is_alias)
+                             break;
+
+                     fmt = aliased_format->user_format;
+                     for (j=0; j<attempted_aliases_len; j++) {
+                             if (!strcmp(fmt, attempted_aliases[j])) {
+                                     aliased_format = NULL;
+                                     break;
+                             }
+                     }

Example:

       [format "pretty"]
               foo = nonsense
               one = foo
               two = foo

one is treated as an alias, but two is not.  Why?

quoted

+                     if (!aliased_format)
+                             break;

Is to escape from the wider loop?  I think this is a valid use for goto.

quoted

+
+                     ALLOC_GROW(attempted_aliases, attempted_aliases_len+1,
+                                attempted_aliases_alloc);
+                     attempted_aliases[attempted_aliases_len] = fmt;
+                     attempted_aliases_len++;
+             }

Example:

       [format "pretty"]
               foo = medium
               xyzzy = one
               one = foo
               two = foo
               frotz = two

At the end of this loop, attempted_aliases contains:

       one
       foo
       two

Every alias which is itself referred to by an alias is listed.

quoted

+             if (aliased_format) {
+                     commit_formats[i].format = aliased_format->format;
+                     commit_formats[i].user_format = aliased_format->user_format;
+                     commit_formats[i].is_tformat = aliased_format->is_tformat;
+                     commit_formats[i].is_alias = 0;
+             } else
+                     commit_formats[i].format = CMIT_FMT_UNSPECIFIED;
+     }
+}

Why go to the trouble to build attempted_aliases when it is never used?

I suspect I’ve completely misunderstood, so I’m stopping here.  Maybe
someone else can clear it up or take over.

Jonathan

Will Palmer wrote:

Jonathan Nieder wrote:

quoted

Shortlog doesn’t print commit hashes, does it?

Shortlog accepts --format, though this doesn't seem to be documented
(if I type "man" and search
for "format"), so perhaps it should be.

Oh, neat!  Maybe this would save you the trouble.

Jonathan Nieder (3):
  t4201 (shortlog): guard setup with test_expect_success
  t4201 (shortlog): Test output format with multiple authors
  shortlog: Document and test --format option

Will Palmer (1):
  pretty: Respect --abbrev option

 Documentation/git-shortlog.txt |    8 +++
 builtin/shortlog.c             |    3 +-
 pretty.c                       |    7 ++-
 shortlog.h                     |    1 +
 t/t4201-shortlog.sh            |  116 +++++++++++++++++++++++++++++++--------
 t/t6006-rev-list-format.sh     |   19 +++++++
 6 files changed, 126 insertions(+), 28 deletions(-)

Follow the current prevailing style.  This also has the benefit of
capturing any stray output and noticing if any of the setup commands
start failing.

Signed-off-by: Jonathan Nieder <redacted>
---
 t/t4201-shortlog.sh |   68 +++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index a01e55b..438a826 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh

@@ -8,30 +8,38 @@ test_description='git shortlog
 
 . ./test-lib.sh
 
-echo 1 > a1
-git add a1
-tree=$(git write-tree)
-commit=$( (echo "Test"; echo) | git commit-tree $tree )
-git update-ref HEAD $commit
-
-echo 2 > a1
-git commit --quiet -m "This is a very, very long first line for the commit message to see if it is wrapped correctly" a1
-
-# test if the wrapping is still valid when replacing all i's by treble clefs.
-echo 3 > a1
-git commit --quiet -m "$(echo "This is a very, very long first line for the commit message to see if it is wrapped correctly" | sed "s/i/1234/g" | tr 1234 '\360\235\204\236')" a1
-
-# now fsck up the utf8
-git config i18n.commitencoding non-utf-8
-echo 4 > a1
-git commit --quiet -m "$(echo "This is a very, very long first line for the commit message to see if it is wrapped correctly" | sed "s/i/1234/g" | tr 1234 '\370\235\204\236')" a1
-
-echo 5 > a1
-git commit --quiet -m "a								12	34	56	78" a1
-
-git shortlog -w HEAD > out
+test_expect_success 'setup' '
+	echo 1 >a1 &&
+	git add a1 &&
+	tree=$(git write-tree) &&
+	commit=$(printf "%s\n" "Test" "" | git commit-tree "$tree") &&
+	git update-ref HEAD "$commit" &&
+
+	echo 2 >a1 &&
+	git commit --quiet -m "This is a very, very long first line for the commit message to see if it is wrapped correctly" a1 &&
+
+	# test if the wrapping is still valid
+	# when replacing all is by treble clefs.
+	echo 3 >a1 &&
+	git commit --quiet -m "$(
+		echo "This is a very, very long first line for the commit message to see if it is wrapped correctly" |
+		sed "s/i/1234/g" |
+		tr 1234 "\360\235\204\236")" a1 &&
+
+	# now fsck up the utf8
+	git config i18n.commitencoding non-utf-8 &&
+	echo 4 >a1 &&
+	git commit --quiet -m "$(
+		echo "This is a very, very long first line for the commit message to see if it is wrapped correctly" |
+		sed "s/i/1234/g" |
+		tr 1234 "\370\235\204\236")" a1 &&
+
+	echo 5 >a1 &&
+	git commit --quiet -m "a								12	34	56	78" a1
+'
 
-cat > expect << EOF
+test_expect_success 'shortlog wrapping' '
+	cat >expect <<\EOF &&
 A U Thor (5):
       Test
       This is a very, very long first line for the commit message to see if

@@ -44,13 +52,15 @@ A U Thor (5):
          56	78
 
 EOF
+	git shortlog -w HEAD >out &&
+	test_cmp expect out
+'
 
-test_expect_success 'shortlog wrapping' 'test_cmp expect out'
-
-git log HEAD > log
-GIT_DIR=non-existing git shortlog -w < log > out
-
-test_expect_success 'shortlog from non-git directory' 'test_cmp expect out'
+test_expect_success 'shortlog from non-git directory' '
+	git log HEAD >log &&
+	GIT_DIR=non-existing git shortlog -w <log >out &&
+	test_cmp expect out
+'
 
 iconvfromutf8toiso88591() {
 	printf "%s" "$*" | iconv -f UTF-8 -t ISO8859-1

-- 
1.7.1.3.g5f1e.dirty

Signed-off-by: Jonathan Nieder <redacted>
---
This could be squashed with patch 1, if you like.

 t/t4201-shortlog.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 438a826..6bfd0c0 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh

@@ -36,6 +36,10 @@ test_expect_success 'setup' '
 
 	echo 5 >a1 &&
 	git commit --quiet -m "a								12	34	56	78" a1
+
+	echo 6 >a1 &&
+	git commit --quiet -m "Commit by someone else" \
+		--author="Someone else <not!me>" a1
 '
 
 test_expect_success 'shortlog wrapping' '

@@ -51,6 +55,9 @@ A U Thor (5):
       a								12	34
          56	78
 
+Someone else (1):
+      Commit by someone else
+
 EOF
 	git shortlog -w HEAD >out &&
 	test_cmp expect out

-- 
1.7.1.3.g5f1e.dirty

Do not document the --pretty synonym, since it takes too long to
explain the name to people.

Signed-off-by: Jonathan Nieder <redacted>
---
 Documentation/git-shortlog.txt |    8 ++++++
 t/t4201-shortlog.sh            |   53 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
index dfd4d0c..15e92ed 100644
--- a/Documentation/git-shortlog.txt
+++ b/Documentation/git-shortlog.txt

@@ -39,6 +39,14 @@ OPTIONS
 --email::
 	Show the email address of each author.
 
+--format[='<format>']::
+	Instead of the commit subject, use some other information to
+	describe each commit.  '<format>' can be any string accepted
+	by the `--format` option of 'git log', such as '* [%h] %s'.
+	(See the "PRETTY FORMATS" section of linkgit:git-log[1].)
+
+	Each pretty-printed commit will be rewrapped before it is shown.
+
 -w[<width>[,<indent1>[,<indent2>]]]::
 	Linewrap the output by wrapping each line at `width`.  The first
 	line of each entry is indented by `indent1` spaces, and the second

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 6bfd0c0..899ddbe 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh

@@ -39,7 +39,58 @@ test_expect_success 'setup' '
 
 	echo 6 >a1 &&
 	git commit --quiet -m "Commit by someone else" \
-		--author="Someone else <not!me>" a1
+		--author="Someone else <not!me>" a1 &&
+
+	cat >expect.template <<-\EOF
+	A U Thor (5):
+	      SUBJECT
+	      SUBJECT
+	      SUBJECT
+	      SUBJECT
+	      SUBJECT
+
+	Someone else (1):
+	      SUBJECT
+
+	EOF
+'
+
+fuzz() {
+	file=$1 &&
+	sed "
+			s/$_x40/OBJECT_NAME/g
+			s/$_x05/OBJID/g
+			s/^ \{6\}[CTa].*/      SUBJECT/g
+			s/^ \{8\}[^ ].*/        CONTINUATION/g
+		" <"$file" >"$file.fuzzy" &&
+	sed "/CONTINUATION/ d" <"$file.fuzzy"
+}
+
+test_expect_success 'default output format' '
+	git shortlog >log &&
+	fuzz log >log.predictable &&
+	test_cmp expect.template log.predictable
+'
+
+test_expect_success 'pretty format' '
+	sed s/SUBJECT/OBJECT_NAME/ expect.template >expect &&
+	git shortlog --format="%H" >log &&
+	fuzz log >log.predictable &&
+	test_cmp expect log.predictable
+'
+
+test_expect_failure '--abbrev' '
+	sed s/SUBJECT/OBJID/ expect.template >expect &&
+	git shortlog --format="%h" --abbrev=5 >log &&
+	fuzz log >log.predictable &&
+	test_cmp expect log.predictable
+'
+
+test_expect_success 'output from user-defined format is re-wrapped' '
+	sed "s/SUBJECT/two lines/" expect.template >expect &&
+	git shortlog --format="two%nlines" >log &&
+	fuzz log >log.predictable &&
+	test_cmp expect log.predictable
 '
 
 test_expect_success 'shortlog wrapping' '

-- 
1.7.1.3.g5f1e.dirty

From: Will Palmer <redacted>

Prior to this, the output of git log -1 --format=%h was always 7
characters long, without regard to whether --abbrev had been passed.

Signed-off-by: Will Palmer <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
That’s the end of the series.  Thanks for reading.

 builtin/shortlog.c         |    3 ++-
 pretty.c                   |    7 ++++---
 shortlog.h                 |    1 +
 t/t4201-shortlog.sh        |    2 +-
 t/t6006-rev-list-format.sh |   19 +++++++++++++++++++
 5 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 06320f5..5089502 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c

@@ -162,7 +162,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 		    sha1_to_hex(commit->object.sha1));
 	if (log->user_format) {
 		struct pretty_print_context ctx = {0};
-		ctx.abbrev = DEFAULT_ABBREV;
+		ctx.abbrev = log->abbrev;
 		ctx.subject = "";
 		ctx.after_subject = "";
 		ctx.date_mode = DATE_NORMAL;

@@ -290,6 +290,7 @@ parse_done:
 	}
 
 	log.user_format = rev.commit_format == CMIT_FMT_USERFORMAT;
+	log.abbrev = rev.abbrev;
 
 	/* assume HEAD if from a tty */
 	if (!nongit && !rev.pending.nr && isatty(0))