Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-10-13 13:43:33
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2021-10-14 · Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak · Junio C Hamano <hidden>
- 2021-10-14 · Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak · Ævar Arnfjörð Bjarmason <hidden>
- 2021-10-13 · Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak · Junio C Hamano <hidden>
- 2021-10-07 · [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak · Ævar Arnfjörð Bjarmason <hidden>
On Thu, Oct 07 2021, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:quoted
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...