[PATCH 0/2] commit --amend and missing space in ident line

[PATCH 0/2] commit --amend and missing space in ident line

[Jonathan Nieder]

The problem: since fast-import is advertised to expect the format

  "author" SP LT email GT SP date

when there is no display name associated to the author of a commit, it
seems likely that some importers use that format[1].  Current
fast-import passes that input through into the generated commit
objects.  Which would mean there are commit objects like this in
existing repositories generated by such importers.

Since commit --amend looks for the string " <" _after_ the "author "
string, it does not cope well with this sort of ident line.  In fact,
the first " <" is found on the following "committer" line, while "> "
is found on the author line; the distance between them, because
negative, becomes a large unsigned integer; and the display name and
email are considered to be very long but acceptable and happily
used[2].

After this series, commit --amend will instead see an empty display
name and the email from between the angle brackets and accordingly die
after complaining about the empty ident.  It might make sense to
tolerate an empty ident during --amend (silently fixing it up to add
the extra space character when missing), but such a change to the
traditional "GIT_AUTHOR_NAME=" behavior would go beyond the scope of
this series.

Thoughts?
Jonathan Nieder (2):
  test-lib: Let tests specify commands to be run at end of test
  commit --amend: cope better with invalid ident line

 builtin/commit.c  |   20 +++++++++++++-------
 t/t7509-commit.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh     |   28 +++++++++++++++++++++++++++-
 3 files changed, 86 insertions(+), 8 deletions(-)

[1] http://thread.gmane.org/gmane.comp.version-control.git/145651/focus=145825
[2] Earlier I suspected it would die().
http://thread.gmane.org/gmane.comp.version-control.git/145651/focus=145826
Sorry for the nonsense.

[PATCH 1/2] test-lib: Let tests specify commands to be run at end of test

[Jonathan Nieder]

Certain actions can imply that if the test fails early, recovery from
within other tests is too much to expect:

 - creating unwritable directories, like the EACCESS test in t0001-init
 - setting unusual configuration, like user.signingkey in t7004-tag
 - crashing and leaving the index lock held, like t3600-rm once did

Some test scripts work around this by running cleanup actions outside
the supervision of the test harness, with the unfortunate consequence
that those commands are not appropriately echoed and their output not
suppressed.  Others explicitly save exit status, clean up, and then
reset the exit status within the tests, which has excellent behavior
but makes the tests hard to read.  Still others ignore the problem.

Allow tests a fourth option: by calling this function, tests can
stack up commands they would like to be run to clean up.

Commands passed to test_when_finished during a test are
unconditionally run in the test environment immediately before the
test is completed, in last-in-first-out order.  If some cleanup
command fails, then the other cleanup commands are still run before
the failure is reported and the test script allowed to continue.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Really I am more interested in this helper than the bugfix.

 t/test-lib.sh |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index c582964..6dcade3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -354,8 +354,9 @@ test_debug () {
 }
 
 test_run_ () {
+	test_cleanup='eval_ret=$?'
 	eval >&3 2>&4 "$1"
-	eval_ret="$?"
+	eval >&3 2>&4 "$test_cleanup"
 	return 0
 }
 
@@ -533,6 +534,31 @@ test_cmp() {
 	$GIT_TEST_CMP "$@"
 }
 
