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

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 [off-list ref] wrote:

Will Palmer wrote:

quoted

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.

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

      };
+     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.

quoted

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

quoted

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

quoted

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

quoted

+                     if (!aliased_format)
+                             break;

Is to escape from the wider loop?  I think this is a valid use for goto.

quoted

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

quoted

+             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