[PATCH v4 0/3] pretty: format aliases

[PATCH v4 0/3] pretty: format aliases

[Will Palmer]

The following patch series adds the ability to configure aliases for
user-defined formats. The first two 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.

This is the fourth version of the patch series. The most notable change
from the last iteration is the removal of changes to other pretty-format
options, such as the "conditional colors" patch and "%H/%h --abbrev"
patches. I still think those were good patches, but the %H change in
particular was controversial enough that it would have needlessly stood
in the way of the rest of the series, and '%?C' looked out-of-place in
this series without any other format changes. The removed patches will
be worked on independently and will be submitted separately later.

Jeff King noted an artifact of format.pretty.<name> left in the
documentation, thanks.

Junio asked for a better error message when looped-aliases were found,
so that's been put in.

The copyright notice for the added test has been changed to the standard
boilerplate, because there's no reason to bring politics into a patch.

Most other changes since the last iteration were simple style fixes,
mostly a result of muscle memory from the coding conventions I use at
work, though a couple of things (like not initializing static variables)
were more towards the "you can do that? But my CS teacher in highschool
told me not to!" end of the spectrum. Hopefully I've caught all the
style problems this time.

Will Palmer (3):
  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 |    7 ++-
 pretty.c                         |  154 ++++++++++++++++++++++++++++++++------
 t/t4205-log-pretty-formats.sh    |   66 ++++++++++++++++
 4 files changed, 212 insertions(+), 24 deletions(-)
 create mode 100755 t/t4205-log-pretty-formats.sh

[PATCH v4 1/3] 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 7cb3a2a..ecac8f5 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;
+static size_t commit_formats_len;
+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 v4 2/3] 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 |   28 +++++++++++++++++++++++++---
 1 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/pretty.c b/pretty.c
index ecac8f5..4029cc8 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;
 static size_t commit_formats_len;
 static struct cmt_fmt_map *find_commit_format(const char *sought);
@@ -46,14 +48,19 @@ 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,
+							const char *original,
+							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) {
+		die("invalid --pretty format: '%s' references an alias which "
+		    "points to itself", original);
+		return NULL;
+	}
 
 	for (i = 0; i < commit_formats_len; i++) {
 		size_t match_len;
@@ -67,9 +74,24 @@ 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,
+						     original,
+						     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, 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 v4 3/3] 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    |   66 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+), 3 deletions(-)
 create mode 100755 t/t4205-log-pretty-formats.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 92f851e..85d5b90 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
+	"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 1686a54..5e95df6 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 4029cc8..4380b2b 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;
+static size_t builtin_formats_len;
 static size_t commit_formats_len;
+static size_t commit_formats_alloc;
 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,
@@ -117,6 +166,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
new file mode 100755
index 0000000..af96984
--- /dev/null
+++ b/t/t4205-log-pretty-formats.sh
@@ -0,0 +1,66 @@
+#!/bin/sh
+#
+# Copyright (c) 2010, Will Palmer
+#
+
+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"'
+
+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 v4 1/3] pretty: make it easier to add new formats

[Jonathan Nieder]

Will Palmer wrote:

> As the first step towards creating aliases, we make it easier to add new
> formats to the list of builtin formats.
[...]
> +	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));
> +}

nitpick: it should be safe to s/xcalloc/xmalloc/

With or without such a change, the patch looks good to me.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for the clean patch.

diff --git a/pretty.c b/pretty.c
index ecac8f5..41c0145 100644
--- a/pretty.c
+++ b/pretty.c
@@ -40,7 +40,7 @@ static void setup_commit_formats(void)
 		{ "oneline",	CMIT_FMT_ONELINE,	1 }
 	};
 	commit_formats_len = ARRAY_SIZE(builtin_formats);
-	commit_formats = xcalloc(commit_formats_len,
+	commit_formats = xmalloc(commit_formats_len *
 				 sizeof(*builtin_formats));
 	memcpy(commit_formats, builtin_formats,
 	       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));

