Re: [PATCH v2 04/13] fast-rebase: write conflict state to working tree, index, and HEAD

[Elijah Newren <newren@gmail.com>]

On Mon, May 17, 2021 at 6:32 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Previously, when fast-rebase hit a conflict, it simply aborted and left
> > HEAD, the index, and the working tree where they were before the
> > operation started.  While fast-rebase does not support restarting from a
> > conflicted state, write the conflicted state out anyway as it gives us a
> > way to see what the conflicts are and write tests that check for them.
> >
> > This will be important in the upcoming commits, because sequencer.c is
> > only superficially integrated with merge-ort.c; in particular, it calls
> > merge_switch_to_result() after EACH merge instead of only calling it at
> > the end of all the sequence of merges (or when a conflict is hit).  This
> > not only causes needless updates to the working copy and index, but also
> > causes all intermediate data to be freed and tossed, preventing caching
> > information from one merge to the next.  However, integrating
> > sequencer.c more deeply with merge-ort.c is a big task, and making this
> > small extension to fast-rebase.c provides us with a simple way to test
> > the edge and corner cases that we want to make sure continue working.
>
> This seems a noble goal. I'm worried about the ramifications of such
> a change, specifically about the state an automated process would be in
> after hitting a conflict.

Why would an automated process be using test-helper code?  Note that
this is code from t/helper/test-fast-rebase.c.

> If I understand correctly, then the old code would abort the rebase and
> go back to the previous HEAD location. The new code will pause the
> rebase at the previous commit and leave the conflict markers in the
> working directory.

Correct; it now behaves slightly more like a "normal" rebase, but it
still doesn't write state files necessary for resuming the rebase
operation.

> The critical change is here:
>
> > -     strbuf_addf(&reflog_msg, "finish rebase %s onto %s",
> > -                 oid_to_hex(&last_picked_commit->object.oid),
> > -                 oid_to_hex(&last_commit->object.oid));
> > -     if (update_ref(reflog_msg.buf, branch_name.buf,
> > -                    &last_commit->object.oid,
> > -                    &last_picked_commit->object.oid,
> > -                    REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) {
> > -             error(_("could not update %s"), argv[4]);
> > -             die("Failed to update %s", argv[4]);
> > +     if (result.clean) {
> > +             fprintf(stderr, "\nDone.\n");
> > +             strbuf_addf(&reflog_msg, "finish rebase %s onto %s",
> > +                         oid_to_hex(&last_picked_commit->object.oid),
> > +                         oid_to_hex(&last_commit->object.oid));
> > +             if (update_ref(reflog_msg.buf, branch_name.buf,
> > +                            &last_commit->object.oid,
> > +                            &last_picked_commit->object.oid,
> > +                            REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) {
> > +                     error(_("could not update %s"), argv[4]);
> > +                     die("Failed to update %s", argv[4]);
> > +             }
> > +             if (create_symref("HEAD", branch_name.buf, reflog_msg.buf) < 0)
> > +                     die(_("unable to update HEAD"));
> > +             strbuf_release(&reflog_msg);
> > +             strbuf_release(&branch_name);
> > +
> > +             prime_cache_tree(the_repository, the_repository->index,
> > +                              result.tree);
> > +     } else {
> > +             fprintf(stderr, "\nAborting: Hit a conflict.\n");
> > +             strbuf_addf(&reflog_msg, "rebase progress up to %s",
> > +                         oid_to_hex(&last_picked_commit->object.oid));
> > +             if (update_ref(reflog_msg.buf, "HEAD",
> > +                            &last_commit->object.oid,
> > +                            &head,
> > +                            REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) {
> > +                     error(_("could not update %s"), argv[4]);
> > +                     die("Failed to update %s", argv[4]);
> > +             }
> >       }
> > -     if (create_symref("HEAD", branch_name.buf, reflog_msg.buf) < 0)
> > -             die(_("unable to update HEAD"));
> > -     strbuf_release(&reflog_msg);
> > -     strbuf_release(&branch_name);
> > -
> > -     prime_cache_tree(the_repository, the_repository->index, result.tree);
>
> So perhaps this could use a test case to demonstrate the change in
> behavior? Such a test would want to be created before this commit, then
> the behavior change is provided as an edit to the test in this commit.
>
> But maybe I'm misunderstanding the change here and such a test is
> inappropriate.

If this weren't code under t/helper/, I'd agree whole-heartedly with
the suggestion.