Re: [PATCH v2 3/3] pretty: add aliases for pretty formats
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Apr 26, 2010, 07:25 UTC
- Message-ID
- <20100426072516.GA30416@progeny.tock>
- In-Reply-To
- <1272232579-18895-4-git-send-email-wmpalmer@gmail.com>
Will Palmer wrote:
Show 7 quoted lines
> 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.
Show 29 quoted lines
> 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.
Show 6 quoted lines
> + 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...
Show 8 quoted lines
> + 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.
Show 6 quoted lines
> + > + 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.
Show 27 quoted lines
> +
> +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.
Show 7 quoted lines
> }; > + 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.
Show 6 quoted lines
> +
> + 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
Show 5 quoted lines
> +
> + 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.
Show 10 quoted lines
> + 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.
Show 6 quoted lines
> + > + 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.
Show 9 quoted lines
> + 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