threads / patch / 29077

patch, 2 partst/t4131-apply-fake-ancestor.sh: fix broken test

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

## tl;dr

6 messages between Dec 3, 2011 and Dec 5, 2011. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Brandon Casey· Dec 3, 2011, 20:35 UTC · lore

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(-)
Show changes to t/t4131-apply-fake-ancestor.sh +1 −1
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
Brandon Casey· Dec 3, 2011, 20:35 UTC · re: Brandon Casey · lore

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

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
Show changes to 2 files +24 −5

builtin/apply.c, 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
Artem Bityutskiy· Dec 4, 2011, 13:30 UTC · re: Brandon Casey · lore

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

On Sat, 2011-12-03 at 14:35 -0600, Brandon Casey wrote:
Show 17 quoted lines
> 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
Brandon Casey· Dec 4, 2011, 15:39 UTC · re: Artem Bityutskiy · lore

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

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
Junio C Hamano· Dec 5, 2011, 19:27 UTC · re: Brandon Casey · lore

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

Brandon Casey <drafnel@gmail.com> writes:
Show 23 quoted lines
> 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.
Brandon Casey· Dec 5, 2011, 22:38 UTC · re: Junio C Hamano · lore

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

On Mon, Dec 5, 2011 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
> 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.

Show 11 quoted lines
> "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

← back to recent threads