| Message ID | 4ab7431c-6c1b-448c-b4d2-e8b9be0e4eef@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: use strvec_pushf() for format-patch revisions | expand |
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 >
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 >
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
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
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.
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) {
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(-) -- 2.43.0