git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Derrick Stolee <dstolee@microsoft.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Taylor Blau <me@ttaylorr.com>, Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v2 04/13] fast-rebase: write conflict state to working tree, index, and HEAD
Date: Mon, 17 May 2021 09:32:14 -0400	[thread overview]
Message-ID: <30c23689-cdab-143e-159c-93e50c808430@gmail.com> (raw)
In-Reply-To: <887b151c26ff0f175f2da431d77cd377bd066990.1620094339.git.gitgitgadget@gmail.com>

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.

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

  reply	other threads:[~2021-05-17 13:32 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-24 21:32 [PATCH 0/7] Optimization batch 11: avoid repeatedly detecting same renames Elijah Newren via GitGitGadget
2021-03-24 21:32 ` [PATCH 1/7] merge-ort: add data structures for in-memory caching of rename detection Elijah Newren via GitGitGadget
2021-03-24 21:32 ` [PATCH 2/7] merge-ort: populate caches of rename detection results Elijah Newren via GitGitGadget
2021-03-24 21:32 ` [PATCH 3/7] merge-ort: add code to check for whether cached renames can be reused Elijah Newren via GitGitGadget
2021-03-24 21:32 ` [PATCH 4/7] merge-ort: avoid accidental API mis-use Elijah Newren via GitGitGadget
2021-03-24 21:32 ` [PATCH 5/7] merge-ort: preserve cached renames for the appropriate side Elijah Newren via GitGitGadget
2021-03-24 21:32 ` [PATCH 6/7] merge-ort: add helper functions for using cached renames Elijah Newren via GitGitGadget
2021-03-24 21:32 ` [PATCH 7/7] merge-ort, diffcore-rename: employ cached renames when possible Elijah Newren via GitGitGadget
2021-03-24 22:04 ` [PATCH 0/7] Optimization batch 11: avoid repeatedly detecting same renames Junio C Hamano
2021-03-24 23:25   ` Elijah Newren
2021-03-25 18:59     ` Junio C Hamano
2021-03-29 22:34       ` Elijah Newren
2021-03-30 12:07         ` Derrick Stolee
2021-05-04  2:12 ` [PATCH v2 00/13] " Elijah Newren via GitGitGadget
2021-05-04  2:12   ` [PATCH v2 01/13] t6423: rename file within directory that other side renamed Elijah Newren via GitGitGadget
2021-05-04  2:12   ` [PATCH v2 02/13] Documentation/technical: describe remembering renames optimization Elijah Newren via GitGitGadget
2021-05-04  2:12   ` [PATCH v2 03/13] fast-rebase: change assert() to BUG() Elijah Newren via GitGitGadget
2021-05-04  2:12   ` [PATCH v2 04/13] fast-rebase: write conflict state to working tree, index, and HEAD Elijah Newren via GitGitGadget
2021-05-17 13:32     ` Derrick Stolee [this message]
2021-05-18  3:42       ` Elijah Newren
2021-05-18 13:54         ` Derrick Stolee
2021-05-04  2:12   ` [PATCH v2 05/13] t6429: testcases for remembering renames Elijah Newren via GitGitGadget
2021-05-04  2:12   ` [PATCH v2 06/13] merge-ort: add data structures for in-memory caching of rename detection Elijah Newren via GitGitGadget
2021-05-17 13:41     ` Derrick Stolee
2021-05-18  3:55       ` Elijah Newren
2021-05-18 13:57         ` Derrick Stolee
2021-05-04  2:12   ` [PATCH v2 07/13] merge-ort: populate caches of rename detection results Elijah Newren via GitGitGadget
2021-05-17 13:51     ` Derrick Stolee
2021-05-20  0:48       ` Elijah Newren
2021-05-04  2:12   ` [PATCH v2 08/13] merge-ort: add code to check for whether cached renames can be reused Elijah Newren via GitGitGadget
2021-05-17 14:01     ` Derrick Stolee
2021-05-04  2:12   ` [PATCH v2 09/13] merge-ort: avoid accidental API mis-use Elijah Newren via GitGitGadget
2021-05-17 14:10     ` Derrick Stolee
2021-05-04  2:12   ` [PATCH v2 10/13] merge-ort: preserve cached renames for the appropriate side Elijah Newren via GitGitGadget
2021-05-04  2:12   ` [PATCH v2 11/13] merge-ort: add helper functions for using cached renames Elijah Newren via GitGitGadget
2021-05-04  2:12   ` [PATCH v2 12/13] merge-ort: handle interactions of caching and rename/rename(1to1) cases Elijah Newren via GitGitGadget
2021-05-17 14:16     ` Derrick Stolee
2021-05-04  2:12   ` [PATCH v2 13/13] merge-ort, diffcore-rename: employ cached renames when possible Elijah Newren via GitGitGadget
2021-05-17 14:23     ` Derrick Stolee
2021-05-20  0:36       ` Elijah Newren
2021-05-22 11:17         ` Derrick Stolee
2021-05-14 17:37   ` [PATCH v2 00/13] Optimization batch 11: avoid repeatedly detecting same renames Elijah Newren
2021-05-14 21:04     ` Derrick Stolee
2021-05-20  6:09   ` [PATCH v3 " Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 01/13] t6423: rename file within directory that other side renamed Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 02/13] Documentation/technical: describe remembering renames optimization Elijah Newren via GitGitGadget
2021-05-20 11:32       ` Bagas Sanjaya
2021-05-20 15:14         ` Kerry, Richard
2021-05-20 16:34         ` Elijah Newren
2021-05-20  6:09     ` [PATCH v3 03/13] fast-rebase: change assert() to BUG() Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 04/13] fast-rebase: write conflict state to working tree, index, and HEAD Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 05/13] t6429: testcases for remembering renames Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 06/13] merge-ort: add data structures for in-memory caching of rename detection Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 07/13] merge-ort: populate caches of rename detection results Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 08/13] merge-ort: add code to check for whether cached renames can be reused Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 09/13] merge-ort: avoid accidental API mis-use Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 10/13] merge-ort: preserve cached renames for the appropriate side Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 11/13] merge-ort: add helper functions for using cached renames Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 12/13] merge-ort: handle interactions of caching and rename/rename(1to1) cases Elijah Newren via GitGitGadget
2021-05-20  6:09     ` [PATCH v3 13/13] merge-ort, diffcore-rename: employ cached renames when possible Elijah Newren via GitGitGadget
2021-05-22 11:17     ` [PATCH v3 00/13] Optimization batch 11: avoid repeatedly detecting same renames Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=30c23689-cdab-143e-159c-93e50c808430@gmail.com \
    --to=stolee@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jonathantanmy@google.com \
    --cc=me@ttaylorr.com \
    --cc=newren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).