* [PATCH] rebase: use strvec_pushf() for format-patch revisions
@ 2023-12-19 7:42 René Scharfe
2023-12-19 11:07 ` Phillip Wood
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: René Scharfe @ 2023-12-19 7:42 UTC (permalink / raw)
To: Git List
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
2023-12-19 7:42 [PATCH] rebase: use strvec_pushf() for format-patch revisions René Scharfe
@ 2023-12-19 11:07 ` Phillip Wood
2023-12-19 12:25 ` Patrick Steinhardt
2023-12-19 17:12 ` Junio C Hamano
2 siblings, 0 replies; 6+ messages in thread
From: Phillip Wood @ 2023-12-19 11:07 UTC (permalink / raw)
To: René Scharfe, Git List
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
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
2023-12-19 7:42 [PATCH] rebase: use strvec_pushf() for format-patch revisions René Scharfe
2023-12-19 11:07 ` Phillip Wood
@ 2023-12-19 12:25 ` Patrick Steinhardt
2023-12-19 17:12 ` Junio C Hamano
2 siblings, 0 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2023-12-19 12:25 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]
On Tue, Dec 19, 2023 at 08:42:18AM +0100, 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.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
Thanks, this simplification looks good to me!
Patrick
> ---
> 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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
2023-12-19 7:42 [PATCH] rebase: use strvec_pushf() for format-patch revisions René Scharfe
2023-12-19 11:07 ` Phillip Wood
2023-12-19 12:25 ` Patrick Steinhardt
@ 2023-12-19 17:12 ` Junio C Hamano
2023-12-20 8:04 ` René Scharfe
2 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2023-12-19 17:12 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
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.
> @@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
2023-12-19 17:12 ` Junio C Hamano
@ 2023-12-20 8:04 ` René Scharfe
2023-12-20 15:57 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: René Scharfe @ 2023-12-20 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
2023-12-20 8:04 ` René Scharfe
@ 2023-12-20 15:57 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2023-12-20 15:57 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
René Scharfe <l.s.r@web.de> writes:
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-20 15:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-19 7:42 [PATCH] rebase: use strvec_pushf() for format-patch revisions René Scharfe
2023-12-19 11:07 ` Phillip Wood
2023-12-19 12:25 ` Patrick Steinhardt
2023-12-19 17:12 ` Junio C Hamano
2023-12-20 8:04 ` René Scharfe
2023-12-20 15:57 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).