Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak
On Thu, Oct 07 2021, Junio C Hamano wrote:
Show 16 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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...
Ævar Arnfjörð Bjarmason <avarab@gmail.com>