archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <>
Cc: "Junio C Hamano" <>,
	"Johannes Schindelin" <>,
	"Ævar Arnfjörð Bjarmason" <>
Subject: [PATCH 0/3] rebase: don't override --no-reschedule-failed-exec with config
Date: Mon, 22 Mar 2021 12:48:22 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

I recently started using rebase.rescheduleFailedExec=true and noticed
this bug in its implementation. It's conceptually a relatively simple
fix, but as noted in 3/3 rebase being a "start this operation, run
other command verbs later" has an unintuitive interaction with our
usual "command-line options override config".

Everthing after this line has no relevance to this series, it's just a
side musing on another (mis-)feature of --reschedule-failed-exec.

There's another bug/misfeature I've noticed with
rebase.rescheduleFailedExec=true (although maybe it'll be argued by
someone that it's a feature). Let's say you run:

    git rebase -x false --reschedule-failed-exec HEAD~2

You'll now land in a state of (according to our helpful PS1 code, but
also the rebase state) of 2/4. Aside: Because you're rebasing 2
commits, we have 4 because of the 2x exec. So far so good and all of
that's expected.

But now when you do "git rebase --continue" we'll fail as expected,
but not as expected (at least I find it funny) bump the count to 3/5,
then 4/6, 5/7 etc.

With my "I just read the code" hat on this makes perfect sense. Every
time we process a TODO item we bump the count of processed items, and
under --reschedule-failed-exec we simply push the command that just
failed onto the list, hence the increasing done/TODO count when a
command fails.

But I don't see how it makes any sense to expose this as UI via "git
status" and __git_ps1. I asked git to process this item of 4 sequencer
TODO items. If I fail an item it makes more sense that I just don't
get past it, not that under the hood a new identical item is
rescheduled for me. Just don't advance past the current item!

Now if I've tried X times to make the "make test" pass for each commit
in my 3-commit series I'm going to be on item 10/12 or
whatever. There's no way to just look at that and see where I'm at in
the sequence.

As an aside it would arguably make more sense to report 1/3 instead of
2/6 for the first commit of 3 with a failing -x "make test", but you
can have X number of "exec" items and a manually edited list etc. 

So that's probably a no-go but at least once I'm used to it I know if
i'm on 4/6 I'm on commit 2/3, with --reschedule-failed-exec you'll
have no idea what 12/14 or whatever means for where you are in your
3-patch sequence, it has no relation to the TODO list you edited, just
rebase-merge's own internal state.

Getting back on topic: This just seems like needlessly exposing an
implementation detail, I also asked to "pick" a commit, but if that
item "fails" e.g. due to:
    $ git rebase --continue
    You must edit all merge conflicts and then
    mark them as resolved using git add

We don't push a new "pick" item on the list and inflate the count, so
why would we do that for "exec"? Just say the command failed, return
its non-zero status from "git rebase --continue", and don't advance.

Maybe it is useful to keep track of the N number of failures, and
e.g. report in __git_ps1:

    master|REBASE 2/6
    master|REBASE 2(tries: 1)/6
    master|REBASE 2(tries: 2)/6

Instead of the current:

    master|REBASE 2/6
    master|REBASE 3/7
    master|REBASE 4/8

To say I'm on 2/6, but that I've tried 3 times already and failed to
advance past it.

Anyway, I don't have patches for this side-report/rant. Looking at the
implementation it seemed more non-trivial than I was willing to
quickly fix.

We bump the count fairly early before we even get to there being an
"exec" item, to implement this proposed view of the world we'd need to
defer that (or go back and edit it once we see "failed exec" and that
we're using --reschedule-failed-exec).

I'm not familiar enough with the sequencer internals to know if trying
that would lead us down a path of e.g. having inconsistent or bad
state if we'd die in the middle of all of that.

Ævar Arnfjörð Bjarmason (3):
  rebase tests: camel-case rebase.rescheduleFailedExec consistently
  rebase tests: use test_unconfig after test_config
  rebase: don't override --no-reschedule-failed-exec with config

 Documentation/git-rebase.txt |  8 ++++++++
 sequencer.c                  |  5 +++++
 t/   | 30 ++++++++++++++++++++++++++++--
 3 files changed, 41 insertions(+), 2 deletions(-)


  reply	other threads:[~2021-03-22 11:50 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-10 19:04 [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically Johannes Schindelin via GitGitGadget
2018-12-10 19:04 ` [PATCH 1/3] rebase: introduce --reschedule-failed-exec Johannes Schindelin via GitGitGadget
2018-12-10 23:18   ` Elijah Newren
2018-12-11 10:14     ` Johannes Schindelin
2018-12-11 16:16       ` Elijah Newren
2018-12-10 19:04 ` [PATCH 2/3] rebase: add a config option to default to --reschedule-failed-exec Johannes Schindelin via GitGitGadget
2021-03-22 11:48   ` Ævar Arnfjörð Bjarmason [this message]
2021-03-22 11:48     ` [PATCH 1/3] rebase tests: camel-case rebase.rescheduleFailedExec consistently Ævar Arnfjörð Bjarmason
2021-03-22 11:48     ` [PATCH 2/3] rebase tests: use test_unconfig after test_config Ævar Arnfjörð Bjarmason
2021-03-30 13:53       ` Phillip Wood
2021-03-22 11:48     ` [PATCH 3/3] rebase: don't override --no-reschedule-failed-exec with config Ævar Arnfjörð Bjarmason
2021-03-29 14:49       ` Phillip Wood
2021-03-29 16:12         ` Ævar Arnfjörð Bjarmason
2021-03-29 17:15           ` Phillip Wood
2021-03-24 11:50     ` [PATCH 0/3] " Johannes Schindelin
2021-03-30 13:40     ` Phillip Wood
2021-04-09  8:01     ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason
2021-04-09  8:01       ` [PATCH v2 1/2] rebase tests: camel-case rebase.rescheduleFailedExec consistently Ævar Arnfjörð Bjarmason
2021-04-09  8:01       ` [PATCH v2 2/2] rebase: don't override --no-reschedule-failed-exec with config Ævar Arnfjörð Bjarmason
2021-04-15 15:24       ` [PATCH v2 0/2] " Phillip Wood
2018-12-10 19:05 ` [PATCH 3/3] rebase: introduce a shortcut for --reschedule-failed-exec Johannes Schindelin via GitGitGadget
2018-12-10 22:08 ` [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically Johannes Sixt
2018-12-10 22:56   ` Stefan Beller
2018-12-11  3:28     ` Junio C Hamano
2018-12-11 10:31     ` Johannes Schindelin
2018-12-11 17:36       ` Stefan Beller
2018-12-10 23:20 ` Elijah Newren
2018-12-11 10:19   ` email lags, was " Johannes Schindelin

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:

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

  git send-email \ \ \ \ \ \

* 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.
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 NNTP newsgroup(s).