Re: [PATCH] Pretty-format: Ability to add newline after non-empty string
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 7, 2019, 17:29 UTC
- Message-ID
- <xmqq36ozqzg8.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190207063957.11052-1-matni403@gmail.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.
Show 11 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?
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?
Show 20 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.
Show 6 quoted lines
> + test_line_count = 2 actual > +' > + > test_expect_success '--abbrev' ' > echo SHORT SHORT SHORT >expect2 && > echo LONG LONG LONG >expect3 &&
Thanks.