Re: Query on git commit amend

Subsystems: the rest

8 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: Query on git commit amend

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:34

Junio C Hamano [off-list ref] writes:

I almost always use

    $ EDITOR=: git commit --amend

when rewriting the contents without updating the message, but I think
we should allow people to say:

    $ git commit --amend --no-edit

which is accepted from the command line but is not honoured.

And this should fix it (only lightly tested).

-- >8 --
Subject: [PATCH] commit: honor --no-edit

After making fixes to the contents to be committed, it is not unusual to
update the current commit without rewording the message. Idioms to do
tell "commit --amend" that we do not need an editor have been:

    $ EDITOR=: git commit --amend
    $ git commit --amend -C HEAD

but that was only because a more natural

    $ git commit --amend --no-edit

did not honour "--no-edit" option.    

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/commit.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8f2bebe..48bea8f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -81,7 +81,8 @@ static const char *template_file;
 static const char *author_message, *author_message_buffer;
 static char *edit_message, *use_message;
 static char *fixup_message, *squash_message;
-static int all, edit_flag, also, interactive, patch_interactive, only, amend, signoff;
+static int all, also, interactive, patch_interactive, only, amend, signoff;
+static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;

@@ -141,7 +142,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN(0, "reset-author", &renew_authorship, "the commit is authored by me now (used with -C-c/--amend)"),
 	OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
 	OPT_FILENAME('t', "template", &template_file, "use specified template file"),
-	OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),
+	OPT_BOOL('e', "edit", &edit_flag, "force edit of commit"),
 	OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
 	OPT_BOOLEAN(0, "status", &include_status, "include status in commit message template"),
 	/* end commit message options */

@@ -1020,8 +1021,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 	if (logfile || message.len || use_message || fixup_message)
 		use_editor = 0;
-	if (edit_flag)
-		use_editor = 1;
+	if (0 <= edit_flag)
+		use_editor = edit_flag;
 	if (!use_editor)
 		setenv("GIT_EDITOR", ":", 1);

-- 
1.7.8.157.g03e55

[PATCH/RFC 0/4] Re: commit: honor --no-edit

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:34

Junio C Hamano wrote:

And this should fix it (only lightly tested).

-- >8 --
Subject: [PATCH] commit: honor --no-edit

After making fixes to the contents to be committed, it is not unusual to
update the current commit without rewording the message. Idioms to do
tell "commit --amend" that we do not need an editor have been:

    $ EDITOR=: git commit --amend
    $ git commit --amend -C HEAD

but that was only because a more natural

    $ git commit --amend --no-edit

did not honour "--no-edit" option.

I like it.

Here are a couple of tests.  The three patches before are just to make
it less frightening to add to the relevant test script.

Jonathan Nieder (4):
  test: add missing "&&" after echo command
  test: remove a test of porcelain that hardcodes commit ids
  t7501 (commit): modernize style
  test: commit --amend should honor --no-edit

 t/t7501-commit.sh |  335 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 175 insertions(+), 160 deletions(-)

[PATCH 1/4] test: add missing "&&" after echo command

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:34

This test wants to modify a file and commit the change, but because of
a missing separator between commands it is parsed as a single "echo"
command.

Signed-off-by: Jonathan Nieder <redacted>
---
 t/t7501-commit.sh |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 3ad04363..da75abc1 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh

@@ -60,7 +60,7 @@ test_expect_success \
 
 test_expect_success \
 	"next commit" \
-	"echo 'bongo bongo bongo' >file \
+	"echo 'bongo bongo bongo' >file && \
 	 git commit -m next -a"
 
 test_expect_success \

@@ -172,11 +172,12 @@ test_expect_success \
 # easier to isolate bugs.
 
 cat >expected <<\EOF
-72c0dc9855b0c9dadcbfd5a31cab072e0cb774ca
-9b88fc14ce6b32e3d9ee021531a54f18a5cf38a2
-3536bbb352c3a1ef9a420f5b4242d48578b92aa7
-d381ac431806e53f3dd7ac2f1ae0534f36d738b9
-4fd44095ad6334f3ef72e4c5ec8ddf108174b54a
+285fcf7ec0d61b14249dfdb4c1e1fe03eaf15ee0
+0b8148b9afce917b87d71199b900466dc8ea8b6e
+43fb8826314939ce79a856face7953557fdca3d1
+eaa04bc3ae0f0b003f7f1d86bf869ec5d73eaf3e
+ee1963b250ee0f02a3fe37be0e4a02bb5af6a1ad
+b49f306003c627361a0304d151a6b4c8b26af6a1
 402702b49136e7587daa9280e91e4bb7cb2179f7
 EOF

