Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions

["René Scharfe" <l.s.r@web.de>]

Am 19.12.23 um 18:12 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> In run_am(), a strbuf is used to create a revision argument that is then
>> added to the argument list for git format-patch using strvec_push().
>> Use strvec_pushf() to add it directly instead, simplifying the code.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>
> Makes sense.  Between the location of the original strbuf_addf()
> call and the new strvec_pushf() call, nobody mucks with *opts so
> this change won't affect the correctness.  We no longer use the
> extra strbuf, and upon failing to open the rebased-patches file,
> we no longer leak the contents of it.  Good.

Ha!  I didn't even notice that leak on error.  Perhaps the two release
calls at the end gave me the illusion of cleanliness?  Or more likely
the opportunity to use strvec_pushf() grabbed my full attention (tunnel
vision).

>
>> @@ -615,34 +614,32 @@ static int run_am(struct rebase_options *opts)
>>  		return run_command(&am);
>>  	}
>>
>> -	strbuf_addf(&revisions, "%s...%s",
>> -		    oid_to_hex(opts->root ?
>> -			       /* this is now equivalent to !opts->upstream */
>> -			       &opts->onto->object.oid :
>> -			       &opts->upstream->object.oid),
>> -		    oid_to_hex(&opts->orig_head->object.oid));
>> -
>>  	rebased_patches = xstrdup(git_path("rebased-patches"));
>>  	format_patch.out = open(rebased_patches,
>>  				O_WRONLY | O_CREAT | O_TRUNC, 0666);
>>  	if (format_patch.out < 0) {
>>  		status = error_errno(_("could not open '%s' for writing"),
>>  				     rebased_patches);
>>  		free(rebased_patches);
>>  		strvec_clear(&am.args);
>>  		return status;
>>  	}
>>
>>  	format_patch.git_cmd = 1;
>>  	strvec_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
>>  		     "--full-index", "--cherry-pick", "--right-only",
>>  		     "--default-prefix", "--no-renames",
>>  		     "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
>>  		     "--no-base", NULL);
>>  	if (opts->git_format_patch_opt.len)
>>  		strvec_split(&format_patch.args,
>>  			     opts->git_format_patch_opt.buf);
>> -	strvec_push(&format_patch.args, revisions.buf);
>> +	strvec_pushf(&format_patch.args, "%s...%s",
>> +		     oid_to_hex(opts->root ?
>> +				/* this is now equivalent to !opts->upstream */
>> +				&opts->onto->object.oid :
>> +				&opts->upstream->object.oid),
>> +		     oid_to_hex(&opts->orig_head->object.oid));
>>  	if (opts->restrict_revision)
>>  		strvec_pushf(&format_patch.args, "^%s",
>>  			     oid_to_hex(&opts->restrict_revision->object.oid));
>> @@ -665,10 +662,8 @@ static int run_am(struct rebase_options *opts)
>>  			"As a result, git cannot rebase them."),
>>  		      opts->revisions);
>>
>> -		strbuf_release(&revisions);
>>  		return status;
>>  	}
>> -	strbuf_release(&revisions);
>>
>>  	am.in = open(rebased_patches, O_RDONLY);
>>  	if (am.in < 0) {
>> --
>> 2.43.0