Re: [PATCH v3] commit: restore --edit when combined with --fixup

Hi Joel

On 13/08/2021 16:35, Joel Klinghed wrote:

On Fri, Aug 13, 2021, at 15:06, Phillip Wood wrote:

quoted

On 12/08/2021 11:01, Joel Klinghed wrote:

quoted

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"