# [PATCH] git-apply: fix rubbish handling in --check case 3 messages from 2011-11-23 to 2011-11-23. Participants: Artem Bityutskiy, Brandon Casey. Thread: https://gitlist.dev/t/29011 ## Artem Bityutskiy, 2011-11-23 16:26 Subject: [PATCH] git-apply: fix rubbish handling in --check case Message-ID: <1322065563-3651-1-git-send-email-dedekind1@gmail.com> URL: https://gitlist.dev/e/1322065563-3651-1-git-send-email-dedekind1%40gmail.com ``` From: Artem Bityutskiy 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 --- 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 ``` ## Artem Bityutskiy, 2011-11-23 16:26 Subject: [PATCH] git-apply: fix rubbish handling in --check case Message-ID: <1322065563-3651-2-git-send-email-dedekind1@gmail.com> URL: https://gitlist.dev/e/1322065563-3651-2-git-send-email-dedekind1%40gmail.com In-Reply-To: <1322065563-3651-1-git-send-email-dedekind1@gmail.com> ``` From: Artem Bityutskiy 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 --- 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 ``` ## Brandon Casey, 2011-11-23 18:44 Subject: Re: [PATCH] git-apply: fix rubbish handling in --check case Message-ID: URL: https://gitlist.dev/e/CA%2BsFfMf2%2BXJGtQACeh%3DJ4BA1rGP%3DKsFDnzw0UECjmTPp2-ZUzQ%40mail.gmail.com In-Reply-To: <1322065563-3651-1-git-send-email-dedekind1@gmail.com> ``` On Wed, Nov 23, 2011 at 10:26 AM, Artem Bityutskiy wrote: > From: Artem Bityutskiy > $ 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 > --- > > 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 ```