Hi Kristoffer

On 26/05/2025 20:44, Kristoffer Haugsbakk wrote:

On Mon, May 26, 2025, at 16:01, Phillip Wood wrote:

quoted

quoted

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.

quoted

quoted

   	write_or_die(fd, cbuf.buf, cbuf.len);

  > [...]> +test_expect_success 'git notes add has no trailing whitespace
in the editor template' '

quoted

+	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