[PATCH v3 0/5] pretty: format aliases

[PATCH v3 0/5] pretty: format aliases

[Will Palmer]

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 built-in formats are handled. The third and
fourth patches add infrastructure for the ease of adding additional
formats, and infrastructure for defining format aliases, respectively.
The final patch adds support for defining format aliases via a config
option.

Regarding conditional colors:
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.

Regarding pretty aliases:
My original goal was to make it possible to define all of the built-in
formats as built-in 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.

This is the third version of the patch. In the second version of the
patch, (Following feedback from Jeff King <peff@peff.net>), 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.

Thanks goes to Jeff King <peff@peff.net>,
Alex Riesen <raa.lkml@gmail.com>, and especially to
Jonathan Nieder <jrnieder@gmail.com>, who took time to do a full review
on the original patch. Most of their concerns have been addressed
through the new patches themselves, but of note:

	- Changes to %H need documentation/tests
	I disagree that a documentation change is needed, as it is my
	opinion that the documentation as-is implies the behavior these
	patches introduce. Tests have been added.

	- Documentation for %C?... needs clarification
	There seemed to be some confusion as to how the new syntax
	actually worked. I've split the documentation lines into
	separate entries for %C?... and %C?(...), to make it more
	clear that both %C?green and %C(white bold) are supported.

	- xstrdup is not necessary in config
	tested, and it seems that the strdup actually is needed. Ideally
	we would avoid an extra strdup of the format itself (which is
	rather needlessly copied to the static global *user_format), but
	the alternative would be adding an entirely separate codepath
	for "pre-specified" formats. It was an awful long way to go to
	avoid one extra strdup, so I just left the call to
	save_user_format in.

	- could the code which checks for format/tformat/etc be shared?
	probably, but I can't see a good way to do so.

	- (various confusion regarding the alias code)
	The alias code sucked. It's been ripped out and replaced with a
	much saner recursive model for find_commit_format. Thanks for
	steering me away from that mess.

	- setup_commit_formats() should be in setup_revisions()?
	While it might make the call-graph shorter, and would make
	some dependencies explicit, I think that keeping
	setup_commit_formats() in get_commit_format() makes more sense,
	by keeping "commit format" things together in one place. It's
	also less of a deviation from the code it was based on.

	- a = nothing, one = a, two = a. one is alias, two is not, why?
	Not sure. By the time I got to this one, the alias code had been
	completely changed and I couldn't reproduce. I'll declare this
	fixed.

Finally, after consensus among those discussing it, the config option
is now pretty.<name>, rather than format.pretty.<name>

Changes unrelated to any mailing-list discussion:

	- ignore aliases with the same name as built-in formats
	- fix a bug in the last patch where --pretty=full output fuller
	- fix a bug in the last patch where --abbrev-commit effected raw
	- be explicit that a redefined alias should overwrite the old

Lots of changes from v2 of the patchset, but overall I think a smaller
and cleaner patch.

Will Palmer (5):
  pretty: add conditional %C?colorname placeholders
  pretty: make %H/%h/etc respect --abbrev[-commit]
  pretty: make it easier to add new formats
  pretty: add infrastructure to allow format aliases
  pretty: add aliases for pretty formats

 Documentation/config.txt         |    9 ++
 Documentation/pretty-formats.txt |    9 ++-
 builtin/log.c                    |    2 +-
 builtin/rev-list.c               |    2 +
 builtin/shortlog.c               |    7 +-
 commit.h                         |    4 +-
 log-tree.c                       |    7 +-
 pretty.c                         |  228 +++++++++++++++++++++++++++++---------
 shortlog.h                       |    2 +-
 t/t4205-log-pretty-formats.sh    |  116 +++++++++++++++++++
 10 files changed, 327 insertions(+), 59 deletions(-)
 create mode 100755 t/t4205-log-pretty-formats.sh

[PATCH v3 1/5] pretty: add conditional %C?colorname placeholders

[Will Palmer]

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 <wmpalmer@gmail.com>
---
 Documentation/pretty-formats.txt |    2 +
 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    |   52 ++++++++++++++++++++++++++++++++++++++
 9 files changed, 94 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..5861256 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -132,6 +132,8 @@ 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, as in %Cred, %Creset, etc, if color output is enabled
+- '%C?(...)': switch to specified color specification, as in %C(...), if color output is enabled
 - '%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..b7ec943
