# [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch 7 messages from 2008-04-13 to 2008-04-16. Participants: Alberto Bertogli, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/13093 ## Alberto Bertogli, 2008-04-13 18:56 Subject: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Message-ID: <1208112991-21598-1-git-send-email-albertito@gmail.com> URL: https://gitlist.dev/e/1208112991-21598-1-git-send-email-albertito%40gmail.com ``` When a patch can't be opened (it doesn't exist, there are permission problems, etc.) we get the usage text, which is not a proper indication of failure. This patch fixes that by simply doing a perror() instead. Signed-off-by: Alberto Bertogli --- builtin-apply.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index abe73a0..d80b231 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -3120,8 +3120,11 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix) arg = prefix_filename(prefix, prefix_length, arg); fd = open(arg, O_RDONLY); - if (fd < 0) - usage(apply_usage); + if (fd < 0) { + perror("Error opening patch"); + return 1; + } + read_stdin = 0; set_default_whitespace_mode(whitespace_option); errs |= apply_patch(fd, arg, inaccurate_eof); -- 1.5.5.104.ge4331 ``` ## Junio C Hamano, 2008-04-13 19:54 Subject: Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Message-ID: <7vtzi5bkdz.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtzi5bkdz.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1208112991-21598-1-git-send-email-albertito@gmail.com> ``` Alberto Bertogli writes: > When a patch can't be opened (it doesn't exist, there are permission > problems, etc.) we get the usage text, which is not a proper indication of > failure. > > This patch fixes that by simply doing a perror() instead. > > Signed-off-by: Alberto Bertogli > --- > builtin-apply.c | 7 +++++-- > 1 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/builtin-apply.c b/builtin-apply.c > index abe73a0..d80b231 100644 > --- a/builtin-apply.c > +++ b/builtin-apply.c > @@ -3120,8 +3120,11 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix) > arg = prefix_filename(prefix, prefix_length, arg); > > fd = open(arg, O_RDONLY); > - if (fd < 0) > - usage(apply_usage); > + if (fd < 0) { > + perror("Error opening patch"); > + return 1; > + } This makes sense, but I wonder if we want to parrot "arg" back. The problem could be that the command and the user are disagreeing which parameter on the command line is the name of the patch file... > + > read_stdin = 0; > set_default_whitespace_mode(whitespace_option); > errs |= apply_patch(fd, arg, inaccurate_eof); > -- > 1.5.5.104.ge4331 ``` ## Johannes Schindelin, 2008-04-14 14:03 Subject: Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0804141502590.28504%40racer In-Reply-To: <1208112991-21598-1-git-send-email-albertito@gmail.com> ``` Hi, On Sun, 13 Apr 2008, Alberto Bertogli wrote: > diff --git a/builtin-apply.c b/builtin-apply.c > index abe73a0..d80b231 100644 > --- a/builtin-apply.c > +++ b/builtin-apply.c > @@ -3120,8 +3120,11 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix) > arg = prefix_filename(prefix, prefix_length, arg); > > fd = open(arg, O_RDONLY); > - if (fd < 0) > - usage(apply_usage); > + if (fd < 0) { > + perror("Error opening patch"); > + return 1; > + } Would "return error("...: '%s'", arg);" not be much more appropriate and consistent with the resto of the source code? Ciao, Dscho ``` ## Alberto Bertogli, 2008-04-14 14:23 Subject: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Message-ID: <1208183018-27887-1-git-send-email-albertito@gmail.com> URL: https://gitlist.dev/e/1208183018-27887-1-git-send-email-albertito%40gmail.com In-Reply-To: <7vtzi5bkdz.fsf@gitster.siamese.dyndns.org> ``` When a patch can't be opened (it doesn't exist, there are permission problems, etc.) we get the usage text, which is not a proper indication of failure. This patch fixes that by calling error() instead. Signed-off-by: Alberto Bertogli --- This one (hopefuly) addresses both Junio and Johannes' comments. Thanks, Alberto builtin-apply.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index abe73a0..ebbe609 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -3120,8 +3120,12 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix) arg = prefix_filename(prefix, prefix_length, arg); fd = open(arg, O_RDONLY); - if (fd < 0) - usage(apply_usage); + if (fd < 0) { + error("can't open patch '%s': %s", arg, + strerror(errno)); + return 1; + } + read_stdin = 0; set_default_whitespace_mode(whitespace_option); errs |= apply_patch(fd, arg, inaccurate_eof); -- 1.5.5.105.g9d11e.dirty ``` ## Johannes Schindelin, 2008-04-14 14:33 Subject: Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0804141532390.28504%40racer In-Reply-To: <1208183018-27887-1-git-send-email-albertito@gmail.com> ``` Hi, On Mon, 14 Apr 2008, Alberto Bertogli wrote: > + if (fd < 0) { > + error("can't open patch '%s': %s", arg, > + strerror(errno)); > + return 1; > + } Do you absolutely want to retain the curly braces, and have two statements? I would prefer "return error(...)", and if you absolutely insist on a return 1: "return !!error(...)". Ciao, Dscho ``` ## Alberto Bertogli, 2008-04-14 15:30 Subject: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Message-ID: <1208187027-29198-1-git-send-email-albertito@gmail.com> URL: https://gitlist.dev/e/1208187027-29198-1-git-send-email-albertito%40gmail.com In-Reply-To: ``` When a patch can't be opened (it doesn't exist, there are permission problems, etc.) we get the usage text, which is not a proper indication of failure. This patch fixes that by calling error() instead. Signed-off-by: Alberto Bertogli --- On Mon, Apr 14, 2008 at 03:33:54PM +0100, Johannes Schindelin wrote: > On Mon, 14 Apr 2008, Alberto Bertogli wrote: > > > + if (fd < 0) { > > + error("can't open patch '%s': %s", arg, > > + strerror(errno)); > > + return 1; > > + } > > Do you absolutely want to retain the curly braces, and have two > statements? I would prefer "return error(...)", and if you absolutely > insist on a return 1: "return !!error(...)". No, I'm not insisting on any version, I just thought returning 1 would be better since it will become the script exit status; Now that I think a bit more about it, maybe I should just use die() instead. Anyway, here's the version returning directly from error(); if you prefer it some other way just let me know. Thanks, Alberto builtin-apply.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index abe73a0..56032ce 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -3121,7 +3121,9 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix) fd = open(arg, O_RDONLY); if (fd < 0) - usage(apply_usage); + return error("can't open patch '%s': %s", arg, + strerror(errno)); + read_stdin = 0; set_default_whitespace_mode(whitespace_option); errs |= apply_patch(fd, arg, inaccurate_eof); -- 1.5.5.105.g7849.dirty ``` ## Junio C Hamano, 2008-04-16 05:20 Subject: Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Message-ID: <7vy77etlyb.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy77etlyb.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1208187027-29198-1-git-send-email-albertito@gmail.com> ``` Alberto Bertogli writes: > When a patch can't be opened (it doesn't exist, there are permission > problems, etc.) we get the usage text, which is not a proper indication of > failure. > > This patch fixes that by calling error() instead. > > Signed-off-by: Alberto Bertogli > --- > > On Mon, Apr 14, 2008 at 03:33:54PM +0100, Johannes Schindelin wrote: >> On Mon, 14 Apr 2008, Alberto Bertogli wrote: >> >> > + if (fd < 0) { >> > + error("can't open patch '%s': %s", arg, >> > + strerror(errno)); >> > + return 1; >> > + } >> >> Do you absolutely want to retain the curly braces, and have two >> statements? I would prefer "return error(...)", and if you absolutely >> insist on a return 1: "return !!error(...)". > > No, I'm not insisting on any version, I just thought returning 1 would be > better since it will become the script exit status; Now that I think a bit > more about it, maybe I should just use die() instead. > > Anyway, here's the version returning directly from error(); if you prefer it > some other way just let me know. I would apply this while changing "return error()" to "die()", as the original usage() call would have exited here and we do not have a good reason to change it. Thanks. ```