{"thread":{"id":"13093","subject":"[PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch","startedAt":"2008-04-13T18:56:31Z","lastAt":"2008-04-16T05:20:28Z","messageCount":7,"participants":["Alberto Bertogli","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"74263","messageId":"1208112991-21598-1-git-send-email-albertito@gmail.com","threadId":"13093","inReplyTo":null,"subject":"[PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch","fromName":"Alberto Bertogli","fromEmail":"albertito@gmail.com","sentAt":"2008-04-13T18:56:31Z","receivedAt":"2008-04-13T18:56:31Z","isPatch":true,"sender":{"key":"albertito@gmail.com","avatar":null},"body":"When a patch can't be opened (it doesn't exist, there are permission\nproblems, etc.) we get the usage text, which is not a proper indication of\nfailure.\n\nThis patch fixes that by simply doing a perror() instead.\n\nSigned-off-by: Alberto Bertogli <albertito@gmail.com>\n---\n builtin-apply.c |    7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex abe73a0..d80b231 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -3120,8 +3120,11 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)\n \t\t\targ = prefix_filename(prefix, prefix_length, arg);\n \n \t\tfd = open(arg, O_RDONLY);\n-\t\tif (fd < 0)\n-\t\t\tusage(apply_usage);\n+\t\tif (fd < 0) {\n+\t\t\tperror(\"Error opening patch\");\n+\t\t\treturn 1;\n+\t\t}\n+\n \t\tread_stdin = 0;\n \t\tset_default_whitespace_mode(whitespace_option);\n \t\terrs |= apply_patch(fd, arg, inaccurate_eof);\n-- \n1.5.5.104.ge4331\n"},{"id":"74267","messageId":"7vtzi5bkdz.fsf@gitster.siamese.dyndns.org","threadId":"13093","inReplyTo":"1208112991-21598-1-git-send-email-albertito@gmail.com","subject":"Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-13T19:54:48Z","receivedAt":"2008-04-13T19:54:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alberto Bertogli <albertito@gmail.com> writes:\n\n> When a patch can't be opened (it doesn't exist, there are permission\n> problems, etc.) we get the usage text, which is not a proper indication of\n> failure.\n>\n> This patch fixes that by simply doing a perror() instead.\n>\n> Signed-off-by: Alberto Bertogli <albertito@gmail.com>\n> ---\n>  builtin-apply.c |    7 +++++--\n>  1 files changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index abe73a0..d80b231 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -3120,8 +3120,11 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)\n>  \t\t\targ = prefix_filename(prefix, prefix_length, arg);\n>  \n>  \t\tfd = open(arg, O_RDONLY);\n> -\t\tif (fd < 0)\n> -\t\t\tusage(apply_usage);\n> +\t\tif (fd < 0) {\n> +\t\t\tperror(\"Error opening patch\");\n> +\t\t\treturn 1;\n> +\t\t}\n\nThis makes sense, but I wonder if we want to parrot \"arg\" back.  The\nproblem could be that the command and the user are disagreeing which\nparameter on the command line is the name of the patch file...\n\n> +\n>  \t\tread_stdin = 0;\n>  \t\tset_default_whitespace_mode(whitespace_option);\n>  \t\terrs |= apply_patch(fd, arg, inaccurate_eof);\n> -- \n> 1.5.5.104.ge4331\n"},{"id":"74351","messageId":"alpine.DEB.1.00.0804141502590.28504@racer","threadId":"13093","inReplyTo":"1208112991-21598-1-git-send-email-albertito@gmail.com","subject":"Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-04-14T14:03:45Z","receivedAt":"2008-04-14T14:03:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 13 Apr 2008, Alberto Bertogli wrote:\n\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index abe73a0..d80b231 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -3120,8 +3120,11 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)\n>  \t\t\targ = prefix_filename(prefix, prefix_length, arg);\n>  \n>  \t\tfd = open(arg, O_RDONLY);\n> -\t\tif (fd < 0)\n> -\t\t\tusage(apply_usage);\n> +\t\tif (fd < 0) {\n> +\t\t\tperror(\"Error opening patch\");\n> +\t\t\treturn 1;\n> +\t\t}\n\nWould \"return error(\"...: '%s'\", arg);\" not be much more appropriate and \nconsistent with the resto of the source code?\n\nCiao,\nDscho\n"},{"id":"74353","messageId":"1208183018-27887-1-git-send-email-albertito@gmail.com","threadId":"13093","inReplyTo":"7vtzi5bkdz.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch","fromName":"Alberto Bertogli","fromEmail":"albertito@gmail.com","sentAt":"2008-04-14T14:23:38Z","receivedAt":"2008-04-14T14:23:38Z","isPatch":true,"sender":{"key":"albertito@gmail.com","avatar":null},"body":"When a patch can't be opened (it doesn't exist, there are permission\nproblems, etc.) we get the usage text, which is not a proper indication of\nfailure.\n\nThis patch fixes that by calling error() instead.\n\nSigned-off-by: Alberto Bertogli <albertito@gmail.com>\n---\n\nThis one (hopefuly) addresses both Junio and Johannes' comments.\n\nThanks,\n\t\tAlberto\n\n\n builtin-apply.c |    8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex abe73a0..ebbe609 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -3120,8 +3120,12 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)\n \t\t\targ = prefix_filename(prefix, prefix_length, arg);\n \n \t\tfd = open(arg, O_RDONLY);\n-\t\tif (fd < 0)\n-\t\t\tusage(apply_usage);\n+\t\tif (fd < 0) {\n+\t\t\terror(\"can't open patch '%s': %s\", arg,\n+\t\t\t\t\tstrerror(errno));\n+\t\t\treturn 1;\n+\t\t}\n+\n \t\tread_stdin = 0;\n \t\tset_default_whitespace_mode(whitespace_option);\n \t\terrs |= apply_patch(fd, arg, inaccurate_eof);\n-- \n1.5.5.105.g9d11e.dirty\n"},{"id":"74354","messageId":"alpine.DEB.1.00.0804141532390.28504@racer","threadId":"13093","inReplyTo":"1208183018-27887-1-git-send-email-albertito@gmail.com","subject":"Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-04-14T14:33:54Z","receivedAt":"2008-04-14T14:33:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 14 Apr 2008, Alberto Bertogli wrote:\n\n> +\t\tif (fd < 0) {\n> +\t\t\terror(\"can't open patch '%s': %s\", arg,\n> +\t\t\t\t\tstrerror(errno));\n> +\t\t\treturn 1;\n> +\t\t}\n\nDo you absolutely want to retain the curly braces, and have two \nstatements?   I would prefer \"return error(...)\", and if you absolutely \ninsist on a return 1: \"return !!error(...)\".\n\nCiao,\nDscho\n"},{"id":"74357","messageId":"1208187027-29198-1-git-send-email-albertito@gmail.com","threadId":"13093","inReplyTo":"alpine.DEB.1.00.0804141532390.28504@racer","subject":"[PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch","fromName":"Alberto Bertogli","fromEmail":"albertito@gmail.com","sentAt":"2008-04-14T15:30:27Z","receivedAt":"2008-04-14T15:30:27Z","isPatch":true,"sender":{"key":"albertito@gmail.com","avatar":null},"body":"When a patch can't be opened (it doesn't exist, there are permission\nproblems, etc.) we get the usage text, which is not a proper indication of\nfailure.\n\nThis patch fixes that by calling error() instead.\n\nSigned-off-by: Alberto Bertogli <albertito@gmail.com>\n---\n\nOn Mon, Apr 14, 2008 at 03:33:54PM +0100, Johannes Schindelin wrote:\n> On Mon, 14 Apr 2008, Alberto Bertogli wrote:\n> \n> > +           if (fd < 0) {\n> > +                   error(\"can't open patch '%s': %s\", arg,\n> > +                                   strerror(errno));\n> > +                   return 1;\n> > +           }\n> \n> Do you absolutely want to retain the curly braces, and have two \n> statements?   I would prefer \"return error(...)\", and if you absolutely \n> insist on a return 1: \"return !!error(...)\".\n\nNo, I'm not insisting on any version, I just thought returning 1 would be\nbetter since it will become the script exit status; Now that I think a bit\nmore about it, maybe I should just use die() instead.\n\nAnyway, here's the version returning directly from error(); if you prefer it\nsome other way just let me know.\n\nThanks,\n\t\tAlberto\n\n\n builtin-apply.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex abe73a0..56032ce 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -3121,7 +3121,9 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)\n \n \t\tfd = open(arg, O_RDONLY);\n \t\tif (fd < 0)\n-\t\t\tusage(apply_usage);\n+\t\t\treturn error(\"can't open patch '%s': %s\", arg,\n+\t\t\t\t\tstrerror(errno));\n+\n \t\tread_stdin = 0;\n \t\tset_default_whitespace_mode(whitespace_option);\n \t\terrs |= apply_patch(fd, arg, inaccurate_eof);\n-- \n1.5.5.105.g7849.dirty\n"},{"id":"74521","messageId":"7vy77etlyb.fsf@gitster.siamese.dyndns.org","threadId":"13093","inReplyTo":"1208187027-29198-1-git-send-email-albertito@gmail.com","subject":"Re: [PATCH] builtin-apply: Show a more descriptive error on failure when opening a patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-16T05:20:28Z","receivedAt":"2008-04-16T05:20:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alberto Bertogli <albertito@gmail.com> writes:\n\n> When a patch can't be opened (it doesn't exist, there are permission\n> problems, etc.) we get the usage text, which is not a proper indication of\n> failure.\n>\n> This patch fixes that by calling error() instead.\n>\n> Signed-off-by: Alberto Bertogli <albertito@gmail.com>\n> ---\n>\n> On Mon, Apr 14, 2008 at 03:33:54PM +0100, Johannes Schindelin wrote:\n>> On Mon, 14 Apr 2008, Alberto Bertogli wrote:\n>> \n>> > +           if (fd < 0) {\n>> > +                   error(\"can't open patch '%s': %s\", arg,\n>> > +                                   strerror(errno));\n>> > +                   return 1;\n>> > +           }\n>> \n>> Do you absolutely want to retain the curly braces, and have two \n>> statements?   I would prefer \"return error(...)\", and if you absolutely \n>> insist on a return 1: \"return !!error(...)\".\n>\n> No, I'm not insisting on any version, I just thought returning 1 would be\n> better since it will become the script exit status; Now that I think a bit\n> more about it, maybe I should just use die() instead.\n>\n> Anyway, here's the version returning directly from error(); if you prefer it\n> some other way just let me know.\n\nI would apply this while changing \"return error()\" to \"die()\", as the\noriginal usage() call would have exited here and we do not have a good\nreason to change it.\n\nThanks.\n"}]}