Re: [PATCH v2 2/6] rebase -i: remove patch file after conflict resolution

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 14, 2023, 10:08 UTC
Message-ID: <55dd6194-25e5-1a66-9c39-27cb19bfbb3c@gmail.com>
In-Reply-To: <kl6lsfakr2sr.fsf@chooglen-macbookpro.roam.corp.google.com>

On 21/06/2023 21:14, Glen Choo wrote:

Show 14 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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.

Show 12 quoted lines

>> @@ -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