built-ins: use free() not UNLEAK() if trivial, rm dead code

For a lot of uses of UNLEAK() it would be quite tricky to release the
memory involved, or we're missing the relevant *_(release|clear)()
functions. But in these cases we have them already, and can just
invoke them on the variable(s) involved, instead of UNLEAK().

For "builtin/worktree.c" the UNLEAK() was also added in [1], but the
struct member it's unleaking was removed in [2]. The only non-"int"
member of that structure is "const char *keep_locked", which comes to
us via "argv" or a string literal[3].

We have good visibility via the compiler and
tooling (e.g. SANITIZE=address) on bad free()-ing, but none on
UNLEAK() we don't need anymore. So let's prefer releasing the memory
when it's easy.

For "bugreport", "worktree" and "config" we need to start using a "ret
= ..." return pattern. For "builtin/bugreport.c" these UNLEAK() were
added in [4], and for "builtin/config.c" in [1].

For "config" the code seen here was the only user of the "value"
variable. For "ACTION_{RENAME,REMOVE}_SECTION" we need to be sure to
return the right exit code in the cases where we were relying on
falling through to the top-level.

I think there's still a use-case for UNLEAK(), but hat it's changed
since then. Using it so that "we can see the real leaks" is
counter-productive in these cases.

It's more useful to have UNLEAK() be a marker of the remaining odd
cases where it's hard to free() the memory for whatever reason. With
this change less than 20 of them remain in-tree.

1. 0e5bba53af7 (add UNLEAK annotation for reducing leak false
   positives, 2017-09-08)
2. d861d34a6ed (worktree: remove extra members from struct add_opts,
   2018-04-24)
3. 0db4961c49b (worktree: teach `add` to accept --reason <string> with
  --lock, 2021-07-15)
4. 0e5bba53af7 and 00d8c311050 (commit: fix "author_ident" leak,
   2022-05-12).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>

diff --git a/builtin/add.c b/builtin/add.c
index f84372964c8c486601941c927d6e39bb8da42084..c68ebafed5ed2c20a808b8bea294f171e5b7059c 100644 (file)
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -695,6 +695,6 @@ finish:
                die(_("Unable to write new index file"));
 
        dir_clear(&dir);
-       UNLEAK(pathspec);
+       clear_pathspec(&pathspec);
        return exit_status;
 }

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 96052541cbfe74cda5aff77396c94f4fb6397ae7..5bc254be80f97e7bb33c732039b7bab620fb71ad 100644 (file)
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -106,6 +106,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
        const char *user_relative_path = NULL;
        char *prefixed_filename;
        size_t output_path_len;
+       int ret;
 
        const struct option bugreport_options[] = {
                OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"),
@@ -182,7 +183,9 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
                user_relative_path);
 
        free(prefixed_filename);
-       UNLEAK(buffer);
-       UNLEAK(report_path);
-       return !!launch_editor(report_path.buf, NULL, NULL);
+       strbuf_release(&buffer);
+
+       ret = !!launch_editor(report_path.buf, NULL, NULL);
+       strbuf_release(&report_path);
+       return ret;
 }

diff --git a/builtin/commit.c b/builtin/commit.c
index c291199b70417149a848b48105e625a6aa2feaf8..f88a29167f42d0458b9a6b7659de5f33b85efa8b 100644 (file)
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1874,8 +1874,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
        apply_autostash(git_path_merge_autostash(the_repository));
 
 cleanup:
-       UNLEAK(author_ident);
-       UNLEAK(err);
-       UNLEAK(sb);
+       strbuf_release(&author_ident);
+       strbuf_release(&err);
+       strbuf_release(&sb);
        return ret;
 }

diff --git a/builtin/config.c b/builtin/config.c
index 753e5fac297e08763317adcab2fefe386ea68421..060cf9f3e05e6718ae02923e132e4804dbdcb291 100644 (file)
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -639,8 +639,9 @@ static char *default_user_config(void)
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
        int nongit = !startup_info->have_repository;
-       char *value;
+       char *value = NULL;
        int flags = 0;
+       int ret = 0;
 
        given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
 
@@ -856,44 +857,38 @@ int cmd_config(int argc, const char **argv, const char *prefix)
                free(config_file);
        }
        else if (actions == ACTION_SET) {
-               int ret;
                check_write();
                check_argc(argc, 2, 2);
                value = normalize_value(argv[0], argv[1]);
-               UNLEAK(value);
                ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);
                if (ret == CONFIG_NOTHING_SET)
                        error(_("cannot overwrite multiple values with a single value\n"
                        "       Use a regexp, --add or --replace-all to change %s."), argv[0]);
