{"thread":{"id":"21257","subject":"[PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","startedAt":"2009-10-16T08:28:19Z","lastAt":"2009-11-08T01:02:20Z","messageCount":22,"participants":["Junio C Hamano","Johannes Schindelin","Jakub Narebski","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"125147","messageId":"1255681702-5215-1-git-send-email-gitster@pobox.com","threadId":"21257","inReplyTo":null,"subject":"[PATCH 0/3] Generalized \"string function\" syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-16T08:28:19Z","receivedAt":"2009-10-16T08:28:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I mentioned an idea to enhance the pretty=format language with a string\nfunction syntax that people can extend by adding new functions in one of\nthe \"What's cooking\" messages earlier. The general syntax would be like\n\n %[function(args...)any string here%]\n\nwhere \"any string here\" part would have the usual pretty=format strings.\nE.g. git show -s --format='%{w(72,8,4)%s%+b%]' should give you a line\nwrapped commit log message if w(width,in1,in2) is such a function.\n\nThis series is a proof of concept, as I didn't actually plug the\n\"wrapping\" code into it; it would be fairly straightforward to integrate\nthe logic Dscho made strbuf capable in js/log-wrap series (queued in 'pu')\nto finish this.\n\nJunio C Hamano (3):\n format_commit_message(): fix function signature\n strbuf_nested_expand(): allow expansion to interrupt in the middle\n Add proof-of-concept %[w(width,in1,in2)<>%]\n implementation\n\n commit.h | 2 +-\n pretty.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n strbuf.c | 23 +++++++++++++---\n strbuf.h | 3 +-\n 4 files changed, 107 insertions(+), 7 deletions(-)\n"},{"id":"125146","messageId":"1255681702-5215-2-git-send-email-gitster@pobox.com","threadId":"21257","inReplyTo":"1255681702-5215-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/3] format_commit_message(): fix function signature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-16T08:28:20Z","receivedAt":"2009-10-16T08:28:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The format template string was declared as \"const void *\" for some unknown\nreason, even though it obviously is meant to be passed a string. Make it\n\"const char *\".\n\nSigned-off-by: Junio C Hamano \n---\n commit.h | 2 +-\n pretty.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex f4fc5c5..95f981a 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -70,7 +70,7 @@ extern char *reencode_commit_message(const struct commit *commit,\n \t\t\t\t const char **encoding_p);\n extern void get_commit_format(const char *arg, struct rev_info *);\n extern void format_commit_message(const struct commit *commit,\n-\t\t\t\t const void *format, struct strbuf *sb,\n+\t\t\t\t const char *format, struct strbuf *sb,\n \t\t\t\t enum date_mode dmode);\n extern void pretty_print_commit(enum cmit_fmt fmt, const struct commit*,\n struct strbuf *,\ndiff --git a/pretty.c b/pretty.c\nindex f5983f8..587101f 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -740,7 +740,7 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n }\n \n void format_commit_message(const struct commit *commit,\n-\t\t\t const void *format, struct strbuf *sb,\n+\t\t\t const char *format, struct strbuf *sb,\n \t\t\t enum date_mode dmode)\n {\n \tstruct format_commit_context context;\n-- \n1.6.5.99.g9ed7e\n"},{"id":"125149","messageId":"1255681702-5215-3-git-send-email-gitster@pobox.com","threadId":"21257","inReplyTo":"1255681702-5215-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] strbuf_nested_expand(): allow expansion to interrupt in the middle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-16T08:28:21Z","receivedAt":"2009-10-16T08:28:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This itself does not do a \"nested\" expansion, but it paves a way for\nsupporting an extended syntax to express a function that works on an\nexpanded substring, e.g. %[function(param...)expanded-string%], by\nallowing the callback function to tell where the argument to the function\nends.\n\nSigned-off-by: Junio C Hamano \n---\n strbuf.c | 23 +++++++++++++++++++----\n strbuf.h | 3 ++-\n 2 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex a6153dc..2bbc49c 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -214,25 +214,40 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n \tstrbuf_setlen(sb, sb->len + len);\n }\n \n-void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,\n-\t\t void *context)\n+void strbuf_nested_expand(struct strbuf *sb, const char **format_p,\n+\t\t\t expand_fn_t fn, void *context)\n {\n+\tconst char *format = *format_p;\n \tfor (;;) {\n \t\tconst char *percent;\n \t\tsize_t consumed;\n \n \t\tpercent = strchrnul(format, '%');\n \t\tstrbuf_add(sb, format, percent - format);\n+\t\tformat = percent;\n \t\tif (!*percent)\n \t\t\tbreak;\n-\t\tformat = percent + 1;\n+\t\tformat++;\n \n \t\tconsumed = fn(sb, format, context);\n-\t\tif (consumed)\n+\t\tif ((ssize_t) consumed < 0)\n+\t\t\tbreak;\n+\t\telse if (consumed)\n \t\t\tformat += consumed;\n \t\telse\n \t\t\tstrbuf_addch(sb, '%');\n \t}\n+\t*format_p = format;\n+}\n+\n+void strbuf_expand(struct strbuf *sb, const char *o_format, expand_fn_t fn,\n+\t\t void *context)\n+{\n+\tconst char *format = o_format;\n+\tstrbuf_nested_expand(sb, &format, fn, context);\n+\tif (*format)\n+\t\tdie(\"format error: negative return from expand function: %s\",\n+\t\t o_format);\n }\n \n size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\ndiff --git a/strbuf.h b/strbuf.h\nindex d05e056..e602899 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -109,8 +109,9 @@ static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2) {\n }\n extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);\n \n-typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);\n+typedef size_t (*expand_fn_t)(struct strbuf *sb, const char *placeholder, void *context);\n extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);\n+extern void strbuf_nested_expand(struct strbuf *sb, const char **format_p, expand_fn_t fn, void *context);\n struct strbuf_expand_dict_entry {\n \tconst char *placeholder;\n \tconst char *value;\n-- \n1.6.5.99.g9ed7e\n"},{"id":"125145","messageId":"1255681702-5215-4-git-send-email-gitster@pobox.com","threadId":"21257","inReplyTo":"1255681702-5215-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-16T08:28:22Z","receivedAt":"2009-10-16T08:28:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This uses the strbuf_nested_expand() mechanism introduced earlier\nto demonstrate how to implement a nested string function. It does\nnot \"wrap\" using the line-wrap code, but lifting the change by Dscho\nand plugging it in should be a trivial exercise.\n\nThe overall idea is to parse something like \"%[w(72,4,8)%an %ae %s%]\" in\nthese steps:\n\n #1 \"%[\" introduces the nested string function.\n\n #2 After that, a name identifies what function to call.\n\n #3 The function parses its parameters (\"(72,4,8)\" in the above example),\n and makes a nested expansion on the remainder of the format string.\n\n #4 The nested expansion is terminated at \"%]\" and returned to the\n function.\n\n #5 The function massages the string returned from #4, and the result\n becomes the expansion of the whole thing.\n\nSigned-off-by: Junio C Hamano \n---\n pretty.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 84 insertions(+), 0 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 587101f..a8a38c3 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -595,6 +595,80 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)\n \t\tstrbuf_addch(sb, ')');\n }\n \n+typedef int (*string_fmt_fn)(struct strbuf *sb, const char *placeholder, void *context);\n+static size_t format_commit_item(struct strbuf *, const char *, void *);\n+\n+static int wrap_fn(struct strbuf *sb, const char *placeholder, void *context)\n+{\n+\tconst char *template = placeholder;\n+\tchar *endptr;\n+\tlong width = 0, indent1 = 0, indent2 = 0;\n+\n+\twidth = strtol(template, &endptr, 10);\n+\tif (*endptr == ',') {\n+\t\ttemplate = endptr + 1;\n+\t\tindent1 = strtol(template, &endptr, 10);\n+\t\tif (*endptr == ',') {\n+\t\t\ttemplate = endptr + 1;\n+\t\t\tindent2 = strtol(template, &endptr, 10);\n+\t\t}\n+\t}\n+\tif (*endptr++ != ')')\n+\t\treturn 0;\n+\n+\ttemplate = endptr;\n+\tstrbuf_nested_expand(sb, &template, format_commit_item, context);\n+\tif (*template++ != ']')\n+\t\treturn 0;\n+\n+\t/*\n+\t * NEEDSWORK: here you wrap the contents of substr with\n+\t * strbuf_wrap(&substr, width, indent1, indent2);\n+\t *\n+\t * ... but I am too lazy to do that here, and I just demonstrate\n+\t * how it should work by just upcasing the result ;-)\n+\t */\n+\t{\n+\t\tint i;\n+\t\tfor (i = 0; i < sb->len; i++)\n+\t\t\tsb->buf[i] = toupper(sb->buf[i]);\n+\t}\n+\treturn template - placeholder;\n+}\n+\n+static struct {\n+\tconst char *name;\n+\tstring_fmt_fn fn;\n+} format_fn_list[] = {\n+\t{ \"w(\", wrap_fn }\n+};\n+\n+static size_t format_fn(struct strbuf *sb, const char *placeholder,\n+\t\t\tvoid *context)\n+{\n+\tconst char *template = placeholder;\n+\tsize_t consumed;\n+\tstruct strbuf substr = STRBUF_INIT;\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(format_fn_list); i++)\n+\t\tif (!prefixcmp(template, format_fn_list[i].name))\n+\t\t\tbreak;\n+\tif (ARRAY_SIZE(format_fn_list) <= i)\n+\t\treturn 0;\n+\ttemplate += strlen(format_fn_list[i].name);\n+\tconsumed = format_fn_list[i].fn(&substr, template, context);\n+\tif (!consumed) {\n+\t\tstrbuf_release(&substr);\n+\t\treturn 0;\n+\t}\n+\n+\tstrbuf_add(sb, substr.buf, substr.len);\n+\ttemplate += consumed;\n+\tstrbuf_release(&substr);\n+\treturn template - placeholder;\n+}\n+\n static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n void *context)\n {\n@@ -603,9 +677,19 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \tconst char *msg = commit->buffer;\n \tstruct commit_list *p;\n \tint h1, h2;\n+\tsize_t nested;\n \n \t/* these are independent of the commit */\n \tswitch (placeholder[0]) {\n+\tcase ']':\n+\t\treturn -1;\n+\tcase '[':\n+\t\t/*\n+\t\t * %[func(arg...) string %]: we consumed the opening '['\n+\t\t * and the callee consumed up to the closing '%]'.\n+\t\t */\n+\t\tnested = format_fn(sb, placeholder + 1, context);\n+\t\treturn nested ? 1 + nested : 0;\n \tcase 'C':\n \t\tif (placeholder[1] == '(') {\n \t\t\tconst char *end = strchr(placeholder + 2, ')');\n-- \n1.6.5.99.g9ed7e\n"},{"id":"125159","messageId":"alpine.DEB.1.00.0910161329270.4985@pacific.mpi-cbg.de","threadId":"21257","inReplyTo":"1255681702-5215-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/3] strbuf_nested_expand(): allow expansion to interrupt in the middle","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-16T11:30:18Z","receivedAt":"2009-10-16T11:30:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 Oct 2009, Junio C Hamano wrote:\n\n> \t\tconsumed = fn(sb, format, context);\n> -\t\tif (consumed)\n> +\t\tif ((ssize_t) consumed < 0)\n> +\t\t\tbreak;\n\nWould it not be much better to fix the signature of fn in a separate \ncommit before this one?\n\nCiao,\nDscho\n"},{"id":"125160","messageId":"alpine.DEB.1.00.0910161331350.4985@pacific.mpi-cbg.de","threadId":"21257","inReplyTo":"1255681702-5215-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-16T11:32:38Z","receivedAt":"2009-10-16T11:32:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nmaybe \"rewrap\" would be a better name than \"w\"?\n\nOn Fri, 16 Oct 2009, Junio C Hamano wrote:\n\n> #1 \"%[\" introduces the nested string function.\n> \n> #2 After that, a name identifies what function to call.\n> \n> #3 The function parses its parameters (\"(72,4,8)\" in the above example),\n> and makes a nested expansion on the remainder of the format string.\n\nCan't we parse it once, i.e. the first time?\n\nCiao,\nDscho\n"},{"id":"125193","messageId":"7vr5t3uuca.fsf@alter.siamese.dyndns.org","threadId":"21257","inReplyTo":"alpine.DEB.1.00.0910161329270.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3] strbuf_nested_expand(): allow expansion to interrupt in the middle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-16T17:22:45Z","receivedAt":"2009-10-16T17:22:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 16 Oct 2009, Junio C Hamano wrote:\n>\n>> \t\tconsumed = fn(sb, format, context);\n>> -\t\tif (consumed)\n>> +\t\tif ((ssize_t) consumed < 0)\n>> +\t\t\tbreak;\n>\n> Would it not be much better to fix the signature of fn in a separate \n> commit before this one?\n\nYes, I considered it and it is a reasonable thing to do if this were to\nbecome a real series for includion, but I thought it would add unnecessary\nnoise to the patch when the main purpose of posting the series is still to\nbe a proof-of-concept for discussing the design and future directions\n(including \"it should not have any future---it is useless code churn for\nsupporting only one example user 'rewrap'\").\n\nPlease remind and yell at me if (1) this turns out to be going in the\nright direction and (2) I forget to fix it when I redo the series after\ndiscussion to apply them for real.\n\nThanks.\n"},{"id":"125194","messageId":"7vk4yvuu79.fsf@alter.siamese.dyndns.org","threadId":"21257","inReplyTo":"alpine.DEB.1.00.0910161331350.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-16T17:25:46Z","receivedAt":"2009-10-16T17:25:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> maybe \"rewrap\" would be a better name than \"w\"?\n\nPerhaps, but I do not know if wrap() is even better. The only reason I\nsaid w() here is because I saw you used w() and this is meant to be a\nsuperset replacement for it, as this can re-wrap anything, not just one\nparticular field from the commit object.\n\n> On Fri, 16 Oct 2009, Junio C Hamano wrote:\n>\n>> #1 \"%[\" introduces the nested string function.\n>> \n>> #2 After that, a name identifies what function to call.\n>> \n>> #3 The function parses its parameters (\"(72,4,8)\" in the above example),\n>> and makes a nested expansion on the remainder of the format string.\n>\n> Can't we parse it once, i.e. the first time?\n\nI may be missing the issue you are raising here, but it parses the string\nonly once; it stuffs the expansion of the enclosed string into a separate\nbuffer (while noting where it ends), applies the function to the result\nobtained in the separate buffer and appends the result of the function\napplication to the main buffer, and the main expansion resumes where the\nnested one finished.\n"},{"id":"125199","messageId":"m33a5jfc6p.fsf@localhost.localdomain","threadId":"21257","inReplyTo":"7vk4yvuu79.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-16T18:02:56Z","receivedAt":"2009-10-16T18:02:56Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano writes:\n> Johannes Schindelin writes:\n> \n>> maybe \"rewrap\" would be a better name than \"w\"?\n> \n> Perhaps, but I do not know if wrap() is even better. The only reason I\n> said w() here is because I saw you used w() and this is meant to be a\n> superset replacement for it, as this can re-wrap anything, not just one\n> particular field from the commit object.\n\nwrap(initial_prefix, subsequent_prefix, columns) would for example\nfollow somewhat Text::Wrap syntax, together with 'fill'. With columns\nbeing 0, not set, or negative it could simply indent result; this way\nwe finally would be able to get default git-log / git-rev-list format\nusing --pretty format.\n\nI don't remember what were original parameters to w(72,4,8) means...\n\n> \n>> On Fri, 16 Oct 2009, Junio C Hamano wrote:\n>>\n>>> #1 \"%[\" introduces the nested string function.\n>>> \n>>> #2 After that, a name identifies what function to call.\n>>> \n>>> #3 The function parses its parameters (\"(72,4,8)\" in the above example),\n>>> and makes a nested expansion on the remainder of the format string.\n>>\n>> Can't we parse it once, i.e. the first time?\n> \n> I may be missing the issue you are raising here, but it parses the string\n> only once; it stuffs the expansion of the enclosed string into a separate\n> buffer (while noting where it ends), applies the function to the result\n> obtained in the separate buffer and appends the result of the function\n> application to the main buffer, and the main expansion resumes where the\n> nested one finished.\n\nDo I understand it correctly that generic syntax is the following:\n\n %[function(params) format specifiers]\n\nwhich would run given function, with given extra parameters, on the\nresult of expansion of the rest of the group? That is a very\npowerfull syntax... I wonder how other tools solved such problem...\n\nBTW. can we have this also for git-for-each-ref format parameter?\n\n\nNote that for single parameter we have different syntax (for\ngit-for-each-ref), namely\n\n %(field:modifier)\n\nwhich could be expanded to allow for parametrized modifiers with one\nof the following:\n\n %(field:modifier=param)\n %(field:modifier[param])\n %(field:modifier(param))\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"125203","messageId":"7v3a5jupr7.fsf@alter.siamese.dyndns.org","threadId":"21257","inReplyTo":"m33a5jfc6p.fsf@localhost.localdomain","subject":"Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-16T19:01:48Z","receivedAt":"2009-10-16T19:01:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> I don't remember what were original parameters to w(72,4,8) means...\n\n\"man git-shortlog\" look for -w.\n"},{"id":"125213","messageId":"200910170020.01756.jnareb@gmail.com","threadId":"21257","inReplyTo":"7v3a5jupr7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-16T22:19:59Z","receivedAt":"2009-10-16T22:19:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 16 Oct 2009, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > I don't remember what were original parameters to w(72,4,8) means...\n> \n> \"man git-shortlog\" look for -w.\n\nThanks. So those would be:\n\n-w[[,[,]]]::\n Linewrap the output by wrapping each line at `width`. The first\n line of each entry is indented by `indent1` spaces, and the second\n and subsequent lines are indented by `indent2` spaces. `width`,\n `indent1`, and `indent2` default to 76, 6 and 9 respectively.\n\nI think better solution might be to give _string_ to use for initial and\nsubsequent indent rather than number of spaces... well, at least more\ngeneric one, allowing one to use e.g. \"\\t\" (TAB) character to indent,\nor indent in the following way:\n\n * Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod\n tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim\n veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea\n commodo consequat. [...]\n\nBut even with original w(,,) we can get output\nof bare \"git log\" using pretty format... well, almost; it would be the\nsame if there was ability to put infinite width, and there doesn't seem\nto be specifier for the whole, unchanged commit message (subject,\nunwrapped + separating lines + body).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"125218","messageId":"7v7huurkif.fsf@alter.siamese.dyndns.org","threadId":"21257","inReplyTo":"200910170020.01756.jnareb@gmail.com","subject":"Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-16T23:23:20Z","receivedAt":"2009-10-16T23:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> I think better solution might be to give _string_ to use for initial and\n> subsequent indent rather than number of spaces... well, at least more\n> generic one, allowing one to use e.g. \"\\t\" (TAB) character to indent,\n> or indent in the following way:\n\nI do not see there is anything _better_ or _worse_. It may simply be a\nuseful _addition_. I'd usually say \"care to send in a patch?\" here, but\ngiven that the general framework and the \"wrap like the shortlog\" hasn't\nbeen plugged-in yet, there is no need to rush.\n\n> But even with original w(,,) we can get output\n> of bare \"git log\" using pretty format... well, almost; it would be the\n> same if there was ability to put infinite width, and there doesn't seem\n> to be specifier for the whole, unchanged commit message (subject,\n> unwrapped + separating lines + body).\n\nI think I already discussed this when I sent out %s%+b patch. You would\nneed to adjust and apply both series, but essentially it would become\nsomething like:\n\n %s%+[w(-1,4,4)%b]\n\nI.e. a single subject line, potentially followed by a LF and body indented\nby 4-place, but the LF will be there only when the body is not empty.\n"},{"id":"125220","messageId":"200910170200.03681.jnareb@gmail.com","threadId":"21257","inReplyTo":"7v7huurkif.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-10-17T00:00:02Z","receivedAt":"2009-10-17T00:00:02Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 17 Oct 2009, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > But even with original w(,,) we can get output\n> > of bare \"git log\" using pretty format... well, almost; it would be the\n> > same if there was ability to put infinite width, and there doesn't seem\n> > to be specifier for the whole, unchanged commit message (subject,\n> > unwrapped + separating lines + body).\n> \n> I think I already discussed this when I sent out %s%+b patch. You would\n> need to adjust and apply both series, but essentially it would become\n> something like:\n> \n> %s%+[w(-1,4,4)%b]\n> \n> I.e. a single subject line, potentially followed by a LF and body indented\n> by 4-place, but the LF will be there only when the body is not empty.\n\nWhy not\n\n %[w(-1,4,4)%s%+b]\n\nOr is it\n\n %[w(-1,4,4)%s%+%b]\n\n(i.e. %+ is this empty line between subject and body, if it exists).\n\nThe %+x seems a bit strange... but I guess implementing conditional\nexpansion a la shell or rpn spec/queryformat would be out of question\n(i.e. %?s:+ )...\n-- \nJakub Narebski\nPoland\n"},{"id":"125221","messageId":"7vy6naq3do.fsf@alter.siamese.dyndns.org","threadId":"21257","inReplyTo":"200910170200.03681.jnareb@gmail.com","subject":"Re: [PATCH 3/3] Add proof-of-concept %[w(width,in1,in2)<>%] implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-17T00:18:43Z","receivedAt":"2009-10-17T00:18:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Why not\n>\n> %[w(-1,4,4)%s%+b]\n\nYeah. As you said \"subject unwrapped and body indented\" or something like\nthat, I excluded that part outside the indentation, but log output indents\neverything, so placing both inside %[w] would be correct.\n\n> (i.e. %+ is this empty line between subject and body, if it exists).\n\nAs %+ is to add LF iff the next expansion is non-empty, it must always be\nfollowed by an expansion, i.e. %something. That means \"%+%something\" is\nunnecessary verbose and \"%+something\" is enough. \"%+anything\" syntax,\njust like \"%[func()anything]\" syntax, is not limited to any particular\nexpansion.\n\n> The %+x seems a bit strange... but I guess implementing conditional\n> expansion a la shell or rpn spec/queryformat would be out of question\n> (i.e. %?s:+ )...\n\nWhy not?\n\nI can see us doing something like %[conditional%|iftrue%|iffalse%] quite\neasily, now we have the \"nested\" variant of the strbuf_expand()\ninfrastructure.\n\nBut that is not the primary focus of the two patch series I've\ndemonstrated so far.\n"},{"id":"125258","messageId":"4ADA3153.7070606@lsrfire.ath.cx","threadId":"21257","inReplyTo":"1255681702-5215-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/3] Generalized \"string function\" syntax","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-10-17T21:04:19Z","receivedAt":"2009-10-17T21:04:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Junio C Hamano schrieb:\n> I mentioned an idea to enhance the pretty=format language with a\n> string function syntax that people can extend by adding new functions\n> in one of the \"What's cooking\" messages earlier. The general syntax\n> would be like\n> \n> %[function(args...)any string here%]\n> \n> where \"any string here\" part would have the usual pretty=format\n> strings. E.g. git show -s --format='%{w(72,8,4)%s%+b%]' should give\n> you a line wrapped commit log message if w(width,in1,in2) is such a\n> function.\n\nI pondered line wrapping with format strings briefly a long time ago, and\nI always considered it to be more similar to a colour, i.e. a state that\none can change and that is applied to all following text until the next\nstate change. (Except that it's always reset at the end of the format\nstring.) The example above would then turn into '%w(72,8,4)%s%+b'.\n\nHere's a patch to implement this behaviour. It leaves the implementation\nof the actual wrap function as an exercise to the reader, too. ;-)\n\n\n pretty.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 66 insertions(+), 0 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex f5983f8..33464f1 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -445,6 +445,7 @@ struct format_commit_context {\n \tenum date_mode dmode;\n \tunsigned commit_header_parsed:1;\n \tunsigned commit_message_parsed:1;\n+\tsize_t width, indent1, indent2;\n \n \t/* These offsets are relative to the start of the commit message. */\n \tstruct chunk author;\n@@ -458,6 +459,7 @@ struct format_commit_context {\n \tstruct chunk abbrev_commit_hash;\n \tstruct chunk abbrev_tree_hash;\n \tstruct chunk abbrev_parent_hashes;\n+\tsize_t wrap_start;\n };\n \n static int add_again(struct strbuf *sb, struct chunk *chunk)\n@@ -595,6 +597,44 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)\n \t\tstrbuf_addch(sb, ')');\n }\n \n+static void strbuf_wrap(struct strbuf *sb, size_t pos, size_t len,\n+\t\t\tsize_t width, size_t indent1, size_t indent2)\n+{\n+\tstruct strbuf tmp = STRBUF_INIT;\n+\n+\tif (!len)\n+\t\treturn;\n+\tif (pos)\n+\t\tstrbuf_add(&tmp, sb->buf, pos);\n+\n+\t/* XXX: all that's missing is the wrapping code itself.. */\n+\tstrbuf_addf(&tmp, \"w(%u,%u,%u)[\\n\",\n+\t\t (unsigned)width, (unsigned)indent1, (unsigned)indent2);\n+\tstrbuf_add(&tmp, sb->buf + pos, len);\n+\tstrbuf_addstr(&tmp, \"\\n]\");\n+\n+\tif (pos + len < sb->len)\n+\t\tstrbuf_add(&tmp, sb->buf + pos + len, sb->len - pos - len);\n+\tstrbuf_swap(&tmp, sb);\n+\tstrbuf_release(&tmp);\n+}\n+\n+static void rewrap_message_tail(struct strbuf *sb,\n+\t\t\t\tstruct format_commit_context *c,\n+\t\t\t\tsize_t new_width, size_t new_indent1,\n+\t\t\t\tsize_t new_indent2)\n+{\n+\tif (c->width == new_width && c->indent1 == new_indent1 &&\n+\t c->indent2 == new_indent2)\n+\t\treturn;\n+\tstrbuf_wrap(sb, c->wrap_start, sb->len - c->wrap_start, c->width,\n+\t\t c->indent1, c->indent2);\n+\tc->wrap_start = sb->len;\n+\tc->width = new_width;\n+\tc->indent1 = new_indent1;\n+\tc->indent2 = new_indent2;\n+}\n+\n static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n void *context)\n {\n@@ -645,6 +685,30 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \t\t\treturn 3;\n \t\t} else\n \t\t\treturn 0;\n+\tcase 'w':\n+\t\tif (placeholder[1] == '(') {\n+\t\t\tunsigned long width = 0, indent1 = 0, indent2 = 0;\n+\t\t\tchar *next;\n+\t\t\tconst char *start = placeholder + 2;\n+\t\t\tconst char *end = strchr(start, ')');\n+\t\t\tif (!end)\n+\t\t\t\treturn 0;\n+\t\t\tif (end > start) {\n+\t\t\t\twidth = strtoul(start, &next, 10);\n+\t\t\t\tif (*next == ',') {\n+\t\t\t\t\tindent1 = strtoul(next + 1, &next, 10);\n+\t\t\t\t\tif (*next == ',') {\n+\t\t\t\t\t\tindent2 = strtoul(next + 1,\n+\t\t\t\t\t\t\t\t &next, 10);\n+\t\t\t\t\t}\n+\t\t\t\t}\n+\t\t\t\tif (*next != ')')\n+\t\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\trewrap_message_tail(sb, c, width, indent1, indent2);\n+\t\t\treturn end - placeholder + 1;\n+\t\t} else\n+\t\t\treturn 0;\n \t}\n \n \t/* these depend on the commit */\n@@ -748,7 +812,9 @@ void format_commit_message(const struct commit *commit,\n \tmemset(&context, 0, sizeof(context));\n \tcontext.commit = commit;\n \tcontext.dmode = dmode;\n+\tcontext.wrap_start = sb->len;\n \tstrbuf_expand(sb, format, format_commit_item, &context);\n+\trewrap_message_tail(sb, &context, 0, 0, 0);\n }\n \n static void pp_header(enum cmit_fmt fmt,\n"},{"id":"125259","messageId":"4ADA3158.6000909@lsrfire.ath.cx","threadId":"21257","inReplyTo":"1255681702-5215-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/3] format_commit_message(): fix function signature","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-10-17T21:04:24Z","receivedAt":"2009-10-17T21:04:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Junio C Hamano schrieb:\n> The format template string was declared as \"const void *\" for some unknown\n> reason, even though it obviously is meant to be passed a string. Make it\n> \"const char *\".\n\nYes. I seem to have introduced that type in commit 7b95089c, but I\ncan't see (even less remember) why. Also note that commit message and\nheader file call the parameter \"template\", while it's called \"format\" in\ncommit.c (where the function used to live back then). What was I thinking?\n\nThanks for cleaning up after me.\n\nRené\n"},{"id":"125265","messageId":"7v63ad5o8p.fsf@alter.siamese.dyndns.org","threadId":"21257","inReplyTo":"4ADA3153.7070606@lsrfire.ath.cx","subject":"Re: [PATCH 0/3] Generalized \"string function\" syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-18T04:18:14Z","receivedAt":"2009-10-18T04:18:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Junio C Hamano schrieb:\n>> I mentioned an idea to enhance the pretty=format language with a\n>> string function syntax that people can extend by adding new functions\n>> in one of the \"What's cooking\" messages earlier. The general syntax\n>> would be like\n>> \n>> %[function(args...)any string here%]\n>> \n>> where \"any string here\" part would have the usual pretty=format\n>> strings. E.g. git show -s --format='%{w(72,8,4)%s%+b%]' should give\n>> you a line wrapped commit log message if w(width,in1,in2) is such a\n>> function.\n>\n> I pondered line wrapping with format strings briefly a long time ago, and\n> I always considered it to be more similar to a colour, i.e. a state that\n> one can change and that is applied to all following text until the next\n> state change. (Except that it's always reset at the end of the format\n> string.) The example above would then turn into '%w(72,8,4)%s%+b'.\n\nAs a syntax to express \"wrapping\" behaviour alone, I think this is much\nsimpler and more superiour. I guess with this if you want to wrap\nsomething to 72 columns and then wrap something else to 66 columns, you\nwould write '%w(72,8,4)something%w(66,8,4)something else', right?\n\nI used %] only for two reasons.\n\n - Without an explicit \"here it ends\", I couldn't come up with a good way\n to express '%[w(72,8,4)something%]something else'. IOW, how I can say\n \"wrap something to 72 columns and then place something else without any\n wrapping\"?\n \n - When we need to support more than one string function like this, it is\n unclear what '%f()one string%g()another one' in your syntax means.\n Does it mean '%[f()one string%]%[g()another one%]' (i.e. concatenate\n the result of applying string function f to 'one string' and the result\n of applying string function g to 'another one')? Or does it mean\n '%[f()one string%[g()another one%]%]' (apply 'f' to concatenation of\n 'one string' and the result of applying 'g' to 'another one')?\n"},{"id":"125271","messageId":"4ADAD0D2.504@lsrfire.ath.cx","threadId":"21257","inReplyTo":"7v63ad5o8p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Generalized \"string function\" syntax","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-10-18T08:24:50Z","receivedAt":"2009-10-18T08:24:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Junio C Hamano schrieb:\n> René Scharfe writes:\n> \n>> Junio C Hamano schrieb:\n>>> I mentioned an idea to enhance the pretty=format language with a\n>>> string function syntax that people can extend by adding new functions\n>>> in one of the \"What's cooking\" messages earlier. The general syntax\n>>> would be like\n>>>\n>>> %[function(args...)any string here%]\n>>>\n>>> where \"any string here\" part would have the usual pretty=format\n>>> strings. E.g. git show -s --format='%{w(72,8,4)%s%+b%]' should give\n>>> you a line wrapped commit log message if w(width,in1,in2) is such a\n>>> function.\n>> I pondered line wrapping with format strings briefly a long time ago, and\n>> I always considered it to be more similar to a colour, i.e. a state that\n>> one can change and that is applied to all following text until the next\n>> state change. (Except that it's always reset at the end of the format\n>> string.) The example above would then turn into '%w(72,8,4)%s%+b'.\n> \n> As a syntax to express \"wrapping\" behaviour alone, I think this is much\n> simpler and more superiour. I guess with this if you want to wrap\n> something to 72 columns and then wrap something else to 66 columns, you\n> would write '%w(72,8,4)something%w(66,8,4)something else', right?\n\nThat's right.\n\n> I used %] only for two reasons.\n> \n> - Without an explicit \"here it ends\", I couldn't come up with a good way\n> to express '%[w(72,8,4)something%]something else'. IOW, how I can say\n> \"wrap something to 72 columns and then place something else without any\n> wrapping\"?\n\nMy patch makes '%w()' reset the wrapping parameters to their defaults.\n\n> - When we need to support more than one string function like this, it is\n> unclear what '%f()one string%g()another one' in your syntax means.\n> Does it mean '%[f()one string%]%[g()another one%]' (i.e. concatenate\n> the result of applying string function f to 'one string' and the result\n> of applying string function g to 'another one')? Or does it mean\n> '%[f()one string%[g()another one%]%]' (apply 'f' to concatenation of\n> 'one string' and the result of applying 'g' to 'another one')?\n\nI was going to say that we already have something like that with %C, and\nthat the natural way (to me) is to apply them both, independently. Case\nmodification functions (upper, lower, capitalized) could be treated the\nsame way -- as state changes (like pressing caps lock when typing text).\n\nWhich other text functions are we going to add which would break this\nmodel? The only thing I can think of right now is nesting such\nfunctions themselves, e.g. when indenting a list in an indented\nsub-paragraph in an indented paragraph. Useful?\n\nBut then something else hit me: the line wrap function needs to consider\ncolour codes as having a length of zero. Ugh.\n\nRené\n"},{"id":"125295","messageId":"7vr5t0nwu8.fsf@alter.siamese.dyndns.org","threadId":"21257","inReplyTo":"4ADAD0D2.504@lsrfire.ath.cx","subject":"Re: [PATCH 0/3] Generalized \"string function\" syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-18T22:47:27Z","receivedAt":"2009-10-18T22:47:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Which other text functions are we going to add which would break this\n> model? The only thing I can think of right now is nesting such\n> functions themselves, e.g. when indenting a list in an indented\n> sub-paragraph in an indented paragraph. Useful?\n\nI was more worried about painting ourselves now in a corner we cannot get\nout of easily later. Even if my answer to question \"what are we going to\nadd\" may be \"nothing I can think of right now\", it does not make me happy.\n\nSomething off the top of my head are combinations like these.\n\n %[toupper()%cD%] => 'SUN, 18 OCT 2009 12:34:56 -0700'\n %[substr(7,3)%[toupper()%cD%]] => 'OCT'\n\n %[sanitize()%s%] === %f (i.e. format-patch filename)\n %[sanitize()%[substr(0,7)%[toupper()%aN%]%]%s] (with upcased author name)\n\nBy the way, I think that date formatting can be helped by introducing a\nstrftime() function that takes %ct/%at as input, e.g. %aD would become\n\n %[strftime(%a, %d %b %Y %H:%M:%S %z)%at]\n\nand we do not have to worry about keep adding random %[ac]X formats and\nrunning out of X. Right now we use d/D/r/i and there were talks of adding\na shortened 8601 format without time or something we did not implement.\n\nAlso, if we had this %[func() any string%] mechanism, we probably wouldn't\nhave had to add distinction between n/N and e/E after %a and %c.\n"},{"id":"125423","messageId":"4ADCF117.2030905@lsrfire.ath.cx","threadId":"21257","inReplyTo":"7vr5t0nwu8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Generalized \"string function\" syntax","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-10-19T23:07:03Z","receivedAt":"2009-10-19T23:07:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Junio C Hamano schrieb:\n> René Scharfe writes:\n> \n>> Which other text functions are we going to add which would break this\n>> model? The only thing I can think of right now is nesting such\n>> functions themselves, e.g. when indenting a list in an indented\n>> sub-paragraph in an indented paragraph. Useful?\n> \n> I was more worried about painting ourselves now in a corner we cannot get\n> out of easily later. Even if my answer to question \"what are we going to\n> add\" may be \"nothing I can think of right now\", it does not make me happy.\n\nIf wrapping wasn't implemented as a nested function, nesting could still\nbe introduced independently and used for other things -- once these\nother things arrive.\n\n> Something off the top of my head are combinations like these.\n> \n> %[toupper()%cD%] => 'SUN, 18 OCT 2009 12:34:56 -0700'\n> %[substr(7,3)%[toupper()%cD%]] => 'OCT'\n> \n> %[sanitize()%s%] === %f (i.e. format-patch filename)\n> %[sanitize()%[substr(0,7)%[toupper()%aN%]%]%s] (with upcased author name)\n\nInteresting examples, I particular like sanitize().\n\n> By the way, I think that date formatting can be helped by introducing a\n> strftime() function that takes %ct/%at as input, e.g. %aD would become\n> \n> %[strftime(%a, %d %b %Y %H:%M:%S %z)%at]\n> \n> and we do not have to worry about keep adding random %[ac]X formats and\n> running out of X. Right now we use d/D/r/i and there were talks of adding\n> a shortened 8601 format without time or something we did not implement.\n\nThe number of date formats is scary, but this could be solved e.g. by\nintroducing \"%aT()\", without nesting.\n\n> Also, if we had this %[func() any string%] mechanism, we probably wouldn't\n> have had to add distinction between n/N and e/E after %a and %c.\n\nYeah, the place holders multiplied, and some of that growth could have\nbeen avoided by providing ways to change the change the output instead\nof providing the processed results.\n\nHowever, I think that nesting is such a big addition that it warrants\nfurther planning. It turns the simple \"see place holder, fill in value\"\ninterpolator into more of a programming language. Is that really\nneeded? And if yes, do we want to keep all these percent signs around\nor is it better to invent a nicer syntax? Or borrow it from somewhere\nelse? Or perhaps I'm just afraid of change and complexity.\n\nAnyway, all of the functions that accept strings need to be able to skip\nover escape codes, which includes all of those mentioned above except\nperhaps strftime. This is ugly. Or one could forbid colour codes in\nfunction arguments.\n\nI'm more in favour of adding ways to customize the shape of the elements\nrather than adding string functions. %S(width=76,indent=4) over\n%[wrap(76,4)%s%].\n\nI feel I need to think a bit more about this; currently I'm a bit scared\nby %[...%]. But first to catch some sleep..\n\nRené\n"},{"id":"125424","messageId":"7vd44jx9at.fsf@alter.siamese.dyndns.org","threadId":"21257","inReplyTo":"4ADCF117.2030905@lsrfire.ath.cx","subject":"Re: [PATCH 0/3] Generalized \"string function\" syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-19T23:18:02Z","receivedAt":"2009-10-19T23:18:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Junio C Hamano schrieb:\n> ...\n>> I was more worried about painting ourselves now in a corner we cannot get\n>> out of easily later. Even if my answer to question \"what are we going to\n>> add\" may be \"nothing I can think of right now\", it does not make me happy.\n>\n> If wrapping wasn't implemented as a nested function, nesting could still\n> be introduced independently and used for other things -- once these\n> other things arrive.\n\nTrue. I do not think we _need_ nested expansion; obviously we have lived\nwithout it for a long time.\n\n> I'm more in favour of adding ways to customize the shape of the elements\n> rather than adding string functions. %S(width=76,indent=4) over\n> %[wrap(76,4)%s%].\n\nYeah, %X(some modifier) that can apply to any 'X' looks much simpler and\neasier to look at. The way the code is structured currently it might be\nmore work and risk to break things, though.\n"},{"id":"127058","messageId":"4AF6189C.9050500@lsrfire.ath.cx","threadId":"21257","inReplyTo":"7vd44jx9at.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] Generalized \"string function\" syntax","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-11-08T01:02:20Z","receivedAt":"2009-11-08T01:02:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Junio C Hamano schrieb:\n> René Scharfe writes:\n>> I'm more in favour of adding ways to customize the shape of the elements\n>> rather than adding string functions. %S(width=76,indent=4) over\n>> %[wrap(76,4)%s%].\n> \n> Yeah, %X(some modifier) that can apply to any 'X' looks much simpler and\n> easier to look at. The way the code is structured currently it might be\n> more work and risk to break things, though.\n\nHere's something along this line. It's an experiment that tries to\nexplore named parameters and extending individual place holders instead of\nadding a generic modifier (%w).\n\nThe patch implements a way to pass named parameters to %s and %b. Those\nparameters are width, indent1 and indent2, which are then passed to\nstrbuf_add_wrapped_text() to indent and wrap the subject or body text,\nrespectively.\n\nHandling wrapping for the individual place holders avoids having to deal\nwith terminal colour codes. The patch that implements %w() currently in\nnext ignores this issue.\n\nIt also allows to avoid copying the results around -- no temporary strbuf\nis needed for %b(). However, quick tests showed no measurable performance\nimprovement.\n\nWhile indent1 and indent2 are numerical parameters in this patch, they\nreally should be strings, in order to allow indenting using tabs etc..\nFor that to work, strbuf_add_wrapped_text() would need to be changed\nfirst, though.\n\nThe parser parse_params() supports strings and numerical values, but only\nthe latter are used in this patch. String parameters are intended to be\nfed to unquote_c_style(). It never complains about syntax errors. It\naborts if the string ends prematurely, but otherwise ignores invalid input\nand just keeps going. That's how the % place holder parser has always\nworked, but perhaps the introduction of named parameters justifies a more\nstrict approach?\n\n\nMy main motivation for this experiment was to avoid extra copies in the\nhope to speed things up. However, my basic timing tests show that it's\nnot that much of an improvement.\n\nThe remaining reason is the handling of colour codes. I think we can keep\nignoring this issue -- the only impact is that lines with colour codes and\nwrapping combined (e.g. \"%w(72)%Cred%s\") shorter than they should be,\nbecause the colour code is considered (incorrectly) to have a non-zero\nwidth. I think we can get away with mentioning that fact in the docs.\nAfter all, one can simple use \"%Cred%w(72)%s\".\n\nAnyway, here's the patch.\n\n\n pretty.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 150 insertions(+), 4 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex da15cf2..09272ec 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -596,6 +596,117 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)\n \t\tstrbuf_addch(sb, ')');\n }\n \n+struct param {\n+\tconst char *name;\n+\tlong *numeric_value;\n+\tconst char *value;\n+\tsize_t value_len;\n+};\n+\n+static size_t prefixlen(const char *s, const char *prefix)\n+{\n+\tsize_t len = 0;\n+\n+\twhile (*prefix) {\n+\t\tif (*s != *prefix)\n+\t\t\treturn 0;\n+\t\ts++;\n+\t\tprefix++;\n+\t\tlen++;\n+\t}\n+\treturn len;\n+}\n+\n+static size_t parse_params(const char *s, struct param *params, int count,\n+\t\t\t int end)\n+{\n+\tconst char *start = s;\n+\tint i;\n+\n+\tfor (;;) {\n+\t\twhile (isspace(*s))\n+\t\t\ts++;\n+again:\n+\t\tif (*s == end)\n+\t\t\treturn s - start + 1;\n+\t\tif (*s == '\\0')\n+\t\t\treturn 0;\n+\n+\t\tfor (i = 0; i < count; i++) {\n+\t\t\tsize_t len = prefixlen(s, params[i].name);\n+\t\t\tif (len) {\n+\t\t\t\tif (s[len] == end)\n+\t\t\t\t\treturn s - start + 1;\n+\t\t\t\tif (s[len] == '\\0')\n+\t\t\t\t\treturn 0;\n+\t\t\t\tif (isspace(s[len])) {\n+\t\t\t\t\ts += len + 1;\n+\t\t\t\t\twhile (isspace(*s))\n+\t\t\t\t\t\ts++;\n+\t\t\t\t\tif (*s != '=')\n+\t\t\t\t\t\tgoto again;\n+\t\t\t\t\ts++;\n+\t\t\t\t\tbreak;\n+\t\t\t\t} else if (s[len] == '=') {\n+\t\t\t\t\ts += len + 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (i < count) {\n+\t\t\twhile (isspace(*s))\n+\t\t\t\ts++;\n+\t\t\tif (*s == end)\n+\t\t\t\treturn s - start + 1;\n+\t\t\tif (*s == '\\0')\n+\t\t\t\treturn 0;\n+\n+\t\t\tparams[i].value = s;\n+\t\t\tif (*s == '\"') {\n+\t\t\t\tfor (;;) {\n+\t\t\t\t\tint quoted = 0;\n+\t\t\t\t\tconst char *q;\n+\n+\t\t\t\t\ts = strchr(s + 1, '\"');\n+\t\t\t\t\tif (!s)\n+\t\t\t\t\t\treturn 0;\n+\n+\t\t\t\t\tfor (q = s - 1; *q == '\\\\'; q--)\n+\t\t\t\t\t\tquoted = !quoted;\n+\t\t\t\t\tif (!quoted)\n+\t\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\ts++;\n+\t\t\t} else {\n+\t\t\t\tchar *endp;\n+\t\t\t\tlong num = strtol(s, &endp, 10);\n+\n+\t\t\t\ts = endp;\n+\t\t\t\tif (isspace(*s) || *s == end) {\n+\t\t\t\t\tif (params[i].numeric_value)\n+\t\t\t\t\t\t*params[i].numeric_value = num;\n+\t\t\t\t} else {\n+\t\t\t\t\twhile (!isspace(*s) && *s != end) {\n+\t\t\t\t\t\tif (*s == '\\0')\n+\t\t\t\t\t\t\treturn 0;\n+\t\t\t\t\t\ts++;\n+\t\t\t\t\t}\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tparams[i].value_len = s - params[i].value;\n+\t\t} else {\n+\t\t\twhile (!isspace(*s)) {\n+\t\t\t\tif (*s == end)\n+\t\t\t\t\treturn s - start + 1;\n+\t\t\t\tif (*s == '\\0')\n+\t\t\t\t\treturn 0;\n+\t\t\t\ts++;\n+\t\t\t}\n+\t\t}\n+\t}\n+}\n+\n static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n void *context)\n {\n@@ -744,14 +855,49 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n \n \tswitch (placeholder[0]) {\n \tcase 's':\t/* subject */\n-\t\tformat_subject(sb, msg + c->subject_off, \" \");\n-\t\treturn 1;\n+\t\tif (placeholder[1] == '(') {\n+\t\t\tstruct strbuf subject = STRBUF_INIT;\n+\t\t\tlong indent1 = 0, indent2 = 0, width = 0;\n+\t\t\tstruct param params[] = {\n+\t\t\t\t{ \"indent1\", &indent1 },\n+\t\t\t\t{ \"indent2\", &indent2 },\n+\t\t\t\t{ \"width\", &width },\n+\t\t\t};\n+\t\t\tsize_t consumed = parse_params(placeholder + 2, params,\n+\t\t\t\t\t\t ARRAY_SIZE(params), ')');\n+\t\t\tif (!consumed)\n+\t\t\t\treturn 0;\n+\t\t\tformat_subject(&subject, msg + c->subject_off, \" \");\n+\t\t\tstrbuf_add_wrapped_text(sb, subject.buf, indent1,\n+\t\t\t\t\t\tindent2, width);\n+\t\t\tstrbuf_release(&subject);\n+\t\t\treturn consumed + 2;\n+\t\t} else {\n+\t\t\tformat_subject(sb, msg + c->subject_off, \" \");\n+\t\t\treturn 1;\n+\t\t}\n \tcase 'f':\t/* sanitized subject */\n \t\tformat_sanitized_subject(sb, msg + c->subject_off);\n \t\treturn 1;\n \tcase 'b':\t/* body */\n-\t\tstrbuf_addstr(sb, msg + c->body_off);\n-\t\treturn 1;\n+\t\tif (placeholder[1] == '(') {\n+\t\t\tlong indent1 = 0, indent2 = 0, width = 0;\n+\t\t\tstruct param params[] = {\n+\t\t\t\t{ \"indent1\", &indent1 },\n+\t\t\t\t{ \"indent2\", &indent2 },\n+\t\t\t\t{ \"width\", &width },\n+\t\t\t};\n+\t\t\tsize_t consumed = parse_params(placeholder + 2, params,\n+\t\t\t\t\t\t ARRAY_SIZE(params), ')');\n+\t\t\tif (!consumed)\n+\t\t\t\treturn 0;\n+\t\t\tstrbuf_add_wrapped_text(sb, msg + c->body_off, indent1,\n+\t\t\t\t\t\tindent2, width);\n+\t\t\treturn consumed + 2;\n+\t\t} else {\n+\t\t\tstrbuf_addstr(sb, msg + c->body_off);\n+\t\t\treturn 1;\n+\t\t}\n \t}\n \treturn 0;\t/* unknown placeholder */\n }\n"}]}