git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Phillip Wood" <phillip.wood123@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2 2/2] rebase: don't override --no-reschedule-failed-exec with config
Date: Fri,  9 Apr 2021 10:01:38 +0200	[thread overview]
Message-ID: <patch-2.3-7991160de3-20210409T075713Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.3-0000000000-20210409T075713Z-avarab@gmail.com>

Fix a bug in how --no-reschedule-failed-exec interacts with
rebase.rescheduleFailedExec=true being set in the config. Before this
change the --no-reschedule-failed-exec config option would be
overridden by the config.

This bug happened because of the particulars of how "rebase" works
v.s. most other git commands when it comes to parsing options and
config:

When we read the config and parse the CLI options we correctly prefer
the --no-reschedule-failed-exec option over
rebase.rescheduleFailedExec=true in the config. So far so good.

However the --reschedule-failed-exec option doesn't take effect when
the rebase starts (we'd just create a
".git/rebase-merge/reschedule-failed-exec" file if it was true). It
only takes effect when the exec command fails, at which point we'll
reschedule the failed "exec" command.

Since we only wrote out the positive
".git/rebase-merge/reschedule-failed-exec" under
--reschedule-failed-exec, but nothing with --no-reschedule-failed-exec
we'll forget that we asked not to reschedule failed "exec", and would
happily re-read the config and see that
rebase.rescheduleFailedExec=true is set.

So the config will effectively override the user having explicitly
disabled the option on the command-line.

Even more confusingly: Since rebase accepts different options based on
its state there wasn't even a way to get around this with "rebase
--continue --no-reschedule-failed-exec" (but you could of course set
the config with "rebase -c ...").

I think the least bad way out of this is to declare that for such
options and config whatever we decide at the beginning of the rebase
goes. So we'll now always create either a "reschedule-failed-exec" or
a "no-reschedule-failed-exec file at the start, not just the former if
we decided we wanted the feature.

With this new worldview you can no longer change the setting once a
rebase has started except by manually removing the state files
discussed above. I think making it work like that is the the least
confusing thing we can do.

In the future we might want to learn to change the setting in the
middle by combining "--edit-todo" with
"--[no-]reschedule-failed-exec", we currently don't support combining
those options, or any other way to change the state in the middle of
the rebase short of manually editing the files in
".git/rebase-merge/*".

The bug being fixed here originally came about because of a
combination of the behavior of the code added in d421afa0c66 (rebase:
introduce --reschedule-failed-exec, 2018-12-10) and the addition of
the config variable in 969de3ff0e0 (rebase: add a config option to
default to --reschedule-failed-exec, 2018-12-10).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-rebase.txt |  8 ++++++++
 sequencer.c                  |  5 +++++
 t/t3418-rebase-continue.sh   | 23 +++++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f08ae27e2a..93afe5ce3c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -623,6 +623,14 @@ See also INCOMPATIBLE OPTIONS below.
 --no-reschedule-failed-exec::
 	Automatically reschedule `exec` commands that failed. This only makes
 	sense in interactive mode (or when an `--exec` option was provided).
++
+Even though this option applies once a rebase is started, it's set for
+the whole rebase at the start based on either the
+`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]
+or "CONFIGURATION" below) or whether this option is
+provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
+start would be overridden by the presence of
+`rebase.rescheduleFailedExec=true` configuration.
 
 INCOMPATIBLE OPTIONS
 --------------------
diff --git a/sequencer.c b/sequencer.c
index b4c63ae207..8a7e7414eb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
+static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-reschedule-failed-exec")
 static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
 static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
 
@@ -2854,6 +2855,8 @@ static int read_populate_opts(struct replay_opts *opts)
 
 		if (file_exists(rebase_path_reschedule_failed_exec()))
 			opts->reschedule_failed_exec = 1;
+		else if (file_exists(rebase_path_no_reschedule_failed_exec()))
+			opts->reschedule_failed_exec = 0;
 
 		if (file_exists(rebase_path_drop_redundant_commits()))
 			opts->drop_redundant_commits = 1;
@@ -2954,6 +2957,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 		write_file(rebase_path_ignore_date(), "%s", "");
 	if (opts->reschedule_failed_exec)
 		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
+	else
+		write_file(rebase_path_no_reschedule_failed_exec(), "%s", "");
 
 	return 0;
 }
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index fe407e63cf..f4c2ee02bc 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -290,4 +290,27 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' '
 	git rebase HEAD^
 '
 
+test_expect_success 'rebase.rescheduleFailedExec=true & --no-reschedule-failed-exec' '
+	test_when_finished "git rebase --abort" &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_config rebase.rescheduleFailedExec true &&
+	test_must_fail git rebase --continue 2>err &&
+	! grep "has been rescheduled" err
+'
+
+test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -x false HEAD~2 &&
+	test_expect_code 129 git rebase --continue --no-reschedule-failed-exec &&
+	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
+'
+
 test_done
-- 
2.31.1.584.gf4baedee75


  parent reply	other threads:[~2021-04-09  8:01 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   ` [PATCH 0/3] rebase: don't override --no-reschedule-failed-exec with config Ævar Arnfjörð Bjarmason
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       ` Ævar Arnfjörð Bjarmason [this message]
2021-04-15 15:24       ` [PATCH v2 0/2] rebase: don't override --no-reschedule-failed-exec with config 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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=patch-2.3-7991160de3-20210409T075713Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood123@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.
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).