On 12/08/2021 11:01, Joel Klinghed wrote:
On Thu, Aug 12, 2021, at 11:32, Phillip Wood wrote:
Hi Joel
@@ -1170,7 +1206,7 @@ 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 || fixup_message)
+ if (logfile || have_option_m || use_message)
use_editor = 0;
if (0 <= edit_flag)
use_editor = edit_flag;
I think it should have moved those last two context lines that set
`use_editor` to below the part that you are fixing. Then the `use_editor
= 0` line that you are changing continues to work as before. (As you see
there are quite a few legacy Yoda conditions in this file, nowadays we
avoid adding new ones). I'm not sure if it is worth re working this
patch to do that, but it does have the advantage of only testing
edit_flag once.
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.
Best wishes
Phillip
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 7d02f79c0de..a48fe859235 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -281,6 +281,19 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
extra"
'
+test_expect_success 'commit --fixup --edit' '
+ commit_for_rebase_autosquash_setup &&
+ write_script e-append <<-\EOF &&
+ sed -e "2a\\
+something\\
+extra" <"$1" >"$1-"
+ mv "$1-" "$1"
+ EOF
Again I'm not sure it is worth changing it now but for future reference
this is a rather complicated way of appending to the commit message. The
test file has an example using set_fake_editor() together with
FAKE_COMMIT_AMEND just below where you have added this test or you can
just have
EDITOR="echo something extra >>" git commit --fixup=HEAD~1 --edit
Best Wishes
Phillip
Yeah, especially getting sed in a POSIX and BSD compatible mode took some
doing. I wanted to have a similar output to the test above this one, with a line break
between something and extra, and frankly, my shell-foo lacked for
getting either FAKE_COMMIT_AMEND or EDITOR="... >>" to include a newline.
But looking at it again, I realize that EDITOR="printf \"something\nextra\" >>"
works just fine.
/JK
