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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 13, 2022, 10:23 UTC
Message-ID: <221013.86sfjsknlw.gmgdl@evledraar.gmail.com>
In-Reply-To: <c0211f35-bb26-7ca8-6f9d-a62507e55e8a@gmail.com>

On Thu, Oct 13 2022, Phillip Wood wrote:

Show 37 quoted lines

> On 13/10/2022 10:36, Ævar Arnfjörð Bjarmason wrote:
>> 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.

Yes, sorry about not being clear. It *does* need to be uninitialized, I was just narrating my surprise at this not being a case where my compiler caught it when I was locally testing this.

Then I remembered this is one of those cases where clang is slightly better at analysis, gcc without -fanalyzer says nothing, but clang by default will note (if you remove the NULL initialization here):

	builtin/notes.c:641:13: error: variable 'logmsg' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
	        } else if (!cp.buf.len) {
	                   ^~~~~~~~~~~
	builtin/notes.c:653:7: note: uninitialized use occurs here
	        free(logmsg);
	             ^~~~~~
	builtin/notes.c:641:9: note: remove the 'if' if its condition is always false
	        } else if (!cp.buf.len) {
	               ^~~~~~~~~~~~~~~~~~
	builtin/notes.c:570:14: note: initialize the variable 'logmsg' to silence this warning
	        char *logmsg;
	                    ^
	                     = NULL

Anyway, nothing needs to be changed about the code here, sorry about the distraction. I should have left it at something like "this NULL initialization is needed".

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

That's a good catch, and one I didn't catch on my read-through, i.e. in general this seems like a "was the strbuf empty?" pattern, before we re-append to it.

But playing with it further:
	
	diff --git a/builtin/notes.c b/builtin/notes.c
	index cc1e3aa2b6b..262bbffa375 100644
	--- a/builtin/notes.c
	+++ b/builtin/notes.c
	@@ -570,7 +570,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)
	 	char *logmsg = NULL;
	 	const char * const *usage;
	 	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
	-	struct note_data cp = { 0, 0, NULL, STRBUF_INIT };
	 	struct option options[] = {
	 		OPT_CALLBACK_F('m', "message", &d, N_("message"),
	 			N_("note contents as a string"), PARSE_OPT_NONEG,
	@@ -616,8 +615,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)
	 
	 	prepare_note_data(&object, &d, edit && note ? note : NULL);
	 
	-	strbuf_addbuf(&cp.buf, &d.buf);
	-
	 	if (note && !edit) {
	 		/* Append buf to previous note contents */
	 		unsigned long size;
	@@ -638,21 +635,16 @@ static int append_edit(int argc, const char **argv, const char *prefix)
	 			BUG("combine_notes_overwrite failed");
	 		logmsg = xstrfmt("Notes added by 'git notes %s'", argv[0]);
	 		commit_notes(the_repository, t, logmsg);
	-	} else if (!cp.buf.len) {
	+	} else if (!d.buf.len) {
	 		fprintf(stderr,
	 			_("Both original and appended notes are empty in %s, do nothing\n"),
	 			oid_to_hex(&object));
	 	} else {
	-		fprintf(stderr, _("Removing note for object %s\n"),
	-			oid_to_hex(&object));
	-		remove_note(t, object.hash);
	-		logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
	-		commit_notes(the_repository, t, logmsg);
	+		BUG("this is not reachable by any test now");
	 	}
	 
	 	free(logmsg);
	 	free_note_data(&d);
	-	free_note_data(&cp);
	 	free_notes(t);
	 	return 0;
	 }
	
This 2/2 makes that "else" test-unreachable, so whatever else we do here
we should start by making sure that by adding the "else if" we still
have test coverage for the "else".