threads / patch / 63517

patchnotes: remove trailing whitespace from editor template

Subject: [PATCH] notes: remove trailing whitespace from editor template

## tl;dr

17 messages between May 24, 2025 and Jun 10, 2025. Diffs are folded; open one to read it.

replies: 16people: 6as markdown or json

kristofferhaugsbakk@fastmail.com· May 24, 2025, 21:35 UTC · lore
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
The editor template for editing a note consists of the commented block:
    git show --stat --no-notes <object>

The indented commit message will introduce trailing whitespace for paragraph breaks (blank lines). Some editors will highlight those lines as an error immediately when you open the editor.

Let’s strip all unnecessary whitespace from the template to avoid that very small problem.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 builtin/notes.c  | 2 ++
 t/t3301-notes.sh | 6 ++++++
 2 files changed, 8 insertions(+)
Show changes to 2 files +8 −0

builtin/notes.c, t/t3301-notes.sh

diff --git a/builtin/notes.c b/builtin/notes.c
index a3f433ca4c0..ca4782eca19 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -180,6 +180,8 @@ static void write_commented_object(int fd, const struct object_id *object)
 	if (strbuf_read(&buf, show.out, 0) < 0)
 		die_errno(_("could not read 'show' output"));
 	strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
+	/* strip trailing whitespace introduced by blank lines */
+	strbuf_stripspace(&cbuf, NULL);
 	write_or_die(fd, cbuf.buf, cbuf.len);
 
 	strbuf_release(&cbuf);
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index d6c50460d08..70a21be54fc 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1629,4 +1629,10 @@ test_expect_success 'git notes append aborts when editor fails with -e' '
 	test_must_fail git notes show
 '
 
+test_expect_success 'git notes add has no trailing whitespace in the editor template' '
+	test_commit --signoff 23rd &&
+	GIT_EDITOR="cat >actual" git notes add &&
+	test_grep ! " $" actual
+'
+
 test_done

base-commit: 8613c2bb6cd16ef530dc5dd74d3b818a1ccbf1c0

-- 
2.49.0.780.g892193c3f50
Kristoffer Haugsbakk· May 25, 2025, 20:46 UTC · re: kristofferhaugsbakk@fastmail.com · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

On Sat, May 24, 2025, at 23:35, kristofferhaugsbakk@fastmail.com wrote:
Show 6 quoted lines
> +	test_commit --signoff 23rd &&
> +	GIT_EDITOR="cat >actual" git notes add &&
> +	test_grep ! " $" actual
> +'
> +
>  test_done
Or alternatively I could assert on the whole template:
```
test_expect_success 'git notes add editor template' '
	test_commit --signoff 23rd &&
	cat <<-EOF >expect &&
	#
	# Write/edit the notes for the following object:
	#
	$(git show --stat --no-notes 23rd |
		    git stripspace |
		    git stripspace --comment-lines)
	EOF
	GIT_EDITOR="cat >actual" git notes add &&
	test_cmp expect actual
'
```
-- 
Kristoffer
Phillip Wood· May 26, 2025, 14:01 UTC · re: kristofferhaugsbakk@fastmail.com · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

Hi Kristoffer
On 24/05/2025 22:35, kristofferhaugsbakk@fastmail.com wrote:
Show 11 quoted lines
> 
> diff --git a/builtin/notes.c b/builtin/notes.c
> index a3f433ca4c0..ca4782eca19 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -180,6 +180,8 @@ static void write_commented_object(int fd, const struct object_id *object)
>   	if (strbuf_read(&buf, show.out, 0) < 0)
>   		die_errno(_("could not read 'show' output"));
>   	strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
> +	/* strip trailing whitespace introduced by blank lines */
> +	strbuf_stripspace(&cbuf, NULL);

It doesn't make any difference at the moment but I'd be happier if we stripped the trailing space from the commit message before commenting it out. That way we know we are only stripping space from the indented lines produced by "git show". If in the future this function were to start appending the commented log message to a buffer passed in by the caller rather than a file passed by the caller we wont mess up the rest of the buffer content.

