Re: [PATCHv4 5/9] builtin/notes: Simplify early exit code in add()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 10, 2014, 20:36 UTC
- Message-ID
- <xmqqmw7y94jm.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1415536255-19961-6-git-send-email-johan@herland.net>
Johan Herland <johan@herland.net> writes:
Show 25 quoted lines
> 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.
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?
Show 29 quoted lines
> /*
> * Redirect to "edit" subcommand.
> @@ -450,14 +452,11 @@ static int add(int argc, const char **argv, const char *prefix)
> * therefore still in argv[0-1].
> */
> argv[0] = "edit";
> - free_note_data(&d);
> - free_notes(t);
> return append_edit(argc, argv, prefix);
> }
> - retval = error(_("Cannot add notes. Found existing notes "
> + return error(_("Cannot add notes. Found existing notes "
> "for object %s. Use '-f' to overwrite "
> "existing notes"), sha1_to_hex(object));
> - goto out;
> }
> fprintf(stderr, _("Overwriting existing notes for object %s\n"),
> sha1_to_hex(object));
> @@ -474,9 +473,8 @@ static int add(int argc, const char **argv, const char *prefix)
> snprintf(logmsg, sizeof(logmsg), "Notes %s by 'git notes %s'",
> is_null_sha1(new_note) ? "removed" : "added", "add");
> commit_notes(t, logmsg);
> -out:
> free_notes(t);
> - return retval;
> + return 0;
> }
>
> static int copy(int argc, const char **argv, const char *prefix)