commit: restore --edit when combined with --fixup #1014

thejk · 2021-08-11T10:43:47Z

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.

Changes since v1:
Added test verifying that --fixup --edit brings up editor.

Changes since v2:
Clarify if condition and use write_script helper in test.

Changes since v3:
Simplify test.

Changes since v4:
Using cleaner fix by Phillip Wood instead and added
an explicit verification to a test for --fixup without --edit.

Signed-off-by: Joel Klinghed the_jk@spawned.biz

cc: Jeff King peff@peff.net
cc: "brian m. carlson" sandals@crustytoothpaste.net
cc: Phillip Wood phillip.wood123@gmail.com

gitgitgadget · 2021-08-11T10:44:32Z

Welcome to GitGitGadget

Hi @thejk, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

the lines should not exceed 76 columns,
the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
the commit messages' body should be describing the "why?" of the change.
Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

gitgitgadget · 2021-08-11T10:44:33Z

There is an issue in commit 6ad4b46:
Prefixed commit message must be in lower case: commit: Restore --edit when combined with --fixup

dscho · 2021-08-11T13:16:55Z

/allow

gitgitgadget · 2021-08-11T13:17:37Z

User thejk is now allowed to use GitGitGadget.

WARNING: thejk has no public email address set on GitHub

thejk · 2021-08-11T13:48:34Z

/submit

gitgitgadget · 2021-08-11T13:49:25Z

Submitted as pull.1014.git.1628689758413.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v1

To fetch this version to local tag pr-1014/thejk/fixup_edit-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1014/thejk/fixup_edit-v1

gitgitgadget · 2021-08-11T20:46:18Z

On the Git mailing list, Jeff King wrote (reply to this):

On Wed, Aug 11, 2021 at 01:49:18PM +0000, 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.

This is definitely a change in behavior due to 494d314a05 (commit: add
amend suboption to --fixup to create amend! commit, 2021-03-15). That
was in v2.32.0, so it's not a regression in the upcoming v2.33 which
needs to be handled in the next few days.

My inclination is to call it a regression and restore the original
behavior. But when I was going to suggest that you add a test, it made
me wonder: what would we be testing for?

If the user says "git commit --fixup HEAD --edit", it seems reasonable
for them to expect that the editor is run, and that is easy to check.
But what are they planning to edit? If they modify the subject line of
the commit, it will wreck the "fixup!" mechanism. If they modify the
body (which starts blank), it's going to be discarded by the fixup
operation.

Is the goal that they might leave notes for themselves, which they can
view in the meantime before they run "rebase --autosquash"?

-Peff

gitgitgadget · 2021-08-11T20:46:20Z

User Jeff King <peff@peff.net> has been added to the cc: list.

gitgitgadget · 2021-08-11T22:12:43Z

On the Git mailing list, "Joel Klinghed" wrote (reply to this):



On Wed, Aug 11, 2021, at 22:24, Jeff King wrote:
> On Wed, Aug 11, 2021 at 01:49:18PM +0000, 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.
> 
> This is definitely a change in behavior due to 494d314a05 (commit: add
> amend suboption to --fixup to create amend! commit, 2021-03-15). That
> was in v2.32.0, so it's not a regression in the upcoming v2.33 which
> needs to be handled in the next few days.
> 
> My inclination is to call it a regression and restore the original
> behavior. But when I was going to suggest that you add a test, it made
> me wonder: what would we be testing for?
> 
> If the user says "git commit --fixup HEAD --edit", it seems reasonable
> for them to expect that the editor is run, and that is easy to check.
> But what are they planning to edit? If they modify the subject line of
> the commit, it will wreck the "fixup!" mechanism. If they modify the
> body (which starts blank), it's going to be discarded by the fixup
> operation.
> 
> Is the goal that they might leave notes for themselves, which they can
> view in the meantime before they run "rebase --autosquash"?
> 

At my work we use "fixup!" when pushing fixes to a review, using
the modified body to outline what issue the commit is addressing,
helping the reviewers to see intent and not just the end result.
All "fixup!" are then ofc. squashed before integration.
Same can be achieved with -m but --edit is generally
easier to work with in my experience.

I've also once or twice used it to avoid a "fixup!" of a "fixup!" instead
of looking up the original target commit hash but that's just me being
lazy.

I'll add a test to check for previous behavior.

