Re: [PATCHv4 5/9] builtin/notes: Simplify early exit code in add()

From: Johan Herland <johan@herland.net>
Date: Nov 11, 2014, 00:49 UTC
Message-ID: <CALKQrgdBhm91tipHRQ3sF_DacSh5nRJmS7eP4S5Z4vB8R8O4_w@mail.gmail.com>
In-Reply-To: <xmqqmw7y94jm.fsf@gitster.dls.corp.google.com>

On Mon, Nov 10, 2014 at 9:36 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 34 quoted lines

> Johan Herland <johan@herland.net> writes:
>
>> Signed-off-by: Johan Herland <johan@herland.net>
>> ---
>>  builtin/notes.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index 1017472..f1480cf 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -399,7 +399,7 @@ static int append_edit(int argc, const char **argv, const char *prefix);
>>
>>  static int add(int argc, const char **argv, const char *prefix)
>>  {
>> -     int retval = 0, force = 0;
>> +     int force = 0;
>>       const char *object_ref;
>>       struct notes_tree *t;
>>       unsigned char object[20], new_note[20];
>> @@ -441,6 +441,8 @@ static int add(int argc, const char **argv, const char *prefix)
>>
>>       if (note) {
>>               if (!force) {
>> +                     free_note_data(&d);
>> +                     free_notes(t);
>>                       if (!d.given) {
>
> It looks a bit strange to refer to d.given after calling a function
> that sounds as if it is meant to clear what is recorded in and to
> invalidate &d; yes, I can read the implementation to see that
> d.given keeps its stale value, but that is something other people
> may want to "clean up" later and this reference to d.given will get
> in the way when that happens.

Yes, that was obviously an oversight on my part.

> At this point of the code, it makes sense to free t in preparation
> to either switching to append codepath or erroring out, but does &d
> have anything in it already to necessitate its freeing?

Actually, no, although verifying that required double-checking that each of the -m/-F/-c/-C parsers which store data into &d, do in fact set d.given.

I suggest to either move the free_note_data(&d) call below the "if (!d.given)" block, or reorganize into this (which at the moment reads better to me):

    if (note) {
        if (!force) {
            free_notes(t);
            if (d.given) {
                free_note_data(&d);
                return error(_("Cannot add notes. "
                    "Found existing notes for object %s. "
                    "Use '-f' to overwrite existing notes"),
                    sha1_to_hex(object));
            }
            /*
             * Redirect to "edit" subcommand.
             *
             * We only end up here if none of -m/-F/-c/-C or -f are
             * given. The original args are therefore still in
             * argv[0-1].
             */
            argv[0] = "edit";
            return append_edit(argc, argv, prefix);
        }
        fprintf(stderr, _("Overwriting existing notes for object %s\n"),
            sha1_to_hex(object));
    }

This keeps the two free_* calls close together, only separated by if (d.given), which nicely indicates whether we need to call free_notes_data(&d) at all.

If that looks good to you, do you want a re-roll, or is it easier to fix up yourself?

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net