Den tors 7 feb. 2019 kl 18:29 skrev Junio C Hamano <gitster@xxxxxxxxx>:
>
> matni403@xxxxxxxxx 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