/JK

gitgitgadget · 2021-08-11T22:24:44Z

On the Git mailing list, Jeff King wrote (reply to this):

On Thu, Aug 12, 2021 at 12:10:28AM +0200, Joel Klinghed wrote:

> > Is the goal that they might leave notes for themselves, which they can
> > view in the meantime before they run "rebase --autosquash"?
> > 
> 
> At my work we use "fixup!" when pushing fixes to a review, using
> the modified body to outline what issue the commit is addressing,
> helping the reviewers to see intent and not just the end result.
> All "fixup!" are then ofc. squashed before integration.
> Same can be achieved with -m but --edit is generally
> easier to work with in my experience.
> 
> I've also once or twice used it to avoid a "fixup!" of a "fixup!" instead
> of looking up the original target commit hash but that's just me being
> lazy.

Ah, thanks for explaining. That makes sense.

> I'll add a test to check for previous behavior.

This is what I worked up, in case it helps.

diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 38a532d81c..3e4364066f 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -940,4 +940,22 @@ EOF
 	)
 '
 
+test_expect_success 'commit --fixup respects --edit' '
+	echo broken >foo.c &&
+	git add foo.c &&
+	git commit -m "wip of foo.c" &&
+	echo fixed >foo.c &&
+	(
+		test_set_editor "$TEST_DIRECTORY/t7500/add-content" &&
+		git commit --fixup HEAD --edit foo.c
+	) &&
+	cat >expect <<-\EOF &&
+	fixup! wip of foo.c
+
+	commit message
+	EOF
+	git log -1 --pretty=format:%B HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done

-Peff

gitgitgadget · 2021-08-11T23:30:54Z

On the Git mailing list, "brian m. carlson" wrote (reply to this):


--Hz7kgiVFAz3pEq5c
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On 2021-08-11 at 22:10:28, Joel Klinghed wrote:
> At my work we use "fixup!" when pushing fixes to a review, using
> the modified body to outline what issue the commit is addressing,
> helping the reviewers to see intent and not just the end result.
> All "fixup!" are then ofc. squashed before integration.
> Same can be achieved with -m but --edit is generally
> easier to work with in my experience.

Yeah, I do exactly the same thing.  I want to write a nice explanation
of the change I made for the reviewer, but I don't want to preserve it
for the future.

I recently had a series that was 33 commits after squashing but with 117
before squashing, thanks to a series of very thorough and thoughtful
reviews, so in this kind of scenario, it can really be kind to the
reviewer to help them match up the change with their comment.

> I've also once or twice used it to avoid a "fixup!" of a "fixup!" instead
> of looking up the original target commit hash but that's just me being
> lazy.
>=20
> I'll add a test to check for previous behavior.

Thanks for catching this.
--=20
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

--Hz7kgiVFAz3pEq5c
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.3.1 (GNU/Linux)

iHUEABYKAB0WIQQILOaKnbxl+4PRw5F8DEliiIeigQUCYRRc0wAKCRB8DEliiIei
gZMgAQCfDZGhg2loHzSkUhznHjk6IgtStI1JcRgpQVG3Z4WyMQD9GLIiaydebmlP
RT0tKMy1xrKi82zWxP9cEo2Vsg6KLA8=
=Xrax
-----END PGP SIGNATURE-----

--Hz7kgiVFAz3pEq5c--

gitgitgadget · 2021-08-11T23:30:56Z

User "brian m. carlson" <sandals@crustytoothpaste.net> has been added to the cc: list.

thejk · 2021-08-11T23:42:58Z

/submit

gitgitgadget · 2021-08-11T23:44:07Z

Submitted as pull.1014.v2.git.1628725421868.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v2

To fetch this version to local tag pr-1014/thejk/fixup_edit-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1014/thejk/fixup_edit-v2

gitgitgadget · 2021-08-12T05:25:32Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Joel Klinghed via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 190d215d43b..4c5286840c5 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 (0 > edit_flag)

Writing this as

			if (edit_flag < 0)

makes it far easier to immediately see that we are talking about a
nagetive edit_flag.

> +				use_editor = 0;
>  		}
>  	}
>  
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 7d02f79c0de..d71c7812180 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -281,6 +281,21 @@ test_expect_success 'commit --fixup -m"something" -m"extra"' '
>  
>  extra"
>  '
> +test_expect_success 'commit --fixup --edit' '
> +	commit_for_rebase_autosquash_setup &&

