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

[Jonathan Nieder <jrnieder@gmail.com>]

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.

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

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