Re: [PATCH v2 2/2] merge: remember conflict labels
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 5, 2026, 16:19 UTC
- Message-ID
- <xmqqld8cktlc.fsf@gitster.g>
- In-Reply-To
- <18bdf7df49dde2c8e7f73f3b46c656abb6b26293.1791206658.git.phillip.wood@dunelm.org.uk>
Phillip Wood <phillip.wood123@gmail.com> writes:
Show 5 quoted lines
> @@ -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.
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 16 quoted lines
> @@ -946,7 +957,8 @@ static void report_tracking(struct branch_info *new_branch_info)
>
> static void update_refs_for_switch(const struct checkout_opts *opts,
> struct branch_info *old_branch_info,
> - struct branch_info *new_branch_info)
> + struct branch_info *new_branch_info,
> + bool merge_conflicts)
> {
> struct strbuf msg = STRBUF_INIT;
> const char *old_desc, *reflog_msg;
> @@ -1048,6 +1060,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
> }
> if (!opts->quiet)
> flags |= REMOVE_BRANCH_STATE_VERBOSE;
> + if (merge_conflicts)
> + flags |= REMOVE_BRANCH_STATE_PRESERVE_CONFLICT_LABELS;OK.
Show 11 quoted lines
> remove_branch_state(the_repository, flags);
> strbuf_release(&msg);
> if (!opts->quiet &&
> @@ -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?
Show 12 quoted lines
> diff --git a/builtin/commit.c b/builtin/commit.c > index 205fbd57e3..c374d5e0d5 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1977,6 +1977,7 @@ int cmd_commit(int argc, > > sequencer_post_commit_cleanup(the_repository, 0); > unlink(git_path_merge_head(the_repository)); > + unlink(git_path_merge_labels(the_repository)); > unlink(git_path_merge_msg(the_repository)); > unlink(git_path_merge_mode(the_repository)); > unlink(git_path_squash_msg(the_repository));
Here we clean it up unconditionally after we are about to successfully finish "git commit".
Show 14 quoted lines
> @@ -4969,6 +4973,13 @@ void merge_switch_to_result(struct merge_options *opt,
> return;
> }
> trace2_region_leave("merge", "write_auto_merge", opt->repo);
> +
> + trace2_region_enter("merge", "write_merge_labels", opt->repo);
> + opt->priv = result->priv;
> + write_merge_labels(opt->repo, opt->priv->labels[0], opt->priv->labels[1],
> + opt->priv->labels[2]);
> + opt->priv = NULL;
> + trace2_region_leave("merge", "write_merge_labels", opt->repo);
> }
> if (display_update_msgs)
> merge_display_update_messages(opt, /* detailed */ 0, result);Show 12 quoted lines
> @@ -5234,6 +5245,14 @@ static void move_opt_priv_to_result_priv(struct merge_options *opt,
> * to move it.
> */
> assert(opt->priv && !result->priv);
> + if (!result->clean) {
> + opt->priv->labels[0] =
> + mem_pool_strdup(&opt->priv->pool, opt->ancestor);
> + opt->priv->labels[1] =
> + mem_pool_strdup(&opt->priv->pool, opt->branch1);
> + opt->priv->labels[2] =
> + mem_pool_strdup(&opt->priv->pool, opt->branch2);
> + }OK, merge_switch_to_result() is the only thing that consumes these, and it will never happen after we call merge_finalize() where we destroy the mempool, so this allocation should be safe.
Show 7 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 20 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.
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;
> +}