Hi Joel
On 13/08/2021 16:35, Joel Klinghed wrote:
On Fri, Aug 13, 2021, at 15:06, Phillip Wood wrote:
On 12/08/2021 11:01, Joel Klinghed wrote:
I looked at moving the condition to one place but as use_editor = 0
is only set for --fixup if there isn't a suboption specified I didn't want
to have to duplicate the check for a suboption when deciding if
use_editor should default to zero.
I don't think you need to duplicate the check for a suboption, can't you
just do this on top of master (i.e without you patch applied)?
diff --git a/builtin/commit.c b/builtin/commit.c
index 243c626307..67a84ff6e4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1251,11 +1251,6 @@ static int parse_and_validate_options(int argc,
const char *argv[],
if (force_author && renew_authorship)
die(_("Using both --reset-author and --author does not
make sense"));
- if (logfile || have_option_m || use_message)
- use_editor = 0;
- if (0 <= edit_flag)
- use_editor = edit_flag;
-
/* Sanity check options */
if (amend && !current_head)
die(_("You have nothing to amend."));
@@ -1344,6 +1339,11 @@ static int parse_and_validate_options(int argc,
const char *argv[],
}
}
+ if (logfile || have_option_m || use_message)
+ use_editor = 0;
+ if (0 <= edit_flag)
+ use_editor = edit_flag;
+
cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
handle_untracked_files_arg(s);
I chose to move the other clause that sets use_editor as well so they
stay together.
With the above change use_editor no longer defaults to 0 for --fixup as
it used to do.
My expected behavior (based on old versions):
git commit --fixup <hash> /// No editor
git commit --fixup <hash> --edit /// Editor
As far as I can see your change would display an editor in both cases.
I've just tested it and it works as expected. However moving the
'if (logfile...)' breaks the test "commit --squash works with -c" so we
need to just move the second if clause. This is what I have on top of
master (i.e. without your patch so a plain fixup is still setting
use_editor=0)
diff --git a/builtin/commit.c b/builtin/commit.c
index 243c626307..7c9b1e7be3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1253,8 +1253,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
if (logfile || have_option_m || use_message)
use_editor = 0;
- if (0 <= edit_flag)
- use_editor = edit_flag;
/* Sanity check options */
if (amend && !current_head)
@@ -1344,6 +1342,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
}
}
+ if (0 <= edit_flag)
+ use_editor = edit_flag;
+
cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
handle_untracked_files_arg(s);
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 54c2082acb..3fa674e52d 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -270,7 +270,7 @@ EOF
test_expect_success 'commit --fixup provides correct one-line commit message' '
commit_for_rebase_autosquash_setup &&
- git commit --fixup HEAD~1 &&
+ EDITOR="printf \"something\nextra\" >>" git commit --fixup HEAD~1 &&
commit_msg_is "fixup! target message subject line"
'
@@ -281,6 +281,14 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
extra"
'
+
+test_expect_success 'commit --fixup --edit' '
+ commit_for_rebase_autosquash_setup &&
+ EDITOR="printf \"something\nextra\" >>" git commit --fixup HEAD~1 --edit &&
+ commit_msg_is "fixup! target message subject linesomething
+extra"
+'
+
get_commit_msg () {
rev="$1" &&
git log -1 --pretty=format:"%B" "$rev"
