{"thread":{"id":"29077","subject":"[PATCH 1/2] t/t4131-apply-fake-ancestor.sh: fix broken test","startedAt":"2011-12-03T20:35:49Z","lastAt":"2011-12-05T22:38:31Z","messageCount":6,"participants":["Brandon Casey","Artem Bityutskiy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"180285","messageId":"1322944550-27344-1-git-send-email-drafnel@gmail.com","threadId":"29077","inReplyTo":null,"subject":"[PATCH 1/2] t/t4131-apply-fake-ancestor.sh: fix broken test","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-12-03T20:35:49Z","receivedAt":"2011-12-03T20:35:49Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"The third test \"apply --build-fake-ancestor in a subdirectory\" has been\nbroken since it was introduced. It intended to modify a tracked file named\n'sub/3.t' and then produce a diff which could be git apply'ed, but the file\nnamed 'sub/3.t' does not exist. The file that exists in the repo is called\n'sub/3'. Since no tracked files were modified, an empty diff was produced,\nand the test succeeded.\n\nCorrect this test by supplying the intended name of the tracked file,\n'sub/3.t', to test_commit in the first test.\n\nSigned-off-by: Brandon Casey \n---\n t/t4131-apply-fake-ancestor.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t4131-apply-fake-ancestor.sh b/t/t4131-apply-fake-ancestor.sh\nindex 94373ca..b1361ce 100755\n--- a/t/t4131-apply-fake-ancestor.sh\n+++ b/t/t4131-apply-fake-ancestor.sh\n@@ -11,7 +11,7 @@ test_expect_success 'setup' '\n \ttest_commit 1 &&\n \ttest_commit 2 &&\n \tmkdir sub &&\n-\ttest_commit 3 sub/3 &&\n+\ttest_commit 3 sub/3.t &&\n \ttest_commit 4\n '\n \n-- \n1.7.8\n"},{"id":"180286","messageId":"1322944550-27344-2-git-send-email-drafnel@gmail.com","threadId":"29077","inReplyTo":"1322944550-27344-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 2/2] builtin/apply.c: report error on failure to recognize input","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-12-03T20:35:50Z","receivedAt":"2011-12-03T20:35:50Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"When git apply is passed something that is not a patch, it does not produce\nan error message or exit with a non-zero status if it was not actually\n\"applying\" the patch i.e. --check or --numstat etc were supplied on the\ncommand line.\n\nFix this by producing an error when apply fails to find any hunks whatsoever\nwhile parsing the patch.\n\nThis will cause some of the output formats (--numstat, --diffstat, etc) to\nproduce an error when they formerly would have reported zero changes and\nexited successfully. That seems like the correct behavior though. Failure\nto recognize the input as a patch should be an error.\n\nPlus, add a test.\n\nReported-by: Artem Bityutskiy \nSigned-off-by: Brandon Casey \n---\n\n\nInitially, I was reluctant to change the error message, thinking that\nerror messages for plumbing commands were not supposed to change. But I\nthink I was wrong in that thought, so I changed the error message so it\nwas a more descriptive \"unrecognized input\".\n\n-Brandon\n\n\n builtin/apply.c | 10 +++++-----\n t/t4136-apply-check.sh | 19 +++++++++++++++++++\n 2 files changed, 24 insertions(+), 5 deletions(-)\n create mode 100755 t/t4136-apply-check.sh\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 84a8a0b..46dcf3c 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3590,15 +3590,12 @@ static int write_out_one_reject(struct patch *patch)\n \treturn -1;\n }\n \n-static int write_out_results(struct patch *list, int skipped_patch)\n+static int write_out_results(struct patch *list)\n {\n \tint phase;\n \tint errs = 0;\n \tstruct patch *l;\n \n-\tif (!list && !skipped_patch)\n-\t\treturn error(\"No changes\");\n-\n \tfor (phase = 0; phase < 2; phase++) {\n \t\tl = list;\n \t\twhile (l) {\n@@ -3724,6 +3721,9 @@ static int apply_patch(int fd, const char *filename, int options)\n \t\toffset += nr;\n \t}\n \n+\tif (!list && !skipped_patch)\n+\t\tdie(\"unrecognized input\");\n+\n \tif (whitespace_error && (ws_error_action == die_on_ws_error))\n \t\tapply = 0;\n \n@@ -3741,7 +3741,7 @@ static int apply_patch(int fd, const char *filename, int options)\n \t !apply_with_reject)\n \t\texit(1);\n \n-\tif (apply && write_out_results(list, skipped_patch))\n+\tif (apply && write_out_results(list))\n \t\texit(1);\n \n \tif (fake_ancestor)\ndiff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh\nnew file mode 100755\nindex 0000000..a321f7c\n--- /dev/null\n+++ b/t/t4136-apply-check.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+\n+test_description='git apply should exit non-zero with unrecognized input.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit 1\n+'\n+\n+test_expect_success 'apply --check exits non-zero with unrecognized input' '\n+\ttest_must_fail git apply --check - <<-\\EOF\n+\tI am not a patch\n+\tI look nothing like a patch\n+\tgit apply must fail\n+\tEOF\n+'\n+\n+test_done\n-- \n1.7.8\n"},{"id":"180292","messageId":"1323005418.9400.49.camel@sauron.fi.intel.com","threadId":"29077","inReplyTo":"1322944550-27344-2-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 2/2] builtin/apply.c: report error on failure to recognize input","fromName":"Artem Bityutskiy","fromEmail":"artem.bityutskiy@linux.intel.com","sentAt":"2011-12-04T13:30:16Z","receivedAt":"2011-12-04T13:30:16Z","isPatch":true,"sender":{"key":"artem.bityutskiy@linux.intel.com","avatar":null},"body":"On Sat, 2011-12-03 at 14:35 -0600, Brandon Casey wrote:\n> When git apply is passed something that is not a patch, it does not produce\n> an error message or exit with a non-zero status if it was not actually\n> \"applying\" the patch i.e. --check or --numstat etc were supplied on the\n> command line.\n> \n> Fix this by producing an error when apply fails to find any hunks whatsoever\n> while parsing the patch.\n> \n> This will cause some of the output formats (--numstat, --diffstat, etc) to\n> produce an error when they formerly would have reported zero changes and\n> exited successfully. That seems like the correct behavior though. Failure\n> to recognize the input as a patch should be an error.\n> \n> Plus, add a test.\n> \n> Reported-by: Artem Bityutskiy \n> Signed-off-by: Brandon Casey \n\nBrandon, Thanks a lot for picking this. I did not reply because I did\nnot have time to look at this after your review yet, it was in my TODO\nlist. But I am happy you picked this and fixed the issue.\n\nTested-by: Artem Bityutskiy \n\n-- \nBest Regards,\nArtem Bityutskiy\n"},{"id":"180294","messageId":"197987ca-1fbd-473f-be7e-9e47439c47ee@email.android.com","threadId":"29077","inReplyTo":"1323005418.9400.49.camel@sauron.fi.intel.com","subject":"Re: [PATCH 2/2] builtin/apply.c: report error on failure to recognize input","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-12-04T15:39:07Z","receivedAt":"2011-12-04T15:39:07Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Artem Bityutskiy wrote:\n\n>Brandon, Thanks a lot for picking this. I did not reply because I did\n>not have time to look at this after your review yet, it was in my TODO\n>list. But I am happy you picked this and fixed the issue.\n\nNo problem.\n\n>Tested-by: Artem Bityutskiy \n\nThanks.\n\n-Brandon\n"},{"id":"180311","messageId":"7vzkf6lsyx.fsf@alter.siamese.dyndns.org","threadId":"29077","inReplyTo":"1322944550-27344-2-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 2/2] builtin/apply.c: report error on failure to recognize input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-05T19:27:18Z","receivedAt":"2011-12-05T19:27:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> When git apply is passed something that is not a patch, it does not produce\n> an error message or exit with a non-zero status if it was not actually\n> \"applying\" the patch i.e. --check or --numstat etc were supplied on the\n> command line.\n>\n> Fix this by producing an error when apply fails to find any hunks whatsoever\n> while parsing the patch.\n>\n> This will cause some of the output formats (--numstat, --diffstat, etc) to\n> produce an error when they formerly would have reported zero changes and\n> exited successfully. That seems like the correct behavior though. Failure\n> to recognize the input as a patch should be an error.\n>\n> Plus, add a test.\n>\n> Reported-by: Artem Bityutskiy \n> Signed-off-by: Brandon Casey \n> ---\n>\n> Initially, I was reluctant to change the error message, thinking that\n> error messages for plumbing commands were not supposed to change. But I\n> think I was wrong in that thought, so I changed the error message so it\n> was a more descriptive \"unrecognized input\".\n\nI am still reluctant to see\n\n $ git apply wrote:\n> Brandon Casey writes:\n>\n>> When git apply is passed something that is not a patch, it does not produce\n>> an error message or exit with a non-zero status if it was not actually\n>> \"applying\" the patch i.e. --check or --numstat etc were supplied on the\n>> command line.\n>>\n>> Fix this by producing an error when apply fails to find any hunks whatsoever\n>> while parsing the patch.\n>>\n>> This will cause some of the output formats (--numstat, --diffstat, etc) to\n>> produce an error when they formerly would have reported zero changes and\n>> exited successfully.  That seems like the correct behavior though.  Failure\n>> to recognize the input as a patch should be an error.\n>>\n>> Plus, add a test.\n>>\n>> Reported-by: Artem Bityutskiy \n>> Signed-off-by: Brandon Casey \n>> ---\n>>\n>> Initially, I was reluctant to change the error message, thinking that\n>> error messages for plumbing commands were not supposed to change.  But I\n>> think I was wrong in that thought, so I changed the error message so it\n>> was a more descriptive \"unrecognized input\".\n>\n> I am still reluctant to see\n>\n>    $ git apply    error: unrecognized input\n>\n> instead of \"error: No changes\", though.\n\nI'm not partial to \"unrecognized input\", but I thought it was more\ndescriptive of what happened than \"No changes\". This error message is\nonly printed out when absolutely no hunks were found while parsing the\ninput.\n\n> \"git apply --check\" is about asking \"do you see anything offending in the\n> diff?\" and it is not \"git apply --dry-run\" that asks \"do you promise if I\n> feed this for real to you you will apply it without complaint?\".\n>\n> I am slightly in favor of answering \"well you do not have a diff to begin\n> with, which in itself is suspicious\" to \"do you see anything offending?\"\n> question, but I have to admit that it is an equally valid answer to say\n> \"no, there is nothing offending in the diff.\", which is what we do with\n> the current code.\n>\n> So, I dunno.\n\nI think the current code is a little inconsistent with respect to\nempty or bogus non-diff input.\n\nIt seems more consistent that if it is an error to tell git apply to\napply zero hunks, then it is also an error to --check zero hunks, or\n--stat etc. In all cases the cause is the same: failure to find any\nhunks in the input because the input was not a diff.\n\nAlso, the man page description of --check says that it checks \"if the\npatch is applicable to the current working tree and/or the index\".\nThe new behavior would answer that with \"no, this patch is not\napplicable ... since no hunks were found\", rather than \"yes, because\nno hunks were found\". But I'm really arguing on the side of\n\"unrecognized input should be an error\", since the new behavior would\nalso be an error for --stat, --numstat, etc.\n\n-Brandon\n"}]}