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

From: "Joel Klinghed" <the_jk@spawned.biz>
To: phillip.wood@dunelm.org.uk,
	"Joel Klinghed via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: "Jeff King" <peff@peff.net>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH v3] commit: restore --edit when combined with --fixup
Date: Fri, 13 Aug 2021 17:35:43 +0200	[thread overview]
Message-ID: <1fc066c5-a085-4865-9eb9-853dfcbe33c2@www.fastmail.com> (raw)
In-Reply-To: <0151003c-d544-1fab-18e9-34eb84842555@gmail.com>

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.

An alternative would be:
+       if (logfile || have_option_m || use_message || fixup_message)
+               use_editor = 0;
+       if (0 <= edit_flag)
+               use_editor = edit_flag;

That would fix the above cases but in 
"commit: add amend suboption to --fixup to create amend! commit"
the implementation left
git commit --fixup amend:<hash>  // Editor
and I didn't want to change that. But if the default should be no editor
here as well then the above would be a better patch.

/JK