| Message ID | d41ba140505aa3459330afd652ff6a0f456222a0.1673490953.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | notes.c: introduce "--separator" optio | expand |
On Thu, Jan 12 2023, 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 too, we could print > the exact tips to end-user. > > Signed-off-by: Teng Long <dyroneteng@gmail.com> > --- > builtin/notes.c | 5 ++++- > t/t3301-notes.sh | 5 +++-- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/builtin/notes.c b/builtin/notes.c > index b71a81bff1..f2efb3736c 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -631,7 +631,10 @@ 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 (!d.buf.len && !note) > + fprintf(stderr, > + _("Both original and appended notes are empty in %s, do nothing\n"), > + oid_to_hex(&object)); Style: This arm should have {}-braces too, see the CodingGuidelines.
On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote: > When "git notes append <object>" is executed, if there is no note in > the given object and the appended note is empty too, we could print > the exact tips to end-user. > > Signed-off-by: Teng Long <dyroneteng@gmail.com> > --- > diff --git a/builtin/notes.c b/builtin/notes.c > @@ -631,7 +631,10 @@ static int append_edit(int argc, const char **argv, const char *prefix) > + } else if (!d.buf.len && !note) > + fprintf(stderr, > + _("Both original and appended notes are empty in %s, do nothing\n"), > + oid_to_hex(&object)); My knee-jerk reaction is between "meh" and "thumbs down". The commit message says we "can do this" but doesn't explain "why we should do this". Is this condition important enough to break the Unix maxim of "Rule of Silence" (or "Silence is Golden")? I also wonder if this change is going to cause problems (or at least annoyance) for automated tooling. At present, tooling doesn't have to worry whether or not the existing or new note is empty; everything just works as expected without complaint. However, following this change, such tooling may now be greeted with an unexpected diagnostic message.
diff --git a/builtin/notes.c b/builtin/notes.c index b71a81bff1..f2efb3736c 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -631,7 +631,10 @@ 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 (!d.buf.len && !note) + fprintf(stderr, + _("Both original and appended notes are empty in %s, do nothing\n"), + oid_to_hex(&object)); free(logmsg); free_note_data(&d); diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 3288aaec7d..e7807e052a 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -552,6 +552,7 @@ test_expect_success 'appending empty string does not change existing note' ' ' test_expect_success 'git notes append == add when there is no existing note' ' + test_when_finished git notes remove HEAD && git notes remove HEAD && test_must_fail git notes list HEAD && git notes append -m "Initial set of notes${LF}${LF}More notes appended with git notes append" && @@ -560,9 +561,9 @@ 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 '