Hi Ævar
On 13/10/2022 11:23, Ævar Arnfjörð Bjarmason wrote:
On Thu, Oct 13 2022, Phillip Wood wrote:
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.
Ah I think I slightly misunderstood your comment - I agree it is
surprising that the compiler didn't catch that.
@@ -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");
}
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".
Oh so we can just use d.buf.len directly - nicely spotted and kudos for
checking the test coverage. Looking at the existing tests they are
checking if an empty note is removed which suggests this patch is
failing to distinguish between an existing empty note and no note. I
think we probably need to be doing "else if (note && d.buf.len)" but
I've not looked very closely.
Best Wishes
Phillip
