threads / patch / 29011

patchgit-apply: fix rubbish handling in --check case

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

## tl;dr

3 messages between Nov 23, 2011 and Nov 23, 2011. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Artem Bityutskiy· Nov 23, 2011, 16:26 UTC · lore
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(-)
Show changes to builtin/apply.c +5 −3
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
Artem Bityutskiy· Nov 23, 2011, 16:26 UTC · re: Artem Bityutskiy · lore
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(-)
Show changes to builtin/apply.c +5 −3
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
Brandon Casey· Nov 23, 2011, 18:44 UTC · re: Artem Bityutskiy · lore

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

On Wed, Nov 23, 2011 at 10:26 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Show 26 quoted lines
> $ 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.
Show 10 quoted lines
>        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

← back to recent threads