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

Commit Message

Elijah Newren May 4, 2021, 2:12 a.m. UTC

From: Elijah Newren <[e-mail]>

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.

Signed-off-by: Elijah Newren <[e-mail]>
---
 t/helper/test-fast-rebase.c | 51 +++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 19 deletions(-)

Comments

Derrick Stolee May 17, 2021, 1:32 p.m. UTC | #1

On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> 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.

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.

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.

Thanks,
-Stolee

Elijah Newren May 18, 2021, 3:42 a.m. UTC | #2

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

Derrick Stolee May 18, 2021, 1:54 p.m. UTC | #3

On 5/17/2021 11:42 PM, Elijah Newren wrote:
> On Mon, May 17, 2021 at 6:32 AM Derrick Stolee <[e-mail]> wrote:
>>
>> On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <[e-mail]>
>>>
>>> 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.
...
>> 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.

You're right. Ignore me.

-Stolee

Patch

diff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c
index 39fb7f41e8c1..fc2d46090434 100644
--- a/t/helper/test-fast-rebase.c
+++ b/t/helper/test-fast-rebase.c
@@ -91,7 +91,6 @@  int cmd__fast_rebase(int argc, const char **argv)
 	struct commit *last_commit = NULL, *last_picked_commit = NULL;
 	struct object_id head;
 	struct lock_file lock = LOCK_INIT;
-	int clean = 1;
 	struct strvec rev_walk_args = STRVEC_INIT;
 	struct rev_info revs;
 	struct commit *commit;
@@ -176,11 +175,10 @@  int cmd__fast_rebase(int argc, const char **argv)
 		free((char*)merge_opt.ancestor);
 		merge_opt.ancestor = NULL;
 		if (!result.clean)
-			die("Aborting: Hit a conflict and restarting is not implemented.");
+			break;
 		last_picked_commit = commit;
 		last_commit = create_commit(result.tree, commit, last_commit);
 	}
-	fprintf(stderr, "\nDone.\n");
 	/* TODO: There should be some kind of rev_info_free(&revs) call... */
 	memset(&revs, 0, sizeof(revs));
 
@@ -189,24 +187,39 @@  int cmd__fast_rebase(int argc, const char **argv)
 	if (result.clean < 0)
 		exit(128);
 
-	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);
 	if (write_locked_index(&the_index, &lock,
 			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		die(_("unable to write %s"), get_index_file());
-	return (clean == 0);
+	return (result.clean == 0);
 }