{"thread":{"id":"37533","subject":"[PATCH 1/2] log-tree: make format_decorations more flexible","startedAt":"2014-09-10T21:58:48Z","lastAt":"2014-09-11T22:39:55Z","messageCount":6,"participants":["Harry Jeffery","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"249155","messageId":"5410C998.5060701@exec64.co.uk","threadId":"37533","inReplyTo":null,"subject":"[PATCH 1/2] log-tree: make format_decorations more flexible","fromName":"Harry Jeffery","fromEmail":"harry@exec64.co.uk","sentAt":"2014-09-10T21:58:48Z","receivedAt":"2014-09-10T21:58:48Z","isPatch":true,"sender":{"key":"harry@exec64.co.uk","avatar":"https://avatars.githubusercontent.com/u/808760?v=4"},"body":"The prefix, separator and suffix for decorations are hard-coded. Make\nformat_decorations more flexible by having the caller specify the\nprefix, separator and suffix.\n\nSigned-off-by: Harry Jeffery <harry@exec64.co.uk>\n---\n  log-tree.c | 16 +++++++++-------\n  log-tree.h |  2 +-\n  pretty.c   |  2 +-\n  3 files changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 95e9b1d..860694c 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -184,9 +184,11 @@ static void show_children(struct rev_info *opt, \nstruct commit *commit, int abbre\n   */\n  void format_decorations(struct strbuf *sb,\n  \t\t\tconst struct commit *commit,\n-\t\t\tint use_color)\n+\t\t\tint use_color,\n+\t\t\tconst char* prefix,\n+\t\t\tconst char* sep,\n+\t\t\tconst char* suffix)\n  {\n-\tconst char *prefix;\n  \tstruct name_decoration *decoration;\n  \tconst char *color_commit =\n  \t\tdiff_get_color(use_color, DIFF_COMMIT);\n@@ -196,20 +198,20 @@ void format_decorations(struct strbuf *sb,\n  \tdecoration = lookup_decoration(&name_decoration, &commit->object);\n  \tif (!decoration)\n  \t\treturn;\n-\tprefix = \" (\";\n+\tstrbuf_addstr(sb, prefix);\n  \twhile (decoration) {\n  \t\tstrbuf_addstr(sb, color_commit);\n-\t\tstrbuf_addstr(sb, prefix);\n  \t\tstrbuf_addstr(sb, decorate_get_color(use_color, decoration->type));\n  \t\tif (decoration->type == DECORATION_REF_TAG)\n  \t\t\tstrbuf_addstr(sb, \"tag: \");\n  \t\tstrbuf_addstr(sb, decoration->name);\n+\t\tif(decoration->next)\n+\t\t\tstrbuf_addstr(sb, sep);\n  \t\tstrbuf_addstr(sb, color_reset);\n-\t\tprefix = \", \";\n  \t\tdecoration = decoration->next;\n  \t}\n  \tstrbuf_addstr(sb, color_commit);\n-\tstrbuf_addch(sb, ')');\n+\tstrbuf_addstr(sb, suffix);\n  \tstrbuf_addstr(sb, color_reset);\n  }\n\n@@ -221,7 +223,7 @@ void show_decorations(struct rev_info *opt, struct \ncommit *commit)\n  \t\tprintf(\"\\t%s\", (char *) commit->util);\n  \tif (!opt->show_decorations)\n  \t\treturn;\n-\tformat_decorations(&sb, commit, opt->diffopt.use_color);\n+\tformat_decorations(&sb, commit, opt->diffopt.use_color, \" (\", \", \", \")\");\n  \tfputs(sb.buf, stdout);\n  \tstrbuf_release(&sb);\n  }\ndiff --git a/log-tree.h b/log-tree.h\nindex d6ecd4d..4816911 100644\n--- a/log-tree.h\n+++ b/log-tree.h\n@@ -13,7 +13,7 @@ int log_tree_diff_flush(struct rev_info *);\n  int log_tree_commit(struct rev_info *, struct commit *);\n  int log_tree_opt_parse(struct rev_info *, const char **, int);\n  void show_log(struct rev_info *opt);\n-void format_decorations(struct strbuf *sb, const struct commit *commit, \nint use_color);\n+void format_decorations(struct strbuf *sb, const struct commit *commit, \nint use_color, const char* prefix, const char* sep, const char* suffix);\n  void show_decorations(struct rev_info *opt, struct commit *commit);\n  void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n  \t\t\t     const char **subject_p,\ndiff --git a/pretty.c b/pretty.c\nindex 44b9f64..e4dc093 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1195,7 +1195,7 @@ static size_t format_commit_one(struct strbuf *sb, \n/* in UTF-8 */\n  \t\treturn 1;\n  \tcase 'd':\n  \t\tload_ref_decorations(DECORATE_SHORT_REFS);\n-\t\tformat_decorations(sb, commit, c->auto_color);\n+\t\tformat_decorations(sb, commit, c->auto_color, \" (\", \", \", \")\");\n  \t\treturn 1;\n  \tcase 'g':\t\t/* reflog info */\n  \t\tswitch(placeholder[1]) {\n-- \n2.1.0\n"},{"id":"249156","messageId":"5410C9EF.9080509@exec64.co.uk","threadId":"37533","inReplyTo":"5410C998.5060701@exec64.co.uk","subject":"[PATCH 2/2] pretty: add %D format specifier","fromName":"Harry Jeffery","fromEmail":"harry@exec64.co.uk","sentAt":"2014-09-10T22:00:15Z","receivedAt":"2014-09-10T22:00:15Z","isPatch":true,"sender":{"key":"harry@exec64.co.uk","avatar":"https://avatars.githubusercontent.com/u/808760?v=4"},"body":"%d prints decorations wrapped by \" (\" and \")\". %D provides the same\noutput without the parenthesis, making \" (%D)\" and \"%d\" equivalent to\none another.\n\nSigned-off-by: Harry Jeffery <harry@exec64.co.uk>\n---\n  Documentation/pretty-formats.txt | 6 ++++--\n  pretty.c                         | 4 ++++\n  2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt \nb/Documentation/pretty-formats.txt\nindex eac7909..2632e1a 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -128,6 +128,7 @@ The placeholders are:\n  - '%ct': committer date, UNIX timestamp\n  - '%ci': committer date, ISO 8601 format\n  - '%d': ref names, like the --decorate option of linkgit:git-log[1]\n+- '%D': ref names without the \" (\", \")\" wrapping.\n  - '%e': encoding\n  - '%s': subject\n  - '%f': sanitized subject line, suitable for a filename\n@@ -182,8 +183,9 @@ The placeholders are:\n  NOTE: Some placeholders may depend on other options given to the\n  revision traversal engine. For example, the `%g*` reflog options will\n  insert an empty string unless we are traversing reflog entries (e.g., by\n-`git log -g`). The `%d` placeholder will use the \"short\" decoration\n-format if `--decorate` was not already provided on the command line.\n+`git log -g`). The `%d` and `%D` placeholders will use the \"short\"\n+decoration format if `--decorate` was not already provided on the command\n+line.\n\n  If you add a `+` (plus sign) after '%' of a placeholder, a line-feed\n  is inserted immediately before the expansion if and only if the\ndiff --git a/pretty.c b/pretty.c\nindex e4dc093..a75ad0d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1197,6 +1197,10 @@ static size_t format_commit_one(struct strbuf \n*sb, /* in UTF-8 */\n  \t\tload_ref_decorations(DECORATE_SHORT_REFS);\n  \t\tformat_decorations(sb, commit, c->auto_color, \" (\", \", \", \")\");\n  \t\treturn 1;\n+\tcase 'D':\n+\t\tload_ref_decorations(DECORATE_SHORT_REFS);\n+\t\tformat_decorations(sb, commit, c->auto_color, \"\", \", \", \"\");\n+\t\treturn 1;\n  \tcase 'g':\t\t/* reflog info */\n  \t\tswitch(placeholder[1]) {\n  \t\tcase 'd':\t/* reflog selector */\n-- \n2.1.0\n"},{"id":"249235","messageId":"xmqq61guf641.fsf@gitster.dls.corp.google.com","threadId":"37533","inReplyTo":"5410C9EF.9080509@exec64.co.uk","subject":"Re: [PATCH 2/2] pretty: add %D format specifier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T16:56:46Z","receivedAt":"2014-09-11T16:56:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Because patch 1/2 alone does not make much sense without 2/2, it\nprobably would have been better to do these as a single patch.\n\nAnd of course a few additional tests to t4205 would not hurt ;-)\n\nThanks.\n"},{"id":"249240","messageId":"5411DB50.8080602@exec64.co.uk","threadId":"37533","inReplyTo":"xmqq61guf641.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] pretty: add %D format specifier","fromName":"Harry Jeffery","fromEmail":"harry@exec64.co.uk","sentAt":"2014-09-11T17:26:40Z","receivedAt":"2014-09-11T17:26:40Z","isPatch":true,"sender":{"key":"harry@exec64.co.uk","avatar":"https://avatars.githubusercontent.com/u/808760?v=4"},"body":"On 11/09/14 17:56, Junio C Hamano wrote:\n > Because patch 1/2 alone does not make much sense without 2/2, it\n > probably would have been better to do these as a single patch.\n\nWould you like me to resubmit it as a single patch, or are you applying \nthem as is?\n\n > And of course a few additional tests to t4205 would not hurt ;-)\n\nSure. Should the tests be in the same patch, or a subsequent one?\n"},{"id":"249252","messageId":"xmqq8ulqdjv6.fsf@gitster.dls.corp.google.com","threadId":"37533","inReplyTo":"5411DB50.8080602@exec64.co.uk","subject":"Re: [PATCH 2/2] pretty: add %D format specifier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T19:42:37Z","receivedAt":"2014-09-11T19:42:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Harry Jeffery <harry@exec64.co.uk> writes:\n\n> On 11/09/14 17:56, Junio C Hamano wrote:\n>> Because patch 1/2 alone does not make much sense without 2/2, it\n>> probably would have been better to do these as a single patch.\n>\n> Would you like me to resubmit it as a single patch, or are you\n> applying them as is?\n>\n>> And of course a few additional tests to t4205 would not hurt ;-)\n>\n> Sure. Should the tests be in the same patch, or a subsequent one?\n\nAll in one; that way, \"git show $that_single_patch\" later can become\nmore useful by demonstrating expected uses in its test.\n"},{"id":"249267","messageId":"xmqq7g19dbno.fsf@gitster.dls.corp.google.com","threadId":"37533","inReplyTo":"5410C998.5060701@exec64.co.uk","subject":"Re: [PATCH 1/2] log-tree: make format_decorations more flexible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T22:39:55Z","receivedAt":"2014-09-11T22:39:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Harry Jeffery <harry@exec64.co.uk> writes:\n\n> The prefix, separator and suffix for decorations are hard-coded. Make\n> format_decorations more flexible by having the caller specify the\n> prefix, separator and suffix.\n>\n> Signed-off-by: Harry Jeffery <harry@exec64.co.uk>\n> ---\n>  log-tree.c | 16 +++++++++-------\n>  log-tree.h |  2 +-\n>  pretty.c   |  2 +-\n>  3 files changed, 11 insertions(+), 9 deletions(-)\n>\n> diff --git a/log-tree.c b/log-tree.c\n> index 95e9b1d..860694c 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -184,9 +184,11 @@ static void show_children(struct rev_info *opt,\n> struct commit *commit, int abbre\n>   */\n>  void format_decorations(struct strbuf *sb,\n>  \t\t\tconst struct commit *commit,\n> -\t\t\tint use_color)\n> +\t\t\tint use_color,\n> +\t\t\tconst char* prefix,\n> +\t\t\tconst char* sep,\n> +\t\t\tconst char* suffix)\n\nIn our codebase, please make asterisks stick to the variable not the\ntype, i.e.\n\n\tconst char *prefix,\n        const char *separator,\n        const char *suffix)\n\nWas there a reason why \"sep\" alone needed to be abbreviated?\n\n>  \tif (!decoration)\n>  \t\treturn;\n> -\tprefix = \" (\";\n> +\tstrbuf_addstr(sb, prefix);\n>  \twhile (decoration) {\n>  \t\tstrbuf_addstr(sb, color_commit);\n> -\t\tstrbuf_addstr(sb, prefix);\n>  \t\tstrbuf_addstr(sb, decorate_get_color(use_color, decoration->type));\n>  \t\tif (decoration->type == DECORATION_REF_TAG)\n>  \t\t\tstrbuf_addstr(sb, \"tag: \");\n>  \t\tstrbuf_addstr(sb, decoration->name);\n> +\t\tif(decoration->next)\n\nHave SP between the control statement (i.e. not a function name) and\nits opening parenthesis, i.e.\n\n\t\tif (decoration->next)\n\n> +\t\t\tstrbuf_addstr(sb, sep);\n>  \t\tstrbuf_addstr(sb, color_reset);\n> -\t\tprefix = \", \";\n>  \t\tdecoration = decoration->next;\n>  \t}\n\nHmph.  I was kind of found of the nice trick to use a punctuation,\nwhich first points at the prefix \" (\" and then later points at the\nseparator \", \", to allow the code that prefixes the punctuation\nbefore showing a new item.  It is now lost.\n\n>  \tstrbuf_addstr(sb, color_commit);\n> -\tstrbuf_addch(sb, ')');\n> +\tstrbuf_addstr(sb, suffix);\n>  \tstrbuf_addstr(sb, color_reset);\n>  }\n>\n> @@ -221,7 +223,7 @@ void show_decorations(struct rev_info *opt, struct\n> commit *commit)\n>  \t\tprintf(\"\\t%s\", (char *) commit->util);\n>  \tif (!opt->show_decorations)\n>  \t\treturn;\n> -\tformat_decorations(&sb, commit, opt->diffopt.use_color);\n> +\tformat_decorations(&sb, commit, opt->diffopt.use_color, \" (\", \", \", \")\");\n>  \tfputs(sb.buf, stdout);\n>  \tstrbuf_release(&sb);\n>  }\n> diff --git a/log-tree.h b/log-tree.h\n> index d6ecd4d..4816911 100644\n> --- a/log-tree.h\n> +++ b/log-tree.h\n> @@ -13,7 +13,7 @@ int log_tree_diff_flush(struct rev_info *);\n>  int log_tree_commit(struct rev_info *, struct commit *);\n>  int log_tree_opt_parse(struct rev_info *, const char **, int);\n>  void show_log(struct rev_info *opt);\n> -void format_decorations(struct strbuf *sb, const struct commit\n> *commit, int use_color);\n> +void format_decorations(struct strbuf *sb, const struct commit\n> *commit, int use_color, const char* prefix, const char* sep, const\n\nLinewrapped by your MUA, perhaps?\n\nAgain, please check where your asterisks are.\n\n> char* suffix);\n>  void show_decorations(struct rev_info *opt, struct commit *commit);\n>  void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n>  \t\t\t     const char **subject_p,\n> diff --git a/pretty.c b/pretty.c\n> index 44b9f64..e4dc093 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1195,7 +1195,7 @@ static size_t format_commit_one(struct strbuf\n> *sb, /* in UTF-8 */\n>  \t\treturn 1;\n>  \tcase 'd':\n>  \t\tload_ref_decorations(DECORATE_SHORT_REFS);\n> -\t\tformat_decorations(sb, commit, c->auto_color);\n> +\t\tformat_decorations(sb, commit, c->auto_color, \" (\", \", \", \")\");\n\nMy eyes hurt staring at this line and the same one in the other\nfile, trying to see which comma is which.\n\nI wonder if doing something like this once at a single place:\n\n#define format_decorations_std(strbuf, commit, color) \\\n\t\tformat_decorations((strbuf), (commit), (color), \" (\", \", \", \")\")\n\nand using\n\n\tformat_decorations_std(sb, commit, opt->diffopt.use_color);\n\tformat_decorations_std(sb, commit, c->auto_color);\n\nor even better, name the one that takes three extra parameters as\nformat_decorations_extended(), and keep the behaviour of the\noriginal one the same, i.e.\n\n\t#define format_decorations(strbuf, commit, color) \\\n\t\tformat_decorations_extended((strbuf), (commit), (color), \" (\", \", \", \")\")\n\nThat way you do not have to touch the original callers, nor you\nwould have to worry about breaking any topic that somebody else may\nbe preparing that adds new calls to format_decorations().\n\n>  \t\treturn 1;\n>  \tcase 'g':\t\t/* reflog info */\n>  \t\tswitch(placeholder[1]) {\n"}]}