Re: [PATCHv3 3/5] builtin/notes: Add --allow-empty, to allow storing empty notes

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 7, 2014, 18:04 UTC
Message-ID: <xmqq1tpehopp.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1415351961-31567-4-git-send-email-johan@herland.net>

Johan Herland <johan@herland.net> writes:

Show 16 quoted lines

> 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@trifork.com>
> Improved-by: Kyle J. McKay <mackyle@gmail.com>
> Improved-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---

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();
	}

Show 5 quoted lines

>  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?