>   	write_or_die(fd, cbuf.buf, cbuf.len);
 > [...]> +test_expect_success 'git notes add has no trailing whitespace 
in the editor template' '
> +	test_commit --signoff 23rd &&
> +	GIT_EDITOR="cat >actual" git notes add &&
> +	test_grep ! " $" actual

Should that be " \$"? What you've got seems to work with dash but I'm not sure if it is POSIX compliant or not.

Best Wishes
Phillip
Kristoffer Haugsbakk· May 26, 2025, 19:44 UTC · re: Phillip Wood · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

Hi there Phillip
On Mon, May 26, 2025, at 16:01, Phillip Wood wrote:
Show 18 quoted lines
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index a3f433ca4c0..ca4782eca19 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -180,6 +180,8 @@ static void write_commented_object(int fd, const struct object_id *object)
>>   	if (strbuf_read(&buf, show.out, 0) < 0)
>>   		die_errno(_("could not read 'show' output"));
>>   	strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
>> +	/* strip trailing whitespace introduced by blank lines */
>> +	strbuf_stripspace(&cbuf, NULL);
>
> It doesn't make any difference at the moment but I'd be happier if we
> stripped the trailing space from the commit message before commenting it
> out. That way we know we are only stripping space from the indented
> lines produced by "git show". If in the future this function were to
> start appending the commented log message to a buffer passed in by the
> caller rather than a file passed by the caller we wont mess up the rest
> of the buffer content.
Do you mean doing the operation on the output buffer instead?:
	if (strbuf_read(&buf, show.out, 0) < 0)
		die_errno(_("could not read 'show' output"));
	/* strip trailing whitespace introduced by blank lines */
	strbuf_stripspace(&buf, NULL);
	strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
	write_or_die(fd, cbuf.buf, cbuf.len);

I think that’s cleaner. But I don’t see how it makes the code more future-proof.

Show 10 quoted lines
>
>>   	write_or_die(fd, cbuf.buf, cbuf.len);
>  > [...]> +test_expect_success 'git notes add has no trailing whitespace
> in the editor template' '
>> +	test_commit --signoff 23rd &&
>> +	GIT_EDITOR="cat >actual" git notes add &&
>> +	test_grep ! " $" actual
>
> Should that be " \$"? What you've got seems to work with dash but I'm
> not sure if it is POSIX compliant or not.

`$` is the anchor metacharacter in this context (end of string) according to Posix.

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html#tag_09_03
    $
        The <dollar-sign> shall be special when used as an anchor.
Phillip Wood· May 27, 2025, 08:24 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

Hi Kristoffer
On 26/05/2025 20:44, Kristoffer Haugsbakk wrote:
Show 31 quoted lines
> On Mon, May 26, 2025, at 16:01, Phillip Wood wrote:
>>> diff --git a/builtin/notes.c b/builtin/notes.c
>>> index a3f433ca4c0..ca4782eca19 100644
>>> --- a/builtin/notes.c
>>> +++ b/builtin/notes.c
>>> @@ -180,6 +180,8 @@ static void write_commented_object(int fd, const struct object_id *object)
>>>    	if (strbuf_read(&buf, show.out, 0) < 0)
>>>    		die_errno(_("could not read 'show' output"));
>>>    	strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
>>> +	/* strip trailing whitespace introduced by blank lines */
>>> +	strbuf_stripspace(&cbuf, NULL);
>>
>> It doesn't make any difference at the moment but I'd be happier if we
>> stripped the trailing space from the commit message before commenting it
>> out. That way we know we are only stripping space from the indented
>> lines produced by "git show". If in the future this function were to
>> start appending the commented log message to a buffer passed in by the
>> caller rather than a file passed by the caller we wont mess up the rest
>> of the buffer content.
> 
> Do you mean doing the operation on the output buffer instead?:
> 
> 	if (strbuf_read(&buf, show.out, 0) < 0)
> 		die_errno(_("could not read 'show' output"));
> 	/* strip trailing whitespace introduced by blank lines */
> 	strbuf_stripspace(&buf, NULL);
> 	strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
> 	write_or_die(fd, cbuf.buf, cbuf.len);
> 
> I think that’s cleaner.  But I don’t see how it makes the code more
> future-proof.

