From: Phillip Wood <phillip.wood123@gmail.com>
To: Joel Klinghed <the_jk@spawned.biz>,
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: Sat, 14 Aug 2021 16:20:37 +0100 [thread overview]
Message-ID: <ec5f6698-46e9-c8c8-057d-b04851cb9265@gmail.com> (raw)
In-Reply-To: <1fc066c5-a085-4865-9eb9-853dfcbe33c2@www.fastmail.com>
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"
next prev parent reply other threads:[~2021-08-14 15:20 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-11 13:49 [PATCH] commit: restore --edit when combined with --fixup Joel Klinghed via GitGitGadget
2021-08-11 20:24 ` Jeff King
2021-08-11 22:10 ` Joel Klinghed
2021-08-11 22:22 ` Jeff King
2021-08-11 23:27 ` brian m. carlson
2021-08-11 23:43 ` [PATCH v2] " Joel Klinghed via GitGitGadget
2021-08-12 5:21 ` Junio C Hamano
2021-08-12 7:42 ` Joel Klinghed
2021-08-12 19:51 ` Junio C Hamano
2021-08-12 8:02 ` [PATCH v3] " Joel Klinghed via GitGitGadget
2021-08-12 9:32 ` Phillip Wood
2021-08-12 10:01 ` Joel Klinghed
2021-08-13 13:06 ` Phillip Wood
2021-08-13 15:35 ` Joel Klinghed
2021-08-14 15:20 ` Phillip Wood [this message]
2021-08-14 21:19 ` Joel Klinghed
2021-08-12 11:55 ` [PATCH v4] " Joel Klinghed via GitGitGadget
2021-08-14 21:40 ` [PATCH v5] " Joel Klinghed via GitGitGadget
2021-08-17 10:06 ` Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ec5f6698-46e9-c8c8-057d-b04851cb9265@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=phillip.wood@dunelm.org.uk \
--cc=sandals@crustytoothpaste.net \
--cc=the_jk@spawned.biz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).