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

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

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

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

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

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

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

> @@ -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);


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

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

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

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