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

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

[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

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

[Junio C Hamano]

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

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

[Johannes Schindelin]

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

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

[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

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

[Johannes Schindelin]

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

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

[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

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

[Junio C Hamano]

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.