| Message ID | pull.1014.v3.git.1628755346354.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] commit: restore --edit when combined with --fixup | expand |
Hi Joel On 12/08/2021 09:02, Joel Klinghed via GitGitGadget wrote: > From: Joel Klinghed <the_jk@spawned.biz> > > Recent changes to --fixup, adding amend suboption, caused the > --edit flag to be ignored as use_editor was always set to zero. > > Restore edit_flag having higher priority than fixup_message when > deciding the value of use_editor by only changing the default > if edit_flag is not set. Thanks for fixing this > diff --git a/builtin/commit.c b/builtin/commit.c > index 190d215d43b..560aecd21b1 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[], > } else { > fixup_commit = fixup_message; > fixup_prefix = "fixup"; > - use_editor = 0; > + if (edit_flag < 0) > + use_editor = 0; > } > } > Commit 494d314a05 ("commit: add amend suboption to --fixup to create amend! commit", 2021-03-15) that broke this has the following hunk above this change @@ -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. > 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 > + EDITOR="./e-append" 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" > > base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb >
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. > > 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
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 >
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
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"
On Sat, Aug 14, 2021, at 17:20, Phillip Wood wrote: > On 13/08/2021 16:35, Joel Klinghed wrote: > > > > 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) > Ah, my bad. I misunderstood and thought your first patch was to be applied without my fixes. This way is cleaner, no doubt. > 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" > ' Good idea to make --fixup without edit behavior verification explicit. /JK
diff --git a/builtin/commit.c b/builtin/commit.c index 190d215d43b..560aecd21b1 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1333,7 +1333,8 @@ static int parse_and_validate_options(int argc, const char *argv[], } else { fixup_commit = fixup_message; fixup_prefix = "fixup"; - use_editor = 0; + if (edit_flag < 0) + use_editor = 0; } } 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 + EDITOR="./e-append" 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"