{"thread":{"id":"23666","subject":"[PATCH 0/2] commit --amend and missing space in ident line","startedAt":"2010-05-02T08:49:45Z","lastAt":"2010-05-02T08:57:12Z","messageCount":3,"participants":["Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"140737","messageId":"20100502084827.GA1690@progeny.tock","threadId":"23666","inReplyTo":null,"subject":"[PATCH 0/2] commit --amend and missing space in ident line","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-02T08:49:45Z","receivedAt":"2010-05-02T08:49:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The problem: since fast-import is advertised to expect the format\n\n  \"author\" SP LT email GT SP date\n\nwhen there is no display name associated to the author of a commit, it\nseems likely that some importers use that format[1].  Current\nfast-import passes that input through into the generated commit\nobjects.  Which would mean there are commit objects like this in\nexisting repositories generated by such importers.\n\nSince commit --amend looks for the string \" <\" _after_ the \"author \"\nstring, it does not cope well with this sort of ident line.  In fact,\nthe first \" <\" is found on the following \"committer\" line, while \"> \"\nis found on the author line; the distance between them, because\nnegative, becomes a large unsigned integer; and the display name and\nemail are considered to be very long but acceptable and happily\nused[2].\n\nAfter this series, commit --amend will instead see an empty display\nname and the email from between the angle brackets and accordingly die\nafter complaining about the empty ident.  It might make sense to\ntolerate an empty ident during --amend (silently fixing it up to add\nthe extra space character when missing), but such a change to the\ntraditional \"GIT_AUTHOR_NAME=\" behavior would go beyond the scope of\nthis series.\n\nThoughts?\nJonathan Nieder (2):\n  test-lib: Let tests specify commands to be run at end of test\n  commit --amend: cope better with invalid ident line\n\n builtin/commit.c  |   20 +++++++++++++-------\n t/t7509-commit.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh     |   28 +++++++++++++++++++++++++++-\n 3 files changed, 86 insertions(+), 8 deletions(-)\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/145651/focus=145825\n[2] Earlier I suspected it would die().\nhttp://thread.gmane.org/gmane.comp.version-control.git/145651/focus=145826\nSorry for the nonsense.\n"},{"id":"140739","messageId":"20100502085341.GA1767@progeny.tock","threadId":"23666","inReplyTo":"20100502084827.GA1690@progeny.tock","subject":"[PATCH 1/2] test-lib: Let tests specify commands to be run at end of test","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-02T08:53:41Z","receivedAt":"2010-05-02T08:53:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Certain actions can imply that if the test fails early, recovery from\nwithin other tests is too much to expect:\n\n - creating unwritable directories, like the EACCESS test in t0001-init\n - setting unusual configuration, like user.signingkey in t7004-tag\n - crashing and leaving the index lock held, like t3600-rm once did\n\nSome test scripts work around this by running cleanup actions outside\nthe supervision of the test harness, with the unfortunate consequence\nthat those commands are not appropriately echoed and their output not\nsuppressed.  Others explicitly save exit status, clean up, and then\nreset the exit status within the tests, which has excellent behavior\nbut makes the tests hard to read.  Still others ignore the problem.\n\nAllow tests a fourth option: by calling this function, tests can\nstack up commands they would like to be run to clean up.\n\nCommands passed to test_when_finished during a test are\nunconditionally run in the test environment immediately before the\ntest is completed, in last-in-first-out order.  If some cleanup\ncommand fails, then the other cleanup commands are still run before\nthe failure is reported and the test script allowed to continue.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nReally I am more interested in this helper than the bugfix.\n\n t/test-lib.sh |   28 +++++++++++++++++++++++++++-\n 1 files changed, 27 insertions(+), 1 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c582964..6dcade3 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -354,8 +354,9 @@ test_debug () {\n }\n \n test_run_ () {\n+\ttest_cleanup='eval_ret=$?'\n \teval >&3 2>&4 \"$1\"\n-\teval_ret=\"$?\"\n+\teval >&3 2>&4 \"$test_cleanup\"\n \treturn 0\n }\n \n@@ -533,6 +534,31 @@ test_cmp() {\n \t$GIT_TEST_CMP \"$@\"\n }\n \n+# This function can be used to schedule some commands to be run\n+# unconditionally at the end of the test to restore sanity:\n+#\n+#\ttest_expect_success 'test core.capslock' '\n+#\t\tgit config core.capslock true &&\n+#\t\ttest_when_finished \"git config --unset core.capslock\" &&\n+#\t\thello world\n+#\t'\n+#\n+# That would be roughly equivalent to\n+#\n+#\ttest_expect_success 'test core.capslock' '\n+#\t\tgit config core.capslock true &&\n+#\t\thello world\n+#\t\tgit config --unset core.capslock\n+#\t'\n+#\n+# except that the greeting and config --unset must both succeed for\n+# the test to pass.\n+\n+test_when_finished () {\n+\ttest_cleanup=\"eval_ret=\\$?; { $*\n+\t\t} && (exit \\\"\\$eval_ret\\\"); $test_cleanup\"\n+}\n+\n # Most tests can use the created repository, but some may need to create more.\n # Usage: test_create_repo <directory>\n test_create_repo () {\n-- \n1.7.1\n"},{"id":"140740","messageId":"20100502085712.GB1767@progeny.tock","threadId":"23666","inReplyTo":"20100502084827.GA1690@progeny.tock","subject":"[PATCH 2/2] commit --amend: cope with missing display name","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-05-02T08:57:12Z","receivedAt":"2010-05-02T08:57:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Though I have not seen this in the wild, it has been said that there\nare likely to be git repositories converted from other version control\nsystems with an invalid ident line like this one:\n\n  author <user@example.com> 18746342 +0000\n\nBecause there is no space between the (empty) user name and the email\naddress, commit --amend chokes.  When searching for a\nspace-left-bracket sequence on the ident line, it finds it in the\ncommitter line, ending up utterly confused.\n\nBetter for commit --amend to treat this like a valid ident line with\nempty username and complain.\n\nThe tests remove the questionable commit objects after use so there is\nno chance for them to confuse later tests.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nThanks for reading.  I look forward to your thoughts.\n\n builtin/commit.c  |   20 +++++++++++++-------\n t/t7509-commit.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 59 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex c5ab683..a3b6fc4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -455,15 +455,21 @@ static void determine_author_info(void)\n \t\tif (!a)\n \t\t\tdie(\"invalid commit: %s\", use_message);\n \n-\t\tlb = strstr(a + 8, \" <\");\n-\t\trb = strstr(a + 8, \"> \");\n-\t\teol = strchr(a + 8, '\\n');\n-\t\tif (!lb || !rb || !eol)\n+\t\tlb = strchrnul(a + strlen(\"\\nauthor \"), '<');\n+\t\trb = strchrnul(lb, '>');\n+\t\teol = strchrnul(rb, '\\n');\n+\t\tif (!*lb || !*rb || !*eol)\n \t\t\tdie(\"invalid commit: %s\", use_message);\n \n-\t\tname = xstrndup(a + 8, lb - (a + 8));\n-\t\temail = xstrndup(lb + 2, rb - (lb + 2));\n-\t\tdate = xstrndup(rb + 2, eol - (rb + 2));\n+\t\tif (lb == a + strlen(\"\\nauthor \"))\n+\t\t\t/* \\nauthor <foo@example.com> */\n+\t\t\tname = xcalloc(1, 1);\n+\t\telse\n+\t\t\tname = xmemdupz(a + strlen(\"\\nauthor \"),\n+\t\t\t                (lb - strlen(\" \") -\n+\t\t\t                 (a + strlen(\"\\nauthor \"))));\n+\t\temail = xmemdupz(lb + strlen(\"<\"), rb - (lb + strlen(\"<\")));\n+\t\tdate = xmemdupz(rb + strlen(\"> \"), eol - (rb + strlen(\"> \")));\n \t}\n \n \tif (force_author) {\ndiff --git a/t/t7509-commit.sh b/t/t7509-commit.sh\nindex d52c060..3ea33db 100755\n--- a/t/t7509-commit.sh\n+++ b/t/t7509-commit.sh\n@@ -83,6 +83,52 @@ test_expect_success '--amend option copies authorship' '\n \ttest_cmp expect actual\n '\n \n+sha1_file() {\n+\techo \"$*\" | sed \"s#..#.git/objects/&/#\"\n+}\n+remove_object() {\n+\trm -f $(sha1_file \"$*\")\n+}\n+no_reflog() {\n+\tcp .git/config .git/config.saved &&\n+\techo \"[core] logallrefupdates = false\" >>.git/config &&\n+\ttest_when_finished \"mv -f .git/config.saved .git/config\" &&\n+\n+\tif test -e .git/logs\n+\tthen\n+\t\tmv .git/logs . &&\n+\t\ttest_when_finished \"mv logs .git/\"\n+\tfi\n+}\n+\n+test_expect_success '--amend option with empty author' '\n+\tgit cat-file commit Initial >tmp &&\n+\tsed \"s/author [^<]* </author  </\" tmp >empty-author &&\n+\tno_reflog &&\n+\tsha=$(git hash-object -t commit -w empty-author) &&\n+\ttest_when_finished \"remove_object $sha\" &&\n+\tgit checkout $sha &&\n+\ttest_when_finished \"git checkout Initial\" &&\n+\techo \"Empty author test\" >>foo &&\n+\ttest_tick &&\n+\t! git commit -a -m \"empty author\" --amend 2>err &&\n+\tgrep \"empty ident\" err\n+'\n+\n+test_expect_success '--amend option with missing author' '\n+\tgit cat-file commit Initial >tmp &&\n+\tsed \"s/author [^<]* </author </\" tmp >malformed &&\n+\tno_reflog &&\n+\tsha=$(git hash-object -t commit -w malformed) &&\n+\ttest_when_finished \"remove_object $sha\" &&\n+\tgit checkout $sha &&\n+\ttest_when_finished \"git checkout Initial\" &&\n+\techo \"Missing author test\" >>foo &&\n+\ttest_tick &&\n+\t! git commit -a -m \"malformed author\" --amend 2>err &&\n+\tgrep \"empty ident\" err\n+'\n+\n test_expect_success '--reset-author makes the commit ours even with --amend option' '\n \tgit checkout Initial &&\n \techo \"Test 6\" >>foo &&\n-- \n1.7.1\n"}]}