On Thu, Oct 13 2022, Phillip Wood wrote:
> On 13/10/2022 10:36, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Oct 13 2022, Teng Long wrote:
>>
>>> From: Teng Long <dyroneteng@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxx>
>>> ---
>>> 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".
>>> 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".
