From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 27 May 2025 08:24:26 GMT
Subject: Re: [PATCH] notes: remove trailing whitespace from editor template
Message-ID: <ab009472-d15d-4894-aa83-0ab8b0d2dfbd@gmail.com>
In-Reply-To: <10280d7d-af36-468d-82b8-e0e780c38ef1@app.fastmail.com>

Hi Kristoffer

On 26/05/2025 20:44, Kristoffer Haugsbakk wrote:
> 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.

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