On Mon, May 17, 2021 at 6:32 AM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>
> On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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.
