Re: [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 13, 2022, 10:10 UTC
Message-ID: <c0211f35-bb26-7ca8-6f9d-a62507e55e8a@gmail.com>
In-Reply-To: <221013.86wn94kqq1.gmgdl@evledraar.gmail.com>

On 13/10/2022 10:36, Ævar Arnfjörð Bjarmason wrote:

Show 35 quoted lines

> 
> On Thu, Oct 13 2022, Teng Long wrote:
> 
>> From: Teng Long <dyroneteng@gmail.com>
>>
>> When "git notes append <object>" is executed, if there is no note in
>> the given object and the appended note is empty, the command line
>> prompt will be as follows:
>>
>>       "Removing note for object <object>"
>>
>> Actually, this tip is not that accurate, because there is no note in
>> the original <object>, and it also does no remove work on the notes
>> reference, so let's fix this and give the correct tip.
>>
>> Signed-off-by: Teng Long <dyroneteng@gmail.com>
>> ---
>>   builtin/notes.c  | 13 +++++++++++--
>>   t/t3301-notes.sh |  3 ++-
>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index 1ca0476a27..cc1e3aa2b6 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>>   	struct notes_tree *t;
>>   	struct object_id object, new_note;
>>   	const struct object_id *note;
>> -	char *logmsg;
>> +	char *logmsg = NULL;
> 
> Hrm, interesting that (at least my) gcc doesn't catch if we don't
> NULL-initialize this, but -fanalyzer does (usually it's not needed for
> such trivial cases0. Anyawy...

I don't think its written to if we take the 'else if' branch added by this patch so we need to initialize it for the free() at the end.

Show 6 quoted lines

>>   	const char * const *usage;
>>   	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
>> +	struct note_data cp = { 0, 0, NULL, STRBUF_INIT };
> 
> This is probably better "fixed while at it" to set both to use "{ .buf =
> STRBUF_INIT }", rather than copying the pre-C99 pattern.

We only seem to be using cp.buf.len so we can test check if the original note was empty so I think it would be better just to add

	int note_was_empty;

`	...

	note_was_empty = !d.buf.len

instead.

Best Wishes

Phillip