Re: [PATCH v4 2/3] pretty: add infrastructure to allow format aliases

[Jonathan Nieder]

Will Palmer wrote:

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

Style.  Maybe:

	Subject: pretty: add infrastructure for commit format aliases

	Allow named commit formats to alias one another;
	find_commit_format() will 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.

	If an alias loop is detected, the function die()s.

[...]
> -static struct cmt_fmt_map *find_commit_format(const char *sought)
> +static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
> +							const char *original,
> +							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) {
> +		die("invalid --pretty format: '%s' references an alias which "
> +		    "points to itself", original);
> +		return NULL;

nitpicks:

 1. If the caller might like the chance to add more information or
    recover (as in code used by git daemon or that might become part of
    libgit2), you can error() and return NULL.  This would print an
    "error: " message and let the caller take care of exiting.

    Otherwise, this should probably die() (which prints a "fatal: "
    message) without returning anything.

    Not important, of course; just something to avoid confusion for
    the reader and static analyzers.

 2. It might be helpful to wrap differently in case someone tries to
    grep for "alias which points to itself" after encountering the
    error message.

With or without the changes mentioned above:

  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Nice, thanks.  I hope GCC notices the tail recursion (though since
this is not so performance critical for git afaict, if it doesn’t, it
is probably better to fix that in GCC than make the git code uglier).

diff --git a/pretty.c b/pretty.c
index 02665d0..0ba056a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -56,11 +56,10 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
 	size_t found_match_len;
 	int i;
 
-	if (num_redirections >= commit_formats_len) {
-		die("invalid --pretty format: '%s' references an alias which "
-		    "points to itself", original);
-		return NULL;
-	}
+	if (num_redirections >= commit_formats_len)
+		die("invalid --pretty format: "
+		    "'%s' references an alias which points to itself",
+		    original);
 
 	for (i = 0; i < commit_formats_len; i++) {
 		size_t match_len;
-- 

Re: [PATCH v4 3/3] pretty: add aliases for pretty formats

[Jonathan Nieder]

Will Palmer wrote:

> Signed-off-by: Will Palmer <wmpalmer@gmail.com>
[...]
> +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
> +	"pretty.hash = format:%H" would cause the invocation
> +	"git log --pretty=hash" to be equivalent to running
> +	"git log --pretty=format:%H".

This “section.key = value” syntax neither matches the ‘git config’
command line nor .gitconfig file syntax.  I know alias.<name> uses the
same wording, but maybe we can be clearer this time.  E.g., since this
is the git-config(1) manual page,

	For example, running `git config pretty.changelog "format:* %H %s"`
	would cause the invocation `git log --pretty=changelog` to be
	equivalent to running `git log '--pretty=format:* %H %s'`.

> +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];

Can this magic number be avoided?  Maybe

	name = &var[strlen("pretty.")];

> +++ b/t/t4205-log-pretty-formats.sh
[...]
> +test_expect_code 128 'alias non-existant format' '
> +	git config pretty.test-alias format-that-will-never-exist &&
> +	git log --pretty=test-alias'

This will succeed if the ‘git config’ fails.  Why not use

	test_expect_success 'alias non-existent format' '
		git config ... &&
		test_must_fail git log --pretty=test-alias
	'

or something like

	test_exit_status() {
		test "${2+set}" || return 1
		code=$1
		shift
		eval "$*"
		test $code -eq $?
	}

	test_expect_success 'alias non-existent format' '
		git config ... &&
		test_exit_status 128 git log --pretty=test-alias
	'

?

[...]
> +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'

Likewise.

With whatever subset of the above changes you deem suitable,

  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks again.

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 85d5b90..03f2a29 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1469,11 +1469,12 @@ pager.<cmd>::
 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
-	"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.
+	as the built-in pretty formats could. For example,
+	running `git config pretty.changelog "format:* %H %s"`
+	would cause the invocation `git log --pretty=changelog`
+	to be equivalent to running `git log '--pretty=format:* %H %s'`.
+	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
diff --git a/pretty.c b/pretty.c
index 1c16bf8..9e3b26b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -42,7 +42,7 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
 	if (prefixcmp(var, "pretty."))
 		return 0;
 
-	name = &var[7];
+	name = &var[strlen("pretty.")];
 	for (i = 0; i < builtin_formats_len; i++) {
 		if (!strcmp(commit_formats[i].name, name))
 			return 0;
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index af96984..cb9f2bd 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -14,53 +14,61 @@ test_expect_success 'set up basic repos' '
 	git commit -m initial &&
 	git add bar &&
 	test_tick &&
-	git commit -m "add bar"'
+	git commit -m "add bar"
+'
 
 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_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_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_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_cmp expected actual
+'
 
-test_expect_code 128 'alias non-existant format' '
+test_expect_success 'alias non-existant format' '
 	git config pretty.test-alias format-that-will-never-exist &&
-	git log --pretty=test-alias'
+	test_must_fail 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'
+	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_cmp expected actual
+'
 
-test_expect_code 128 'alias loop' '
+test_expect_success 'alias loop' '
 	git config pretty.test-foo test-bar &&
 	git config pretty.test-bar test-foo &&
-	git log --pretty=test-foo'
+	test_must_fail git log --pretty=test-foo
+'
 
 test_done

Re: [PATCH v4 3/3] pretty: add aliases for pretty formats

[Will Palmer]

On Sun, 2010-05-02 at 07:30 -0500, Jonathan Nieder wrote:

> Likewise.
> 
> With whatever subset of the above changes you deem suitable,
> 
>   Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> Thanks again.
> 

I'm not familiar enough with the patch submission process. Should I add
your changes (all except for the config documentation I agree with, and
that one I agree with the reasoning for) into my series and re-submit,
or is saying "I agree with those changes" enough to assume that they
will be applied to my series if it is accepted?


-- 
-- Will

Re: [PATCH v4 0/3] pretty: format aliases

[Junio C Hamano]

Thanks, both.  Will take a look and queue.

[PATCH] pretty: initialize new cmt_fmt_map to 0

[Jonathan Nieder]

Without this change, is_alias is likely to happen to be nonzero,
resulting in "fatal: invalid --pretty format" when the fake alias
cannot be resolved.

Use memset instead of initializing the members one by one to make it
easier to expand the struct in the future if needed.

t4205 (log --pretty) does not pass for me without this fix.

Cc: Will Palmer <wmpalmer@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Sorry I missed this before.  Sane?

Jonathan

 pretty.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/pretty.c b/pretty.c
index aaf8020..4784f67 100644
--- a/pretty.c
+++ b/pretty.c
@@ -59,6 +59,7 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
 		ALLOC_GROW(commit_formats, commit_formats_len+1,
 			   commit_formats_alloc);
 		commit_format = &commit_formats[commit_formats_len];
+		memset(commit_format, 0, sizeof(*commit_format));
 		commit_formats_len++;
 	}
 
-- 
1.7.1

Re: [PATCH] pretty: initialize new cmt_fmt_map to 0

[Will Palmer]

On Sat, 2010-05-08 at 16:07 -0500, Jonathan Nieder wrote:
> Without this change, is_alias is likely to happen to be nonzero,
> resulting in "fatal: invalid --pretty format" when the fake alias
> cannot be resolved.
> 
> Use memset instead of initializing the members one by one to make it
> easier to expand the struct in the future if needed.
> 
> t4205 (log --pretty) does not pass for me without this fix.
> 
> Cc: Will Palmer <wmpalmer@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Sorry I missed this before.  Sane?
> 
> Jonathan

Ah, looks right. I think previous versions of my patch were building in
a local variable (which /was/ initialized), then copied the whole thing
into the newly-allocated space. When this was changed to building
in-place, the initialization was lost. Good catch, thanks.

Not sure if Signed-off-by or Reviewed-by is the appropriate tag to
mention here, but one of those, I assume.
-- 
-- Will