From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 06 Oct 2026 15:21:43 GMT
Subject: Re: [PATCH v2 2/2] merge: remember conflict labels
Message-ID: <b19fc30c-290d-471c-a5b8-57f44339f243@gmail.com>
In-Reply-To: <xmqqld8cktlc.fsf@gitster.g>

On 05/10/2026 17:19, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> >> @@ -128,6 +128,7 @@ int validate_branchname(const char *name, struct strbuf *ref);
>>   int validate_new_branchname(const char *name, struct strbuf *ref, int force);
>>   >>   #define REMOVE_BRANCH_STATE_VERBOSE (1u << 0)
>> +#define REMOVE_BRANCH_STATE_PRESERVE_CONFLICT_LABELS (1u << 1)
> > Not complaining and I have no improvement suggestions, but this
> phrasing made me imagine that we would be passing this flag bit
> in code paths where we want to write the extra file out.

I can see why you'd think that, would appending "_FILE" make it clearer? (though the name is long enough already)
> But that does not match the reality.  merge_switch_to_result() calls
> write_merge_labels() unconditionally.  The bit controls if the file
> written survives the clean-up after the operation.

>> @@ -1262,7 +1276,9 @@ static int switch_branches(const struct checkout_opts *opts,
>>   >>   	if (autostash_res == STASH_APPLY_CONFLICT && !opts->quiet)
>>   		fputc('\n', stderr);
>> -	update_refs_for_switch(opts, &old_branch_info, new_branch_info);
>> +
>> +	update_refs_for_switch(opts, &old_branch_info, new_branch_info,
>> +			       autostash_res == STASH_APPLY_CONFLICT);
> > OK, so here we assume STASH_APPLY_CONFLICT result means we called
> write_merge_labels() and left the file.  If not, we did not call it
> and the file should not be there.
> > But then can't we just unconditionally leave the file, instead of
> not removing what we wouldn't have created?

Hmm, If a previous command such as "git stash pop" or "git checkout -m" had conflicts and wrote the file, and then the user resolves the conflicts and runs "git checkout" without "-m" (or with "-m" without creating conflicts) don't we want to remove the file?

>> +static char *parse_merge_label_line(struct strbuf *buf, FILE *fp)
>> +{
>> +	if (strbuf_getline(buf, fp) == EOF)
>> +		return NULL;
>> +
>> +	return xmemdupz(buf->buf, buf->len);
>> +}
> > Wouldn't strbuf_detach() be more intuitive?
> >> +int read_merge_labels(struct repository *r,
>> +		      char **pbase, char** pours, char** ptheirs)
> > Be consistent.  Asterisk sticks to variables, not types.

Oops, I'll fix those.

>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +	char *base = NULL, *ours = NULL, *theirs = NULL;
>> +	int ret = -1;
>> +	FILE *fp = fopen(git_path_merge_labels(r), "r");
>> +
>> +	if (!fp)
>> +		return -1;
>> +
>> +	base = parse_merge_label_line(&buf, fp);
>> +	if (!base)
>> +		goto out;
>> +
>> +	ours = parse_merge_label_line(&buf, fp);
>> +	if (!ours)
>> +		goto out;
>> +
>> +	theirs = parse_merge_label_line(&buf, fp);
>> +	if (!theirs)
>> +		goto out;
> > The repetitions are a bit annoying, but it does not get much better:
> > 	int i;
> 	char bot[3] = {0}; /* base, ours, theirs */
> > 	for (i = 0; i < ARRAY_SIZE(bot); i++)
>          	if (!(bot[i] = parse_merge_label_line(&buf, fp)))
> 			goto out;
> > so I am OK with what was posted.

Yeah, they are a bit annoying, but as there are only three of them it isn't too bad.

> It may be helpful to future developers to leave a comment that we
> deliberately ignore cruft after these three lines in the file and
> why, instead of diagnosing it as an error.

Will do

Thanks

Phillip

>> +	ret = 0;
>> +	*pbase = base;
>> +	*pours = ours;
>> +	*ptheirs = theirs;
>> +out:
>> +	if (ret) {
>> +		free(base);
>> +		free(ours);
>> +		free(theirs);
>> +	}
>> +	fclose(fp);
>> +	strbuf_release(&buf);
>> +
>> +	return ret;
>> +}