Because it is now stripping buf and not cbuf. If in the future we decide to build the message in a buffer rather than writing it piecemeal to disk we would change signature of this function to take an strbuf instead of a file descriptor and use the buffer provided by the caller instead of cbuf. If we were to strip cbuf then a naive conversion would end up stripping the buffer passed by caller, not just the output from "git show". Various git notes subcommands have a --stripspace option and calling strbuf_stripspace() on the caller provided buffer would break that.

Show 12 quoted lines
>>>    	write_or_die(fd, cbuf.buf, cbuf.len);
>>   > [...]> +test_expect_success 'git notes add has no trailing whitespace
>> in the editor template' '
>>> +	test_commit --signoff 23rd &&
>>> +	GIT_EDITOR="cat >actual" git notes add &&
>>> +	test_grep ! " $" actual
>>
>> Should that be " \$"? What you've got seems to work with dash but I'm
>> not sure if it is POSIX compliant or not.
> 
> `$` is the anchor metacharacter in this context (end of string)
> according to Posix.

Right but what does the shell do to that '$'? It is not escaped and inside a double quoted string.

Best Wishes
Phillip
Kristoffer Haugsbakk· May 27, 2025, 16:11 UTC · re: Phillip Wood · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

On Tue, May 27, 2025, at 10:24, Phillip Wood wrote:
Show 20 quoted lines
>> Do you mean doing the operation on the output buffer instead?:
>>
>> 	if (strbuf_read(&buf, show.out, 0) < 0)
>> 		die_errno(_("could not read 'show' output"));
>> 	/* strip trailing whitespace introduced by blank lines */
>> 	strbuf_stripspace(&buf, NULL);
>> 	strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
>> 	write_or_die(fd, cbuf.buf, cbuf.len);
>>
>> I think that’s cleaner.  But I don’t see how it makes the code more
>> future-proof.
>
> Because it is now stripping buf and not cbuf. If in the future we decide
> to build the message in a buffer rather than writing it piecemeal to
> disk we would change signature of this function to take an strbuf
> instead of a file descriptor and use the buffer provided by the caller
> instead of cbuf. If we were to strip cbuf then a naive conversion would
> end up stripping the buffer passed by caller, not just the output from
> "git show". Various git notes subcommands have a --stripspace option and
> calling strbuf_stripspace() on the caller provided buffer would break that.
So stripping `buf` is what I should do?
Show 8 quoted lines
>>> Should that be " \$"? What you've got seems to work with dash but I'm
>>> not sure if it is POSIX compliant or not.
>>
>> `$` is the anchor metacharacter in this context (end of string)
>> according to Posix.
>
> Right but what does the shell do to that '$'? It is not escaped and
> inside a double quoted string.
Oh right, it’s about the shell.  I’ll fix it.
Thanks for spotting.
Junio C Hamano· May 27, 2025, 17:18 UTC · re: Phillip Wood · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 17 quoted lines
> Hi Kristoffer
>
> On 24/05/2025 22:35, kristofferhaugsbakk@fastmail.com wrote:
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index a3f433ca4c0..ca4782eca19 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -180,6 +180,8 @@ static void write_commented_object(int fd, const struct object_id *object)
>>   	if (strbuf_read(&buf, show.out, 0) < 0)
>>   		die_errno(_("could not read 'show' output"));
>>   	strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
>> +	/* strip trailing whitespace introduced by blank lines */
>> +	strbuf_stripspace(&cbuf, NULL);
>
> It doesn't make any difference at the moment but I'd be happier if we
> stripped the trailing space from the commit message before commenting
> it out.

Yes. I had the same thought. If Kristof does not like the fact that one automated source of information consistently indents its output lines, even an empty one, and if users may have legitimate reason to place in the final output a trailing whitespace in the comment, it is better for the patch not to close the door to the others.

