{"thread":{"id":"23670","subject":"[PATCH v4 1/3] pretty: make it easier to add new formats","startedAt":"2010-05-02T11:00:41Z","lastAt":"2010-05-08T22:04:54Z","messageCount":11,"participants":["Will Palmer","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":3},"messages":[{"id":"140754","messageId":"1272798044-10487-1-git-send-email-wmpalmer@gmail.com","threadId":"23670","inReplyTo":null,"subject":"[PATCH v4 0/3] pretty: format aliases","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-05-02T11:00:41Z","receivedAt":"2010-05-02T11:00:41Z","isPatch":true,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"The following patch series adds the ability to configure aliases for\nuser-defined formats. The first two patches add infrastructure for the\nease of adding additional formats, and infrastructure for defining\nformat aliases, respectively. The final patch adds support for defining\nformat aliases via a config option.\n\nThis is the fourth version of the patch series. The most notable change\nfrom the last iteration is the removal of changes to other pretty-format\noptions, such as the \"conditional colors\" patch and \"%H/%h --abbrev\"\npatches. I still think those were good patches, but the %H change in\nparticular was controversial enough that it would have needlessly stood\nin the way of the rest of the series, and '%?C' looked out-of-place in\nthis series without any other format changes. The removed patches will\nbe worked on independently and will be submitted separately later.\n\nJeff King noted an artifact of format.pretty.<name> left in the\ndocumentation, thanks.\n\nJunio asked for a better error message when looped-aliases were found,\nso that's been put in.\n\nThe copyright notice for the added test has been changed to the standard\nboilerplate, because there's no reason to bring politics into a patch.\n\nMost other changes since the last iteration were simple style fixes,\nmostly a result of muscle memory from the coding conventions I use at\nwork, though a couple of things (like not initializing static variables)\nwere more towards the \"you can do that? But my CS teacher in highschool\ntold me not to!\" end of the spectrum. Hopefully I've caught all the\nstyle problems this time.\n\nWill Palmer (3):\n  pretty: make it easier to add new formats\n  pretty: add infrastructure to allow format aliases\n  pretty: add aliases for pretty formats\n\n Documentation/config.txt         |    9 ++\n Documentation/pretty-formats.txt |    7 ++-\n pretty.c                         |  154 ++++++++++++++++++++++++++++++++------\n t/t4205-log-pretty-formats.sh    |   66 ++++++++++++++++\n 4 files changed, 212 insertions(+), 24 deletions(-)\n create mode 100755 t/t4205-log-pretty-formats.sh\n"},{"id":"140752","messageId":"1272798044-10487-2-git-send-email-wmpalmer@gmail.com","threadId":"23670","inReplyTo":"1272798044-10487-1-git-send-email-wmpalmer@gmail.com","subject":"[PATCH v4 1/3] pretty: make it easier to add new formats","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-05-02T11:00:42Z","receivedAt":"2010-05-02T11:00:42Z","isPatch":true,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"As the first step towards creating aliases, we make it easier to add new\nformats to the list of builtin formats. To do this, we move the\ninitialization of the formats array into a new function,\nsetup_commit_formats(), which we can easily extend later. Then, rather\nthan looping through only the list of known formats, we make a more\ngeneric find_commit_format function, which will return the commit format\nwhose name is the shortest which is prefixed with the passed-in sought\nformat, the same rules which were more-or-less hard-coded in before.\n\nSigned-off-by: Will Palmer <wmpalmer@gmail.com>\n---\n pretty.c |   81 +++++++++++++++++++++++++++++++++++++++++++------------------\n 1 files changed, 57 insertions(+), 24 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 7cb3a2a..ecac8f5 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -11,6 +11,13 @@\n #include \"reflog-walk.h\"\n \n static char *user_format;\n+static struct cmt_fmt_map {\n+\tconst char *name;\n+\tenum cmit_fmt format;\n+\tint is_tformat;\n+} *commit_formats;\n+static size_t commit_formats_len;\n+static struct cmt_fmt_map *find_commit_format(const char *sought);\n \n static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)\n {\n@@ -21,22 +28,51 @@ static void save_user_format(struct rev_info *rev, const char *cp, int is_tforma\n \trev->commit_format = CMIT_FMT_USERFORMAT;\n }\n \n-void get_commit_format(const char *arg, struct rev_info *rev)\n+static void setup_commit_formats(void)\n {\n-\tint i;\n-\tstatic struct cmt_fmt_map {\n-\t\tconst char *n;\n-\t\tsize_t cmp_len;\n-\t\tenum cmit_fmt v;\n-\t} cmt_fmts[] = {\n-\t\t{ \"raw\",\t1,\tCMIT_FMT_RAW },\n-\t\t{ \"medium\",\t1,\tCMIT_FMT_MEDIUM },\n-\t\t{ \"short\",\t1,\tCMIT_FMT_SHORT },\n-\t\t{ \"email\",\t1,\tCMIT_FMT_EMAIL },\n-\t\t{ \"full\",\t5,\tCMIT_FMT_FULL },\n-\t\t{ \"fuller\",\t5,\tCMIT_FMT_FULLER },\n-\t\t{ \"oneline\",\t1,\tCMIT_FMT_ONELINE },\n+\tstruct cmt_fmt_map builtin_formats[] = {\n+\t\t{ \"raw\",\tCMIT_FMT_RAW,\t\t0 },\n+\t\t{ \"medium\",\tCMIT_FMT_MEDIUM,\t0 },\n+\t\t{ \"short\",\tCMIT_FMT_SHORT,\t\t0 },\n+\t\t{ \"email\",\tCMIT_FMT_EMAIL,\t\t0 },\n+\t\t{ \"fuller\",\tCMIT_FMT_FULLER,\t0 },\n+\t\t{ \"full\",\tCMIT_FMT_FULL,\t\t0 },\n+\t\t{ \"oneline\",\tCMIT_FMT_ONELINE,\t1 }\n \t};\n+\tcommit_formats_len = ARRAY_SIZE(builtin_formats);\n+\tcommit_formats = xcalloc(commit_formats_len,\n+\t\t\t\t sizeof(*builtin_formats));\n+\tmemcpy(commit_formats, builtin_formats,\n+\t       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));\n+}\n+\n+static struct cmt_fmt_map *find_commit_format(const char *sought)\n+{\n+\tstruct cmt_fmt_map *found = NULL;\n+\tsize_t found_match_len;\n+\tint i;\n+\n+\tif (!commit_formats)\n+\t\tsetup_commit_formats();\n+\n+\tfor (i = 0; i < commit_formats_len; i++) {\n+\t\tsize_t match_len;\n+\n+\t\tif (prefixcmp(commit_formats[i].name, sought))\n+\t\t\tcontinue;\n+\n+\t\tmatch_len = strlen(commit_formats[i].name);\n+\t\tif (found == NULL || found_match_len > match_len) {\n+\t\t\tfound = &commit_formats[i];\n+\t\t\tfound_match_len = match_len;\n+\t\t}\n+\t}\n+\treturn found;\n+}\n+\n+void get_commit_format(const char *arg, struct rev_info *rev)\n+{\n+\tstruct cmt_fmt_map *commit_format;\n \n \trev->use_terminator = 0;\n \tif (!arg || !*arg) {\n@@ -47,21 +83,18 @@ void get_commit_format(const char *arg, struct rev_info *rev)\n \t\tsave_user_format(rev, strchr(arg, ':') + 1, arg[0] == 't');\n \t\treturn;\n \t}\n-\tfor (i = 0; i < ARRAY_SIZE(cmt_fmts); i++) {\n-\t\tif (!strncmp(arg, cmt_fmts[i].n, cmt_fmts[i].cmp_len) &&\n-\t\t    !strncmp(arg, cmt_fmts[i].n, strlen(arg))) {\n-\t\t\tif (cmt_fmts[i].v == CMIT_FMT_ONELINE)\n-\t\t\t\trev->use_terminator = 1;\n-\t\t\trev->commit_format = cmt_fmts[i].v;\n-\t\t\treturn;\n-\t\t}\n-\t}\n+\n \tif (strchr(arg, '%')) {\n \t\tsave_user_format(rev, arg, 1);\n \t\treturn;\n \t}\n \n-\tdie(\"invalid --pretty format: %s\", arg);\n+\tcommit_format = find_commit_format(arg);\n+\tif (!commit_format)\n+\t\tdie(\"invalid --pretty format: %s\", arg);\n+\n+\trev->commit_format = commit_format->format;\n+\trev->use_terminator = commit_format->is_tformat;\n }\n \n /*\n-- \n1.7.1.rc1.13.gbb0a0a.dirty\n"},{"id":"140753","messageId":"1272798044-10487-3-git-send-email-wmpalmer@gmail.com","threadId":"23670","inReplyTo":"1272798044-10487-1-git-send-email-wmpalmer@gmail.com","subject":"[PATCH v4 2/3] pretty: add infrastructure to allow format aliases","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-05-02T11:00:43Z","receivedAt":"2010-05-02T11:00:43Z","isPatch":true,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"here we modify the find_commit_format function to make it recursively\ndereference aliases when they are specified. At this point, there are\nno aliases specified and there is no way to specify an alias, but the\nsupport is there for any which are added.\n\nSigned-off-by: Will Palmer <wmpalmer@gmail.com>\n---\n pretty.c |   28 +++++++++++++++++++++++++---\n 1 files changed, 25 insertions(+), 3 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex ecac8f5..4029cc8 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -15,6 +15,8 @@ static struct cmt_fmt_map {\n \tconst char *name;\n \tenum cmit_fmt format;\n \tint is_tformat;\n+\tint is_alias;\n+\tconst char *user_format;\n } *commit_formats;\n static size_t commit_formats_len;\n static struct cmt_fmt_map *find_commit_format(const char *sought);\n@@ -46,14 +48,19 @@ static void setup_commit_formats(void)\n \t       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));\n }\n \n-static struct cmt_fmt_map *find_commit_format(const char *sought)\n+static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,\n+\t\t\t\t\t\t\tconst char *original,\n+\t\t\t\t\t\t\tint num_redirections)\n {\n \tstruct cmt_fmt_map *found = NULL;\n \tsize_t found_match_len;\n \tint i;\n \n-\tif (!commit_formats)\n-\t\tsetup_commit_formats();\n+\tif (num_redirections >= commit_formats_len) {\n+\t\tdie(\"invalid --pretty format: '%s' references an alias which \"\n+\t\t    \"points to itself\", original);\n+\t\treturn NULL;\n+\t}\n \n \tfor (i = 0; i < commit_formats_len; i++) {\n \t\tsize_t match_len;\n@@ -67,9 +74,24 @@ static struct cmt_fmt_map *find_commit_format(const char *sought)\n \t\t\tfound_match_len = match_len;\n \t\t}\n \t}\n+\n+\tif (found && found->is_alias) {\n+\t\tfound = find_commit_format_recursive(found->user_format,\n+\t\t\t\t\t\t     original,\n+\t\t\t\t\t\t     num_redirections+1);\n+\t}\n+\n \treturn found;\n }\n \n+static struct cmt_fmt_map *find_commit_format(const char *sought)\n+{\n+\tif (!commit_formats)\n+\t\tsetup_commit_formats();\n+\n+\treturn find_commit_format_recursive(sought, sought, 0);\n+}\n+\n void get_commit_format(const char *arg, struct rev_info *rev)\n {\n \tstruct cmt_fmt_map *commit_format;\n-- \n1.7.1.rc1.13.gbb0a0a.dirty\n"},{"id":"140755","messageId":"1272798044-10487-4-git-send-email-wmpalmer@gmail.com","threadId":"23670","inReplyTo":"1272798044-10487-1-git-send-email-wmpalmer@gmail.com","subject":"[PATCH v4 3/3] pretty: add aliases for pretty formats","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-05-02T11:00:44Z","receivedAt":"2010-05-02T11:00:44Z","isPatch":true,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"previously the only ways to alias a --pretty format within git were\neither to set the format as your default format (via the format.pretty\nconfiguration variable), or by using a regular git alias. This left the\ndefinition of more complicated formats to the realm of \"builtin or\nnothing\", with user-defined formats usually being reserved for quick\none-offs.\n\nHere we allow user-defined formats to enjoy more or less the same\nbenefits of builtins. By defining pretty.myalias, \"myalias\" can be\nused in place of whatever would normally come after --pretty=. This\ncan be a format:, tformat:, raw (ie, defaulting to tformat), or the name\nof another builtin or user-defined pretty format.\n\nSigned-off-by: Will Palmer <wmpalmer@gmail.com>\n---\n Documentation/config.txt         |    9 +++++\n Documentation/pretty-formats.txt |    7 +++-\n pretty.c                         |   57 +++++++++++++++++++++++++++++++-\n t/t4205-log-pretty-formats.sh    |   66 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 136 insertions(+), 3 deletions(-)\n create mode 100755 t/t4205-log-pretty-formats.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 92f851e..85d5b90 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1466,6 +1466,15 @@ pager.<cmd>::\n \tit takes precedence over this option.  To disable pagination for\n \tall commands, set `core.pager` or `GIT_PAGER` to `cat`.\n \n+pretty.<name>::\n+\tAlias for a --pretty= format string, as specified in\n+\tlinkgit:git-log[1]. Any aliases defined here can be used just\n+\tas the built-in pretty formats could. For example, defining\n+\t\"pretty.hash = format:%H\" would cause the invocation\n+\t\"git log --pretty=hash\" to be equivalent to running\n+\t\"git log --pretty=format:%H\". Note that an alias with the same\n+\tname as a built-in format will be silently ignored.\n+\n pull.octopus::\n \tThe default merge strategy to use when pulling multiple branches\n \tat once.\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 1686a54..5e95df6 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -11,7 +11,12 @@ have limited your view of history: for example, if you are\n only interested in changes related to a certain directory or\n file.\n \n-Here are some additional details for each format:\n+There are several built-in formats, and you can define\n+additional formats by setting a pretty.<name>\n+config option to either another format name, or a\n+'format:' string, as described below (see\n+linkgit:git-config[1]). Here are the details of the\n+built-in formats:\n \n * 'oneline'\n \ndiff --git a/pretty.c b/pretty.c\nindex 4029cc8..4380b2b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -18,7 +18,9 @@ static struct cmt_fmt_map {\n \tint is_alias;\n \tconst char *user_format;\n } *commit_formats;\n+static size_t builtin_formats_len;\n static size_t commit_formats_len;\n+static size_t commit_formats_alloc;\n static struct cmt_fmt_map *find_commit_format(const char *sought);\n \n static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)\n@@ -30,6 +32,51 @@ static void save_user_format(struct rev_info *rev, const char *cp, int is_tforma\n \trev->commit_format = CMIT_FMT_USERFORMAT;\n }\n \n+static int git_pretty_formats_config(const char *var, const char *value, void *cb)\n+{\n+\tstruct cmt_fmt_map *commit_format = NULL;\n+\tconst char *name;\n+\tconst char *fmt;\n+\tint i;\n+\n+\tif (prefixcmp(var, \"pretty.\"))\n+\t\treturn 0;\n+\n+\tname = &var[7];\n+\tfor (i = 0; i < builtin_formats_len; i++) {\n+\t\tif (!strcmp(commit_formats[i].name, name))\n+\t\t\treturn 0;\n+\t}\n+\n+\tfor (i = builtin_formats_len; i < commit_formats_len; i++) {\n+\t\tif (!strcmp(commit_formats[i].name, name)) {\n+\t\t\tcommit_format = &commit_formats[i];\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (!commit_format) {\n+\t\tALLOC_GROW(commit_formats, commit_formats_len+1,\n+\t\t\t   commit_formats_alloc);\n+\t\tcommit_format = &commit_formats[commit_formats_len];\n+\t\tcommit_formats_len++;\n+\t}\n+\n+\tcommit_format->name = xstrdup(name);\n+\tcommit_format->format = CMIT_FMT_USERFORMAT;\n+\tgit_config_string(&fmt, var, value);\n+\tif (!prefixcmp(fmt, \"format:\") || !prefixcmp(fmt, \"tformat:\")) {\n+\t\tcommit_format->is_tformat = fmt[0] == 't';\n+\t\tfmt = strchr(fmt, ':') + 1;\n+\t} else if (strchr(fmt, '%'))\n+\t\tcommit_format->is_tformat = 1;\n+\telse\n+\t\tcommit_format->is_alias = 1;\n+\tcommit_format->user_format = fmt;\n+\n+\treturn 0;\n+}\n+\n static void setup_commit_formats(void)\n {\n \tstruct cmt_fmt_map builtin_formats[] = {\n@@ -42,10 +89,12 @@ static void setup_commit_formats(void)\n \t\t{ \"oneline\",\tCMIT_FMT_ONELINE,\t1 }\n \t};\n \tcommit_formats_len = ARRAY_SIZE(builtin_formats);\n-\tcommit_formats = xcalloc(commit_formats_len,\n-\t\t\t\t sizeof(*builtin_formats));\n+\tbuiltin_formats_len = commit_formats_len;\n+\tALLOC_GROW(commit_formats, commit_formats_len, commit_formats_alloc);\n \tmemcpy(commit_formats, builtin_formats,\n \t       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));\n+\n+\tgit_config(git_pretty_formats_config, NULL);\n }\n \n static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,\n@@ -117,6 +166,10 @@ void get_commit_format(const char *arg, struct rev_info *rev)\n \n \trev->commit_format = commit_format->format;\n \trev->use_terminator = commit_format->is_tformat;\n+\tif (commit_format->format == CMIT_FMT_USERFORMAT) {\n+\t\tsave_user_format(rev, commit_format->user_format,\n+\t\t\t\t commit_format->is_tformat);\n+\t}\n }\n \n /*\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nnew file mode 100755\nindex 0000000..af96984\n--- /dev/null\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -0,0 +1,66 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2010, Will Palmer\n+#\n+\n+test_description='Test pretty formats'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up basic repos' '\n+\t>foo &&\n+\t>bar &&\n+\tgit add foo &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\tgit add bar &&\n+\ttest_tick &&\n+\tgit commit -m \"add bar\"'\n+\n+test_expect_success 'alias builtin format' '\n+\tgit log --pretty=oneline >expected &&\n+\tgit config pretty.test-alias oneline &&\n+\tgit log --pretty=test-alias >actual &&\n+\ttest_cmp expected actual'\n+\n+test_expect_success 'alias masking builtin format' '\n+\tgit log --pretty=oneline >expected &&\n+\tgit config pretty.oneline \"%H\" &&\n+\tgit log --pretty=oneline >actual &&\n+\ttest_cmp expected actual'\n+\n+test_expect_success 'alias user-defined format' '\n+\tgit log --pretty=\"format:%h\" >expected &&\n+\tgit config pretty.test-alias \"format:%h\" &&\n+\tgit log --pretty=test-alias >actual &&\n+\ttest_cmp expected actual'\n+\n+test_expect_success 'alias user-defined tformat' '\n+\tgit log --pretty=\"tformat:%h\" >expected &&\n+\tgit config pretty.test-alias \"tformat:%h\" &&\n+\tgit log --pretty=test-alias >actual &&\n+\ttest_cmp expected actual'\n+\n+test_expect_code 128 'alias non-existant format' '\n+\tgit config pretty.test-alias format-that-will-never-exist &&\n+\tgit log --pretty=test-alias'\n+\n+test_expect_success 'alias of an alias' '\n+\tgit log --pretty=\"tformat:%h\" >expected &&\n+\tgit config pretty.test-foo \"tformat:%h\" &&\n+\tgit config pretty.test-bar test-foo &&\n+\tgit log --pretty=test-bar >actual &&\n+\ttest_cmp expected actual'\n+\n+test_expect_success 'alias masking an alias' '\n+\tgit log --pretty=format:\"Two %H\" >expected &&\n+\tgit config pretty.duplicate \"format:One %H\" &&\n+\tgit config --add pretty.duplicate \"format:Two %H\" &&\n+\tgit log --pretty=duplicate >actual &&\n+\ttest_cmp expected actual'\n+\n+test_expect_code 128 'alias loop' '\n+\tgit config pretty.test-foo test-bar &&\n+\tgit config pretty.test-bar test-foo &&\n+\tgit log --pretty=test-foo'\n+\n+test_done\n-- \n1.7.1.rc1.13.gbb0a0a.dirty\n"},{"id":"140758","messageId":"20100502112231.GA2806@progeny.tock","threadId":"23670","inReplyTo":"1272798044-10487-2-git-send-email-wmpalmer@gmail.com","subject":"Re: [PATCH v4 1/3] pretty: make it easier to add new formats","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-02T11:22:32Z","receivedAt":"2010-05-02T11:22:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Will Palmer wrote:\n\n> As the first step towards creating aliases, we make it easier to add new\n> formats to the list of builtin formats.\n[...]\n> +\tcommit_formats_len = ARRAY_SIZE(builtin_formats);\n> +\tcommit_formats = xcalloc(commit_formats_len,\n> +\t\t\t\t sizeof(*builtin_formats));\n> +\tmemcpy(commit_formats, builtin_formats,\n> +\t       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));\n> +}\n\nnitpick: it should be safe to s/xcalloc/xmalloc/\n\nWith or without such a change, the patch looks good to me.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for the clean patch.\n\ndiff --git a/pretty.c b/pretty.c\nindex ecac8f5..41c0145 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -40,7 +40,7 @@ static void setup_commit_formats(void)\n \t\t{ \"oneline\",\tCMIT_FMT_ONELINE,\t1 }\n \t};\n \tcommit_formats_len = ARRAY_SIZE(builtin_formats);\n-\tcommit_formats = xcalloc(commit_formats_len,\n+\tcommit_formats = xmalloc(commit_formats_len *\n \t\t\t\t sizeof(*builtin_formats));\n \tmemcpy(commit_formats, builtin_formats,\n \t       sizeof(*builtin_formats)*ARRAY_SIZE(builtin_formats));\n"},{"id":"140760","messageId":"20100502115511.GA13419@progeny.tock","threadId":"23670","inReplyTo":"1272798044-10487-3-git-send-email-wmpalmer@gmail.com","subject":"Re: [PATCH v4 2/3] pretty: add infrastructure to allow format aliases","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-02T11:55:11Z","receivedAt":"2010-05-02T11:55:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Will Palmer wrote:\n\n> here we modify the find_commit_format function to make it recursively\n> dereference aliases when they are specified. At this point, there are\n> no aliases specified and there is no way to specify an alias, but the\n> support is there for any which are added.\n\nStyle.  Maybe:\n\n\tSubject: pretty: add infrastructure for commit format aliases\n\n\tAllow named commit formats to alias one another;\n\tfind_commit_format() will recursively dereference aliases when\n\tthey are specified.  At this point, there are no aliases\n\tspecified and there is no way to specify an alias, but the\n\tsupport is there for any which are added.\n\n\tIf an alias loop is detected, the function die()s.\n\n[...]\n> -static struct cmt_fmt_map *find_commit_format(const char *sought)\n> +static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,\n> +\t\t\t\t\t\t\tconst char *original,\n> +\t\t\t\t\t\t\tint num_redirections)\n>  {\n>  \tstruct cmt_fmt_map *found = NULL;\n>  \tsize_t found_match_len;\n>  \tint i;\n>  \n> -\tif (!commit_formats)\n> -\t\tsetup_commit_formats();\n> +\tif (num_redirections >= commit_formats_len) {\n> +\t\tdie(\"invalid --pretty format: '%s' references an alias which \"\n> +\t\t    \"points to itself\", original);\n> +\t\treturn NULL;\n\nnitpicks:\n\n 1. If the caller might like the chance to add more information or\n    recover (as in code used by git daemon or that might become part of\n    libgit2), you can error() and return NULL.  This would print an\n    \"error: \" message and let the caller take care of exiting.\n\n    Otherwise, this should probably die() (which prints a \"fatal: \"\n    message) without returning anything.\n\n    Not important, of course; just something to avoid confusion for\n    the reader and static analyzers.\n\n 2. It might be helpful to wrap differently in case someone tries to\n    grep for \"alias which points to itself\" after encountering the\n    error message.\n\nWith or without the changes mentioned above:\n\n  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nNice, thanks.  I hope GCC notices the tail recursion (though since\nthis is not so performance critical for git afaict, if it doesn’t, it\nis probably better to fix that in GCC than make the git code uglier).\n\ndiff --git a/pretty.c b/pretty.c\nindex 02665d0..0ba056a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -56,11 +56,10 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,\n \tsize_t found_match_len;\n \tint i;\n \n-\tif (num_redirections >= commit_formats_len) {\n-\t\tdie(\"invalid --pretty format: '%s' references an alias which \"\n-\t\t    \"points to itself\", original);\n-\t\treturn NULL;\n-\t}\n+\tif (num_redirections >= commit_formats_len)\n+\t\tdie(\"invalid --pretty format: \"\n+\t\t    \"'%s' references an alias which points to itself\",\n+\t\t    original);\n \n \tfor (i = 0; i < commit_formats_len; i++) {\n \t\tsize_t match_len;\n-- \n"},{"id":"140762","messageId":"20100502123039.GB13419@progeny.tock","threadId":"23670","inReplyTo":"1272798044-10487-4-git-send-email-wmpalmer@gmail.com","subject":"Re: [PATCH v4 3/3] pretty: add aliases for pretty formats","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-02T12:30:39Z","receivedAt":"2010-05-02T12:30:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Will Palmer wrote:\n\n> Signed-off-by: Will Palmer <wmpalmer@gmail.com>\n[...]\n> +pretty.<name>::\n> +\tAlias for a --pretty= format string, as specified in\n> +\tlinkgit:git-log[1]. Any aliases defined here can be used just\n> +\tas the built-in pretty formats could. For example, defining\n> +\t\"pretty.hash = format:%H\" would cause the invocation\n> +\t\"git log --pretty=hash\" to be equivalent to running\n> +\t\"git log --pretty=format:%H\".\n\nThis “section.key = value” syntax neither matches the ‘git config’\ncommand line nor .gitconfig file syntax.  I know alias.<name> uses the\nsame wording, but maybe we can be clearer this time.  E.g., since this\nis the git-config(1) manual page,\n\n\tFor example, running `git config pretty.changelog \"format:* %H %s\"`\n\twould cause the invocation `git log --pretty=changelog` to be\n\tequivalent to running `git log '--pretty=format:* %H %s'`.\n\n> +static int git_pretty_formats_config(const char *var, const char *value, void *cb)\n> +{\n> +\tstruct cmt_fmt_map *commit_format = NULL;\n> +\tconst char *name;\n> +\tconst char *fmt;\n> +\tint i;\n> +\n> +\tif (prefixcmp(var, \"pretty.\"))\n> +\t\treturn 0;\n> +\n> +\tname = &var[7];\n\nCan this magic number be avoided?  Maybe\n\n\tname = &var[strlen(\"pretty.\")];\n\n> +++ b/t/t4205-log-pretty-formats.sh\n[...]\n> +test_expect_code 128 'alias non-existant format' '\n> +\tgit config pretty.test-alias format-that-will-never-exist &&\n> +\tgit log --pretty=test-alias'\n\nThis will succeed if the ‘git config’ fails.  Why not use\n\n\ttest_expect_success 'alias non-existent format' '\n\t\tgit config ... &&\n\t\ttest_must_fail git log --pretty=test-alias\n\t'\n\nor something like\n\n\ttest_exit_status() {\n\t\ttest \"${2+set}\" || return 1\n\t\tcode=$1\n\t\tshift\n\t\teval \"$*\"\n\t\ttest $code -eq $?\n\t}\n\n\ttest_expect_success 'alias non-existent format' '\n\t\tgit config ... &&\n\t\ttest_exit_status 128 git log --pretty=test-alias\n\t'\n\n?\n\n[...]\n> +test_expect_code 128 'alias loop' '\n> +\tgit config pretty.test-foo test-bar &&\n> +\tgit config pretty.test-bar test-foo &&\n> +\tgit log --pretty=test-foo'\n\nLikewise.\n\nWith whatever subset of the above changes you deem suitable,\n\n  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks again.\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 85d5b90..03f2a29 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1469,11 +1469,12 @@ pager.<cmd>::\n pretty.<name>::\n \tAlias for a --pretty= format string, as specified in\n \tlinkgit:git-log[1]. Any aliases defined here can be used just\n-\tas the built-in pretty formats could. For example, defining\n-\t\"pretty.hash = format:%H\" would cause the invocation\n-\t\"git log --pretty=hash\" to be equivalent to running\n-\t\"git log --pretty=format:%H\". Note that an alias with the same\n-\tname as a built-in format will be silently ignored.\n+\tas the built-in pretty formats could. For example,\n+\trunning `git config pretty.changelog \"format:* %H %s\"`\n+\twould cause the invocation `git log --pretty=changelog`\n+\tto be equivalent to running `git log '--pretty=format:* %H %s'`.\n+\tNote that an alias with the same name as a built-in format\n+\twill be silently ignored.\n \n pull.octopus::\n \tThe default merge strategy to use when pulling multiple branches\ndiff --git a/pretty.c b/pretty.c\nindex 1c16bf8..9e3b26b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -42,7 +42,7 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c\n \tif (prefixcmp(var, \"pretty.\"))\n \t\treturn 0;\n \n-\tname = &var[7];\n+\tname = &var[strlen(\"pretty.\")];\n \tfor (i = 0; i < builtin_formats_len; i++) {\n \t\tif (!strcmp(commit_formats[i].name, name))\n \t\t\treturn 0;\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex af96984..cb9f2bd 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -14,53 +14,61 @@ test_expect_success 'set up basic repos' '\n \tgit commit -m initial &&\n \tgit add bar &&\n \ttest_tick &&\n-\tgit commit -m \"add bar\"'\n+\tgit commit -m \"add bar\"\n+'\n \n test_expect_success 'alias builtin format' '\n \tgit log --pretty=oneline >expected &&\n \tgit config pretty.test-alias oneline &&\n \tgit log --pretty=test-alias >actual &&\n-\ttest_cmp expected actual'\n+\ttest_cmp expected actual\n+'\n \n test_expect_success 'alias masking builtin format' '\n \tgit log --pretty=oneline >expected &&\n \tgit config pretty.oneline \"%H\" &&\n \tgit log --pretty=oneline >actual &&\n-\ttest_cmp expected actual'\n+\ttest_cmp expected actual\n+'\n \n test_expect_success 'alias user-defined format' '\n \tgit log --pretty=\"format:%h\" >expected &&\n \tgit config pretty.test-alias \"format:%h\" &&\n \tgit log --pretty=test-alias >actual &&\n-\ttest_cmp expected actual'\n+\ttest_cmp expected actual\n+'\n \n test_expect_success 'alias user-defined tformat' '\n \tgit log --pretty=\"tformat:%h\" >expected &&\n \tgit config pretty.test-alias \"tformat:%h\" &&\n \tgit log --pretty=test-alias >actual &&\n-\ttest_cmp expected actual'\n+\ttest_cmp expected actual\n+'\n \n-test_expect_code 128 'alias non-existant format' '\n+test_expect_success 'alias non-existant format' '\n \tgit config pretty.test-alias format-that-will-never-exist &&\n-\tgit log --pretty=test-alias'\n+\ttest_must_fail git log --pretty=test-alias\n+'\n \n test_expect_success 'alias of an alias' '\n \tgit log --pretty=\"tformat:%h\" >expected &&\n \tgit config pretty.test-foo \"tformat:%h\" &&\n \tgit config pretty.test-bar test-foo &&\n-\tgit log --pretty=test-bar >actual &&\n-\ttest_cmp expected actual'\n+\tgit log --pretty=test-bar >actual && test_cmp expected actual\n+'\n \n test_expect_success 'alias masking an alias' '\n \tgit log --pretty=format:\"Two %H\" >expected &&\n \tgit config pretty.duplicate \"format:One %H\" &&\n \tgit config --add pretty.duplicate \"format:Two %H\" &&\n \tgit log --pretty=duplicate >actual &&\n-\ttest_cmp expected actual'\n+\ttest_cmp expected actual\n+'\n \n-test_expect_code 128 'alias loop' '\n+test_expect_success 'alias loop' '\n \tgit config pretty.test-foo test-bar &&\n \tgit config pretty.test-bar test-foo &&\n-\tgit log --pretty=test-foo'\n+\ttest_must_fail git log --pretty=test-foo\n+'\n \n test_done\n"},{"id":"140769","messageId":"1272807500.24767.4.camel@dreddbeard","threadId":"23670","inReplyTo":"20100502123039.GB13419@progeny.tock","subject":"Re: [PATCH v4 3/3] pretty: add aliases for pretty formats","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-05-02T13:38:20Z","receivedAt":"2010-05-02T13:38:20Z","isPatch":true,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"On Sun, 2010-05-02 at 07:30 -0500, Jonathan Nieder wrote:\n\n> Likewise.\n> \n> With whatever subset of the above changes you deem suitable,\n> \n>   Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> \n> Thanks again.\n> \n\nI'm not familiar enough with the patch submission process. Should I add\nyour changes (all except for the config documentation I agree with, and\nthat one I agree with the reasoning for) into my series and re-submit,\nor is saying \"I agree with those changes\" enough to assume that they\nwill be applied to my series if it is accepted?\n\n\n-- \n-- Will\n"},{"id":"140781","messageId":"7viq76uwpf.fsf@alter.siamese.dyndns.org","threadId":"23670","inReplyTo":"1272798044-10487-1-git-send-email-wmpalmer@gmail.com","subject":"Re: [PATCH v4 0/3] pretty: format aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-02T15:53:16Z","receivedAt":"2010-05-02T15:53:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, both.  Will take a look and queue.\n"},{"id":"141261","messageId":"20100508210739.GA6486@progeny.tock","threadId":"23670","inReplyTo":"1272798044-10487-4-git-send-email-wmpalmer@gmail.com","subject":"[PATCH] pretty: initialize new cmt_fmt_map to 0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-08T21:07:39Z","receivedAt":"2010-05-08T21:07:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Without this change, is_alias is likely to happen to be nonzero,\nresulting in \"fatal: invalid --pretty format\" when the fake alias\ncannot be resolved.\n\nUse memset instead of initializing the members one by one to make it\neasier to expand the struct in the future if needed.\n\nt4205 (log --pretty) does not pass for me without this fix.\n\nCc: Will Palmer <wmpalmer@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nSorry I missed this before.  Sane?\n\nJonathan\n\n pretty.c |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex aaf8020..4784f67 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -59,6 +59,7 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c\n \t\tALLOC_GROW(commit_formats, commit_formats_len+1,\n \t\t\t   commit_formats_alloc);\n \t\tcommit_format = &commit_formats[commit_formats_len];\n+\t\tmemset(commit_format, 0, sizeof(*commit_format));\n \t\tcommit_formats_len++;\n \t}\n \n-- \n1.7.1\n"},{"id":"141269","messageId":"1273356294.12996.5.camel@walleee","threadId":"23670","inReplyTo":"20100508210739.GA6486@progeny.tock","subject":"Re: [PATCH] pretty: initialize new cmt_fmt_map to 0","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-05-08T22:04:54Z","receivedAt":"2010-05-08T22:04:54Z","isPatch":true,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"On Sat, 2010-05-08 at 16:07 -0500, Jonathan Nieder wrote:\n> Without this change, is_alias is likely to happen to be nonzero,\n> resulting in \"fatal: invalid --pretty format\" when the fake alias\n> cannot be resolved.\n> \n> Use memset instead of initializing the members one by one to make it\n> easier to expand the struct in the future if needed.\n> \n> t4205 (log --pretty) does not pass for me without this fix.\n> \n> Cc: Will Palmer <wmpalmer@gmail.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Sorry I missed this before.  Sane?\n> \n> Jonathan\n\nAh, looks right. I think previous versions of my patch were building in\na local variable (which /was/ initialized), then copied the whole thing\ninto the newly-allocated space. When this was changed to building\nin-place, the initialization was lost. Good catch, thanks.\n\nNot sure if Signed-off-by or Reviewed-by is the appropriate tag to\nmention here, but one of those, I assume.\n-- \n-- Will\n"}]}