> +	cat >e-append <<-\EOF &&
> +	#!/bin/sh
> +	sed -e "2a\\
> +something\\
> +extra" <"$1" >"$1-"
> +	mv "$1-" "$1"
> +	EOF
> +	chmod 755 e-append &&

Use write_script helper from test-lib-functions.sh here and lose the
hardcoded reference to /bin/sh.

> +	EDITOR="./e-append" git commit --fixup HEAD~1 --edit &&
> +	commit_msg_is "fixup! target message subject linesomething
> +extra"
> +'


Thanks.

gitgitgadget · 2021-08-12T07:44:47Z

On the Git mailing list, "Joel Klinghed" wrote (reply to this):

On Thu, Aug 12, 2021, at 07:21, Junio C Hamano wrote:
> "Joel Klinghed via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index 190d215d43b..4c5286840c5 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 (0 > edit_flag)
> 
> Writing this as
> 
> 			if (edit_flag < 0)
> 
> makes it far easier to immediately see that we are talking about a
> nagetive edit_flag.
> 

Agree, I'll change it.
I was unsure of the style and copied from the earlier condition:
	if (0 <= edit_flag)
		use_editor = edit_flag;

> > +				use_editor = 0;
> >  		}
> >  	}
> >  

> 
> Use write_script helper from test-lib-functions.sh here and lose the
> hardcoded reference to /bin/sh.
> 

Ah, missed that one.

Thanks.

/JK

thejk · 2021-08-12T08:01:40Z

/submit

gitgitgadget · 2021-08-12T08:02:32Z

Submitted as pull.1014.v3.git.1628755346354.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v3

To fetch this version to local tag pr-1014/thejk/fixup_edit-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1014/thejk/fixup_edit-v3

gitgitgadget · 2021-08-12T09:36:23Z

On the Git mailing list, Phillip Wood wrote (reply to this):

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
>

gitgitgadget · 2021-08-12T09:36:25Z

User Phillip Wood <phillip.wood123@gmail.com> has been added to the cc: list.

gitgitgadget · 2021-08-12T10:03:11Z

On the Git mailing list, "Joel Klinghed" wrote (reply to this):



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

thejk · 2021-08-12T11:54:53Z

/submit

gitgitgadget · 2021-08-12T11:55:43Z

Submitted as pull.1014.v4.git.1628769334197.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v4

To fetch this version to local tag pr-1014/thejk/fixup_edit-v4:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1014/thejk/fixup_edit-v4

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 moving the edit flag condition later in the method. Signed-off-by: Joel Klinghed <the_jk@spawned.biz>

gitgitgadget · 2021-08-14T21:21:43Z

On the Git mailing list, "Joel Klinghed" wrote (reply to this):

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

thejk · 2021-08-14T21:39:45Z

/submit

gitgitgadget · 2021-08-14T21:40:43Z

Submitted as pull.1014.v5.git.1628977230247.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v5

To fetch this version to local tag pr-1014/thejk/fixup_edit-v5:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1014/thejk/fixup_edit-v5

gitgitgadget · 2021-08-15T20:54:09Z

This patch series was integrated into seen via git@0b2c664.

gitgitgadget · 2021-08-15T22:00:44Z

This patch series was integrated into seen via git@40d8f88.

gitgitgadget · 2021-08-16T21:12:00Z

This patch series was integrated into seen via git@3c28555.

gitgitgadget · 2021-08-16T23:09:18Z

There was a status update in the "Cooking" section about the branch jk/commit-edit-fixup-fix on the Git mailing list:

"git commit --fixup" now works with "--edit" again, after it was
broken in v2.32.

Will merge to 'next'?

gitgitgadget · 2021-08-17T10:09:40Z

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Joel

On 14/08/2021 22:40, 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 moving the edit flag condition
> later in the method.

This version looks good, thanks for revising it

Best Wishes

Phillip

