{"thread":{"id":"50420","subject":"[PATCH] Pretty-format: Ability to add newline after non-empty string","startedAt":"2019-02-07T06:40:11Z","lastAt":"2019-02-13T09:11:11Z","messageCount":3,"participants":["matni403@gmail.com","Junio C Hamano","Mats Nilsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"368667","messageId":"20190207063957.11052-1-matni403@gmail.com","threadId":"50420","inReplyTo":null,"subject":"[PATCH] Pretty-format: Ability to add newline after non-empty string","fromName":"","fromEmail":"matni403@gmail.com","sentAt":"2019-02-07T06:39:57Z","receivedAt":"2019-02-07T06:40:11Z","isPatch":true,"sender":{"key":"matni403@gmail.com","avatar":null},"body":"From: Mats Nilsson <matni403@gmail.com>\n\nThis allows for expansion of %*x to %x followed by a LF if\nand only if %x is non-empty.\n\nSigned-off-by: Mats Nilsson <matni403@gmail.com>\n---\n Documentation/pretty-formats.txt |  4 ++++\n pretty.c                         | 11 +++++++++--\n t/t6006-rev-list-format.sh       |  5 +++++\n 3 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex de6953108c..cddd21e27e 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -236,6 +236,10 @@ If you add a ` ` (space) after '%' of a placeholder, a space\n is inserted immediately before the expansion if and only if the\n placeholder expands to a non-empty string.\n \n+If you add a '*' (star) after '%' of a placeholder, a line-feed\n+is added immediately after the expansion if and only if the\n+placeholder expands to a non-empty string.\n+\n * 'tformat:'\n +\n The 'tformat:' format works exactly like 'format:', except that it\ndiff --git a/pretty.c b/pretty.c\nindex 0ab45d10d7..fedea05acc 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1457,7 +1457,8 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t\tNO_MAGIC,\n \t\tADD_LF_BEFORE_NON_EMPTY,\n \t\tDEL_LF_BEFORE_EMPTY,\n-\t\tADD_SP_BEFORE_NON_EMPTY\n+\t\tADD_SP_BEFORE_NON_EMPTY,\n+\t\tADD_LF_AFTER_NON_EMPTY\n \t} magic = NO_MAGIC;\n \n \tswitch (placeholder[0]) {\n@@ -1470,6 +1471,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \tcase ' ':\n \t\tmagic = ADD_SP_BEFORE_NON_EMPTY;\n \t\tbreak;\n+\tcase '*':\n+\t\tmagic = ADD_LF_AFTER_NON_EMPTY;\n+\t\tbreak;\n \tdefault:\n \t\tbreak;\n \t}\n@@ -1492,6 +1496,8 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t\t\tstrbuf_insert(sb, orig_len, \"\\n\", 1);\n \t\telse if (magic == ADD_SP_BEFORE_NON_EMPTY)\n \t\t\tstrbuf_insert(sb, orig_len, \" \", 1);\n+\t\telse if (magic == ADD_LF_AFTER_NON_EMPTY)\n+\t\t\tstrbuf_addstr(sb, \"\\n\");\n \t}\n \treturn consumed + 1;\n }\n@@ -1501,7 +1507,8 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n {\n \tstruct userformat_want *w = context;\n \n-\tif (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ')\n+\tif (*placeholder == '+' || *placeholder == '-' ||\n+\t\t*placeholder == ' ' || *placeholder == '*')\n \t\tplaceholder++;\n \n \tswitch (*placeholder) {\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex da113d975b..e333ed91d3 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -445,6 +445,11 @@ test_expect_success 'add SP before non-empty (2)' '\n \ttest $(wc -w <actual) = 6\n '\n \n+test_expect_success 'add LF after non-empty' '\n+\tgit show -s --pretty=format:\"%s%*sThanks\" HEAD^^ >actual &&\n+\ttest_line_count = 2 actual\n+'\n+\n test_expect_success '--abbrev' '\n \techo SHORT SHORT SHORT >expect2 &&\n \techo LONG LONG LONG >expect3 &&\n-- \n2.14.1.windows.1\n\n"},{"id":"368711","messageId":"xmqq36ozqzg8.fsf@gitster-ct.c.googlers.com","threadId":"50420","inReplyTo":"20190207063957.11052-1-matni403@gmail.com","subject":"Re: [PATCH] Pretty-format: Ability to add newline after non-empty string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-07T17:29:43Z","receivedAt":"2019-02-07T17:29:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"matni403@gmail.com writes:\n\n> Subject: Re: [PATCH] Pretty-format: Ability to add newline after non-empty string\n\nDowncasing 'P' and 'A' would make this fit better when it appears in\nthe \"git shortlog --no-merges\" output, I think.  Or perhaps\n\n\t[PATCH] pretty: allow to add LF only after non-empty string\n\nor something may fit even better.\n\n> diff --git a/pretty.c b/pretty.c\n> index 0ab45d10d7..fedea05acc 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1457,7 +1457,8 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n>  \t\tNO_MAGIC,\n>  \t\tADD_LF_BEFORE_NON_EMPTY,\n>  \t\tDEL_LF_BEFORE_EMPTY,\n> -\t\tADD_SP_BEFORE_NON_EMPTY\n> +\t\tADD_SP_BEFORE_NON_EMPTY,\n> +\t\tADD_LF_AFTER_NON_EMPTY\n\nAppending at the end in this case does not make less sense than\ninserting at the right place in the middle.  Noticing that earlier\nones are all about LF, perhaps\n\n\tNO_MAGIC\n\tADD_LF_BEFORE_NON_EMPTY\n\tADD_LF_AFTER_NON_EMPTY\n\tDEL_LF_BEFORE_EMPTY\n\tADD_SP_BEFORE_NON_EMPTY\n\nmay make more sense?  \n\nAn obvious question that would come to the reader's mind would be if\nwe also want ADD_SP_AFTER and DEL_SP_BEFORE for completeness, once\nthe list is ordered in a more logical way like we see above.\n\nI am not suggesting to add these two right now, but we still must\nthink about them before adding this new one, because it would affect\nthe choice of the letter used for this new one.  It is irresponsible\nto say \"I do not need it, so somebody else can add them later\",\nwithout making sure that somebody else can later choose sensible\nletters that make the parallel between those two they are adding\nwith the existing ones.\n\nWe have '+' and '-' as a matching pair that adds or deletes a LF\nbefore the expansion, and we are adding '*' to that group to make\nthe repertoire <add before non-empty, del before empty, add after\nnon-empty>.  On the SP side, we currently only use ' ', whose\ncounterpart on the LF side is '+'.  Can we easily find counterparts\nfor '-' and this new '*' for the SP side, when we want to add \"add\nafter non-empty\" and \"delete before empty\", or would it become\neasier if we chose something other than '*', and if so what letter?\n\n> @@ -1501,7 +1507,8 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n>  {\n>  \tstruct userformat_want *w = context;\n>  \n> -\tif (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ')\n> +\tif (*placeholder == '+' || *placeholder == '-' ||\n> +\t\t*placeholder == ' ' || *placeholder == '*')\n>  \t\tplaceholder++;\n>  \n>  \tswitch (*placeholder) {\n> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\n> index da113d975b..e333ed91d3 100755\n> --- a/t/t6006-rev-list-format.sh\n> +++ b/t/t6006-rev-list-format.sh\n> @@ -445,6 +445,11 @@ test_expect_success 'add SP before non-empty (2)' '\n>  \ttest $(wc -w <actual) = 6\n>  '\n>  \n> +test_expect_success 'add LF after non-empty' '\n> +\tgit show -s --pretty=format:\"%s%*sThanks\" HEAD^^ >actual &&\n\nHere the subject is expanded, LF is added, and then the subject is\nexpanded _again_ before 'Thanks'.  That does not sound like a\nrealistic example that shows off the situation in which this new\nfeature becomes useful.\n\n> +\ttest_line_count = 2 actual\n> +'\n> +\n>  test_expect_success '--abbrev' '\n>  \techo SHORT SHORT SHORT >expect2 &&\n>  \techo LONG LONG LONG >expect3 &&\n\nThanks.\n"},{"id":"369211","messageId":"CAOhgf94JD3w4SedpuSDdEb=rO_oDh+GXLEtdmcGv27NXdDqLmQ@mail.gmail.com","threadId":"50420","inReplyTo":"xmqq36ozqzg8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Pretty-format: Ability to add newline after non-empty string","fromName":"Mats Nilsson","fromEmail":"matni403@gmail.com","sentAt":"2019-02-13T09:11:00Z","receivedAt":"2019-02-13T09:11:11Z","isPatch":true,"sender":{"key":"matni403@gmail.com","avatar":null},"body":"Den tors 7 feb. 2019 kl 18:29 skrev Junio C Hamano <gitster@pobox.com>:\n>\n> matni403@gmail.com writes:\n>\n> > Subject: Re: [PATCH] Pretty-format: Ability to add newline after non-empty string\n>\n> Downcasing 'P' and 'A' would make this fit better when it appears in\n> the \"git shortlog --no-merges\" output, I think.  Or perhaps\n>\n>         [PATCH] pretty: allow to add LF only after non-empty string\n>\n> or something may fit even better.\n>\n\nWill fix this is the updated patch!\n\n> > diff --git a/pretty.c b/pretty.c\n> > index 0ab45d10d7..fedea05acc 100644\n> > --- a/pretty.c\n> > +++ b/pretty.c\n> > @@ -1457,7 +1457,8 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n> >               NO_MAGIC,\n> >               ADD_LF_BEFORE_NON_EMPTY,\n> >               DEL_LF_BEFORE_EMPTY,\n> > -             ADD_SP_BEFORE_NON_EMPTY\n> > +             ADD_SP_BEFORE_NON_EMPTY,\n> > +             ADD_LF_AFTER_NON_EMPTY\n>\n> Appending at the end in this case does not make less sense than\n> inserting at the right place in the middle.  Noticing that earlier\n> ones are all about LF, perhaps\n>\n>         NO_MAGIC\n>         ADD_LF_BEFORE_NON_EMPTY\n>         ADD_LF_AFTER_NON_EMPTY\n>         DEL_LF_BEFORE_EMPTY\n>         ADD_SP_BEFORE_NON_EMPTY\n>\n> may make more sense?\n>\n\nI though a bit about the placement, my reasoning was to have the\n\"before\"-magic and then the \"after\"-magic, but it is an easy change.\n\n> An obvious question that would come to the reader's mind would be if\n> we also want ADD_SP_AFTER and DEL_SP_BEFORE for completeness, once\n> the list is ordered in a more logical way like we see above.\n>\n> I am not suggesting to add these two right now, but we still must\n> think about them before adding this new one, because it would affect\n> the choice of the letter used for this new one.  It is irresponsible\n> to say \"I do not need it, so somebody else can add them later\",\n> without making sure that somebody else can later choose sensible\n> letters that make the parallel between those two they are adding\n> with the existing ones.\n>\n\nI think I could pretty easily implement those two as well while I am\ndoing this, the code is all there already.\n\n> We have '+' and '-' as a matching pair that adds or deletes a LF\n> before the expansion, and we are adding '*' to that group to make\n> the repertoire <add before non-empty, del before empty, add after\n> non-empty>.  On the SP side, we currently only use ' ', whose\n> counterpart on the LF side is '+'.  Can we easily find counterparts\n> for '-' and this new '*' for the SP side, when we want to add \"add\n> after non-empty\" and \"delete before empty\", or would it become\n> easier if we chose something other than '*', and if so what letter?\n>\n\nI don't have a good answer to this. Maybe , and . can be used as a pair?\nI'm all ears regarding the choice of letter\n\n> > @@ -1501,7 +1507,8 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n> >  {\n> >       struct userformat_want *w = context;\n> >\n> > -     if (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ')\n> > +     if (*placeholder == '+' || *placeholder == '-' ||\n> > +             *placeholder == ' ' || *placeholder == '*')\n> >               placeholder++;\n> >\n> >       switch (*placeholder) {\n> > diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\n> > index da113d975b..e333ed91d3 100755\n> > --- a/t/t6006-rev-list-format.sh\n> > +++ b/t/t6006-rev-list-format.sh\n> > @@ -445,6 +445,11 @@ test_expect_success 'add SP before non-empty (2)' '\n> >       test $(wc -w <actual) = 6\n> >  '\n> >\n> > +test_expect_success 'add LF after non-empty' '\n> > +     git show -s --pretty=format:\"%s%*sThanks\" HEAD^^ >actual &&\n>\n> Here the subject is expanded, LF is added, and then the subject is\n> expanded _again_ before 'Thanks'.  That does not sound like a\n> realistic example that shows off the situation in which this new\n> feature becomes useful.\n>\n\nMy problem, and why I wanted to add this feature comes from when I googled\nand found the following stackoverflow question which sums up my problem as well:\nhttps://stackoverflow.com/questions/34829747/git-log-pretty-format-newline-after-placeholder-if-non-empty\nThis is the similar to what I wanted for my log, if %d is non-empty,\nadd a newline after the placeholder.\nBut I didn't know have to write a test which would simulate this since\nall the tests around the previous LFs\njust did git log, which would make the refs move around?\n\nThe easiest case I could think of was the to make sure a placeholder\nadded a newline after expansion.\nThough it worked on my machine, there must be some problem with it\nsince the build failed.\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=1082\nhttps://travis-ci.org/mnil/git/builds/489689952\nI'm not sure how to debug it when it works on my local setup...\n\n> > +     test_line_count = 2 actual\n> > +'\n> > +\n> >  test_expect_success '--abbrev' '\n> >       echo SHORT SHORT SHORT >expect2 &&\n> >       echo LONG LONG LONG >expect3 &&\n>\n> Thanks.\n\nKind Regards\n"}]}