Re: [PATCH v3] commit: restore --edit when combined with --fixup
- From
Joel Klinghed <the_jk@spawned.biz>
- Date
- Aug 13, 2021, 15:35 UTC
- Message-ID
- <1fc066c5-a085-4865-9eb9-853dfcbe33c2@www.fastmail.com>
- In-Reply-To
- <0151003c-d544-1fab-18e9-34eb84842555@gmail.com>
On Fri, Aug 13, 2021, at 15:06, Phillip Wood wrote:
Show 44 quoted lines
> 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