> 
> Signed-off-by: Joel Klinghed <the_jk@spawned.biz>
> ---
>      commit: restore --edit when combined with --fixup
>      
>      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.
>      
>      Changes since v1: Added test verifying that --fixup --edit brings up
>      editor.
>      
>      Changes since v2: Clarify if condition and use write_script helper in
>      test.
>      
>      Changes since v3: Simplify test.
>      
>      Changes since v4: Using cleaner fix by Phillip Wood instead and added an
>      explicit verification to a test for --fixup without --edit.
>      
>      Signed-off-by: Joel Klinghed the_jk@spawned.biz
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1014%2Fthejk%2Ffixup_edit-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1014/thejk/fixup_edit-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/1014
> 
> Range-diff vs v4:
> 
>   1:  0c0cb647e03 ! 1:  1c608daf0cd commit: restore --edit when combined with --fixup
>       @@ Commit message
>            --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.
>       +    deciding the value of use_editor by moving the edit flag condition
>       +    later in the method.
>        
>            Signed-off-by: Joel Klinghed <the_jk@spawned.biz>
>        
>         ## builtin/commit.c ##
>        @@ builtin/commit.c: 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;
>       +
>       + 	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)
>       +@@ builtin/commit.c: 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);
>        
>         ## t/t7500-commit-template-squash-signoff.sh ##
>       +@@ t/t7500-commit-template-squash-signoff.sh: EOF
>       +
>       + test_expect_success 'commit --fixup provides correct one-line commit message' '
>       + 	commit_for_rebase_autosquash_setup &&
>       +-	git commit --fixup HEAD~1 &&
>       ++	EDITOR="echo ignored >>" git commit --fixup HEAD~1 &&
>       + 	commit_msg_is "fixup! target message subject line"
>       + '
>       +
>        @@ t/t7500-commit-template-squash-signoff.sh: test_expect_success 'commit --fixup -m"something" -m"extra"' '
>         
>         extra"
> 
> 
>   builtin/commit.c                          | 5 +++--
>   t/t7500-commit-template-squash-signoff.sh | 9 ++++++++-
>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 190d215d43b..854903ad113 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1246,8 +1246,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)
> @@ -1337,6 +1335,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 7d02f79c0de..8515736003a 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="echo ignored >>" git commit --fixup HEAD~1 &&
>   	commit_msg_is "fixup! target message subject line"
>   '
>   
> @@ -281,6 +281,13 @@ 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"
> 
> base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
>

gitgitgadget · 2021-08-19T21:19:48Z

This patch series was integrated into seen via git@662d0f9.

gitgitgadget · 2021-08-20T00:13:30Z

This patch series was integrated into seen via git@c07d7b3.

gitgitgadget · 2021-08-20T17:49:20Z

There was a status update in the "Cooking" section about the branch jk/commit-edit-fixup-fix on the Git mailing list:

"git commit --fixup" now works with "--edit" again, after it was
broken in v2.32.

Will merge to 'next'.

gitgitgadget · 2021-08-23T21:47:46Z

This patch series was integrated into seen via git@3b799c3.

gitgitgadget · 2021-08-23T21:59:10Z

There was a status update in the "Cooking" section about the branch jk/commit-edit-fixup-fix on the Git mailing list:

"git commit --fixup" now works with "--edit" again, after it was
broken in v2.32.

Will merge to 'next'.

gitgitgadget · 2021-08-24T23:32:01Z

This patch series was integrated into seen via git@15e1f0e.

gitgitgadget · 2021-08-24T23:32:02Z

This patch series was integrated into next via git@0c62543.

gitgitgadget · 2021-08-30T00:18:29Z

This patch series was integrated into seen via git@cc52b2b.

gitgitgadget · 2021-08-30T01:23:57Z

There was a status update in the "Cooking" section about the branch jk/commit-edit-fixup-fix on the Git mailing list:

"git commit --fixup" now works with "--edit" again, after it was
broken in v2.32.

Will merge to 'master'.

gitgitgadget · 2021-08-31T00:06:44Z

This patch series was integrated into seen via git@9561a07.

gitgitgadget · 2021-09-01T00:13:49Z

There was a status update in the "Cooking" section about the branch jk/commit-edit-fixup-fix on the Git mailing list:

"git commit --fixup" now works with "--edit" again, after it was
broken in v2.32.

Will merge to 'master'.

gitgitgadget · 2021-09-02T01:11:55Z

This patch series was integrated into seen via git@f68c532.

gitgitgadget · 2021-09-02T22:53:12Z

There was a status update in the "Cooking" section about the branch jk/commit-edit-fixup-fix on the Git mailing list:

