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

[Elijah Newren <newren@gmail.com>]

On Mon, Mar 8, 2021 at 5:11 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> 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

Looks like your comment got garbled/truncated.  Do you remember the
rest of what you were going to say here?

> >       }
> >
> >       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()?

Perhaps, but I think that should be a follow-on series or
#leftoverbits.  I'm having enough trouble getting reviews (I think I'm
burning Stolee out after the last half year) without making my series
longer for tangential cleanups.  :-)

I'm starting to worry that despite having all the remaining patches
ready (and most have been ready for months), that we won't be able to
get merge-ort done before git-2.32 -- the -rc0 for it is now just
under three months away.

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