Re: [PATCH 2/2] merge: remember conflict labels
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2026, 16:42 UTC
- Message-ID
- <xmqq1paad71z.fsf@gitster.g>
- In-Reply-To
- <fdaf3da993366878b51bd0b2a950888710cafb8a.1790761727.git.phillip.wood@dunelm.org.uk>
Phillip Wood <phillip.wood123@gmail.com> writes:
Show 36 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk> > > When recreating merge conflicts with "git checkout -m <path>" 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 <branch>", 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 <path>" when recreating the conflicts. > > As "git checkout -m <branch>" 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 <path>". 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 <phillip.wood@dunelm.org.uk> > --- > 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?
Show 17 quoted lines
> +
> +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-<.
Show 12 quoted lines
> +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?
Show 10 quoted lines
> +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.
Show 8 quoted lines
> + > + 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.
Show 11 quoted lines
> + 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.