Re: [PATCH 10/11] merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> There are a variety of questions users might ask while resolving
> conflicts:
>   * What changes have been made since the previous (first) parent?
>   * What changes are staged?
>   * What is still unstaged? (or what is still conflicted?)
>   * What changes did I make to resolve conflicts so far?
> The first three of these have simple answers:
>   * git diff HEAD
>   * git diff --cached
>   * git diff
> There was no way to answer the final question previously.  Adding one
> is trivial in merge-ort, since it works by creating a tree representing
> what should be written to the working copy complete with conflict
> markers.  Simply write that tree to .git/AUTO_MERGE, allowing users to
> answer the fourth question with
>   * git diff AUTO_MERGE
>
> I avoided using a name like "MERGE_AUTO", because that would be
> merge-specific (much like MERGE_HEAD, REBASE_HEAD, REVERT_HEAD,
> CHERRY_PICK_HEAD) and I wanted a name that didn't change depending on
> which type of operation the merge was part of.

That's a really cool feature. I'm starting to like this "ort" thing :)

(After knowing almost nothing about it until a few days ago...)

> Ensure that paths which clean out other temporary operation-specific
> files (e.g. CHERRY_PICK_HEAD, MERGE_MSG, rebase-merge/ state directory)
> also clean out this AUTO_MERGE file.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  branch.c         |  1 +
>  builtin/rebase.c |  1 +
>  merge-ort.c      | 10 ++++++++++
>  path.c           |  1 +
>  path.h           |  2 ++
>  sequencer.c      |  5 +++++
>  6 files changed, 20 insertions(+)
>
> diff --git a/branch.c b/branch.c
> index 9c9dae1eae32..b71a2de29dbe 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r)
>  	unlink(git_path_merge_rr(r));
>  	unlink(git_path_merge_msg(r));
>  	unlink(git_path_merge_mode(r));
> +	unlink(git_path_auto_merge(r));
>  	save_autostash(git_path_merge_autostash(r));
>  }
>  
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index de400f9a1973..7d9afe118fd4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -739,6 +739,7 @@ static int finish_rebase(struct rebase_options *opts)
>  	int ret = 0;
>  
>  	delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
> +	unlink(git_path_auto_merge(the_repository));
>  	apply_autostash(state_dir_path("autostash", opts));
>  	close_object_store(the_repository->objects);
>  	/*
> diff --git a/merge-ort.c b/merge-ort.c
> index 37b69cbe0f9a..cf927cd160e1 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3362,6 +3362,9 @@ void merge_switch_to_result(struct merge_options *opt,
>  {
>  	assert(opt->priv == NULL);
>  	if (result->clean >= 0 && update_worktree_and_index) {
> +		const char *filename;
> +		FILE *fp;
> +
>  		trace2_region_enter("merge", "checkout", opt->repo);
>  		if (checkout(opt, head, result->tree)) {
>  			/* failure to function */
> @@ -3380,6 +3383,13 @@ void merge_switch_to_result(struct merge_options *opt,
>  		}
>  		opt->priv = NULL;
>  		trace2_region_leave("merge", "record_conflicted", opt->repo);
> +
> +		trace2_region_enter("merge", "write_auto_merge", opt->repo);
> +		filename = git_path_auto_merge(opt->repo);
> +		fp = xfopen(filename, "w");
> +		fprintf(fp, "%s\n", oid_to_hex(&result->tree->object.oid));
> +		fclose(fp);
> +		trace2_region_leave("merge", "write_auto_merge", opt->repo);

This isn't a new problem since you're just folling an existing pattern,
but here you (rightly) do xopen()< and the:n

>  	}
>  
>  	if (display_update_msgs) {
> diff --git a/path.c b/path.c
> index 7b385e5eb282..9e883eb52446 100644
> --- a/path.c
> +++ b/path.c
> @@ -1534,5 +1534,6 @@ REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
>  REPO_GIT_PATH_FUNC(merge_mode, "MERGE_MODE")
>  REPO_GIT_PATH_FUNC(merge_head, "MERGE_HEAD")
>  REPO_GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH")
> +REPO_GIT_PATH_FUNC(auto_merge, "AUTO_MERGE")
>  REPO_GIT_PATH_FUNC(fetch_head, "FETCH_HEAD")
>  REPO_GIT_PATH_FUNC(shallow, "shallow")
> diff --git a/path.h b/path.h
> index e7e77da6aaa5..251c78d98000 100644
> --- a/path.h
> +++ b/path.h
> @@ -176,6 +176,7 @@ struct path_cache {
>  	const char *merge_mode;
>  	const char *merge_head;
>  	const char *merge_autostash;
> +	const char *auto_merge;
>  	const char *fetch_head;
>  	const char *shallow;
>  };
> @@ -191,6 +192,7 @@ const char *git_path_merge_rr(struct repository *r);
>  const char *git_path_merge_mode(struct repository *r);
>  const char *git_path_merge_head(struct repository *r);
>  const char *git_path_merge_autostash(struct repository *r);
> +const char *git_path_auto_merge(struct repository *r);
>  const char *git_path_fetch_head(struct repository *r);
>  const char *git_path_shallow(struct repository *r);
>  
> diff --git a/sequencer.c b/sequencer.c
> index d2332d3e1787..472cdd8c620d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2096,6 +2096,7 @@ static int do_pick_commit(struct repository *r,
>  		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
>  				NULL, 0);
>  		unlink(git_path_merge_msg(r));
> +		unlink(git_path_auto_merge(r));

Shouldn't this & the rest ideally be at least unlink_or_warn()?

>  		fprintf(stderr,
>  			_("dropping %s %s -- patch contents already upstream\n"),
>  			oid_to_hex(&commit->object.oid), msg.subject);
> @@ -2451,6 +2452,8 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
>  		need_cleanup = 1;
>  	}
>  
> +	unlink(git_path_auto_merge(r));
> +
>  	if (!need_cleanup)
>  		return;
>  
> @@ -4111,6 +4114,7 @@ static int pick_commits(struct repository *r,
>  			unlink(rebase_path_stopped_sha());
>  			unlink(rebase_path_amend());
>  			unlink(git_path_merge_head(r));
> +			unlink(git_path_auto_merge(r));
>  			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
>  
>  			if (item->command == TODO_BREAK) {
> @@ -4505,6 +4509,7 @@ static int commit_staged_changes(struct repository *r,
>  		return error(_("could not commit staged changes."));
>  	unlink(rebase_path_amend());
>  	unlink(git_path_merge_head(r));
> +	unlink(git_path_auto_merge(r));
>  	if (final_fixup) {
>  		unlink(rebase_path_fixup_msg());
>  		unlink(rebase_path_squash_msg());