threads / patch / 60630

patchrebase: use strvec_pushf() for format-patch revisions

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

## tl;dr

6 messages between Dec 19, 2023 and Dec 20, 2023. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

René Scharfe· Dec 19, 2023, 07:42 UTC · lore

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>
---
Formatted with --inter-hunk-context=14 for easier review.
 builtin/rebase.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)
Show changes to builtin/rebase.c +6 −12
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 9f8192e0a5..ddde4cbb87 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -582,7 +582,6 @@ static int run_am(struct rebase_options *opts)
 {
 	struct child_process am = CHILD_PROCESS_INIT;
 	struct child_process format_patch = CHILD_PROCESS_INIT;
-	struct strbuf revisions = STRBUF_INIT;
 	int status;
 	char *rebased_patches;

@@ -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
Phillip Wood· Dec 19, 2023, 11:07 UTC · re: René Scharfe · lore

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

Hi René
On 19/12/2023 07:42, René Scharfe wrote:
> 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.

This looks like a nice simplification and the extra context lines in the patch are much appreciated

Thanks
Phillip
Show 74 quoted lines
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Formatted with --inter-hunk-context=14 for easier review.
> 
>   builtin/rebase.c | 17 ++++++-----------
>   1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 9f8192e0a5..ddde4cbb87 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -582,7 +582,6 @@ static int run_am(struct rebase_options *opts)
>   {
>   	struct child_process am = CHILD_PROCESS_INIT;
>   	struct child_process format_patch = CHILD_PROCESS_INIT;
> -	struct strbuf revisions = STRBUF_INIT;
>   	int status;
>   	char *rebased_patches;
> 
> @@ -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
>
Patrick Steinhardt· Dec 19, 2023, 12:25 UTC · re: René Scharfe · lore

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

On Tue, Dec 19, 2023 at 08:42:18AM +0100, René Scharfe wrote:
Show 5 quoted lines
> 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>
Thanks, this simplification looks good to me!
Patrick
Show 73 quoted lines
> ---
> Formatted with --inter-hunk-context=14 for easier review.
> 
>  builtin/rebase.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 9f8192e0a5..ddde4cbb87 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -582,7 +582,6 @@ static int run_am(struct rebase_options *opts)
>  {
>  	struct child_process am = CHILD_PROCESS_INIT;
>  	struct child_process format_patch = CHILD_PROCESS_INIT;
> -	struct strbuf revisions = STRBUF_INIT;
>  	int status;
>  	char *rebased_patches;
> 
> @@ -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
>
Junio C Hamano· Dec 19, 2023, 17:12 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 6 quoted lines
> 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.

Show 54 quoted lines
> @@ -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
René Scharfe· Dec 20, 2023, 08:04 UTC · re: Junio C Hamano · lore

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

Am 19.12.23 um 18:12 schrieb Junio C Hamano:
Show 14 quoted lines
> 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).

Show 55 quoted lines
>
>> @@ -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
Junio C Hamano· Dec 20, 2023, 15:57 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 20 quoted lines
> 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).

Perhaps I'll amend the end of the log message, like so, before merging it down to 'next', then.

    ..., simplifying the code and plugging a small leak on the error
    codepath.
Thanks.

← back to recent threads