Re: [PATCH v2 3/3] notes.c: drop unreachable code in "append_edit()"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 7, 2022, 14:41 UTC
Message-ID: <221107.868rkmx1pu.gmgdl@evledraar.gmail.com>
In-Reply-To: <2475ea0c04d7b5e2812f13d0c8701a0056b305b8.1667828335.git.dyroneteng@gmail.com>

On Mon, Nov 07 2022, Teng Long wrote:

Show 20 quoted lines

> From: Teng Long <dyroneteng@gmail.com>
>
> Situation of removing note shouldn't happen in "append_edit()",
> unless it's a bug. So, let's drop the unreachable "else" code
> in "append_edit()".
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/notes.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 02b88e54d8..6d42592c79 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -10,6 +10,7 @@
>  #include "cache.h"
>  #include "config.h"
>  #include "builtin.h"
> +#include "git-compat-util.h"

Huh? cache.h includes this already, why are we doing it again?

Show 14 quoted lines

>  #include "notes.h"
>  #include "object-store.h"
>  #include "repository.h"
> @@ -646,11 +647,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  			_("Both original and appended notes are empty in %s, do nothing\n"),
>  			oid_to_hex(&object));
>  	} else {
> -		fprintf(stderr, _("Removing note for object %s\n"),
> -			oid_to_hex(&object));
> -		remove_note(t, object.hash);
> -		logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> -		commit_notes(the_repository, t, logmsg);
> +		BUG("compute_notes failed");
>  	}

Let's squash this into 2/3. So you were adding to it then, but we didn't need to add this commit_notes() at all?

Also, there you did:

	-	char *logmsg;
	+	char *logmsg = NULL;

But here we see that it's only used in the initial "if" arm, so the reason to have it declared earlier has disappeared in this 3/3. Instead just have 2/3 do:

	if (...) {
		char *logmsg;
		[...]

		logmsg = xstrfmt(...);
	        commit_notes(....);
		free(logmsg);
	} else if (...) {
		...
	}

This commit also shows that there wasn't any reason for your "struct note_data d" cleanup, i.e. we weren't adding a field or anything...