[PATCH 1/2] t/t4131-apply-fake-ancestor.sh: fix broken test

[PATCH 1/2] t/t4131-apply-fake-ancestor.sh: fix broken test

[Brandon Casey]

The third test "apply --build-fake-ancestor in a subdirectory" has been
broken since it was introduced.  It intended to modify a tracked file named
'sub/3.t' and then produce a diff which could be git apply'ed, but the file
named 'sub/3.t' does not exist.  The file that exists in the repo is called
'sub/3'.  Since no tracked files were modified, an empty diff was produced,
and the test succeeded.

Correct this test by supplying the intended name of the tracked file,
'sub/3.t', to test_commit in the first test.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 t/t4131-apply-fake-ancestor.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t4131-apply-fake-ancestor.sh b/t/t4131-apply-fake-ancestor.sh
index 94373ca..b1361ce 100755
--- a/t/t4131-apply-fake-ancestor.sh
+++ b/t/t4131-apply-fake-ancestor.sh
@@ -11,7 +11,7 @@ test_expect_success 'setup' '
 	test_commit 1 &&
 	test_commit 2 &&
 	mkdir sub &&
-	test_commit 3 sub/3 &&
+	test_commit 3 sub/3.t &&
 	test_commit 4
 '
 
-- 
1.7.8

[PATCH 2/2] builtin/apply.c: report error on failure to recognize input

[Brandon Casey]

When git apply is passed something that is not a patch, it does not produce
an error message or exit with a non-zero status if it was not actually
"applying" the patch i.e. --check or --numstat etc were supplied on the
command line.

Fix this by producing an error when apply fails to find any hunks whatsoever
while parsing the patch.

This will cause some of the output formats (--numstat, --diffstat, etc) to
produce an error when they formerly would have reported zero changes and
exited successfully.  That seems like the correct behavior though.  Failure
to recognize the input as a patch should be an error.

Plus, add a test.

Reported-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Signed-off-by: Brandon Casey <drafnel@gmail.com>
---


Initially, I was reluctant to change the error message, thinking that
error messages for plumbing commands were not supposed to change.  But I
think I was wrong in that thought, so I changed the error message so it
was a more descriptive "unrecognized input".

-Brandon


 builtin/apply.c        |   10 +++++-----
 t/t4136-apply-check.sh |   19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 5 deletions(-)
 create mode 100755 t/t4136-apply-check.sh

diff --git a/builtin/apply.c b/builtin/apply.c
index 84a8a0b..46dcf3c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3590,15 +3590,12 @@ static int write_out_one_reject(struct patch *patch)
 	return -1;
 }
 
-static int write_out_results(struct patch *list, int skipped_patch)
+static int write_out_results(struct patch *list)
 {
 	int phase;
 	int errs = 0;
 	struct patch *l;
 
-	if (!list && !skipped_patch)
-		return error("No changes");
-
 	for (phase = 0; phase < 2; phase++) {
 		l = list;
 		while (l) {
@@ -3724,6 +3721,9 @@ static int apply_patch(int fd, const char *filename, int options)
 		offset += nr;
 	}
 
+	if (!list && !skipped_patch)
+		die("unrecognized input");
+
 	if (whitespace_error && (ws_error_action == die_on_ws_error))
 		apply = 0;
 
@@ -3741,7 +3741,7 @@ static int apply_patch(int fd, const char *filename, int options)
 	    !apply_with_reject)
 		exit(1);
 
-	if (apply && write_out_results(list, skipped_patch))
+	if (apply && write_out_results(list))
 		exit(1);
 
 	if (fake_ancestor)
diff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh
new file mode 100755
index 0000000..a321f7c
--- /dev/null
+++ b/t/t4136-apply-check.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='git apply should exit non-zero with unrecognized input.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit 1
+'
+
+test_expect_success 'apply --check exits non-zero with unrecognized input' '
+	test_must_fail git apply --check - <<-\EOF
+	I am not a patch
+	I look nothing like a patch
+	git apply must fail
+	EOF
+'
+
+test_done
-- 
1.7.8

Re: [PATCH 2/2] builtin/apply.c: report error on failure to recognize input

[Artem Bityutskiy]

On Sat, 2011-12-03 at 14:35 -0600, Brandon Casey wrote:
> When git apply is passed something that is not a patch, it does not produce
> an error message or exit with a non-zero status if it was not actually
> "applying" the patch i.e. --check or --numstat etc were supplied on the
> command line.
> 
> Fix this by producing an error when apply fails to find any hunks whatsoever
> while parsing the patch.
> 
> This will cause some of the output formats (--numstat, --diffstat, etc) to
> produce an error when they formerly would have reported zero changes and
> exited successfully.  That seems like the correct behavior though.  Failure
> to recognize the input as a patch should be an error.
> 
> Plus, add a test.
> 
> Reported-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>

Brandon, Thanks a lot for picking this. I did not reply because I did
not have time to look at this after your review yet, it was in my TODO
list. But I am happy you picked this and fixed the issue.

Tested-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 2/2] builtin/apply.c: report error on failure to recognize input

