From: Junio C Hamano Date: Wed, 30 Sep 2026 16:42:48 GMT Subject: Re: [PATCH 2/2] merge: remember conflict labels Message-ID: In-Reply-To: Phillip Wood writes: > From: Phillip Wood > > When recreating merge conflicts with "git checkout -m " the > original conflict labels are lost. For commands like "git merge" and > "git cherry-pick" we could use the presence of the related root > ref (MERGE_HEAD and CHERRY_PICK_HEAD respectively) to recreate the > labels. However, if the conflicts are from "git stash pop" or "git > checkout -m ", then there is no ref to deduce the labels from. To > ensure the labels are always available, the merge machinery is updated to > write ".git/MERGE_LABELS" when it updates the worktree and > there are conflicts. The labels are then read from that file by "git > checkout -m " when recreating the conflicts. > > As "git checkout -m " calls remove_branch_state() which > ordinarily removes the labels file, we need to pass a flag down > to optionally prevent that so that the labels are available for any > subsequent "git checkout -m ". Note that merge_switch_to_result() > we assign "result->priv" to "opt->priv" and later clear "opt->priv" in > order to get a pointer to the private struct as result->priv is void*. > > Signed-off-by: Phillip Wood > --- > branch.c | 11 ++++++-- > branch.h | 1 + > builtin/checkout.c | 24 +++++++++++++++--- > builtin/commit.c | 1 + > merge-ort.c | 19 ++++++++++++++ > merge.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ > merge.h | 4 +++ > path.c | 1 + > path.h | 1 + > repository.c | 1 + > repository.h | 1 + > sequencer.c | 1 + > t/t7201-co.sh | 21 ++++++++++++++++ > 13 files changed, 143 insertions(+), 6 deletions(-) Where do we talk about MERGE_HEAD and CHERRY_PICK_HEAD in the current documentation set? Do we want to mention MERGE_LABELS alongside them? > + > +int write_merge_labels(struct repository *r, const char *base, > + const char *ours, const char *theirs) > +{ > + FILE *f = fopen_or_warn(git_path_merge_labels(r), "w"); > + > + if (!f) > + return -1; > + > + fprintf(f, "%s\n%s\n%s\n", base, ours, theirs); > + if (fclose(f)) > + return error_errno("could not write '%s'", > + git_path_merge_labels(r)); > + > + return 0; > +} > + We write three items, one per line, delimited by LF. As this goes through stdio, wouldn't Windows write CRLF-delimited lines? I guess if we read this back through stdio, that will cancel out and we get the LF-delimited lines back? Wait. Do we want to read this file via stdio, one line at a time, using three calls to fgets()? No, we do not give a strict upper limit to the length of these labels. So if we read with strbuf_read_line() or something, we would be safe, I guess, but alas there is no such helper function X-<. > +static int parse_merge_label_line(const char **p, char **line) > +{ > + const char *eol = strchr(*p, '\n'); > + > + if (!eol) > + return -1; > + > + *line = xmemdupz(*p, eol - *p); > + *p = eol + 1; > + > + return 0; > +} OK, this reads one line at a time from the file contents already fully read by strbuf_read_file(), as seen below. Which means that the CRLF fprintf() may have written in write_merge_labels() will come back to this function, and our 'ours' may become 'ours\015' after stripping only the LF at the end? > +int read_merge_labels(struct repository *r, > + char **pbase, char** pours, char** ptheirs) > +{ > + struct strbuf buf = STRBUF_INIT; > + const char *p; > + char *base = NULL, *ours = NULL, *theirs = NULL; > + int ret = -1; > + > + if (strbuf_read_file(&buf, git_path_merge_labels(r), 0) < 0) > + return -1; Can strbuf_read_file() fill '.buf' halfway and return a failure, or does it ensure that it frees '.buf' before returning failure? Just double-checking. ... goes and checks ... strbuf_read_file() calls strbuf_read(), which calls read_in_full() to fill a sufficiently large buffer, and a failure from there results in strbuf_release() or strbuf_setlen() resetting back to the '.len' before strbuf_read() was called (i.e., 0 in this case), so we do not leak anything on the error path and this code is safe, I think. > + > + p = buf.buf; > + if (parse_merge_label_line(&p, &base)) > + goto out; > + if (parse_merge_label_line(&p, &ours)) > + goto out; > + if (parse_merge_label_line(&p, &theirs)) > + goto out; OK, we read three things. > + ret = 0; > + *pbase = base; > + *pours = ours; > + *ptheirs = theirs; > +out: > + if (ret) { > + free(base); > + free(ours); > + free(theirs); > + } > + strbuf_release(&buf); OK, so the contract is that we will not touch p{base,ours,theirs} if we return failure, and we will not leak anything when doing so. Which is very sensible. > + return ret; > +} Looking good so far, modulo a small worry about writing via stdio and reading back while bypassing stdio. But perhaps CRLF is so annoying that the compat/mingw layer takes care of all of the above worries by passing the 'binary' bit down to the msvcrt/ucrt layer, in which case we should not have to worry about it. I dunno. Thanks for working on these patches.