From: Will Palmer Date: Mon, 26 Apr 2010 08:15:04 GMT Subject: Re: [PATCH v2 3/3] pretty: add aliases for pretty formats Message-ID: In-Reply-To: <20100426072516.GA30416@progeny.tock> 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 wrote: > Will Palmer wrote: > >> format.pretty.:: >>        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> +                             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 >