git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <dstolee@microsoft.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v2 04/13] fast-rebase: write conflict state to working tree, index, and HEAD
Date: Mon, 17 May 2021 20:42:21 -0700	[thread overview]
Message-ID: <CABPp-BHpLBjyxsb+BM+ssZVHRJnn+DjazSQi6bHRXHET1vqGhg@mail.gmail.com> (raw)
In-Reply-To: <30c23689-cdab-143e-159c-93e50c808430@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.

  reply	other threads:[~2021-05-18  3:43 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
2021-05-18  3:42       ` Elijah Newren [this message]
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=CABPp-BHpLBjyxsb+BM+ssZVHRJnn+DjazSQi6bHRXHET1vqGhg@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jonathantanmy@google.com \
    --cc=me@ttaylorr.com \
    --cc=stolee@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).