+# This function can be used to schedule some commands to be run
+# unconditionally at the end of the test to restore sanity:
+#
+#	test_expect_success 'test core.capslock' '
+#		git config core.capslock true &&
+#		test_when_finished "git config --unset core.capslock" &&
+#		hello world
+#	'
+#
+# That would be roughly equivalent to
+#
+#	test_expect_success 'test core.capslock' '
+#		git config core.capslock true &&
+#		hello world
+#		git config --unset core.capslock
+#	'
+#
+# except that the greeting and config --unset must both succeed for
+# the test to pass.
+
+test_when_finished () {
+	test_cleanup="eval_ret=\$?; { $*
+		} && (exit \"\$eval_ret\"); $test_cleanup"
+}
+
 # Most tests can use the created repository, but some may need to create more.
 # Usage: test_create_repo <directory>
 test_create_repo () {
-- 
1.7.1

[PATCH 2/2] commit --amend: cope with missing display name

[Jonathan Nieder]

Though I have not seen this in the wild, it has been said that there
are likely to be git repositories converted from other version control
systems with an invalid ident line like this one:

  author <user@example.com> 18746342 +0000

Because there is no space between the (empty) user name and the email
address, commit --amend chokes.  When searching for a
space-left-bracket sequence on the ident line, it finds it in the
committer line, ending up utterly confused.

Better for commit --amend to treat this like a valid ident line with
empty username and complain.

The tests remove the questionable commit objects after use so there is
no chance for them to confuse later tests.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.  I look forward to your thoughts.

 builtin/commit.c  |   20 +++++++++++++-------
 t/t7509-commit.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index c5ab683..a3b6fc4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -455,15 +455,21 @@ static void determine_author_info(void)
 		if (!a)
 			die("invalid commit: %s", use_message);
 
-		lb = strstr(a + 8, " <");
-		rb = strstr(a + 8, "> ");
-		eol = strchr(a + 8, '\n');
-		if (!lb || !rb || !eol)
+		lb = strchrnul(a + strlen("\nauthor "), '<');
+		rb = strchrnul(lb, '>');
+		eol = strchrnul(rb, '\n');
+		if (!*lb || !*rb || !*eol)
 			die("invalid commit: %s", use_message);
 
-		name = xstrndup(a + 8, lb - (a + 8));
-		email = xstrndup(lb + 2, rb - (lb + 2));
-		date = xstrndup(rb + 2, eol - (rb + 2));
+		if (lb == a + strlen("\nauthor "))
+			/* \nauthor <foo@example.com> */
+			name = xcalloc(1, 1);
+		else
+			name = xmemdupz(a + strlen("\nauthor "),
+			                (lb - strlen(" ") -
+			                 (a + strlen("\nauthor "))));
+		email = xmemdupz(lb + strlen("<"), rb - (lb + strlen("<")));
+		date = xmemdupz(rb + strlen("> "), eol - (rb + strlen("> ")));
 	}
 
 	if (force_author) {
diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
index d52c060..3ea33db 100755
--- a/t/t7509-commit.sh
+++ b/t/t7509-commit.sh
@@ -83,6 +83,52 @@ test_expect_success '--amend option copies authorship' '
 	test_cmp expect actual
 '
 
+sha1_file() {
+	echo "$*" | sed "s#..#.git/objects/&/#"
+}
+remove_object() {
+	rm -f $(sha1_file "$*")
+}
+no_reflog() {
+	cp .git/config .git/config.saved &&
+	echo "[core] logallrefupdates = false" >>.git/config &&
+	test_when_finished "mv -f .git/config.saved .git/config" &&
+
+	if test -e .git/logs
+	then
+		mv .git/logs . &&
+		test_when_finished "mv logs .git/"
+	fi
+}
+
+test_expect_success '--amend option with empty author' '
+	git cat-file commit Initial >tmp &&
+	sed "s/author [^<]* </author  </" tmp >empty-author &&
+	no_reflog &&
+	sha=$(git hash-object -t commit -w empty-author) &&
+	test_when_finished "remove_object $sha" &&
+	git checkout $sha &&
+	test_when_finished "git checkout Initial" &&
+	echo "Empty author test" >>foo &&
+	test_tick &&
+	! git commit -a -m "empty author" --amend 2>err &&
+	grep "empty ident" err
+'
+
+test_expect_success '--amend option with missing author' '
+	git cat-file commit Initial >tmp &&
+	sed "s/author [^<]* </author </" tmp >malformed &&
+	no_reflog &&
+	sha=$(git hash-object -t commit -w malformed) &&
+	test_when_finished "remove_object $sha" &&
+	git checkout $sha &&
+	test_when_finished "git checkout Initial" &&
+	echo "Missing author test" >>foo &&
+	test_tick &&
+	! git commit -a -m "malformed author" --amend 2>err &&
+	grep "empty ident" err
+'
+
 test_expect_success '--reset-author makes the commit ours even with --amend option' '
 	git checkout Initial &&
 	echo "Test 6" >>foo &&
-- 
1.7.1