{"thread":{"id":"29011","subject":"[PATCH] git-apply: fix rubbish handling in --check case","startedAt":"2011-11-23T16:26:02Z","lastAt":"2011-11-23T18:44:52Z","messageCount":3,"participants":["Artem Bityutskiy","Brandon Casey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"179889","messageId":"1322065563-3651-1-git-send-email-dedekind1@gmail.com","threadId":"29011","inReplyTo":null,"subject":"[PATCH] git-apply: fix rubbish handling in --check case","fromName":"Artem Bityutskiy","fromEmail":"dedekind1@gmail.com","sentAt":"2011-11-23T16:26:02Z","receivedAt":"2011-11-23T16:26:02Z","isPatch":true,"sender":{"key":"dedekind1@gmail.com","avatar":null},"body":"From: Artem Bityutskiy \n\nThis patch fixes the following inconsistency:\n\nLet's take my bash binary.\n\n$ ls -l /bin/bash\n-rwxr-xr-x. 1 root root 924200 Jun 22 16:49 /bin/bash\n$ file /bin/bash\n/bin/bash: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.32, stripped\n\nLet's try to apply it\n\n$ git apply /bin/bash\nerror: No changes\n$ echo $?\n1\n\nGood, rejected and error code is returned. Let's try with --check:\n\n$ git apply --check /bin/bash\n$ echo $?\n0\n\nNot exactly what I expected :-) The same happnes if you use an empty file.\n\nSigned-off-by: Artem Bityutskiy \n---\n\nNote, I did not extensively test it!\n\n Makefile | 2 +-\n builtin/apply.c | 8 +++++---\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 84a8a0b..2d6862a 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3596,9 +3596,6 @@ static int write_out_results(struct patch *list, int skipped_patch)\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@@ -3741,6 +3738,11 @@ static int apply_patch(int fd, const char *filename, int options)\n \t !apply_with_reject)\n \t\texit(1);\n \n+\tif (!list && !skipped_patch) {\n+\t\terror(\"No changes\");\n+\t\texit(1);\n+\t}\n+\n \tif (apply && write_out_results(list, skipped_patch))\n \t\texit(1);\n \n-- \n1.7.6.4\n"},{"id":"179890","messageId":"1322065563-3651-2-git-send-email-dedekind1@gmail.com","threadId":"29011","inReplyTo":"1322065563-3651-1-git-send-email-dedekind1@gmail.com","subject":"[PATCH] git-apply: fix rubbish handling in --check case","fromName":"Artem Bityutskiy","fromEmail":"dedekind1@gmail.com","sentAt":"2011-11-23T16:26:03Z","receivedAt":"2011-11-23T16:26:03Z","isPatch":true,"sender":{"key":"dedekind1@gmail.com","avatar":null},"body":"From: Artem Bityutskiy \n\nThis patch fixes the following inconsistency:\n\nLet's take my bash binary.\n\n$ ls -l /bin/bash\n-rwxr-xr-x. 1 root root 924200 Jun 22 16:49 /bin/bash\n$ file /bin/bash\n/bin/bash: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.32, stripped\n\nLet's try to apply it\n\n$ git apply /bin/bash\nerror: No changes\n$ echo $?\n1\n\nGood, rejected and error code is returned. Let's try with --check:\n\n$ git apply --check /bin/bash\n$ echo $?\n0\n\nNot exactly what I expected :-) The same happnes if you use an empty file.\n\nSigned-off-by: Artem Bityutskiy \n---\n\nNote, I did not extensively test it!\n\n Makefile | 2 +-\n builtin/apply.c | 8 +++++---\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 84a8a0b..2d6862a 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3596,9 +3596,6 @@ static int write_out_results(struct patch *list, int skipped_patch)\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@@ -3741,6 +3738,11 @@ static int apply_patch(int fd, const char *filename, int options)\n \t !apply_with_reject)\n \t\texit(1);\n \n+\tif (!list && !skipped_patch) {\n+\t\terror(\"No changes\");\n+\t\texit(1);\n+\t}\n+\n \tif (apply && write_out_results(list, skipped_patch))\n \t\texit(1);\n \n-- \n1.7.6.4\n"},{"id":"179898","messageId":"CA+sFfMf2+XJGtQACeh=J4BA1rGP=KsFDnzw0UECjmTPp2-ZUzQ@mail.gmail.com","threadId":"29011","inReplyTo":"1322065563-3651-1-git-send-email-dedekind1@gmail.com","subject":"Re: [PATCH] git-apply: fix rubbish handling in --check case","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2011-11-23T18:44:52Z","receivedAt":"2011-11-23T18:44:52Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Nov 23, 2011 at 10:26 AM, Artem Bityutskiy wrote:\n> From: Artem Bityutskiy \n\n> $ git apply --check /bin/bash\n> $ echo $?\n> 0\n>\n> Not exactly what I expected :-) The same happnes if you use an empty file.\n>\n> Signed-off-by: Artem Bityutskiy \n> ---\n>\n> Note, I did not extensively test it!\n>\n>  Makefile        |    2 +-\n>  builtin/apply.c |    8 +++++---\n>  2 files changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 84a8a0b..2d6862a 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -3596,9 +3596,6 @@ static int write_out_results(struct patch *list, int skipped_patch)\n>        int errs = 0;\n>        struct patch *l;\n>\n> -       if (!list && !skipped_patch)\n> -               return error(\"No changes\");\n> -\n\nI think write_out_results() can lose the skipped_patch parameter now.\n\n>        for (phase = 0; phase < 2; phase++) {\n>                l = list;\n>                while (l) {\n> @@ -3741,6 +3738,11 @@ static int apply_patch(int fd, const char *filename, int options)\n>            !apply_with_reject)\n>                exit(1);\n>\n> +       if (!list && !skipped_patch) {\n> +               error(\"No changes\");\n> +               exit(1);\n\nYou can use die() instead of error followed by exit.\n\nAlso, I don't see any reason why this check shouldn't be moved up so\nthat it is performed immediately after the while loop, avoiding any\nunnecessary work.\n\nbtw. die'ing like this affects diffstat, numstat, summary etc. too\nsince now they will report an error instead of just displaying an\ninformational message describing zero changes when passed a bogus\npatch. But that seems correct to me.\n\nI would have also suggested changing the error message to something\nmore descriptive than \"No changes\", but apparently apply is a plumbing\ncommand. It seems kind of an \"in-between plumbing and porcelain\"\ncommand.\n\nIt still seems like the correct behavior change with respect to\ndiffstat, numstat et al that I mentioned above after rethinking from a\nplumbing perspective. git apply should error out if it cannot\nrecognize its input.\n\n-Brandon\n"}]}