* [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch @ 2008-04-13 18:56 Alberto Bertogli 2008-04-13 19:54 ` Junio C Hamano 2008-04-14 14:03 ` Johannes Schindelin 0 siblings, 2 replies; 7+ messages in thread From: Alberto Bertogli @ 2008-04-13 18:56 UTC (permalink / raw) To: git; +Cc: Alberto Bertogli 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 <albertito@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch 2008-04-13 18:56 [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Alberto Bertogli @ 2008-04-13 19:54 ` Junio C Hamano 2008-04-14 14:23 ` Alberto Bertogli 2008-04-14 14:03 ` Johannes Schindelin 1 sibling, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2008-04-13 19:54 UTC (permalink / raw) To: Alberto Bertogli; +Cc: git Alberto Bertogli <albertito@gmail.com> 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 <albertito@gmail.com> > --- > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch 2008-04-13 19:54 ` Junio C Hamano @ 2008-04-14 14:23 ` Alberto Bertogli 2008-04-14 14:33 ` Johannes Schindelin 0 siblings, 1 reply; 7+ messages in thread From: Alberto Bertogli @ 2008-04-14 14:23 UTC (permalink / raw) To: git, Johannes.Schindelin, gitster; +Cc: Alberto Bertogli 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 <albertito@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch 2008-04-14 14:23 ` Alberto Bertogli @ 2008-04-14 14:33 ` Johannes Schindelin 2008-04-14 15:30 ` Alberto Bertogli 0 siblings, 1 reply; 7+ messages in thread From: Johannes Schindelin @ 2008-04-14 14:33 UTC (permalink / raw) To: Alberto Bertogli; +Cc: git, gitster 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch 2008-04-14 14:33 ` Johannes Schindelin @ 2008-04-14 15:30 ` Alberto Bertogli 2008-04-16 5:20 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Alberto Bertogli @ 2008-04-14 15:30 UTC (permalink / raw) To: Johannes.Schindelin, git, gitster; +Cc: Alberto Bertogli 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 <albertito@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch 2008-04-14 15:30 ` Alberto Bertogli @ 2008-04-16 5:20 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2008-04-16 5:20 UTC (permalink / raw) To: Alberto Bertogli; +Cc: Johannes.Schindelin, git, gitster Alberto Bertogli <albertito@gmail.com> 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 <albertito@gmail.com> > --- > > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch 2008-04-13 18:56 [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Alberto Bertogli 2008-04-13 19:54 ` Junio C Hamano @ 2008-04-14 14:03 ` Johannes Schindelin 1 sibling, 0 replies; 7+ messages in thread From: Johannes Schindelin @ 2008-04-14 14:03 UTC (permalink / raw) To: Alberto Bertogli; +Cc: git 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-04-16 5:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-04-13 18:56 [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch Alberto Bertogli 2008-04-13 19:54 ` Junio C Hamano 2008-04-14 14:23 ` Alberto Bertogli 2008-04-14 14:33 ` Johannes Schindelin 2008-04-14 15:30 ` Alberto Bertogli 2008-04-16 5:20 ` Junio C Hamano 2008-04-14 14:03 ` Johannes Schindelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).