On Sat, Jan 28, 2023 at 6:50 AM Teng Long <dyroneteng@xxxxxxxxx> wrote:
> Eric Sunshine writes:
> > > Situation of note "removing" shouldn't happen in 'append_edit()',
> > > unless it's a bug. So, let's drop the unreachable "else" code
> > > in "append_edit()".
> > >
> > > Signed-off-by: Teng Long <dyroneteng@xxxxxxxxx>
> > > ---
> > > - } 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);
> >
> > This change breaks removal of notes using "git notes edit". Prior to
> > this change, if you delete the content of a note using "git notes
> > edit", then the note is removed. Following this change, the note
> > remains, which contradicts documented[1] behavior.
>
> As the commit msg describes, the subcommands I understand should have clear
> responsibilities as possible (documentaion may have some effect in my mind). So,
> the removal opertion under "append subcommand" here is little wired to me, but
> your suggestion makes sense, this may have compatibility issues. Although I
> think it's weird that someone would use this in the presence of the remove
> subcommand, and my feeling is that this code is actually copied from the add
> method (introduced by 52694cdabbf68f19c8289416e7bb3bbef41d8d27), but I'm not
> sure.
>
> So, it's ok for me to drop this one.
It's unfortunate, perhaps, that the git-notes command-set is not
orthogonal, but it's another case of historic accretion. According to
the history, as originally implemented, git-notes only supported two
commands, "edit" and "show", and "edit" was responsible for _all_
mutation operations, including deletion. git-notes only grew a more
complete set of commands a couple years later.
At any rate, even though the original behaviors may not make as much
sense these days now that git-notes has a more complete set of
commands, we still need to be careful not to break existing workflows
and scripts (tooling). That's why it would be nice to beef up the
tests so that the existing behaviors don't get broken by changes such
as this patch wants to make. But, as mentioned earlier, the additional
tests probably shouldn't be part of this series, but rather can be
done as a separate series (by someone; it doesn't have to be you).