[Brandon Casey]

Artem Bityutskiy <artem.bityutskiy@linux.intel.com> wrote:

>Brandon, Thanks a lot for picking this. I did not reply because I did
>not have time to look at this after your review yet, it was in my TODO
>list. But I am happy you picked this and fixed the issue.

No problem.

>Tested-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Thanks.

-Brandon

Re: [PATCH 2/2] builtin/apply.c: report error on failure to recognize input

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> When git apply is passed something that is not a patch, it does not produce
> an error message or exit with a non-zero status if it was not actually
> "applying" the patch i.e. --check or --numstat etc were supplied on the
> command line.
>
> Fix this by producing an error when apply fails to find any hunks whatsoever
> while parsing the patch.
>
> This will cause some of the output formats (--numstat, --diffstat, etc) to
> produce an error when they formerly would have reported zero changes and
> exited successfully.  That seems like the correct behavior though.  Failure
> to recognize the input as a patch should be an error.
>
> Plus, add a test.
>
> Reported-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> ---
>
> Initially, I was reluctant to change the error message, thinking that
> error messages for plumbing commands were not supposed to change.  But I
> think I was wrong in that thought, so I changed the error message so it
> was a more descriptive "unrecognized input".

I am still reluctant to see

    $ git apply </dev/null
    error: unrecognized input

instead of "error: No changes", though.

"git apply --check" is about asking "do you see anything offending in the
diff?" and it is not "git apply --dry-run" that asks "do you promise if I
feed this for real to you you will apply it without complaint?".

I am slightly in favor of answering "well you do not have a diff to begin
with, which in itself is suspicious" to "do you see anything offending?"
question, but I have to admit that it is an equally valid answer to say
"no, there is nothing offending in the diff.", which is what we do with
the current code.

So, I dunno.

Re: [PATCH 2/2] builtin/apply.c: report error on failure to recognize input

[Brandon Casey]

On Mon, Dec 5, 2011 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> When git apply is passed something that is not a patch, it does not produce
>> an error message or exit with a non-zero status if it was not actually
>> "applying" the patch i.e. --check or --numstat etc were supplied on the
>> command line.
>>
>> Fix this by producing an error when apply fails to find any hunks whatsoever
>> while parsing the patch.
>>
>> This will cause some of the output formats (--numstat, --diffstat, etc) to
>> produce an error when they formerly would have reported zero changes and
>> exited successfully.  That seems like the correct behavior though.  Failure
>> to recognize the input as a patch should be an error.
>>
>> Plus, add a test.
>>
>> Reported-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
>> ---
>>
>> Initially, I was reluctant to change the error message, thinking that
>> error messages for plumbing commands were not supposed to change.  But I
>> think I was wrong in that thought, so I changed the error message so it
>> was a more descriptive "unrecognized input".
>
> I am still reluctant to see
>
>    $ git apply </dev/null
>    error: unrecognized input
>
> instead of "error: No changes", though.

I'm not partial to "unrecognized input", but I thought it was more
descriptive of what happened than "No changes".  This error message is
only printed out when absolutely no hunks were found while parsing the
input.

> "git apply --check" is about asking "do you see anything offending in the
> diff?" and it is not "git apply --dry-run" that asks "do you promise if I
> feed this for real to you you will apply it without complaint?".
>
> I am slightly in favor of answering "well you do not have a diff to begin
> with, which in itself is suspicious" to "do you see anything offending?"
> question, but I have to admit that it is an equally valid answer to say
> "no, there is nothing offending in the diff.", which is what we do with
> the current code.
>
> So, I dunno.

I think the current code is a little inconsistent with respect to
empty or bogus non-diff input.

It seems more consistent that if it is an error to tell git apply to
apply zero hunks, then it is also an error to --check zero hunks, or
--stat etc.  In all cases the cause is the same: failure to find any
hunks in the input because the input was not a diff.

Also, the man page description of --check says that it checks "if the
patch is applicable to the current working tree and/or the index".
The new behavior would answer that with "no, this patch is not
applicable ... since no hunks were found", rather than "yes, because
no hunks were found".  But I'm really arguing on the side of
"unrecognized input should be an error", since the new behavior would
also be an error for --stat, --numstat, etc.

-Brandon