In this case I am not all that sympathetic to the idea of the patch. The consistently indented lines makes it more clear from which line to which line came from a commit log message; running stripspace would break them into paragraph pieces. These editors that complain probaly can be fixed?

Alternatively, if it bothers users of certain editing environments too much, perhaps the indent code in the output phase of "git show" should lose the indents for empty lines uniformly, shoudln't it? It probably should be a fairly isolated change, like the way how the expand_tabs_in_log bit is handled in pretty.c; give another bit and teach pp_handle_indent to return when that bit is set and the payload it was asked to show with indentation is empty, or something like that.

> Should that be " \$"? What you've got seems to work with dash but I'm
> not sure if it is POSIX compliant or not.
"2.6 Word Expansions" ends with this sentence:
    If a '$' that is neither within single-quotes nor escaped by a
    <backslash> is immediately followed by a <space>, <tab>, or a
    <newline>, or is not followed by any character, the '$' shall be
    treated as a literal character.

Taken together with "2.2.3 Double-Quotes", I'd read it as blessing a lone '$' at the end of double-quoted string as a literal dollar sign.

Thanks.
Eric Sunshine· May 27, 2025, 17:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

On Tue, May 27, 2025 at 1:19 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
> > Should that be " \$"? What you've got seems to work with dash but I'm
> > not sure if it is POSIX compliant or not.
>
> "2.6 Word Expansions" ends with this sentence:
>
>     If a '$' that is neither within single-quotes nor escaped by a
>     <backslash> is immediately followed by a <space>, <tab>, or a
>     <newline>, or is not followed by any character, the '$' shall be
>     treated as a literal character.
>
> Taken together with "2.2.3 Double-Quotes", I'd read it as blessing a
> lone '$' at the end of double-quoted string as a literal dollar sign.

Thanks for finding and posting this. I had read the POSIX shell language specification pretty closely when writing the lexer and parser for chainlint.pl and remembered the above statement, and had wanted to cite it after reading Phillip's initial reply but didn't manage to find it again in the limited time I had available for searching through the documentation.

Kristoffer Haugsbakk· May 27, 2025, 21:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

On Tue, May 27, 2025, at 19:18, Junio C Hamano wrote:
Show 8 quoted lines
> Alternatively, if it bothers users of certain editing environments
> too much, perhaps the indent code in the output phase of "git show"
> should lose the indents for empty lines uniformly, shoudln't it?  It
> probably should be a fairly isolated change, like the way how the
> expand_tabs_in_log bit is handled in pretty.c; give another bit and
> teach pp_handle_indent to return when that bit is set and the
> payload it was asked to show with indentation is empty, or something
> like that.
That sounds scary. I’ll make do without.
-- 
Kristoffer Haugsbakk
Junio C Hamano· May 27, 2025, 22:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> It probably should be a fairly isolated change, like the way how
> the expand_tabs_in_log bit is handled in pretty.c; give another
> bit and teach pp_handle_indent to return when that bit is set and
> the payload it was asked to show with indentation is empty, or
> something like that.

An unconditional change based on the above idea may look like the attached patch.

No tests, no docs, no configuration, and I will not be working on this in immediate future. On top of what we can see here, probably the following would need to happen before we can call it completed.

 - "git notes" should take a command line option to control which
   one of "--indent-empty-lines" or "--no-indent-empty-lines" is
   passed.  Optionally, we can introduce a new configuration
   variable notes.something to control the same.
 - "git log" and "git show", but not "git rev-list" plumbing, may
   want to start paying attention to a new configuration variable
   log.indentEmptyLines (which defaults to "yes" if not specified).
   I am not sure if we want to do this item; as there is no explicit
   end-user request to add such a feature, I am inclined to say no,
   at least not as a part of the "while editing notes with 'git
   notes add -e' the shortlog ends up with lines that are commented
   out with trailing whitespaces, which upsets some editors" topic.
   But it should be trivial to do.
 - documentation.
 - tests.
I've only compiled and then manually tested
    $ GIT_EXEC_PATH=$(pwd) ./git notes add -e

