Message ID | 20190207063957.11052-1-matni403@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Pretty-format: Ability to add newline after non-empty string | expand |
matni403@gmail.com writes: > Subject: Re: [PATCH] Pretty-format: Ability to add newline after non-empty string Downcasing 'P' and 'A' would make this fit better when it appears in the "git shortlog --no-merges" output, I think. Or perhaps [PATCH] pretty: allow to add LF only after non-empty string or something may fit even better. > diff --git a/pretty.c b/pretty.c > index 0ab45d10d7..fedea05acc 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1457,7 +1457,8 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ > NO_MAGIC, > ADD_LF_BEFORE_NON_EMPTY, > DEL_LF_BEFORE_EMPTY, > - ADD_SP_BEFORE_NON_EMPTY > + ADD_SP_BEFORE_NON_EMPTY, > + ADD_LF_AFTER_NON_EMPTY Appending at the end in this case does not make less sense than inserting at the right place in the middle. Noticing that earlier ones are all about LF, perhaps NO_MAGIC ADD_LF_BEFORE_NON_EMPTY ADD_LF_AFTER_NON_EMPTY DEL_LF_BEFORE_EMPTY ADD_SP_BEFORE_NON_EMPTY may make more sense? An obvious question that would come to the reader's mind would be if we also want ADD_SP_AFTER and DEL_SP_BEFORE for completeness, once the list is ordered in a more logical way like we see above. I am not suggesting to add these two right now, but we still must think about them before adding this new one, because it would affect the choice of the letter used for this new one. It is irresponsible to say "I do not need it, so somebody else can add them later", without making sure that somebody else can later choose sensible letters that make the parallel between those two they are adding with the existing ones. We have '+' and '-' as a matching pair that adds or deletes a LF before the expansion, and we are adding '*' to that group to make the repertoire <add before non-empty, del before empty, add after non-empty>. On the SP side, we currently only use ' ', whose counterpart on the LF side is '+'. Can we easily find counterparts for '-' and this new '*' for the SP side, when we want to add "add after non-empty" and "delete before empty", or would it become easier if we chose something other than '*', and if so what letter? > @@ -1501,7 +1507,8 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder, > { > struct userformat_want *w = context; > > - if (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ') > + if (*placeholder == '+' || *placeholder == '-' || > + *placeholder == ' ' || *placeholder == '*') > placeholder++; > > switch (*placeholder) { > diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh > index da113d975b..e333ed91d3 100755 > --- a/t/t6006-rev-list-format.sh > +++ b/t/t6006-rev-list-format.sh > @@ -445,6 +445,11 @@ test_expect_success 'add SP before non-empty (2)' ' > test $(wc -w <actual) = 6 > ' > > +test_expect_success 'add LF after non-empty' ' > + git show -s --pretty=format:"%s%*sThanks" HEAD^^ >actual && Here the subject is expanded, LF is added, and then the subject is expanded _again_ before 'Thanks'. That does not sound like a realistic example that shows off the situation in which this new feature becomes useful. > + test_line_count = 2 actual > +' > + > test_expect_success '--abbrev' ' > echo SHORT SHORT SHORT >expect2 && > echo LONG LONG LONG >expect3 && Thanks.
Den tors 7 feb. 2019 kl 18:29 skrev Junio C Hamano <gitster@pobox.com>: > > matni403@gmail.com writes: > > > Subject: Re: [PATCH] Pretty-format: Ability to add newline after non-empty string > > Downcasing 'P' and 'A' would make this fit better when it appears in > the "git shortlog --no-merges" output, I think. Or perhaps > > [PATCH] pretty: allow to add LF only after non-empty string > > or something may fit even better. > Will fix this is the updated patch! > > diff --git a/pretty.c b/pretty.c > > index 0ab45d10d7..fedea05acc 100644 > > --- a/pretty.c > > +++ b/pretty.c > > @@ -1457,7 +1457,8 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ > > NO_MAGIC, > > ADD_LF_BEFORE_NON_EMPTY, > > DEL_LF_BEFORE_EMPTY, > > - ADD_SP_BEFORE_NON_EMPTY > > + ADD_SP_BEFORE_NON_EMPTY, > > + ADD_LF_AFTER_NON_EMPTY > > Appending at the end in this case does not make less sense than > inserting at the right place in the middle. Noticing that earlier > ones are all about LF, perhaps > > NO_MAGIC > ADD_LF_BEFORE_NON_EMPTY > ADD_LF_AFTER_NON_EMPTY > DEL_LF_BEFORE_EMPTY > ADD_SP_BEFORE_NON_EMPTY > > may make more sense? > I though a bit about the placement, my reasoning was to have the "before"-magic and then the "after"-magic, but it is an easy change. > An obvious question that would come to the reader's mind would be if > we also want ADD_SP_AFTER and DEL_SP_BEFORE for completeness, once > the list is ordered in a more logical way like we see above. > > I am not suggesting to add these two right now, but we still must > think about them before adding this new one, because it would affect > the choice of the letter used for this new one. It is irresponsible > to say "I do not need it, so somebody else can add them later", > without making sure that somebody else can later choose sensible > letters that make the parallel between those two they are adding > with the existing ones. > I think I could pretty easily implement those two as well while I am doing this, the code is all there already. > We have '+' and '-' as a matching pair that adds or deletes a LF > before the expansion, and we are adding '*' to that group to make > the repertoire <add before non-empty, del before empty, add after > non-empty>. On the SP side, we currently only use ' ', whose > counterpart on the LF side is '+'. Can we easily find counterparts > for '-' and this new '*' for the SP side, when we want to add "add > after non-empty" and "delete before empty", or would it become > easier if we chose something other than '*', and if so what letter? > I don't have a good answer to this. Maybe , and . can be used as a pair? I'm all ears regarding the choice of letter > > @@ -1501,7 +1507,8 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder, > > { > > struct userformat_want *w = context; > > > > - if (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ') > > + if (*placeholder == '+' || *placeholder == '-' || > > + *placeholder == ' ' || *placeholder == '*') > > placeholder++; > > > > switch (*placeholder) { > > diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh > > index da113d975b..e333ed91d3 100755 > > --- a/t/t6006-rev-list-format.sh > > +++ b/t/t6006-rev-list-format.sh > > @@ -445,6 +445,11 @@ test_expect_success 'add SP before non-empty (2)' ' > > test $(wc -w <actual) = 6 > > ' > > > > +test_expect_success 'add LF after non-empty' ' > > + git show -s --pretty=format:"%s%*sThanks" HEAD^^ >actual && > > Here the subject is expanded, LF is added, and then the subject is > expanded _again_ before 'Thanks'. That does not sound like a > realistic example that shows off the situation in which this new > feature becomes useful. > My problem, and why I wanted to add this feature comes from when I googled and found the following stackoverflow question which sums up my problem as well: https://stackoverflow.com/questions/34829747/git-log-pretty-format-newline-after-placeholder-if-non-empty This is the similar to what I wanted for my log, if %d is non-empty, add a newline after the placeholder. But I didn't know have to write a test which would simulate this since all the tests around the previous LFs just did git log, which would make the refs move around? The easiest case I could think of was the to make sure a placeholder added a newline after expansion. Though it worked on my machine, there must be some problem with it since the build failed. https://dev.azure.com/gitgitgadget/git/_build/results?buildId=1082 https://travis-ci.org/mnil/git/builds/489689952 I'm not sure how to debug it when it works on my local setup... > > + test_line_count = 2 actual > > +' > > + > > test_expect_success '--abbrev' ' > > echo SHORT SHORT SHORT >expect2 && > > echo LONG LONG LONG >expect3 && > > Thanks. Kind Regards
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index de6953108c..cddd21e27e 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -236,6 +236,10 @@ If you add a ` ` (space) after '%' of a placeholder, a space is inserted immediately before the expansion if and only if the placeholder expands to a non-empty string. +If you add a '*' (star) after '%' of a placeholder, a line-feed +is added immediately after the expansion if and only if the +placeholder expands to a non-empty string. + * 'tformat:' + The 'tformat:' format works exactly like 'format:', except that it diff --git a/pretty.c b/pretty.c index 0ab45d10d7..fedea05acc 100644 --- a/pretty.c +++ b/pretty.c @@ -1457,7 +1457,8 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ NO_MAGIC, ADD_LF_BEFORE_NON_EMPTY, DEL_LF_BEFORE_EMPTY, - ADD_SP_BEFORE_NON_EMPTY + ADD_SP_BEFORE_NON_EMPTY, + ADD_LF_AFTER_NON_EMPTY } magic = NO_MAGIC; switch (placeholder[0]) { @@ -1470,6 +1471,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ case ' ': magic = ADD_SP_BEFORE_NON_EMPTY; break; + case '*': + magic = ADD_LF_AFTER_NON_EMPTY; + break; default: break; } @@ -1492,6 +1496,8 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ strbuf_insert(sb, orig_len, "\n", 1); else if (magic == ADD_SP_BEFORE_NON_EMPTY) strbuf_insert(sb, orig_len, " ", 1); + else if (magic == ADD_LF_AFTER_NON_EMPTY) + strbuf_addstr(sb, "\n"); } return consumed + 1; } @@ -1501,7 +1507,8 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder, { struct userformat_want *w = context; - if (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ') + if (*placeholder == '+' || *placeholder == '-' || + *placeholder == ' ' || *placeholder == '*') placeholder++; switch (*placeholder) { diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index da113d975b..e333ed91d3 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -445,6 +445,11 @@ test_expect_success 'add SP before non-empty (2)' ' test $(wc -w <actual) = 6 ' +test_expect_success 'add LF after non-empty' ' + git show -s --pretty=format:"%s%*sThanks" HEAD^^ >actual && + test_line_count = 2 actual +' + test_expect_success '--abbrev' ' echo SHORT SHORT SHORT >expect2 && echo LONG LONG LONG >expect3 &&