--- /dev/null
+++ b/t/t4205-log-pretty-formats.sh
@@ -0,0 +1,52 @@
+#!/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 &&
+	>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
+
+	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 &&
+		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 &&
+		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 &&
+		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 &&
+		test_cmp expected actual"
+
+done
+done
+
+test_done
-- 
1.7.1.rc1.13.gbb0a0a.dirty

[PATCH v3 2/5] pretty: make %H/%h/etc respect --abbrev[-commit]

[Will Palmer]

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 <wmpalmer@gmail.com>
---
 builtin/rev-list.c            |    1 +
 builtin/shortlog.c            |    2 ++
 commit.h                      |    3 ++-
 log-tree.c                    |    6 ++++--
 pretty.c                      |   30 +++++++++++++++++++-----------
 t/t4205-log-pretty-formats.sh |   17 +++++++++++++++++
 6 files changed, 45 insertions(+), 14 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..4c52069 100644
--- a/commit.h
+++ b/commit.h
@@ -65,7 +65,8 @@ enum cmit_fmt {
 
 struct pretty_print_context
 {
-	int abbrev;
+	int abbrev; /* length of abbreviated hashes */
+	int abbrev_commit; /* whether or not to abbreviate commit hashes */
 	const char *subject;
 	const char *after_subject;
 	enum date_mode date_mode;
diff --git a/log-tree.c b/log-tree.c
index 6bb4748..02233ed 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -279,10 +279,13 @@ void show_log(struct rev_info *opt)
 	struct strbuf msgbuf = STRBUF_INIT;
 	struct log_info *log = opt->loginfo;
 	struct commit *commit = log->commit, *parent = log->parent;
-	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
+	int abbrev_commit;
 	const char *extra_headers = opt->extra_headers;
 	struct pretty_print_context ctx = {0};
+	ctx.abbrev = opt->diffopt.abbrev;
+	ctx.abbrev_commit = opt->abbrev_commit && opt->commit_format != CMIT_FMT_RAW;
 	ctx.use_color = DIFF_OPT_TST(&opt->diffopt, COLOR_DIFF);
+	abbrev_commit = ctx.abbrev_commit ? ctx.abbrev : 40;
 
 	opt->loginfo = NULL;
 	ctx.show_notes = opt->show_notes;
@@ -411,7 +414,6 @@ void show_log(struct rev_info *opt)
 	if (ctx.need_8bit_cte >= 0)
 		ctx.need_8bit_cte = has_non_ascii(opt->add_signoff);
 	ctx.date_mode = opt->date_mode;
-	ctx.abbrev = opt->diffopt.abbrev;
 	ctx.after_subject = extra_headers;
 	ctx.reflog_info = opt->reflog_info;
 	pretty_print_commit(opt->commit_format, commit, &msgbuf, &ctx);
diff --git a/pretty.c b/pretty.c
index fdb5e16..60ed9f6 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;
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index b7ec943..a33f157 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -48,5 +48,22 @@ for color in red green blue reset; do
 
 done
 done
+test_expect_success "reset color flags" "git config --unset color.ui"
+
+test_expect_success "%H with --abbrev-commit should be synonym for %h" \
+	"git log -1 --pretty='format:%H' --abbrev-commit > expected &&
+	git log -1 --pretty='format:%h' > actual &&
+	test_cmp expected actual"
+
+test_expect_success "%H with --abbrev-commit should respect --abbrev" \
+	'test 20 = $(git log -1 --pretty="format:%H" --abbrev-commit --abbrev=20 | wc -c)'
+
+test_expect_success "%h should respect --abbrev" \
+	'test 20 = $(git log -1 --pretty="format:%h" --abbrev-commit --abbrev=20 | wc -c)'
+
+test_expect_success "log --pretty=raw should NOT respect --abbrev-commit" \
+	'git log -1 --pretty=raw > expected &&
+	git log -1 --pretty=raw --abbrev-commit > actual &&
+	test_cmp expected actual'
 
 test_done
-- 
1.7.1.rc1.13.gbb0a0a.dirty

[PATCH v3 3/5] pretty: make it easier to add new formats

[Will Palmer]

As the first step towards creating aliases, we make it easier to add new
formats to the list of builtin formats. To do this, we move the
initialization of the formats array into a new function,
setup_commit_formats(), which we can easily extend later. Then, rather
than looping through only the list of known formats, we make a more
generic find_commit_format function, which will return the commit format
whose name is the shortest which is prefixed with the passed-in sought
format, the same rules which were more-or-less hard-coded in before.

Signed-off-by: Will Palmer <wmpalmer@gmail.com>
---
 pretty.c |   81 +++++++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 57 insertions(+), 24 deletions(-)

diff --git a/pretty.c b/pretty.c
index 60ed9f6..55d5721 100644
--- a/pretty.c
+++ b/pretty.c
@@ -11,6 +11,13 @@
 #include "reflog-walk.h"
 
 static char *user_format;
+static struct cmt_fmt_map {
+	const char *name;
+	enum cmit_fmt format;
+	int is_tformat;
+} *commit_formats = NULL;
+static size_t commit_formats_len = 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 +28,51 @@ 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 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 },
+	struct cmt_fmt_map builtin_formats[] = {
+		{ "raw",	CMIT_FMT_RAW,		0 },
+		{ "medium",	CMIT_FMT_MEDIUM,	0 },
+		{ "short",	CMIT_FMT_SHORT,		0 },
+		{ "email",	CMIT_FMT_EMAIL,		0 },
+		{ "fuller",	CMIT_FMT_FULLER,	0 },
+		{ "full",	CMIT_FMT_FULL,		0 },
+		{ "oneline",	CMIT_FMT_ONELINE,	1 }
 	};
+	commit_formats_len = ARRAY_SIZE(builtin_formats);
+	commit_formats = xcalloc(commit_formats_len,
+				 sizeof(*builtin_formats));
+	memcpy(commit_formats, builtin_formats,
+	       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));
+}
+
+static struct cmt_fmt_map *find_commit_format(const char *sought)
+{
+	struct cmt_fmt_map *found = NULL;
+	size_t found_match_len;
+	int i;
+
+	if (!commit_formats)
+		setup_commit_formats();
+
+	for (i = 0; i < commit_formats_len; i++) {
+		size_t match_len;
+
+		if( prefixcmp(commit_formats[i].name, sought) )
+			continue;
+
+		match_len = strlen(commit_formats[i].name);
+		if( found == NULL || found_match_len > match_len ){
+			found = &commit_formats[i];
+			found_match_len = match_len;
+		}
+	}
+	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 +83,18 @@ 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;
 }
 
 /*
-- 
1.7.1.rc1.13.gbb0a0a.dirty

[PATCH v3 4/5] pretty: add infrastructure to allow format aliases

[Will Palmer]

here we modify the find_commit_format function to make it recursively
dereference aliases when they are specified. At this point, there are
no aliases specified and there is no way to specify an alias, but the
support is there for any which are added.

Signed-off-by: Will Palmer <wmpalmer@gmail.com>
---
 pretty.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/pretty.c b/pretty.c
index 55d5721..3d1c4a9 100644
--- a/pretty.c
+++ b/pretty.c
@@ -15,6 +15,8 @@ static struct cmt_fmt_map {
 	const char *name;
 	enum cmit_fmt format;
 	int is_tformat;
+	int is_alias;
+	const char *user_format;
 } *commit_formats = NULL;
 static size_t commit_formats_len = 0;
 static struct cmt_fmt_map *find_commit_format(const char *sought);
@@ -46,14 +48,15 @@ static void setup_commit_formats(void)
 	       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));
 }
 
-static struct cmt_fmt_map *find_commit_format(const char *sought)
+static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
+							int num_redirections)
 {
 	struct cmt_fmt_map *found = NULL;
 	size_t found_match_len;
 	int i;
 
-	if (!commit_formats)
-		setup_commit_formats();
+	if (num_redirections >= commit_formats_len)
+		return NULL;
 
 	for (i = 0; i < commit_formats_len; i++) {
 		size_t match_len;
@@ -67,9 +70,23 @@ static struct cmt_fmt_map *find_commit_format(const char *sought)
 			found_match_len = match_len;
 		}
 	}
+
+	if (found && found->is_alias) {
+		found = find_commit_format_recursive(found->user_format,
+						     num_redirections+1);
+	}
+
 	return found;
 }
 
+static struct cmt_fmt_map *find_commit_format(const char *sought)
+{
+	if (!commit_formats)
+		setup_commit_formats();
+
+	return find_commit_format_recursive(sought, 0);
+}
+
 void get_commit_format(const char *arg, struct rev_info *rev)
 {
 	struct cmt_fmt_map *commit_format;
-- 
1.7.1.rc1.13.gbb0a0a.dirty

[PATCH v3 5/5] pretty: add aliases for pretty formats

[Will Palmer]

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 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 <wmpalmer@gmail.com>
---
 Documentation/config.txt         |    9 ++++++
 Documentation/pretty-formats.txt |    7 ++++-
 pretty.c                         |   57 ++++++++++++++++++++++++++++++++++++-
 t/t4205-log-pretty-formats.sh    |   47 +++++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 92f851e..3987b2d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1466,6 +1466,15 @@ pager.<cmd>::
 	it takes precedence over this option.  To disable pagination for
 	all commands, set `core.pager` or `GIT_PAGER` to `cat`.
 
+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 built-in 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". Note that an alias with the same
+	name as a built-in format will be silently ignored.
+
 pull.octopus::
 	The default merge strategy to use when pulling multiple branches
 	at once.
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 5861256..bb0f0d2 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -11,7 +11,12 @@ have limited your view of history: for example, if you are
 only interested in changes related to a certain directory or
 file.
 
-Here are some additional details for each format:
+There are several built-in formats, and you can define
+additional formats by setting a pretty.<name>
+config option to either another format name, or a
+'format:' string, as described below (see
+linkgit:git-config[1]). Here are the details of the
+built-in formats:
 
 * 'oneline'
 
diff --git a/pretty.c b/pretty.c
index 3d1c4a9..9780e32 100644
--- a/pretty.c
+++ b/pretty.c
@@ -18,7 +18,9 @@ static struct cmt_fmt_map {
 	int is_alias;
 	const char *user_format;
 } *commit_formats = NULL;
+static size_t builtin_formats_len = 0;
 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)
@@ -30,6 +32,51 @@ static void save_user_format(struct rev_info *rev, const char *cp, int is_tforma
 	rev->commit_format = CMIT_FMT_USERFORMAT;
 }
 
+static int git_pretty_formats_config(const char *var, const char *value, void *cb)
+{
+	struct cmt_fmt_map *commit_format = NULL;
+	const char *name;
+	const char *fmt;
+	int i;
+
+	if (prefixcmp(var, "pretty."))
+		return 0;
+
+	name = &var[7];
+	for (i=0; i<builtin_formats_len; i++) {
+		if (!strcmp(commit_formats[i].name, name))
+			return 0;
+	}
+
+	for (i=builtin_formats_len; i<commit_formats_len; i++) {
+		if (!strcmp(commit_formats[i].name, name)) {
+			commit_format = &commit_formats[i];
+			break;
+		}
+	}
+
+	if (!commit_format) {
+		ALLOC_GROW(commit_formats, commit_formats_len+1,
+			   commit_formats_alloc);
+		commit_format = &commit_formats[commit_formats_len];
+		commit_formats_len++;
+	}
+
+	commit_format->name = xstrdup(name);
+	commit_format->format = CMIT_FMT_USERFORMAT;
+	git_config_string(&fmt, var, value);
+	if (!prefixcmp(fmt, "format:") || !prefixcmp(fmt, "tformat:")) {
+		commit_format->is_tformat = fmt[0] == 't';
+		fmt = strchr(fmt, ':') + 1;
+	} else if (strchr(fmt, '%'))
+		commit_format->is_tformat = 1;
+	else
+		commit_format->is_alias = 1;
+	commit_format->user_format = fmt;
+
+	return 0;
+}
+
 static void setup_commit_formats(void)
 {
 	struct cmt_fmt_map builtin_formats[] = {
@@ -42,10 +89,12 @@ static void setup_commit_formats(void)
 		{ "oneline",	CMIT_FMT_ONELINE,	1 }
 	};
 	commit_formats_len = ARRAY_SIZE(builtin_formats);
-	commit_formats = xcalloc(commit_formats_len,
-				 sizeof(*builtin_formats));
+	builtin_formats_len = commit_formats_len;
+	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);
 }
 
 static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
@@ -112,6 +161,10 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 
 	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 a33f157..d8673fb 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -66,4 +66,51 @@ test_expect_success "log --pretty=raw should NOT respect --abbrev-commit" \
 	git log -1 --pretty=raw --abbrev-commit > actual &&
 	test_cmp expected actual'
 
+test_expect_success "alias builtin format" \
+	"git log --pretty=oneline >expected &&
+	git config pretty.test-alias oneline &&
+	git log --pretty=test-alias >actual &&
+	test_cmp expected actual"
+
+test_expect_success "alias masking builtin format" \
+	"git log --pretty=oneline >expected &&
+	git config pretty.oneline '%H' &&
+	git log --pretty=oneline >actual &&
+	test_cmp expected actual"
+
+test_expect_success "alias user-defined format" \
+	"git log --pretty='format:%h' >expected &&
+	git config 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 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 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 pretty.test-foo 'tformat:%h' &&
+	git config pretty.test-bar test-foo &&
+	git log --pretty=test-bar >actual &&
+	test_cmp expected actual"
+
+test_expect_success "alias masking an alias" \
+	"git log --pretty=format:'Two %H' >expected &&
+	git config pretty.duplicate 'format:One %H' &&
+	git config --add pretty.duplicate 'format:Two %H' &&
+	git log --pretty=duplicate >actual &&
+	test_cmp expected actual"
+
+test_expect_code 128 "alias loop" \
+	"git config pretty.test-foo test-bar &&
+	git config pretty.test-bar test-foo &&
+	git log --pretty=test-foo"
+
 test_done
-- 
1.7.1.rc1.13.gbb0a0a.dirty

Re: [PATCH v3 0/5] pretty: format aliases

[Junio C Hamano]

Thanks.  Overall I like the direction this is going.

There however are too many style violations that I prefer not to fix
myself.

 * Format if/while/switch etc. like this:

	if (expression) {
        	statement;
                ...
	}

   not like this:

	if( expression ){
        	...

   - Have one SP between syntactic keyword and an open parenthesis;
   - Do not have extra SP before or after the expression inside
     parentheses;
   - Have one SP between a close parenthesis and an open brace.

 * do not initialise statics to 0 or NULL; e.g. not like
    
	static size_t commit_formats_len = 0;

   but like

	static size_t commit_formats_len;

 * Do not omit SP around operators.

	for (i = 0; i < builtin_formats_len; i++) {
        	...

   I.e. not like this:

	for (i=0; i<builtin_formats_len; i++) {

Re: [PATCH v3 1/5] pretty: add conditional %C?colorname placeholders

[Junio C Hamano]

Will Palmer <wmpalmer@gmail.com> writes:

> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> new file mode 100755
> index 0000000..b7ec943
> --- /dev/null
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -0,0 +1,52 @@
> +#!/bin/sh
> +#
> +# Released into Public Domain by Will Palmer 2010
> +#

Hmm...

> +test_description='Test pretty formats'
> +. ./test-lib.sh
> +
> +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'"

Just a style thing, but it is easier to read to write:

        test_expect_success 'set up basic repos' '
                >foo &&
                >bar &&
                ...
        '

> +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 &&
> +		test_cmp expected actual"

I would really prefer to see a test that checks output with real contents,
not just "switch to color".  E.g. --format="Title: %C${color}%s%C(normal)"
or something like that.

Also you probably would want to make this one round into a shell function
e.g.

	make_expected ()
        {
        	git config --get-color no.such.slot "$1"
	}

	run_format_test ()
        {
		flag=$1 color=$2
                test_expect_success "%C$color with ..." '
			make_expected $color >expected
                        git config color.ui $flag &&
                        git log -1 --pretty=format:'%C($color)' >actual &&
                        test_cmp expected actual"
                '
                ...
	}

and then run this loop:

	for flag in flase true always
        do
        	for color in red green blue reset
                do
                	run_format_test $flag $color
		done
	done

Re: [PATCH v3 2/5] pretty: make %H/%h/etc respect --abbrev[-commit]

[Junio C Hamano]

Will Palmer <wmpalmer@gmail.com> writes:

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

I think it is a good change to make %h follow --abbrev, but %H should stay
the full length no matter what (otherwise why would anybody use %H not %h?).

Re: [PATCH v3 4/5] pretty: add infrastructure to allow format aliases

[Junio C Hamano]

Will Palmer <wmpalmer@gmail.com> writes:

> -static struct cmt_fmt_map *find_commit_format(const char *sought)
> +static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
> +							int num_redirections)
>  {
>  	struct cmt_fmt_map *found = NULL;
>  	size_t found_match_len;
>  	int i;
>  
> -	if (!commit_formats)
> -		setup_commit_formats();
> +	if (num_redirections >= commit_formats_len)
> +		return NULL;

Nice trick to avoid a loopy definition chain.

I however wonder if we would want to be more helpful to the users to
diagnose this error by saying something here with error(), instead of just
letting the caller say "invalid --pretty format: %s".

Re: [PATCH v3 5/5] pretty: add aliases for pretty formats

[Junio C Hamano]

This looks sensible.

I didn't follow the config namespace discussions, though.  Is everybody
happy with the naming used in this patch?

Re: [PATCH v3 2/5] pretty: make %H/%h/etc respect --abbrev[-commit]

[Jeff King]

On Sat, May 01, 2010 at 08:13:00PM -0700, Junio C Hamano wrote:

> Will Palmer <wmpalmer@gmail.com> writes:
> 
> > 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.
> 
> I think it is a good change to make %h follow --abbrev, but %H should stay
> the full length no matter what (otherwise why would anybody use %H not %h?).

But I thought the point of %h was to be abbreviated? If it follows
--abbrev, then "git log --format=%h" would show the full sha1, no?

-Peff

Re: [PATCH v3 5/5] pretty: add aliases for pretty formats

[Jeff King]

On Sat, May 01, 2010 at 08:13:58PM -0700, Junio C Hamano wrote:

> I didn't follow the config namespace discussions, though.  Is everybody
> happy with the naming used in this patch?

Yes, I am happy with pretty.*. But:

On Fri, Apr 30, 2010 at 08:35:28PM +0100, Will Palmer wrote:

> +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 built-in 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". Note that an alias with the same
> +	name as a built-in format will be silently ignored.

The descriptive text mentions the old "format.pretty.hash".

-Peff

Re: [PATCH v3 2/5] pretty: make %H/%h/etc respect --abbrev[-commit]

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, May 01, 2010 at 08:13:00PM -0700, Junio C Hamano wrote:
>
>> Will Palmer <wmpalmer@gmail.com> writes:
>> ... 
>> > 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.
>> 
>> I think it is a good change to make %h follow --abbrev, but %H should stay
>> the full length no matter what (otherwise why would anybody use %H not %h?).
>
> But I thought the point of %h was to be abbreviated? If it follows
> --abbrev, then "git log --format=%h" would show the full sha1, no?

Sorry, but I meant that the point of %h was to be abbreviated and the
point of %H was not to be abbreviated.  So no matter whaqt --abbrev-commit
says on the command line, --format=%H should show the full commit object
name.

By the way, we should update Documentation/pretty-format.txt to use the
proper terminology.  Colloquial use "commit hash", "tree hash", etc. are
Ok but in a formal documentation we should consistently say "object name".

Re: [PATCH v3 2/5] pretty: make %H/%h/etc respect --abbrev[-commit]

[Jeff King]

On Sat, May 01, 2010 at 10:33:01PM -0700, Junio C Hamano 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.
> >> 
> >> I think it is a good change to make %h follow --abbrev, but %H should stay
> >> the full length no matter what (otherwise why would anybody use %H not %h?).
> >
> > But I thought the point of %h was to be abbreviated? If it follows
> > --abbrev, then "git log --format=%h" would show the full sha1, no?
> 
> Sorry, but I meant that the point of %h was to be abbreviated and the
> point of %H was not to be abbreviated.  So no matter whaqt --abbrev-commit
> says on the command line, --format=%H should show the full commit object
> name.

Ah, I see. So the change you called "good" above was that %h should
respect --abbrev=10, but keep the same default?

I do think that's an improvement, but it discards what Will was trying
to accomplish (a placeholder that behaves different depending on whether
abbreviation has been requested). I guess we would need a new syntax for
that.

-Peff

Re: [PATCH v3 2/5] pretty: make %H/%h/etc respect --abbrev[-commit]

[Will Palmer]

On Sat, 2010-05-01 at 22:33 -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Sat, May 01, 2010 at 08:13:00PM -0700, Junio C Hamano wrote:
> >

Sounds like this one remains controversial. I'll pull the format changes
out of the next pretty-alias series, and submit them as separate patches
later on. I still think the documentation implies that %H should be
abbreviated when --abbrev-commit is passed, but for now I'm going to
focus on cleaning up the pretty.alias code.
The eventual solution, I suspect, will involve a more-generic syntax as
was brought up in the previous discussion. Something like
%(abbrev-commit?%H), which would also be able to play with something
like [but, for obvious reasons, not exactly] %(is-merge?Merge: %p %p)

The %?C syntax seems to be generating less comment this time, and is the
one that I would find the most important to extending the usefulness of
format-aliases (ie: so that I can view a custom log with color in a
terminal, then output it to a text file and have it "do the right
thing"). Still, it would be lonely without %H/%h, so I'll stick it in
its own patch as well (once I fix it up a bit more, as mentioned in the
other thread).

-- 
-- Will

Re: [PATCH v3 2/5] pretty: make %H/%h/etc respect --abbrev[-commit]

[Will Palmer]

On Sat, 2010-05-01 at 20:13 -0700, Junio C Hamano wrote:
> I think it is a good change to make %h follow --abbrev, but %H should stay
> the full length no matter what (otherwise why would anybody use %H not %h?).

I could just as easily say "why would anyone pass --abbrev-commit unless
they actually wanted an abbreviated commit?". Synonyms are confusing,
but ignored parameters sound more confusing to me.

While I disagree that --abbrev-commit should have no effect whatsoever
when %H is involved, I agree that the link between %h and --abbrev is
more solid, so I will split %h respecting --abbrev into a separate
patch.

If the current behaviour is actually intentional, we should at least
update the documentation to say that %H is the "full commit hash",
rather than just the "commit hash".

-- 
-- Will

Re: [PATCH v3 4/5] pretty: add infrastructure to allow format aliases

[Will Palmer]

On Sat, 2010-05-01 at 20:13 -0700, Junio C Hamano wrote:
> I however wonder if we would want to be more helpful to the users to
> diagnose this error by saying something here with error(), instead of just
> letting the caller say "invalid --pretty format: %s".

I suppose there's not really any sane reason why we wouldn't be able to
die then and there. If we pass the original name into the recursive
function, then the message could be:

"invalid --pretty format: %s references an alias which points to itself"

sound like okay wording?

-- 
-- Will

Re: [PATCH v3 2/5] pretty: make %H/%h/etc respect --abbrev[-commit]

[Junio C Hamano]

Will Palmer <wmpalmer@gmail.com> writes:

> If the current behaviour is actually intentional, we should at least
> update the documentation to say that %H is the "full commit hash",
> rather than just the "commit hash".

I think it was obvious from the context that lists %H and %h next to each
other, with description without "abbreviated" and with "abbreviated" to
them.  The description for %H should be rewritten as "commit object name"
and %h "abbreviated commit object name".

Re: [PATCH v3 2/5] pretty: make %H/%h/etc respect --abbrev[-commit]

[Jonathan Nieder]

Junio C Hamano wrote:
> Will Palmer <wmpalmer@gmail.com> writes:

>> If the current behaviour is actually intentional, we should at least
>> update the documentation to say that %H is the "full commit hash",
>> rather than just the "commit hash".
>
> I think it was obvious from the context that lists %H and %h next to each
> other, with description without "abbreviated" and with "abbreviated" to
> them.  The description for %H should be rewritten as "commit object name"
> and %h "abbreviated commit object name".

FWIW, this is why I suggested updating the documentation.  I think the
“%H becomes %h in the presence of --abbrev-commit” behavior would be
more useful in practice.

But I am not fanatical about it.  Maybe this functionality should get
some other letter and I would just not use %H.

Jonathan

Re: [PATCH v3 1/5] pretty: add conditional %C?colorname placeholders

[Will Palmer]

On Sat, 2010-05-01 at 20:12 -0700, Junio C Hamano wrote:
> Will Palmer <wmpalmer@gmail.com> writes:
> > +# Released into Public Domain by Will Palmer 2010
> > +#
> 
> Hmm...

I saw the note that a copyright notice should be included at the top of
each test. Personally, while I like the GPL for code for example,
claiming copyright protection on a test script seems counter-productive.
If it could cause problems / make it harder to accept other
contributions, though, I'll change it to the standard boilerplate.

-- 
-- Will