-- 
1.7.8.rc3

[PATCH 2/4] test: remove a porcelain test that hard-codes commit names

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:34

The rev-list output in this test depends on the details of test_tick's
dummy dates and the choice of hash function.  Worse, it depends on the
order and nature of commits made in the earlier tests, so adding new
tests or rearranging existing ones breaks it.

It would be nice to check that "git commit" and commit-tree name
objects consistently and that commit objects' text is as documented,
but this particular test checks everything at once and hence is not a
robust test for that.  Remove it.

Signed-off-by: Jonathan Nieder <redacted>
---
Hence the odd hunk in the previous patch.

Maybe this doesn't belong in this series, but it's the kind of thing
that drives me batty when writing new tests.

 t/t7501-commit.sh |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index da75abc1..9c507b08 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh

@@ -144,10 +144,6 @@ test_expect_success PERL \
 	git diff >diff2 &&
 	test_cmp diff1 diff2"
 
-test_expect_success \
-	"showing committed revisions" \
-	"git rev-list HEAD >current"
-
 cat >editor <<\EOF
 #!/bin/sh
 sed -e "s/good/bad/g" < "$1" > "$1-"

@@ -168,22 +164,6 @@ test_expect_success \
 	 echo "Another good message." | EDITOR=./editor git commit -a -F - &&
 	 git show -s --pretty=format:"%s" | grep -q good
 	 '
-# We could just check the head sha1, but checking each commit makes it
-# easier to isolate bugs.
-
-cat >expected <<\EOF
-285fcf7ec0d61b14249dfdb4c1e1fe03eaf15ee0
-0b8148b9afce917b87d71199b900466dc8ea8b6e
-43fb8826314939ce79a856face7953557fdca3d1
-eaa04bc3ae0f0b003f7f1d86bf869ec5d73eaf3e
-ee1963b250ee0f02a3fe37be0e4a02bb5af6a1ad
-b49f306003c627361a0304d151a6b4c8b26af6a1
-402702b49136e7587daa9280e91e4bb7cb2179f7
-EOF
-
-test_expect_success \
-    'validate git rev-list output.' \
-    'test_cmp expected current'
 
 test_expect_success 'partial commit that involves removal (1)' '

-- 
1.7.8.rc3

[PATCH 3/4] t7501 (commit): modernize style

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:34

Put the opening quote starting each test on the same line as the
test_expect_* invocation.  While at it:

- guard commands that prepare test input for individual tests in
  the same test_expect_success, so their scope is clearer and
  errors at that stage can be caught;
- use the compare_diff_patch helper function when comparing patches;
- use single-quotes in preference to double-quotes and <<\EOF in
  preference to <<EOF, to save readers the trouble of looking for
  variable interpolations;
- lift the setting of the $author variable used throughout the
  test script to the top of the test script;
- include "setup" in the titles of test assertions that prepare for
  later ones to make it more obvious which tests can be skipped;
- use test_must_fail instead of "if ...; then:; else false; fi",
  for clarity and to catch segfaults when they happen;
- break up some pipelines into separate commands that read and write
  to ordinary files, and test the exit status at each stage;
- chain commands with &&.  Breaks in a test assertion's && chain can
  potentially hide failures from earlier commands in the chain;
- combine two initial tests that do not make as much sense alone.

Signed-off-by: Jonathan Nieder <redacted>
---
Miscellaneous cleanups.

 t/t7501-commit.sh |  278 ++++++++++++++++++++++++++---------------------------
 1 files changed, 136 insertions(+), 142 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 9c507b08..bf025df6 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh

@@ -8,39 +8,39 @@
 
 test_description='git commit'
 . ./test-lib.sh
+. "$TEST_DIRECTORY/diff-lib.sh"
+
+author='The Real Author <someguy@his.email.org>'
 
 test_tick
 
-test_expect_success \
-	"initial status" \
-	"echo 'bongo bongo' >file &&
-	 git add file"
-
-test_expect_success "Constructing initial commit" '
+test_expect_success 'initial status' '
+	echo bongo bongo >file &&
+	git add file &&
 	git status >actual &&
 	test_i18ngrep "Initial commit" actual
 '
 
