threads / patch / 13093

patchbuiltin-apply: Show a more descriptive error on failure when opening a patch

Subject: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch

## tl;dr

7 messages between Apr 13, 2008 and Apr 16, 2008. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Alberto Bertogli· Apr 13, 2008, 18:56 UTC · lore

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(-)
Show changes to builtin-apply.c +5 −2
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· Apr 13, 2008, 19:54 UTC · re: Alberto Bertogli · lore

Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch

Alberto Bertogli <albertito@gmail.com> writes:
Show 25 quoted lines
> 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...

Show 6 quoted lines
> +
>  		read_stdin = 0;
>  		set_default_whitespace_mode(whitespace_option);
>  		errs |= apply_patch(fd, arg, inaccurate_eof);
> -- 
> 1.5.5.104.ge4331
Alberto Bertogli· Apr 14, 2008, 14:23 UTC · re: Junio C Hamano · lore

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(-)
Show changes to builtin-apply.c +6 −2
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· Apr 14, 2008, 14:33 UTC · re: Alberto Bertogli · lore

Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch

Hi,
On Mon, 14 Apr 2008, Alberto Bertogli wrote:
Show 5 quoted lines
> +		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· Apr 14, 2008, 15:30 UTC · re: Johannes Schindelin · lore

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:
Show 11 quoted lines
> 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(-)
Show changes to builtin-apply.c +3 −1
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· Apr 16, 2008, 05:20 UTC · re: Alberto Bertogli · lore

Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch

Alberto Bertogli <albertito@gmail.com> writes:
Show 28 quoted lines
> 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.
Johannes Schindelin· Apr 14, 2008, 14:03 UTC · re: Alberto Bertogli · lore

Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch

Hi,
On Sun, 13 Apr 2008, Alberto Bertogli wrote:
Show 14 quoted lines
> 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

← back to recent threads