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

From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: May 26, 2025, 19:44 UTC
Message-ID: <10280d7d-af36-468d-82b8-e0e780c38ef1@app.fastmail.com>
In-Reply-To: <66e92d69-8372-47cf-a350-95365f72ca1c@gmail.com>

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.