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

Johan Herland [off-list ref] writes:

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.

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?

On Mon, Nov 10, 2014 at 9:36 PM, Junio C Hamano [off-list ref] wrote:

Johan Herland [off-list ref] writes:

quoted

Signed-off-by: Johan Herland <redacted>
---
 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, [off-list ref]
www.herland.net