Re: [PATCH v2 00/27] Memory leak fixes (pt.10, final)

>> 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@pks.im>
>>      
>>       ## 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@pks.im>
>>      
>>     - ## 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.

>
>>
>> ---
>> base-commit: b0c643d6a710e2b092902a3941655176b358bfd0
>> change-id: 20241111-b4-pks-leak-fixes-pt10-a6fa657f4fac
>>