to see how the commented out "git show -s" output embedded in the editor buffer looked like.

 builtin/log.c   | 1 +
 builtin/notes.c | 3 ++-
 log-tree.c      | 1 +
 pretty.c        | 8 +++++---
 pretty.h        | 1 +
 revision.c      | 4 ++++
 revision.h      | 2 ++
 7 files changed, 16 insertions(+), 4 deletions(-)
Show changes to diff +16 −4
diff --git c/builtin/log.c w/builtin/log.c
index b450cd3bde..dc2d7d3a07 100644
--- c/builtin/log.c
+++ w/builtin/log.c
@@ -2148,6 +2148,7 @@ int cmd_format_patch(int argc,
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.encode_email_headers = cfg.log.default_encode_email_headers;
 	rev.expand_tabs_in_log_default = 0;
+	rev.indent_empty_lines = 1;
 	rev.verbose_header = 1;
 	rev.diff = 1;
 	rev.max_parents = 1;
diff --git c/builtin/notes.c w/builtin/notes.c
index a3f433ca4c..b559b39cf0 100644
--- c/builtin/notes.c
+++ w/builtin/notes.c
@@ -167,7 +167,8 @@ static void write_commented_object(int fd, const struct object_id *object)
 	struct strbuf cbuf = STRBUF_INIT;
 
 	/* Invoke "git show --stat --no-notes $object" */
-	strvec_pushl(&show.args, "show", "--stat", "--no-notes",
+	strvec_pushl(&show.args, "show", "--stat",
+		     "--no-indent-empty-lines", "--no-notes",
 		     oid_to_hex(object), NULL);
 	show.no_stdin = 1;
 	show.out = -1;
diff --git c/log-tree.c w/log-tree.c
index 1d05dc1c70..e8ea2481ea 100644
--- c/log-tree.c
+++ w/log-tree.c
@@ -879,6 +879,7 @@ void show_log(struct rev_info *opt)
 	ctx.mailmap = opt->mailmap;
 	ctx.color = opt->diffopt.use_color;
 	ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
+	ctx.indent_empty_lines = opt->indent_empty_lines;
 	ctx.output_encoding = get_log_output_encoding();
 	ctx.rev = opt;
 	if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
diff --git c/pretty.c w/pretty.c
index 0bc8ad8a9a..4974af4d02 100644
--- c/pretty.c
+++ w/pretty.c
@@ -2247,13 +2247,14 @@ void pp_remainder(struct pretty_print_context *pp,
 
 	for (;;) {
 		const char *line = *msg_p;
+		int is_blank = 0;
 		int linelen = get_one_line(line);
 		*msg_p += linelen;
 
 		if (!linelen)
 			break;
-
-		if (is_blank_line(line, &linelen)) {
+		is_blank = is_blank_line(line, &linelen);
+		if (is_blank) {
 			if (first)
 				continue;
 			if (pp->fmt == CMIT_FMT_SHORT)
@@ -2262,7 +2263,8 @@ void pp_remainder(struct pretty_print_context *pp,
 		first = 0;
 
 		strbuf_grow(sb, linelen + indent + 20);
-		if (indent)
+
+		if (indent && (!is_blank || pp->indent_empty_lines))
 			pp_handle_indent(pp, sb, indent, line, linelen);
 		else if (pp->expand_tabs_in_log)
 			strbuf_add_tabexpand(sb, opt, pp->color,
diff --git c/pretty.h w/pretty.h
index df267afe4a..21e584e185 100644
--- c/pretty.h
+++ w/pretty.h
@@ -40,6 +40,7 @@ struct pretty_print_context {
 	struct date_mode date_mode;
 	unsigned date_mode_explicit:1;
 	int expand_tabs_in_log;
+	int indent_empty_lines;
 	int need_8bit_cte;
 	char *notes_message;
 	struct reflog_walk_info *reflog_info;
diff --git c/revision.c w/revision.c
index 2c36a9c179..ae6b98e701 100644
--- c/revision.c
+++ w/revision.c
@@ -2564,6 +2564,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(optarg, revs);
+	} else if (!strcmp(arg, "--no-indent-empty-lines")) {
+		revs->indent_empty_lines = 0;
+	} else if (!strcmp(arg, "--indent-empty-lines")) {
+		revs->indent_empty_lines = 1;
 	} else if (!strcmp(arg, "--expand-tabs")) {
 		revs->expand_tabs_in_log = 8;
 	} else if (!strcmp(arg, "--no-expand-tabs")) {
diff --git c/revision.h w/revision.h
index 6d369cdad6..49b123387f 100644
--- c/revision.h
+++ w/revision.h
@@ -278,6 +278,7 @@ struct rev_info {
 	struct date_mode date_mode;
 	int		expand_tabs_in_log; /* unset if negative */
 	int		expand_tabs_in_log_default;
+	int		indent_empty_lines;
 
 	unsigned int	abbrev;
 	enum cmit_fmt	commit_format;
@@ -412,6 +413,7 @@ struct rev_info {
 	.expand_tabs_in_log = -1, \
 	.commit_format = CMIT_FMT_DEFAULT, \
 	.expand_tabs_in_log_default = 8, \
+	.indent_empty_lines = 1, \
 }
 
 /**
Phillip Wood· Jun 2, 2025, 13:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

Hi Junio
On 27/05/2025 18:18, Junio C Hamano wrote:
Show 14 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> Should that be " \$"? What you've got seems to work with dash but I'm
>> not sure if it is POSIX compliant or not.
> 
> "2.6 Word Expansions" ends with this sentence:
> 
>      If a '$' that is neither within single-quotes nor escaped by a
>      <backslash> is immediately followed by a <space>, <tab>, or a
>      <newline>, or is not followed by any character, the '$' shall be
>      treated as a literal character.
> 
> Taken together with "2.2.3 Double-Quotes", I'd read it as blessing a
> lone '$' at the end of double-quoted string as a literal dollar sign.

Thanks for finding that - TIL. I agree with your interpretation that the original is in fact safe.

Thanks
Phillip
D. Ben Knoble· Jun 3, 2025, 20:37 UTC · re: Junio C Hamano · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

On Tue, May 27, 2025 at 1:31 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 33 quoted lines
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Hi Kristoffer
> >
> > On 24/05/2025 22:35, kristofferhaugsbakk@fastmail.com wrote:
> >> diff --git a/builtin/notes.c b/builtin/notes.c
> >> index a3f433ca4c0..ca4782eca19 100644
> >> --- a/builtin/notes.c
> >> +++ b/builtin/notes.c
> >> @@ -180,6 +180,8 @@ static void write_commented_object(int fd, const struct object_id *object)
> >>      if (strbuf_read(&buf, show.out, 0) < 0)
> >>              die_errno(_("could not read 'show' output"));
> >>      strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
> >> +    /* strip trailing whitespace introduced by blank lines */
> >> +    strbuf_stripspace(&cbuf, NULL);
> >
> > It doesn't make any difference at the moment but I'd be happier if we
> > stripped the trailing space from the commit message before commenting
> > it out.
>
> Yes.  I had the same thought.  If Kristof does not like the fact
> that one automated source of information consistently indents its
> output lines, even an empty one, and if users may have legitimate
> reason to place in the final output a trailing whitespace in the
> comment, it is better for the patch not to close the door to the
> others.
>
> In this case I am not all that sympathetic to the idea of the patch.
> The consistently indented lines makes it more clear from which line
> to which line came from a commit log message; running stripspace
> would break them into paragraph pieces.  These editors that complain
> probaly can be fixed?

My editor doesn't complain, but it does highlight trailing whitespace at my behest, and it tends to be an eyesore (on purpose: that way I clean it up). Perhaps Kistoffer is coming from a similar place?

Show 9 quoted lines
>
> Alternatively, if it bothers users of certain editing environments
> too much, perhaps the indent code in the output phase of "git show"
> should lose the indents for empty lines uniformly, shoudln't it?  It
> probably should be a fairly isolated change, like the way how the
> expand_tabs_in_log bit is handled in pretty.c; give another bit and
> teach pp_handle_indent to return when that bit is set and the
> payload it was asked to show with indentation is empty, or something
> like that.

I think this suggestion would also help folks who "git commit -v," which IIRC is also indented in the template.

Show 16 quoted lines
>
> > Should that be " \$"? What you've got seems to work with dash but I'm
> > not sure if it is POSIX compliant or not.
>
> "2.6 Word Expansions" ends with this sentence:
>
>     If a '$' that is neither within single-quotes nor escaped by a
>     <backslash> is immediately followed by a <space>, <tab>, or a
>     <newline>, or is not followed by any character, the '$' shall be
>     treated as a literal character.
>
> Taken together with "2.2.3 Double-Quotes", I'd read it as blessing a
> lone '$' at the end of double-quoted string as a literal dollar sign.
>
> Thanks.
>
-- 
D. Ben Knoble
Eric Sunshine· Jun 3, 2025, 20:46 UTC · re: D. Ben Knoble · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

On Tue, Jun 3, 2025 at 4:37 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:
Show 22 quoted lines
> On Tue, May 27, 2025 at 1:31 PM Junio C Hamano <gitster@pobox.com> wrote:
> > In this case I am not all that sympathetic to the idea of the patch.
> > The consistently indented lines makes it more clear from which line
> > to which line came from a commit log message; running stripspace
> > would break them into paragraph pieces.  These editors that complain
> > probaly can be fixed?
>
> My editor doesn't complain, but it does highlight trailing whitespace
> at my behest, and it tends to be an eyesore (on purpose: that way I
> clean it up). Perhaps Kistoffer is coming from a similar place?
>
> > Alternatively, if it bothers users of certain editing environments
> > too much, perhaps the indent code in the output phase of "git show"
> > should lose the indents for empty lines uniformly, shoudln't it?  It
> > probably should be a fairly isolated change, like the way how the
> > expand_tabs_in_log bit is handled in pretty.c; give another bit and
> > teach pp_handle_indent to return when that bit is set and the
> > payload it was asked to show with indentation is empty, or something
> > like that.
>
> I think this suggestion would also help folks who "git commit -v,"
> which IIRC is also indented in the template.

For what it's worth, there was a previous attempt at something along these lines after which a discussion ensued.

https://lore.kernel.org/git/20210830072118.91921-4-sunshine@sunshineco.com/T/
Kristoffer Haugsbakk· Jun 9, 2025, 21:10 UTC · re: Eric Sunshine · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

On Tue, Jun 3, 2025, at 22:46, Eric Sunshine wrote:
Show 7 quoted lines
>> I think this suggestion would also help folks who "git commit -v,"
>> which IIRC is also indented in the template.
>
> For what it's worth, there was a previous attempt at something along
> these lines after which a discussion ensued.
>
> https://lore.kernel.org/git/20210830072118.91921-4-sunshine@sunshineco.com/T/

That’s very relevant, thanks. It’s not something that I would have noticed since I am too afraid of editing any file with patch contents in them. In part that’s why I find it so convenient that you can send notes along with patches.

-- 
Kristoffer
Kristoffer Haugsbakk· Jun 9, 2025, 21:12 UTC · re: D. Ben Knoble · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

On Tue, Jun 3, 2025, at 22:37, D. Ben Knoble wrote:
Show 9 quoted lines
>> In this case I am not all that sympathetic to the idea of the patch.
>> The consistently indented lines makes it more clear from which line
>> to which line came from a commit log message; running stripspace
>> would break them into paragraph pieces.  These editors that complain
>> probaly can be fixed?
>
> My editor doesn't complain, but it does highlight trailing whitespace
> at my behest, and it tends to be an eyesore (on purpose: that way I
> clean it up). Perhaps Kistoffer is coming from a similar place?
Yes exactly.  Ain’t nothing more to it than that.  :)
Show 13 quoted lines
>
>>
>> Alternatively, if it bothers users of certain editing environments
>> too much, perhaps the indent code in the output phase of "git show"
>> should lose the indents for empty lines uniformly, shoudln't it?  It
>> probably should be a fairly isolated change, like the way how the
>> expand_tabs_in_log bit is handled in pretty.c; give another bit and
>> teach pp_handle_indent to return when that bit is set and the
>> payload it was asked to show with indentation is empty, or something
>> like that.
>
> I think this suggestion would also help folks who "git commit -v,"
> which IIRC is also indented in the template.
In my testing though it doesn’t introduce trailing whitespace.
-- 
Kristoffer Haugsbakk
D. Ben Knoble· Jun 10, 2025, 21:11 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

On Mon, Jun 9, 2025 at 5:13 PM Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com> wrote:

Show 28 quoted lines
>
> On Tue, Jun 3, 2025, at 22:37, D. Ben Knoble wrote:
> >> In this case I am not all that sympathetic to the idea of the patch.
> >> The consistently indented lines makes it more clear from which line
> >> to which line came from a commit log message; running stripspace
> >> would break them into paragraph pieces.  These editors that complain
> >> probaly can be fixed?
> >
> > My editor doesn't complain, but it does highlight trailing whitespace
> > at my behest, and it tends to be an eyesore (on purpose: that way I
> > clean it up). Perhaps Kistoffer is coming from a similar place?
>
> Yes exactly.  Ain’t nothing more to it than that.  :)
> >
> >>
> >> Alternatively, if it bothers users of certain editing environments
> >> too much, perhaps the indent code in the output phase of "git show"
> >> should lose the indents for empty lines uniformly, shoudln't it?  It
> >> probably should be a fairly isolated change, like the way how the
> >> expand_tabs_in_log bit is handled in pretty.c; give another bit and
> >> teach pp_handle_indent to return when that bit is set and the
> >> payload it was asked to show with indentation is empty, or something
> >> like that.
> >
> > I think this suggestion would also help folks who "git commit -v,"
> > which IIRC is also indented in the template.
>
> In my testing though it doesn’t introduce trailing whitespace.

Really? When committing a recent patch (editor template attached) with -v (for testing this), I saw trailing whitespace on the blank line under the added feed.

>
> --
> Kristoffer Haugsbakk
-- 
D. Ben Knoble


news: add new feed
; Veuillez saisir le message de validation pour vos modifications. Les lignes
; commençant par ';' seront ignorées, et un message vide abandonne la validation.
;
; Sur la branche master
; Votre branche est à jour avec 'origin/master'.
;
; Modifications qui seront validées :
;	modifié :         links/newsboat/urls
;
; Fichiers non suivis:
;	links/bin/git-stack
;
; ------------------------ >8 ------------------------
; Ne touchez pas à la ligne ci-dessus.
; Tout ce qui suit sera éliminé.
diff --git c/links/newsboat/urls i/links/newsboat/urls
index 522d955d..d192de8b 100644
--- c/links/newsboat/urls
+++ i/links/newsboat/urls
@@ -21,6 +21,7 @@ https://ln.ht/_/feed/~ddevault "tech"
 https://maia.crimew.gay/feed.xml "tech"
 https://www.felienne.com/feed "tech"
 https://ronjeffries.com/feed.xml "tech"
+https://eev.ee/feeds/rss.xml "tech"
 
 http://journal.stuffwithstuff.com/rss.xml "~Bob Nystrom" "tech"
 https://blog.sanctum.geek.nz/feed/ "tech"
Junio C Hamano· Jun 10, 2025, 22:55 UTC · re: D. Ben Knoble · lore

Re: [PATCH] notes: remove trailing whitespace from editor template

"D. Ben Knoble" <ben.knoble@gmail.com> writes:
> Really? When committing a recent patch (editor template attached) with
> -v (for testing this), I saw trailing whitespace on the blank line
> under the added feed.

True. Unless you use a special configuration, which might break other people's "patch" implementations (I think "git apply" has learned to grok this ages ago).

$ git -c diff.suppressBlankEmpty=yes commit -v

← back to recent threads