Johan Herland <johan@xxxxxxxxxxx> writes:
> Although the "git notes" man page advertises that we support binary-safe
> notes addition (using the -C option), we currently do not support adding
> the empty note (i.e. using the empty blob to annotate an object). Instead,
> an empty note is always treated as an intent to remove the note
> altogether.
>
> Introduce the --allow-empty option to the add/append/edit subcommands,
> to explicitly allow an empty note to be stored into the notes tree.
>
> Also update the documentation, and add test cases for the new option.
>
> Reported-by: James H. Fisher <jhf@xxxxxxxxxxx>
> Improved-by: Kyle J. McKay <mackyle@xxxxxxxxx>
> Improved-by: Junio C Hamano <gitster@xxxxxxxxx>
> Signed-off-by: Johan Herland <johan@xxxxxxxxxxx>
> ---
Assuming that it is a good idea to "allow" empty notes, I think
there are two issues involved here:
* Traditionally, feeding an empty note is taken as a request to
remove an existing note. Therefore, there is no way to
explicitly ask an empty note to be stored for a commit.
* Because feeding an empty note was the way to request removal,
even though "git notes remove" is there, it is underused.
In other words, assuming that it is a good idea to allow empty
notes, isn't the desired endgame, after compatibility transition
period, that "git notes add" will never remove notes?
With that endgame in mind, shouldn't the internal implementation be
moving in a direction where "create_note()" will *not* be doing any
removal, and its caller (i.e. "add") does the switching depending on
the "do we take emptyness as a request to remove"? I.e.
static int add(...)
{
if (!allow_empty && message_is_empty())
remove_note();
else
create_note();
}
> static void create_note(const unsigned char *object, struct msg_arg *msg,
> - int append_only, const unsigned char *prev,
> - unsigned char *result)
> + int append_only, int allow_empty,
> + const unsigned char *prev, unsigned char *result)
In other words, I have this suspicion that create_note() that
removes is a wrong interface in the first place, and giving it
a new allow_empty parameter to conditionally perform removal is
making it worse. No?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html