On 21/06/2023 21:14, Glen Choo wrote:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
@@ -3490,7 +3495,6 @@ static int make_patch(struct repository *r,
return -1;
res |= write_rebase_head(&commit->object.oid);
- strbuf_addf(&buf, "%s/patch", get_dir(opts));
memset(&log_tree_opt, 0, sizeof(log_tree_opt));
repo_init_revisions(r, &log_tree_opt, NULL);
log_tree_opt.abbrev = 0;
I was checking to see if we could remove buf or whether we are reusing
it for unrelated reasons (which is a common Git-ism). We can't remove it
because we reuse it, however...
I had a look at that and we're using it to construct a path that we
should obtain by calling rebase_path_message() - I'll add a fix when I
re-roll.
@@ -3498,7 +3502,7 @@ static int make_patch(struct repository *r,
log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
log_tree_opt.disable_stdin = 1;
log_tree_opt.no_commit_id = 1;
- log_tree_opt.diffopt.file = fopen(buf.buf, "w");
+ log_tree_opt.diffopt.file = fopen(rebase_path_patch(), "w");
log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
if (!log_tree_opt.diffopt.file)
res |= error_errno(_("could not open '%s'"), buf.buf);
this buf.buf was supposed to be the value we populated earlier - this
should be rebase_path_patch() instead.
Oh, well spotted, thanks for pointing that out.
As an aside, I have a mild distaste the Git-ism of reusing "struct
strbuf buf" - using a variable for just a single purpose and naming it
as such makes these sorts of errors much easier to spot. That isn't
something we need to fix here, I'm just venting a little :)
I agree it can get confusing. We occasionally forget to call
strbuf_reset() before reusing the buffer (my first contribution to git
fixed such a case in 4ab867b8fc8 (rebase -i: fix reflog message,
2017-05-18)), or forget to remove a call to reset the buffer that is
no-longer necessary when refactoring. However it does save quite a few
calls to malloc()/free() in the rebase/sequencer code.
Best Wishes
Phillip
