Re: [PATCH] Pretty-format: Ability to add newline after non-empty string

From: Mats Nilsson <matni403@gmail.com>
Date: Feb 13, 2019, 09:11 UTC
Message-ID: <CAOhgf94JD3w4SedpuSDdEb=rO_oDh+GXLEtdmcGv27NXdDqLmQ@mail.gmail.com>
In-Reply-To: <xmqq36ozqzg8.fsf@gitster-ct.c.googlers.com>

Den tors 7 feb. 2019 kl 18:29 skrev Junio C Hamano <gitster@pobox.com>:

Show 12 quoted lines

>
> 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!

Show 24 quoted lines

> > 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.

Show 12 quoted lines

> 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.

Show 9 quoted lines

> 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

Show 26 quoted lines

> > @@ -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...

Show 8 quoted lines

> > +     test_line_count = 2 actual
> > +'
> > +
> >  test_expect_success '--abbrev' '
> >       echo SHORT SHORT SHORT >expect2 &&
> >       echo LONG LONG LONG >expect3 &&
>
> Thanks.

Kind Regards