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

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

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

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