Re: [PATCH v2 2/2] merge: remember conflict labels
On 05/10/2026 17:19, Junio C Hamano wrote:
Show 8 quoted lines
> 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.
Show 12 quoted lines
>> @@ -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?
Show 11 quoted lines
>> +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.Show 27 quoted lines
>> +{
>> + 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.
Show 15 quoted lines
>> + ret = 0;
>> + *pbase = base;
>> + *pours = ours;
>> + *ptheirs = theirs;
>> +out:
>> + if (ret) {
>> + free(base);
>> + free(ours);
>> + free(theirs);
>> + }
>> + fclose(fp);
>> + strbuf_release(&buf);
>> +
>> + return ret;
>> +}