Re: [PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include,
>  		strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
>  
>  	ret = start_command(&cmd);
> -	if (ret)
> +	if (ret) {
> +		child_process_clear(&cmd);
>  		return ret;
> +	}

This happens only when start_command() returns an error.  But the
function always calls child_process_clear() before doing so.

So I am not sure if this hunk is needed.  It didn't exist in v1, if
I recall correctly.  Am I missing something obvious?

> @@ -608,9 +612,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	struct pack_geometry *geometry = NULL;
>  	struct strbuf line = STRBUF_INIT;
>  	struct tempfile *refs_snapshot = NULL;
> -	int i, ext, ret;
> +	int i, ext, ret = 0;
>  	FILE *out;
>  	int show_progress = isatty(2);
> +	int cmd_cleared = 0;
>  
>  	/* variables to be filled by option parsing */
>  	int pack_everything = 0;
> @@ -794,7 +799,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  
>  	ret = start_command(&cmd);
>  	if (ret)
> -		return ret;
> +		goto cleanup;

Likewise, at this point, start_command() should have already cleared
cmd before it returned an error, no?

> @@ -818,8 +823,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	}
>  	fclose(out);
>  	ret = finish_command(&cmd);
> +	cmd_cleared = 1;
>  	if (ret)
> -		return ret;
> +		goto cleanup;

And cmd is also cleared after we pass this point.  So perhaps after
the cleanup label, there is no need to call child_process_clear() at
all, no?