{"thread":{"id":"30154","subject":"[PATCH] compat/mingw.[ch]: Change return type of exec functions to int","startedAt":"2012-04-05T17:48:46Z","lastAt":"2012-04-06T01:00:03Z","messageCount":8,"participants":["Ramsay Jones","Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188575","messageId":"4F7DDAFE.1060902@ramsay1.demon.co.uk","threadId":"30154","inReplyTo":null,"subject":"[PATCH] compat/mingw.[ch]: Change return type of exec functions to int","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-04-05T17:48:46Z","receivedAt":"2012-04-05T17:48:46Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe POSIX standard specifies a return type of int for all six exec\nfunctions. In addition, all exec functions return -1 on error, and\nsimply do not return on success. However, the current emulation of\nthe exec functions on mingw are declared with a void return type.\n\nThis would cause a problem should any code attempt to call the\nexec function in a non-void context. In particular, if an exec\nfunction were used in a conditional it would fail to compile.\n\nIn order to improve the fidelity of the emulation, we change the\nreturn type of the mingw_execv[p] functions to int and return -1\non error.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nHi Jeff,\n\nSince commit b0984f82 (\"run-command: treat inaccessible directories\nas ENOENT\", 30-03-2012), the mingw build fails as follows:\n\n        CC run-command.o\n    run-command.c: In function 'sane_execvp':\n    run-command.c:124: error: invalid use of void expression\n    make: *** [run-command.o] Error 1\n\nMy first reaction was to simply remove the conditional since, if execvp()\nreturns at all, the result will always be -1 and so the condition will\nalways be false. ie. the conditional is pointless.\n\nHowever, I found the incorrect return type of the mingw_execv[p]() to be\na gratuitous incompatibility, so ... :-P\n\nCould you (or Junio?) add this patch prior to your patch on this branch ?\nThanks in advance!\n\nI will let you decide if you want to remove the conditional as well ...\n\nATB,\nRamsay Jones\n\n compat/mingw.c |    6 ++++--\n compat/mingw.h |    4 ++--\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex a0ac487..afc892d 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1003,7 +1003,7 @@ static void mingw_execve(const char *cmd, char *const *argv, char *const *env)\n \t}\n }\n \n-void mingw_execvp(const char *cmd, char *const *argv)\n+int mingw_execvp(const char *cmd, char *const *argv)\n {\n \tchar **path = get_path_split();\n \tchar *prog = path_lookup(cmd, path, 0);\n@@ -1015,11 +1015,13 @@ void mingw_execvp(const char *cmd, char *const *argv)\n \t\terrno = ENOENT;\n \n \tfree_path_split(path);\n+\treturn -1;\n }\n \n-void mingw_execv(const char *cmd, char *const *argv)\n+int mingw_execv(const char *cmd, char *const *argv)\n {\n \tmingw_execve(cmd, argv, environ);\n+\treturn -1;\n }\n \n int mingw_kill(pid_t pid, int sig)\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 0ff1e04..ef5b150 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -274,9 +274,9 @@ int mingw_utime(const char *file_name, const struct utimbuf *times);\n pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,\n \t\t     const char *dir,\n \t\t     int fhin, int fhout, int fherr);\n-void mingw_execvp(const char *cmd, char *const *argv);\n+int mingw_execvp(const char *cmd, char *const *argv);\n #define execvp mingw_execvp\n-void mingw_execv(const char *cmd, char *const *argv);\n+int mingw_execv(const char *cmd, char *const *argv);\n #define execv mingw_execv\n \n static inline unsigned int git_ntohl(unsigned int x)\n-- \n1.7.9\n"},{"id":"188576","messageId":"20120405181600.GA6340@burratino","threadId":"30154","inReplyTo":"4F7DDAFE.1060902@ramsay1.demon.co.uk","subject":"Re: [PATCH] compat/mingw.[ch]: Change return type of exec functions to int","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-05T18:16:01Z","receivedAt":"2012-04-05T18:16:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramsay Jones wrote:\n\n>         CC run-command.o\n>     run-command.c: In function 'sane_execvp':\n>     run-command.c:124: error: invalid use of void expression\n>     make: *** [run-command.o] Error 1\n>\n> My first reaction was to simply remove the conditional since, if execvp()\n> returns at all, the result will always be -1 and so the condition will\n> always be false. ie. the conditional is pointless.\n>\n> However, I found the incorrect return type of the mingw_execv[p]() to be\n> a gratuitous incompatibility, so ... :-P\n\nMy bad.  I agree that in addition to making the return type fix,\nsquashing the following into jk/run-command-eacces would be a good\nidea.\n\ndiff --git i/run-command.c w/run-command.c\nindex 04f0190d..fcd7e192 100644\n--- i/run-command.c\n+++ w/run-command.c\n@@ -59,8 +59,7 @@ static int exists_in_PATH(const char *file)\n \n int sane_execvp(const char *file, char * const argv[])\n {\n-\tif (!execvp(file, argv))\n-\t\treturn 0;\n+\texecvp(file, argv);\n \n \t/*\n \t * When a command can't be found because one of the directories\n"},{"id":"188600","messageId":"20120405220615.GA11751@sigill.intra.peff.net","threadId":"30154","inReplyTo":"20120405181600.GA6340@burratino","subject":"Re: [PATCH] compat/mingw.[ch]: Change return type of exec functions to int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-05T22:06:15Z","receivedAt":"2012-04-05T22:06:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 05, 2012 at 01:16:01PM -0500, Jonathan Nieder wrote:\n\n> Ramsay Jones wrote:\n> \n> >         CC run-command.o\n> >     run-command.c: In function 'sane_execvp':\n> >     run-command.c:124: error: invalid use of void expression\n> >     make: *** [run-command.o] Error 1\n> >\n> > My first reaction was to simply remove the conditional since, if execvp()\n> > returns at all, the result will always be -1 and so the condition will\n> > always be false. ie. the conditional is pointless.\n> >\n> > However, I found the incorrect return type of the mingw_execv[p]() to be\n> > a gratuitous incompatibility, so ... :-P\n\nYeah, it is just asking for trouble to #define a new execvp that does\nnot match the declaration of the existing one, so I think Ramsay's patch\nis good.\n\n> My bad.  I agree that in addition to making the return type fix,\n> squashing the following into jk/run-command-eacces would be a good\n> idea.\n> \n> diff --git i/run-command.c w/run-command.c\n> index 04f0190d..fcd7e192 100644\n> --- i/run-command.c\n> +++ w/run-command.c\n> @@ -59,8 +59,7 @@ static int exists_in_PATH(const char *file)\n>  \n>  int sane_execvp(const char *file, char * const argv[])\n>  {\n> -\tif (!execvp(file, argv))\n> -\t\treturn 0;\n> +\texecvp(file, argv);\n\nI don't have a strong opinion. The \"return 0\" is a little misleading,\nsince it will never be called, but I think we should at least have a\ncomment like:\n\ndiff --git a/run-command.c b/run-command.c\nindex 7123436..5b6a368 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -117,8 +117,11 @@ static int exists_in_PATH(const char *file)\n \n int sane_execvp(const char *file, char * const argv[])\n {\n-\tif (!execvp(file, argv))\n-\t\treturn 0;\n+\t/*\n+\t * No need to check the return value; if it returns at all, an error\n+\t * occurred.\n+\t */\n+\texecvp(file, argv);\n \n \t/*\n \t * When a command can't be found because one of the directories\n"},{"id":"188601","messageId":"20120405223408.GA13910@burratino","threadId":"30154","inReplyTo":"20120405220615.GA11751@sigill.intra.peff.net","subject":"Re: [PATCH] compat/mingw.[ch]: Change return type of exec functions to int","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-05T22:34:36Z","receivedAt":"2012-04-05T22:34:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I don't have a strong opinion. The \"return 0\" is a little misleading,\n> since it will never be called, but I think we should at least have a\n> comment like:\n[...]\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -117,8 +117,11 @@ static int exists_in_PATH(const char *file)\n>  \n>  int sane_execvp(const char *file, char * const argv[])\n>  {\n> -\tif (!execvp(file, argv))\n> -\t\treturn 0;\n> +\t/*\n> +\t * No need to check the return value; if it returns at all, an error\n> +\t * occurred.\n> +\t */\n> +\texecvp(file, argv);\n>  \n\nI'd rather have the \"return 0\" then.  Such a comment that focuses on C\nlibrary details rather than providing additional information for\nunderstanding the sane_execvp function is distracting.\n"},{"id":"188609","messageId":"20120406002454.GB14224@sigill.intra.peff.net","threadId":"30154","inReplyTo":"20120405223408.GA13910@burratino","subject":"Re: [PATCH] compat/mingw.[ch]: Change return type of exec functions to int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-06T00:24:54Z","receivedAt":"2012-04-06T00:24:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 05, 2012 at 05:34:36PM -0500, Jonathan Nieder wrote:\n\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -117,8 +117,11 @@ static int exists_in_PATH(const char *file)\n> >  \n> >  int sane_execvp(const char *file, char * const argv[])\n> >  {\n> > -\tif (!execvp(file, argv))\n> > -\t\treturn 0;\n> > +\t/*\n> > +\t * No need to check the return value; if it returns at all, an error\n> > +\t * occurred.\n> > +\t */\n> > +\texecvp(file, argv);\n> >  \n> \n> I'd rather have the \"return 0\" then.  Such a comment that focuses on C\n> library details rather than providing additional information for\n> understanding the sane_execvp function is distracting.\n\nI think both (attempt to) document the same thing: that if we get past\nexecvp, we know are in error-checking mode. Which is not explicitly said\nanywhere. So maybe:\n\ndiff --git a/run-command.c b/run-command.c\nindex 7123436..e6ece79 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -117,10 +117,12 @@ static int exists_in_PATH(const char *file)\n \n int sane_execvp(const char *file, char * const argv[])\n {\n-\tif (!execvp(file, argv))\n-\t\treturn 0;\n+\texecvp(file, argv);\n \n \t/*\n+\t * If we are still running, we know an error occurred; let's try to\n+\t * diagnose it more specifically.\n+\t *\n \t * When a command can't be found because one of the directories\n \t * listed in $PATH is unsearchable, execvp reports EACCES, but\n \t * careful usability testing (read: analysis of occasional bug\n"},{"id":"188611","messageId":"20120406004226.GA2658@burratino","threadId":"30154","inReplyTo":"20120406002454.GB14224@sigill.intra.peff.net","subject":"Re: [PATCH] compat/mingw.[ch]: Change return type of exec functions to int","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-06T00:42:26Z","receivedAt":"2012-04-06T00:42:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I think both (attempt to) document the same thing: that if we get past\n> execvp, we know are in error-checking mode. Which is not explicitly said\n> anywhere. So maybe:\n>\n> diff --git a/run-command.c b/run-command.c\n> index 7123436..e6ece79 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -117,10 +117,12 @@ static int exists_in_PATH(const char *file)\n>  \n>  int sane_execvp(const char *file, char * const argv[])\n>  {\n> -\tif (!execvp(file, argv))\n> -\t\treturn 0;\n> +\texecvp(file, argv);\n>  \n>  \t/*\n> +\t * If we are still running, we know an error occurred; let's try to\n> +\t * diagnose it more specifically.\n> +\t *\n>  \t * When a command can't be found because one of the directories\n\nLooks fine to me. :)\n\nThanks for your patience.\nJonathan\n"},{"id":"188612","messageId":"20120406004449.GA15125@sigill.intra.peff.net","threadId":"30154","inReplyTo":"20120406004226.GA2658@burratino","subject":"Re: [PATCH] compat/mingw.[ch]: Change return type of exec functions to int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-06T00:44:49Z","receivedAt":"2012-04-06T00:44:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 05, 2012 at 07:42:26PM -0500, Jonathan Nieder wrote:\n\n> >  \t/*\n> > +\t * If we are still running, we know an error occurred; let's try to\n> > +\t * diagnose it more specifically.\n> > +\t *\n> >  \t * When a command can't be found because one of the directories\n> \n> Looks fine to me. :)\n\nThanks. Junio, do you mind squashing this onto the tip of\njk/run-command-eaccess? Patch repeated below for your convenience.\n\nThis should fix Ramsay's problem, but I think his patch is worth\napplying, anyway.\n\n---\ndiff --git a/run-command.c b/run-command.c\nindex 7123436..e6ece79 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -117,10 +117,12 @@ static int exists_in_PATH(const char *file)\n \n int sane_execvp(const char *file, char * const argv[])\n {\n-\tif (!execvp(file, argv))\n-\t\treturn 0;\n+\texecvp(file, argv);\n \n \t/*\n+\t * If we are still running, we know an error occurred; let's try to\n+\t * diagnose it more specifically.\n+\t *\n \t * When a command can't be found because one of the directories\n \t * listed in $PATH is unsearchable, execvp reports EACCES, but\n \t * careful usability testing (read: analysis of occasional bug\n"},{"id":"188613","messageId":"7vbon54quk.fsf@alter.siamese.dyndns.org","threadId":"30154","inReplyTo":"20120406004449.GA15125@sigill.intra.peff.net","subject":"Re: [PATCH] compat/mingw.[ch]: Change return type of exec functions to int","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-06T01:00:03Z","receivedAt":"2012-04-06T01:00:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Apr 05, 2012 at 07:42:26PM -0500, Jonathan Nieder wrote:\n>\n>> >  \t/*\n>> > +\t * If we are still running, we know an error occurred; let's try to\n>> > +\t * diagnose it more specifically.\n>> > +\t *\n>> >  \t * When a command can't be found because one of the directories\n>> \n>> Looks fine to me. :)\n>\n> Thanks. Junio, do you mind squashing this onto the tip of\n> jk/run-command-eaccess? Patch repeated below for your convenience.\n\nThanks. I've pushed out today's round already, so maybe later.\n"}]}