-               return ret;
        }
        else if (actions == ACTION_SET_ALL) {
                check_write();
                check_argc(argc, 2, 3);
                value = normalize_value(argv[0], argv[1]);
-               UNLEAK(value);
-               return git_config_set_multivar_in_file_gently(given_config_source.file,
-                                                             argv[0], value, argv[2],
-                                                             flags);
+               ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+                                                            argv[0], value, argv[2],
+                                                            flags);
        }
        else if (actions == ACTION_ADD) {
                check_write();
                check_argc(argc, 2, 2);
                value = normalize_value(argv[0], argv[1]);
-               UNLEAK(value);
-               return git_config_set_multivar_in_file_gently(given_config_source.file,
-                                                             argv[0], value,
-                                                             CONFIG_REGEX_NONE,
-                                                             flags);
+               ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+                                                            argv[0], value,
+                                                            CONFIG_REGEX_NONE,
+                                                            flags);
        }
        else if (actions == ACTION_REPLACE_ALL) {
                check_write();
                check_argc(argc, 2, 3);
                value = normalize_value(argv[0], argv[1]);
-               UNLEAK(value);
-               return git_config_set_multivar_in_file_gently(given_config_source.file,
-                                                             argv[0], value, argv[2],
-                                                             flags | CONFIG_FLAGS_MULTI_REPLACE);
+               ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+                                                            argv[0], value, argv[2],
+                                                            flags | CONFIG_FLAGS_MULTI_REPLACE);
        }
        else if (actions == ACTION_GET) {
                check_argc(argc, 1, 2);
@@ -934,26 +929,28 @@ int cmd_config(int argc, const char **argv, const char *prefix)
                                                              flags | CONFIG_FLAGS_MULTI_REPLACE);
        }
        else if (actions == ACTION_RENAME_SECTION) {
-               int ret;
                check_write();
                check_argc(argc, 2, 2);
                ret = git_config_rename_section_in_file(given_config_source.file,
                                                        argv[0], argv[1]);
                if (ret < 0)
                        return ret;
-               if (ret == 0)
+               else if (!ret)
                        die(_("no such section: %s"), argv[0]);
+               else
+                       ret = 0;
        }
        else if (actions == ACTION_REMOVE_SECTION) {
-               int ret;
                check_write();
                check_argc(argc, 1, 1);
                ret = git_config_rename_section_in_file(given_config_source.file,
                                                        argv[0], NULL);
                if (ret < 0)
                        return ret;
-               if (ret == 0)
+               else if (!ret)
                        die(_("no such section: %s"), argv[0]);
+               else
+                       ret = 0;
        }
        else if (actions == ACTION_GET_COLOR) {
                check_argc(argc, 1, 2);
@@ -966,5 +963,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
                return get_colorbool(argv[0], argc == 2);
        }
 
-       return 0;
+       free(value);
+       return ret;
 }

diff --git a/builtin/diff.c b/builtin/diff.c
index 854d2c5a5c47f29c125f1b199368e6384c46f41f..cb63f157dd13e50cded7b2c96204e84595e5a7a1 100644 (file)
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -609,7 +609,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
        if (1 < rev.diffopt.skip_stat_unmatch)
                refresh_index_quietly();
        release_revisions(&rev);
-       UNLEAK(ent);
+       object_array_clear(&ent);
        UNLEAK(blob);
        return result;
 }

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4a24d53be15c3a74d6632c68ccacb813c8aa989b..591d659faea1684b19a5371b773c6d8493f8d796 100644 (file)
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -629,6 +629,7 @@ static int add(int ac, const char **av, const char *prefix)
                         N_("try to match the new branch name with a remote-tracking branch")),
                OPT_END()
        };
+       int ret;
 
        memset(&opts, 0, sizeof(opts));
        opts.checkout = 1;
@@ -705,9 +706,9 @@ static int add(int ac, const char **av, const char *prefix)
                die(_("--[no-]track can only be used if a new branch is created"));
        }
 
-       UNLEAK(path);
-       UNLEAK(opts);
-       return add_worktree(path, branch, &opts);
+       ret = add_worktree(path, branch, &opts);
+       free(path);
+       return ret;
 }
 
 static void show_worktree_porcelain(struct worktree *wt, int line_terminator)