Re: [PATCH v2 11/27] git: refactor builtin handling to use a `struct strvec`

[Toon Claes <toon@iotcl.com>]

Patrick Steinhardt <ps@pks.im> writes:

> Similar as with the preceding commit, `handle_builtin()` does not
> properly track lifetimes of the `argv` array and its strings. As it may
> end up modifying the array this can lead to memory leaks in case it
> contains allocated strings.
>
> Refactor the function to use a `struct strvec` instead.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  git.c                  | 66 ++++++++++++++++++++++++--------------------------
>  t/t0211-trace2-perf.sh |  2 +-
>  2 files changed, 32 insertions(+), 36 deletions(-)
>
> diff --git a/git.c b/git.c
> index 88356afe5fb568ccc147f055e3ab253c53a1befa..159dd45b08204c4a89d1dc4ab6990978e2454eb6 100644
> --- a/git.c
> +++ b/git.c
> @@ -696,63 +696,57 @@ void load_builtin_commands(const char *prefix, struct cmdnames *cmds)
>  }
>  
>  #ifdef STRIP_EXTENSION
> -static void strip_extension(const char **argv)
> +static void strip_extension(struct strvec *args)
>  {
>  	size_t len;
>  
> -	if (strip_suffix(argv[0], STRIP_EXTENSION, &len))
> -		argv[0] = xmemdupz(argv[0], len);
> +	if (strip_suffix(args->v[0], STRIP_EXTENSION, &len)) {
> +		char *stripped = xmemdupz(args->v[0], len);
> +		strvec_replace(args, 0, stripped);
> +		free(stripped);
> +	}
>  }
>  #else
>  #define strip_extension(cmd)
>  #endif
>  
> -static void handle_builtin(int argc, const char **argv)
> +static void handle_builtin(struct strvec *args)
>  {
> -	struct strvec args = STRVEC_INIT;
> -	const char **argv_copy = NULL;
>  	const char *cmd;
>  	struct cmd_struct *builtin;
>  
> -	strip_extension(argv);
> -	cmd = argv[0];
> +	strip_extension(args);
> +	cmd = args->v[0];
>  
>  	/* Turn "git cmd --help" into "git help --exclude-guides cmd" */
> -	if (argc > 1 && !strcmp(argv[1], "--help")) {
> -		int i;
> -
> -		argv[1] = argv[0];
> -		argv[0] = cmd = "help";
> -
> -		for (i = 0; i < argc; i++) {
> -			strvec_push(&args, argv[i]);
> -			if (!i)
> -				strvec_push(&args, "--exclude-guides");
> -		}
> +	if (args->nr > 1 && !strcmp(args->v[1], "--help")) {
> +		const char *exclude_guides_arg[] = { "--exclude-guides" };
> +
> +		strvec_replace(args, 1, args->v[0]);
> +		strvec_replace(args, 0, "help");
> +		cmd = "help";
> +		strvec_splice(args, 2, 0, exclude_guides_arg,
> +			      ARRAY_SIZE(exclude_guides_arg));
> +	}
>  
> -		argc++;
> +	builtin = get_builtin(cmd);
> +	if (builtin) {
> +		const char **argv_copy = NULL;
> +		int ret;
>  
>  		/*
>  		 * `run_builtin()` will modify the argv array, so we need to
>  		 * create a shallow copy such that we can free all of its
>  		 * strings.
>  		 */
> -		CALLOC_ARRAY(argv_copy, argc + 1);
> -		COPY_ARRAY(argv_copy, args.v, argc);
> +		if (args->nr)
> +			DUP_ARRAY(argv_copy, args->v, args->nr + 1);
>  
> -		argv = argv_copy;
> -	}
> -
> -	builtin = get_builtin(cmd);
> -	if (builtin) {
> -		int ret = run_builtin(builtin, argc, argv, the_repository);
> -		strvec_clear(&args);
> +		ret = run_builtin(builtin, args->nr, argv_copy, the_repository);
> +		strvec_clear(args);
>  		free(argv_copy);
>  		exit(ret);
>  	}
> -
> -	strvec_clear(&args);
> -	free(argv_copy);
>  }

Just want to give you a little shout out how you've reorganized the code
and it now makes a lot more sense how `argv_copy` is used by putting
it inside the `if (builtin)` code block.

-- 
Toon