On Tue, Nov 12, 2024 at 12:33:16AM +0100, Rubén Justo wrote:
>> Range-diff versus v1:
>>
>> 1: 09dbc39ffb = 1: 76daa9584b builtin/blame: fix leaking blame entries with `--incremental`
>> 2: 15bbe539ea = 2: 6659f0d59b bisect: fix leaking good/bad terms when reading multipe times
>> 3: 739e28c864 = 3: 5a9011e1ef bisect: fix leaking string in `handle_bad_merge_base()`
>> 4: fa91eb9dc6 = 4: 3afc0a7bfe bisect: fix leaking `current_bad_oid`
>> 5: 4ca01afd9e = 5: 15577aaae6 bisect: fix multiple leaks in `bisect_next_all()`
>> 6: 805b91ce20 = 6: 13ddfed4fc bisect: fix leaking commit list items in `check_merge_base()`
>> 7: 0f183fb264 ! 7: 0bc49ed3bb bisect: fix various cases where we leak commit list items
>> @@ bisect.c: static struct commit_list *skip_away(struct commit_list *list, int cou
>> + else if (previous)
>> + result = previous;
>> + else
>> -+ result = list;
>> ++ result = list;
>> + break;
>> }
>> previous = cur;
>> 8: 163c36c78e = 8: addba1afce line-log: fix leak when rewriting commit parents
>> 9: c0bcfbd98b = 9: 6e501effad strvec: introduce new `strvec_splice()` function
>> 10: cf0ac084c5 = 10: 2271761621 git: refactor alias handling to use a `struct strvec`
>> 11: c4989aab86 = 11: 7bff5d167f git: refactor builtin handling to use a `struct strvec`
>> 12: 483d2951cc = 12: e93d461504 split-index: fix memory leak in `move_cache_to_base_index()`
>> 13: 70b12d6347 = 13: a0643da6f5 builtin/sparse-checkout: fix leaking sanitized patterns
>> 14: f57d967cd5 = 14: 881feffeb1 help: refactor to not use globals for reading config
>> 15: 45aacddc8a < -: ---------- help: fix leaking `struct cmdnames`
>> -: ---------- > 15: f1bbb676f2 help: fix leaking `struct cmdnames`
>> 16: b61cc4da84 ! 16: 51c7ad993f help: fix leaking return value from `help_unknown_cmd()`
>> @@ help.c: static const char bad_interpreter_advice[] =
>> {
>> struct help_unknown_cmd_config cfg = { 0 };
>> int i, n, best_similarity = 0;
>> - struct cmdnames main_cmds = { 0 };
>> - struct cmdnames other_cmds = { 0 };
>> - struct cmdname_help *common_cmds;
>> -- const char *assumed = NULL;
>> -+ char *assumed = NULL;
>> -
>> - read_early_config(the_repository, git_unknown_cmd_config, &cfg);
>> -
>> @@ help.c: const char *help_unknown_cmd(const char *cmd)
>> ; /* still counting */
>> }
>> if (cfg.autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) {
>> -- assumed = main_cmds.names[0]->name;
>> +- const char *assumed = main_cmds.names[0]->name;
>> - main_cmds.names[0] = NULL;
>> - cmdnames_release(&main_cmds);
>> -+ assumed = xstrdup(main_cmds.names[0]->name);
>> ++ char *assumed = xstrdup(main_cmds.names[0]->name);
>> +
>> fprintf_ln(stderr,
>> _("WARNING: You called a Git command named '%s', "
>> "which does not exist."),
>> -@@ help.c: const char *help_unknown_cmd(const char *cmd)
>> - strbuf_release(&msg);
>> - if (!(starts_with(answer, "y") ||
>> - starts_with(answer, "Y"))) {
>> -- assumed = NULL;
>> -+ FREE_AND_NULL(assumed);
>> - goto out;
>> - }
>> - } else {
>>
>> ## help.h ##
>> @@ help.h: void list_all_other_cmds(struct string_list *list);
>> 17: b38a39ef04 = 17: 38a25da315 builtin/help: fix leaks in `check_git_cmd()`
>> 18: cd5cde9acb ! 18: 462c9fc110 builtin/init-db: fix leaking directory paths
>> @@ builtin/init-db.c: int cmd_init_db(int argc,
>> -
>> flags |= INIT_DB_EXIST_OK;
>> - return init_db(git_dir, real_git_dir, template_dir, hash_algo,
>> -+ ret = init_db(git_dir, real_git_dir, template_dir, hash_algo,
>> - ref_storage_format, initial_branch,
>> - init_shared_repository, flags);
>> +- ref_storage_format, initial_branch,
>> +- init_shared_repository, flags);
>> ++ ret = init_db(git_dir, real_git_dir, template_dir, hash_algo,
>> ++ ref_storage_format, initial_branch,
>> ++ init_shared_repository, flags);
>> +
>> + free(template_dir_to_free);
>> + free(real_git_dir_to_free);
>> 19: 2c7b50461b = 19: 6eabeed343 builtin/branch: fix leaking sorting options
>> 20: 6865dda344 = 20: 2a44bbfba3 t/helper: fix leaking commit graph in "read-graph" subcommand
>> -: ---------- > 21: fc8a96bac3 global: drop `UNLEAK()` annotation
>> 21: d7d8ece289 ! 22: 10e44976c2 git-compat-util: drop `UNLEAK()` annotation
>> @@ Metadata
>> Author: Patrick Steinhardt <ps@xxxxxx>
>>
>> ## Commit message ##
>> - git-compat-util: drop `UNLEAK()` annotation
>> + git-compat-util: drop now-unused `UNLEAK()` macro
>>
>> - There are two users of `UNLEAK()` left in our codebase:
>> -
>> - - In "builtin/clone.c", annotating the `repo` variable. That leak has
>> - already been fixed though as you can see in the context, where we do
>> - know to free `repo_to_free`.
>> -
>> - - In "builtin/diff.c", to unleak entries of the `blob[]` array. That
>> - leak has also been fixed, because the entries we assign to that
>> - array come from `rev.pending.objects`, and we do eventually release
>> - `rev`.
>> -
>> - This neatly demonstrates one of the issues with `UNLEAK()`: it is quite
>> - easy for the annotation to become stale. A second issue is that its
>> - whole intent is to paper over leaks. And while that has been a necessary
>> - evil in the past, because Git was leaking left and right, it isn't
>> - really much of an issue nowadays where our test suite has no known leaks
>> + The `UNLEAK()` macro has been introduced with 0e5bba53af (add UNLEAK
>> + annotation for reducing leak false positives, 2017-09-08) to help us
>> + reduce the amount of reported memory leaks in cases we don't care about,
>> + e.g. when exiting immediately afterwards. We have since removed all of
>> + its users in favor of freeing the memory and thus don't need the macro
>> anymore.
>>
>> - Remove the last two users and drop the now-unused `UNLEAK()` annotation.
>> + Remove it.
>>
>> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
>>
>> - ## builtin/clone.c ##
>> -@@ builtin/clone.c: int cmd_clone(int argc,
>> - free(dir);
>> - free(path);
>> - free(repo_to_free);
>> -- UNLEAK(repo);
>> - junk_mode = JUNK_LEAVE_ALL;
>> -
>> - transport_ls_refs_options_release(&transport_ls_refs_options);
>> -
>> - ## builtin/diff.c ##
>> -@@ builtin/diff.c: int cmd_diff(int argc,
>> - release_revisions(&rev);
>> - object_array_clear(&ent);
>> - symdiff_release(&sdiff);
>> -- UNLEAK(blob);
>> - return result;
>> - }
>> -
>> ## git-compat-util.h ##
>> @@ git-compat-util.h: int cmd_main(int, const char **);
>> int common_exit(const char *file, int line, int code);
>> 22: d52ca35b05 = 23: 218132b7d5 t5601: work around leak sanitizer issue
>> 23: 44b48929fb = 24: 832c87a045 t: mark some tests as leak free
>> 24: b1fdbe04be = 25: 5fc1319241 t: remove unneeded !SANITIZE_LEAK prerequisites
>> 25: 2b752b71c6 = 26: 06a1e8b27c test-lib: unconditionally enable leak checking
>> 26: 53bc5f7d75 = 27: 19428158c8 t: remove TEST_PASSES_SANITIZE_LEAK annotations
>
> This iteration addresses all of my comments.
>
> Thanks again for working on this. And congratulations, it's not even
> the end of the year and memory leak checks are opt-out.
Junio, reviewing the range-diff again this morning with what has been
queued I noticed this:
$ git range-diff ea42eb76e7...a0c677023d
[...]
25: 67df655518 ! 26: b2863b3f06 test-lib: unconditionally enable leak checking
@@ Commit message
finally fixed the last leak exposed by our test suite, which means that
we are now basically leak free wherever we have branch coverage.
- From hereon, the Git test suite should ideally stay free of memory
+ >From hereon, the Git test suite should ideally stay free of memory
leaks. Most importantly, any test suite that is being added should
automatically be subject to the leak checker, and if that test does not
pass it is a strong signal that the added code introduced new memory
26: ea42eb76e7 = 27: a0c677023d t: remove TEST_PASSES_SANITIZE_LEAK annotations
>
>>
>> ---
>> base-commit: b0c643d6a710e2b092902a3941655176b358bfd0
>> change-id: 20241111-b4-pks-leak-fixes-pt10-a6fa657f4fac
>>
