threads / patch / 23666

patch, 2 partscommit --amend and missing space in ident line

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

## tl;dr

3 messages between May 2, 2010 and May 2, 2010. Diffs are folded; open one to read it.

replies: 2people: 1as markdown or json

Jonathan Nieder· May 2, 2010, 08:49 UTC · lore
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.

Jonathan Nieder· May 2, 2010, 08:53 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to t/test-lib.sh +27 −1
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
Jonathan Nieder· May 2, 2010, 08:57 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to 2 files +59 −7

builtin/commit.c, t/t7509-commit.sh

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

← back to recent threads