[PATCH] git-apply: fix rubbish handling in --check case

[PATCH] git-apply: fix rubbish handling in --check case

[Artem Bityutskiy]

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

This patch fixes the following inconsistency:

Let's take my bash binary.

$ ls -l /bin/bash
-rwxr-xr-x. 1 root root 924200 Jun 22 16:49 /bin/bash
$ file /bin/bash
/bin/bash: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.32, stripped

Let's try to apply it

$ git apply /bin/bash
error: No changes
$ echo $?
1

Good, rejected and error code is returned. Let's try with --check:

$ git apply --check /bin/bash
$ echo $?
0

Not exactly what I expected :-) The same happnes if you use an empty file.

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

Note, I did not extensively test it!

 Makefile        |    2 +-
 builtin/apply.c |    8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 84a8a0b..2d6862a 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3596,9 +3596,6 @@ static int write_out_results(struct patch *list, int skipped_patch)
 	int errs = 0;
 	struct patch *l;
 
-	if (!list && !skipped_patch)
-		return error("No changes");
-
 	for (phase = 0; phase < 2; phase++) {
 		l = list;
 		while (l) {
@@ -3741,6 +3738,11 @@ static int apply_patch(int fd, const char *filename, int options)
 	    !apply_with_reject)
 		exit(1);
 
+	if (!list && !skipped_patch) {
+		error("No changes");
+		exit(1);
+	}
+
 	if (apply && write_out_results(list, skipped_patch))
 		exit(1);
 
-- 
1.7.6.4

[PATCH] git-apply: fix rubbish handling in --check case

[Artem Bityutskiy]

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

This patch fixes the following inconsistency:

Let's take my bash binary.

$ ls -l /bin/bash
-rwxr-xr-x. 1 root root 924200 Jun 22 16:49 /bin/bash
$ file /bin/bash
/bin/bash: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.32, stripped

Let's try to apply it

$ git apply /bin/bash
error: No changes
$ echo $?
1

Good, rejected and error code is returned. Let's try with --check:

$ git apply --check /bin/bash
$ echo $?
0

Not exactly what I expected :-) The same happnes if you use an empty file.

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

Note, I did not extensively test it!

 Makefile        |    2 +-
 builtin/apply.c |    8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 84a8a0b..2d6862a 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3596,9 +3596,6 @@ static int write_out_results(struct patch *list, int skipped_patch)
 	int errs = 0;
 	struct patch *l;
 
-	if (!list && !skipped_patch)
-		return error("No changes");
-
 	for (phase = 0; phase < 2; phase++) {
 		l = list;
 		while (l) {
@@ -3741,6 +3738,11 @@ static int apply_patch(int fd, const char *filename, int options)
 	    !apply_with_reject)
 		exit(1);
 
+	if (!list && !skipped_patch) {
+		error("No changes");
+		exit(1);
+	}
+
 	if (apply && write_out_results(list, skipped_patch))
 		exit(1);
 
-- 
1.7.6.4

Re: [PATCH] git-apply: fix rubbish handling in --check case

[Brandon Casey]

On Wed, Nov 23, 2011 at 10:26 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

> $ git apply --check /bin/bash
> $ echo $?
> 0
>
> Not exactly what I expected :-) The same happnes if you use an empty file.
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>
> Note, I did not extensively test it!
>
>  Makefile        |    2 +-
>  builtin/apply.c |    8 +++++---
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 84a8a0b..2d6862a 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3596,9 +3596,6 @@ static int write_out_results(struct patch *list, int skipped_patch)
>        int errs = 0;
>        struct patch *l;
>
> -       if (!list && !skipped_patch)
> -               return error("No changes");
> -

I think write_out_results() can lose the skipped_patch parameter now.

>        for (phase = 0; phase < 2; phase++) {
>                l = list;
>                while (l) {
> @@ -3741,6 +3738,11 @@ static int apply_patch(int fd, const char *filename, int options)
>            !apply_with_reject)
>                exit(1);
>
> +       if (!list && !skipped_patch) {
> +               error("No changes");
> +               exit(1);

You can use die() instead of error followed by exit.

Also, I don't see any reason why this check shouldn't be moved up so
that it is performed immediately after the while loop, avoiding any
unnecessary work.

btw. die'ing like this affects diffstat, numstat, summary etc. too
since now they will report an error instead of just displaying an
informational message describing zero changes when passed a bogus
patch. But that seems correct to me.

I would have also suggested changing the error message to something
more descriptive than "No changes", but apparently apply is a plumbing
command.  It seems kind of an "in-between plumbing and porcelain"
command.

It still seems like the correct behavior change with respect to
diffstat, numstat et al that I mentioned above after rethinking from a
plumbing perspective. git apply should error out if it cannot
recognize its input.

-Brandon