-test_expect_success \
-	"fail initial amend" \
-	"test_must_fail git commit --amend"
+test_expect_success 'fail initial amend' '
+	test_must_fail git commit --amend
+'
 
-test_expect_success \
-	"initial commit" \
-	"git commit -m initial"
+test_expect_success 'setup: initial commit' '
+	git commit -m initial
+'
 
-test_expect_success \
-	"invalid options 1" \
-	"test_must_fail git commit -m foo -m bar -F file"
+test_expect_success '-m and -F do not mix' '
+	test_must_fail git commit -m foo -m bar -F file
+'
 
-test_expect_success \
-	"invalid options 2" \
-	"test_must_fail git commit -C HEAD -m illegal"
+test_expect_success '-m and -C do not mix' '
+	test_must_fail git commit -C HEAD -m illegal
+'
 
-test_expect_success \
-	"using paths with -a" \
-	"echo King of the bongo >file &&
-	test_must_fail git commit -m foo -a file"
+test_expect_success 'paths and -a do not mix' '
+	echo King of the bongo >file &&
+	test_must_fail git commit -m foo -a file
+'
 
 test_expect_success PERL 'can use paths with --interactive' '
 	echo bong-o-bong >file &&

@@ -50,120 +50,123 @@ test_expect_success PERL 'can use paths with --interactive' '
 	git reset --hard HEAD^
 '
 
-test_expect_success \
-	"using invalid commit with -C" \
-	"test_must_fail git commit -C bogus"
-
-test_expect_success \
-	"testing nothing to commit" \
-	"test_must_fail git commit -m initial"
-
-test_expect_success \
-	"next commit" \
-	"echo 'bongo bongo bongo' >file && \
-	 git commit -m next -a"
-
-test_expect_success \
-	"commit message from non-existing file" \
-	"echo 'more bongo: bongo bongo bongo bongo' >file && \
-	 test_must_fail git commit -F gah -a"
-
-# Empty except stray tabs and spaces on a few lines.
-sed -e 's/@$//' >msg <<EOF
-		@
-
-  @
-Signed-off-by: hula
-EOF
-test_expect_success \
-	"empty commit message" \
-	"test_must_fail git commit -F msg -a"
-
-test_expect_success \
-	"commit message from file" \
-	"echo 'this is the commit message, coming from a file' >msg && \
-	 git commit -F msg -a"
-
-cat >editor <<\EOF
-#!/bin/sh
-sed -e "s/a file/an amend commit/g" < "$1" > "$1-"
-mv "$1-" "$1"
-EOF
-chmod 755 editor
-
-test_expect_success \
-	"amend commit" \
-	"EDITOR=./editor git commit --amend"
-
-test_expect_success \
-	"passing -m and -F" \
-	"echo 'enough with the bongos' >file && \
-	 test_must_fail git commit -F msg -m amending ."
-
-test_expect_success \
-	"using message from other commit" \
-	"git commit -C HEAD^ ."
-
-cat >editor <<\EOF
-#!/bin/sh
-sed -e "s/amend/older/g"  < "$1" > "$1-"
-mv "$1-" "$1"
-EOF
-chmod 755 editor
-
-test_expect_success \
-	"editing message from other commit" \
-	"echo 'hula hula' >file && \
-	 EDITOR=./editor git commit -c HEAD^ -a"
-
-test_expect_success \
-	"message from stdin" \
-	"echo 'silly new contents' >file && \
-	 echo commit message from stdin | git commit -F - -a"
-
-test_expect_success \
-	"overriding author from command line" \
-	"echo 'gak' >file && \
-	 git commit -m 'author' --author 'Rubber Duck <rduck@convoy.org>' -a >output 2>&1"
-
-test_expect_success \
-	"commit --author output mentions author" \
-	"grep Rubber.Duck output"
-
-test_expect_success PERL \
-	"interactive add" \
-	"echo 7 | git commit --interactive | grep 'What now'"
-
-test_expect_success PERL \
-	"commit --interactive doesn't change index if editor aborts" \
-	"echo zoo >file &&
+test_expect_success 'using invalid commit with -C' '
+	test_must_fail git commit -C bogus
+'
+
+test_expect_success 'nothing to commit' '
+	test_must_fail git commit -m initial
+'
+
+test_expect_success 'setup: non-initial commit' '
+	echo bongo bongo bongo >file &&
+	git commit -m next -a
+'
+
+test_expect_success 'commit message from non-existing file' '
+	echo more bongo: bongo bongo bongo bongo >file &&
+	test_must_fail git commit -F gah -a
+'
+
+test_expect_success 'empty commit message' '
+	# Empty except stray tabs and spaces on a few lines.
+	sed -e "s/@//g" >msg <<-\EOF &&
+		@		@
+		@@
+		@  @
+		@Signed-off-by: hula@
+	EOF
+	test_must_fail git commit -F msg -a
+'
+
+test_expect_success 'setup: commit message from file' '
+	echo this is the commit message, coming from a file >msg &&
+	git commit -F msg -a
+'
+
+test_expect_success 'amend commit' '
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	sed -e "s/a file/an amend commit/g" < "$1" > "$1-"
+	mv "$1-" "$1"
+	EOF
+	chmod 755 editor &&
+	EDITOR=./editor git commit --amend
+'
+
+test_expect_success '-m and -F do not mix' '
+	echo enough with the bongos >file &&
+	test_must_fail git commit -F msg -m amending .
+'
+
+test_expect_success 'using message from other commit' '
+	git commit -C HEAD^ .
+'
+
+test_expect_success 'editing message from other commit' '
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	sed -e "s/amend/older/g"  < "$1" > "$1-"
+	mv "$1-" "$1"
+	EOF
+	chmod 755 editor &&
+	echo hula hula >file &&
+	EDITOR=./editor git commit -c HEAD^ -a
+'
+
+test_expect_success 'message from stdin' '
+	echo silly new contents >file &&
+	echo commit message from stdin |
+	git commit -F - -a
+'
+
+test_expect_success 'overriding author from command line' '
+	echo gak >file &&
+	git commit -m author \
+		--author "Rubber Duck <rduck@convoy.org>" -a >output 2>&1 &&
+	grep Rubber.Duck output
+'
+
+test_expect_success PERL 'interactive add' '
+	echo 7 |
+	git commit --interactive |
+	grep "What now"
+'
+
+test_expect_success PERL "commit --interactive doesn't change index if editor aborts" '
+	echo zoo >file &&
 	test_must_fail git diff --exit-code >diff1 &&
