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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 2, 2010, 11:55 UTC
Message-ID: <20100502115511.GA13419@progeny.tock>
In-Reply-To: <1272798044-10487-3-git-send-email-wmpalmer@gmail.com>

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.

[...]

Show 15 quoted lines

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