"git commit --fixup" now works with "--edit" again, after it was
broken in v2.32.

Will merge to 'master'.

gitgitgadget · 2021-09-03T21:52:08Z

This patch series was integrated into seen via git@6e21f71.

gitgitgadget · 2021-09-03T21:52:09Z

This patch series was integrated into next via git@6e21f71.

gitgitgadget · 2021-09-03T21:52:10Z

This patch series was integrated into master via git@6e21f71.

gitgitgadget · 2021-09-03T21:52:12Z

Closed via 6e21f71.

gitgitgadget bot added the new user label Aug 11, 2021

thejk force-pushed the fixup_edit branch from 6ad4b46 to b3e44c6 Compare August 11, 2021 10:49

thejk changed the title ~~commit: Restore --edit when combined with --fixup~~ commit: restore --edit when combined with --fixup Aug 11, 2021

thejk force-pushed the fixup_edit branch from b3e44c6 to abf188b Compare August 11, 2021 22:13

thejk force-pushed the fixup_edit branch from abf188b to 0ee926d Compare August 11, 2021 23:15

thejk force-pushed the fixup_edit branch from 0ee926d to 6bc5d8b Compare August 12, 2021 07:38

thejk force-pushed the fixup_edit branch from 6bc5d8b to 0c0cb64 Compare August 12, 2021 11:26

thejk force-pushed the fixup_edit branch from 0c0cb64 to 1c608da Compare August 14, 2021 21:15

gitgitgadget bot added the next label Aug 24, 2021

gitgitgadget bot added the master label Sep 3, 2021

gitgitgadget bot closed this Sep 3, 2021

commit: restore --edit when combined with --fixup #1014

commit: restore --edit when combined with --fixup #1014

Conversation

thejk commented Aug 11, 2021 • edited

gitgitgadget bot commented Aug 11, 2021

Welcome to GitGitGadget

Contributing the patches

Need help?

gitgitgadget bot commented Aug 11, 2021

dscho commented Aug 11, 2021

gitgitgadget bot commented Aug 11, 2021

thejk commented Aug 11, 2021

gitgitgadget bot commented Aug 11, 2021

gitgitgadget bot commented Aug 11, 2021

gitgitgadget bot commented Aug 11, 2021

gitgitgadget bot commented Aug 11, 2021

gitgitgadget bot commented Aug 11, 2021

gitgitgadget bot commented Aug 11, 2021

gitgitgadget bot commented Aug 11, 2021

thejk commented Aug 11, 2021

gitgitgadget bot commented Aug 11, 2021

gitgitgadget bot commented Aug 12, 2021

gitgitgadget bot commented Aug 12, 2021

thejk commented Aug 12, 2021

gitgitgadget bot commented Aug 12, 2021

gitgitgadget bot commented Aug 12, 2021

gitgitgadget bot commented Aug 12, 2021

gitgitgadget bot commented Aug 12, 2021

thejk commented Aug 12, 2021

gitgitgadget bot commented Aug 12, 2021

gitgitgadget bot commented Aug 14, 2021

thejk commented Aug 14, 2021

gitgitgadget bot commented Aug 14, 2021

gitgitgadget bot commented Aug 15, 2021

gitgitgadget bot commented Aug 15, 2021

gitgitgadget bot commented Aug 16, 2021

gitgitgadget bot commented Aug 16, 2021

gitgitgadget bot commented Aug 17, 2021

gitgitgadget bot commented Aug 19, 2021

gitgitgadget bot commented Aug 20, 2021

gitgitgadget bot commented Aug 20, 2021

gitgitgadget bot commented Aug 23, 2021

gitgitgadget bot commented Aug 23, 2021

gitgitgadget bot commented Aug 24, 2021

gitgitgadget bot commented Aug 24, 2021

gitgitgadget bot commented Aug 30, 2021

gitgitgadget bot commented Aug 30, 2021

gitgitgadget bot commented Aug 31, 2021

gitgitgadget bot commented Sep 1, 2021

gitgitgadget bot commented Sep 2, 2021

gitgitgadget bot commented Sep 2, 2021

gitgitgadget bot commented Sep 3, 2021

gitgitgadget bot commented Sep 3, 2021

gitgitgadget bot commented Sep 3, 2021

gitgitgadget bot commented Sep 3, 2021

thejk commented Aug 11, 2021 •

edited