-	(echo u ; echo '*' ; echo q) |
-	(EDITOR=: && export EDITOR &&
-	 test_must_fail git commit --interactive) &&
+	(echo u ; echo "*" ; echo q) |
+	(
+		EDITOR=: &&
+		export EDITOR &&
+		test_must_fail git commit --interactive
+	) &&
 	git diff >diff2 &&
-	test_cmp diff1 diff2"
+	compare_diff_patch diff1 diff2
+'
 
-cat >editor <<\EOF
-#!/bin/sh
-sed -e "s/good/bad/g" < "$1" > "$1-"
-mv "$1-" "$1"
-EOF
-chmod 755 editor
+test_expect_success 'editor not invoked if -F is given' '
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	sed -e s/good/bad/g <"$1" >"$1-"
+	mv "$1-" "$1"
+	EOF
+	chmod 755 editor &&
 
-cat >msg <<EOF
-A good commit message.
-EOF
+	echo A good commit message. >msg &&
+	echo moo >file &&
 
-test_expect_success \
-	'editor not invoked if -F is given' '
-	 echo "moo" >file &&
-	 EDITOR=./editor git commit -a -F msg &&
-	 git show -s --pretty=format:"%s" | grep -q good &&
-	 echo "quack" >file &&
-	 echo "Another good message." | EDITOR=./editor git commit -a -F - &&
-	 git show -s --pretty=format:"%s" | grep -q good
-	 '
+	EDITOR=./editor git commit -a -F msg &&
+	git show -s --pretty=format:%s >subject &&
+	grep -q good subject &&
+
+	echo quack >file &&
+	echo Another good message. |
+	EDITOR=./editor git commit -a -F - &&
+	git show -s --pretty=format:%s >subject &&
+	grep -q good subject
+'
 
 test_expect_success 'partial commit that involves removal (1)' '

@@ -197,7 +200,6 @@ test_expect_success 'partial commit that involves removal (3)' '
 
 '
 
-author="The Real Author <someguy@his.email.org>"
 test_expect_success 'amend commit to fix author' '
 
 	oldtick=$GIT_AUTHOR_DATE &&

