From: Junio C Hamano <gitster@pobox•com>
To: Johan Herland <johan@herland•net>
Cc: git@vger•kernel.org, mackyle@gmail•com, jhf@trifork•com,
Eric Sunshine <sunshine@sunshineco•com>
Subject: Re: [PATCHv3 3/5] builtin/notes: Add --allow-empty, to allow storing empty notes
Date: Fri, 07 Nov 2014 10:04:18 -0800 [thread overview]
Message-ID: <xmqq1tpehopp.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1415351961-31567-4-git-send-email-johan@herland.net> (Johan Herland's message of "Fri, 7 Nov 2014 10:19:19 +0100")
Johan Herland <johan@herland•net> 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@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();
}
> 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?
next prev parent reply other threads:[~2014-11-07 18:04 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-07 9:19 [PATCHv3 0/5] Handling empty notes Johan Herland
2014-11-07 9:19 ` [PATCHv3 1/5] builtin/notes: Fix premature failure when trying to add the empty blob Johan Herland
2014-11-07 9:19 ` [PATCHv3 2/5] t3301: Verify that 'git notes' removes empty notes by default Johan Herland
2014-11-07 17:48 ` Junio C Hamano
2014-11-07 9:19 ` [PATCHv3 3/5] builtin/notes: Add --allow-empty, to allow storing empty notes Johan Herland
2014-11-07 18:04 ` Junio C Hamano [this message]
2014-11-09 12:31 ` Johan Herland
2014-11-10 20:18 ` Junio C Hamano
2014-11-07 9:19 ` [PATCHv3 4/5] notes: Empty notes should be shown by 'git log' Johan Herland
2014-11-07 18:51 ` Junio C Hamano
2014-11-07 9:19 ` [PATCHv3 5/5] t3301: Use write_script(), nitpick whitespace Johan Herland
2014-11-07 18:55 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq1tpehopp.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jhf@trifork$(echo .)com \
--cc=johan@herland$(echo .)net \
--cc=mackyle@gmail$(echo .)com \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox