Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak

[Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>]

On Thu, Oct 07 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@xxxxxxxxx> writes:
>
>> This does add a bit of complexity, but I think it's worth it to just
>> fix these leaks when it's easy in built-ins. It allows them to serve
>> as canaries for underlying APIs that shouldn't be leaking, it
>> encourages us to make those freeing APIs nicer for all their users,
>> and it prevents other leaking regressions by being able to mark the
>> entire test as TEST_PASSES_SANITIZE_LEAK=true.
>
> This does more than necessary, though.  Introducing "ret", replacing
> an early return with an assignment to it, and returning "ret"
> instead of hardcoded 0, would have been the "fix a trivial leak",
> and the "ah, report_path_error() always returns true" does not
> belong here.
>
> These things look small, but small things add up.

I think you mean that you'd have liked:

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a2000ed6bf2..5e6b6f2d4a0 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -672,6 +672,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
                         N_("suppress duplicate entries")),
                OPT_END()
        };
+       int ret = 0;
 
        if (argc == 2 && !strcmp(argv[1], "-h"))
                usage_with_options(ls_files_usage, builtin_ls_files_options);
@@ -776,15 +777,13 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
                show_ru_info(the_repository->index);
 
        if (ps_matched) {
-               int bad;
-               bad = report_path_error(ps_matched, &pathspec);
-               if (bad)
+               if (report_path_error(ps_matched, &pathspec)) {
                        fprintf(stderr, "Did you forget to 'git add'?\n");
-
-               return bad ? 1 : 0;
+                       ret = 1;
+               }
        }
 
        dir_clear(&dir);
        free(max_prefix);
-       return 0;
+       return ret;
 }

Instead of this:

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a2000ed6bf2..fcc685947f9 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -672,6 +672,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
                         N_("suppress duplicate entries")),
                OPT_END()
        };
+       int ret = 0;
 
        if (argc == 2 && !strcmp(argv[1], "-h"))
                usage_with_options(ls_files_usage, builtin_ls_files_options);
@@ -775,16 +776,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
        if (show_resolve_undo)
                show_ru_info(the_repository->index);
 
-       if (ps_matched) {
-               int bad;
-               bad = report_path_error(ps_matched, &pathspec);
-               if (bad)
-                       fprintf(stderr, "Did you forget to 'git add'?\n");
-
-               return bad ? 1 : 0;
+       if (ps_matched && report_path_error(ps_matched, &pathspec)) {
+               fprintf(stderr, "Did you forget to 'git add'?\n");
+               ret = 1;
        }
 
        dir_clear(&dir);
        free(max_prefix);
-       return 0;
+       return ret;
 }

Doesn't make much sense, but I can re-roll with it if you feel strongly
about it. I think the current version is ready to be picked up.

Yeah we should avoid refactoring-while-at-it, but in cases where a patch
removes the only reason a nested if/if statement exists, unrolling it
into a single "if" handly seems too much. I think the alternative just
leaves the reader squinting at the diff wondering why we still need that
nesting...