@@ -326,7 +328,6 @@ test_expect_success 'multiple -m' '
 
 '
 
-author="The Real Author <someguy@his.email.org>"
 test_expect_success 'amend commit to fix author' '
 
 	oldtick=$GIT_AUTHOR_DATE &&

@@ -353,15 +354,8 @@ test_expect_success 'git commit <file> with dirty index' '
 
 test_expect_success 'same tree (single parent)' '
 
-	git reset --hard
-
-	if git commit -m empty
-	then
-		echo oops -- should have complained
-		false
-	else
-		: happy
-	fi
+	git reset --hard &&
+	test_must_fail git commit -m empty
 
 '

-- 
1.7.8.rc3

[PATCH 4/4] test: commit --amend should honor --no-edit

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:34

A quick test to make sure git doesn't lose the functionality added by
the recent patch "commit: honor --no-edit", plus another test to check
the classical --edit use case (use with "-m").

Signed-off-by: Jonathan Nieder <redacted>
---
 t/t7501-commit.sh |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index bf025df6..c462bf3b 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh

@@ -94,6 +94,46 @@ test_expect_success 'amend commit' '
 	EDITOR=./editor git commit --amend
 '
 
+test_expect_success 'set up editor' '
+	cat >editor <<-\EOF &&
+	#!/bin/sh
+	sed -e "s/unamended/amended/g" <"$1" >"$1-"
+	mv "$1-" "$1"
+	EOF
+	chmod 755 editor
+'
+
+test_expect_success 'amend without launching editor' '
+	echo unamended >expect &&
+	git commit --allow-empty -m "unamended" &&
+	echo needs more bongo >file &&
+	git add file &&
+	EDITOR=./editor git commit --no-edit --amend &&
+	git diff --exit-code HEAD -- file &&
+	git diff-tree -s --format=%s HEAD >msg &&
+	test_cmp expect msg
+'
+
+test_expect_success '--amend --edit' '
+	echo amended >expect &&
+	git commit --allow-empty -m "unamended" &&
+	echo bongo again >file &&
+	git add file &&
+	EDITOR=./editor git commit --edit --amend &&
+	git diff-tree -s --format=%s HEAD >msg &&
+	test_cmp expect msg
+'
+
+test_expect_success '-m --edit' '
+	echo amended >expect &&
+	git commit --allow-empty -m buffer &&
+	echo bongo bongo >file &&
+	git add file &&
+	EDITOR=./editor git commit -m unamended --edit &&
+	git diff-tree -s  --format=%s HEAD >msg &&
+	test_cmp expect msg
+'
+
 test_expect_success '-m and -F do not mix' '
 	echo enough with the bongos >file &&
 	test_must_fail git commit -F msg -m amending .

-- 
1.7.8.rc3

Re: [PATCH 1/4] test: add missing "&&" after echo command

From: Jeff King <hidden>
Date: 2016-06-15 22:52:35

On Wed, Dec 07, 2011 at 08:45:40AM -0600, Jonathan Nieder wrote:

quoted hunk

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 3ad04363..da75abc1 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh

@@ -60,7 +60,7 @@ test_expect_success \
 
 test_expect_success \
 	"next commit" \
-	"echo 'bongo bongo bongo' >file \
+	"echo 'bongo bongo bongo' >file && \
 	 git commit -m next -a"

Patch is obviously correct, but isn't the "\" here just superfluous and
error-prone? Maybe it should just be dropped from the new version (and
possibly from other tests in t7501).

-Peff

Re: [PATCH 1/4] test: add missing "&&" after echo command

From: Jeff King <hidden>
Date: 2016-06-15 22:52:35

On Wed, Dec 07, 2011 at 06:10:03PM -0500, Jeff King wrote:

On Wed, Dec 07, 2011 at 08:45:40AM -0600, Jonathan Nieder wrote:

quoted

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 3ad04363..da75abc1 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh

@@ -60,7 +60,7 @@ test_expect_success \
 
 test_expect_success \
 	"next commit" \
-	"echo 'bongo bongo bongo' >file \
+	"echo 'bongo bongo bongo' >file && \
 	 git commit -m next -a"

Patch is obviously correct, but isn't the "\" here just superfluous and
error-prone? Maybe it should just be dropped from the new version (and
possibly from other tests in t7501).

Oh, nevermind. I just read your patch 3, which does that and much more.
I approve.

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help