| Message ID | 20221013055654.39628-3-tenglong.tl@alibaba-inc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | notes.c: introduce "--no-blankline" option | expand |
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... > 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.
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. >> 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
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@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". >>> 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".
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
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com> writes: > 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... On my local env the warnings shows , show I change the line (initialize with NULL to "logmsg"). But it seems like different as the last time I built... However now "suggest braces around initialization of subobject" appears, is it normal or we should repair this? builtin/merge-file.c:29:23: warning: suggest braces around initialization of subobject [-Wmissing-braces] mmfile_t mmfs[3] = { 0 }; ^ {} builtin/merge-file.c:31:20: warning: suggest braces around initialization of subobject [-Wmissing-braces] xmparam_t xmp = { 0 }; ^ {} 2 warnings generated. builtin/notes.c:641:13: warning: variable 'logmsg' is used uninitialized whenever 'if' condition is true [-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 1 warning generated. builtin/submodule--helper.c:1749:56: warning: suggest braces around initialization of subobject [-Wmissing-braces] struct list_objects_filter_options filter_options = { 0 }; ^ {} builtin/submodule--helper.c:2623:56: warning: suggest braces around initialization of subobject [-Wmissing-braces] struct list_objects_filter_options filter_options = { 0 }; ^ {} builtin/unpack-objects.c:388:26: warning: suggest braces around initialization of subobject [-Wmissing-braces] git_zstream zstream = { 0 }; ^ {} My gcc version: Apple clang version 11.0.0 (clang-1100.0.33.17) Target: x86_64-apple-darwin19.6.0 Thread model: posix > > 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. Thanks for figuring this out, will fix both.
Phillip Wood <phillip.wood123@gmail.com> writes: > 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. Actually, I didn't get it totally (maybe because my English, sorry for that), but indeed the 'else if' expose this problem out, so I think to initialize it is needed. Thanks. > 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. Yes, it actually does as you describe above, your suggestion makes it look better. Thank you very much for your detailed review.
Phillip Wood <phillip.wood123@gmail.com> writes: > 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. Actually, I didn't get it totally (maybe because my English, sorry for that), but indeed the 'else if' expose this problem out, so I think to initialize it is needed. Thanks. > 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. Yes, it actually does as you describe above, your suggestion makes it look better, will apply. Thank you very much for your detailed review.
On Tue, Oct 18 2022, Teng Long wrote: > "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> writes: > >> 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... > > On my local env the warnings shows , show I change the line (initialize with > NULL to "logmsg"). > > But it seems like different as the last time I built... However now "suggest > braces around initialization of subobject" appears, is it normal or we should > repair this? > > builtin/merge-file.c:29:23: warning: suggest braces around initialization of subobject [-Wmissing-braces] > mmfile_t mmfs[3] = { 0 }; > ^ > {} > builtin/merge-file.c:31:20: warning: suggest braces around initialization of subobject [-Wmissing-braces] > xmparam_t xmp = { 0 }; > ^ > {} The fix for this is in "next": 54795d37d9e (config.mak.dev: disable suggest braces error on old clang versions, 2022-10-10) > 2 warnings generated. > builtin/notes.c:641:13: warning: variable 'logmsg' is used uninitialized whenever 'if' condition is true [-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 > 1 warning generated. Yes, we should initialize it to NULL, so this is the expected warning. Clang spots it.
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; 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, @@ -615,6 +616,8 @@ 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; @@ -634,16 +637,22 @@ static int append_edit(int argc, const char **argv, const char *prefix) if (add_note(t, &object, &new_note, combine_notes_overwrite)) 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) { + 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); } - commit_notes(the_repository, t, logmsg); free(logmsg); free_note_data(&d); + free_note_data(&cp); free_notes(t); return 0; } diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 43ac3feb78..967e6bfb67 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -574,7 +574,8 @@ test_expect_success 'git notes append == add when there is no existing note' ' test_expect_success 'appending empty string to non-existing note does not create note' ' git notes remove HEAD && test_must_fail git notes list HEAD && - git notes append -m "" && + git notes append -m "" >output 2>&1 && + grep "Both original and appended notes are empty" output && test_must_fail git notes list HEAD '