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

[Will Palmer <wmpalmer@gmail.com>]

I don't have time right now to reply line-by-line to this, so this is
just an ACK that it was
received, and I'll reply later today.


On Mon, Apr 26, 2010 at 8:25 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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
>