Re: [PATCH v2 2/2] merge: remember conflict labels

On 05/10/2026 17:19, Junio C Hamano wrote:

Phillip Wood [off-list ref] writes:

quoted

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

quoted

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

quoted

+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?

quoted

+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.

quoted

+{
+	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

quoted

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