Re: [PATCH] compat/mingw.[ch]: Change return type of exec functions to int

From: Jeff King <peff@peff.net>
Date: Apr 5, 2012, 22:06 UTC
Message-ID: <20120405220615.GA11751@sigill.intra.peff.net>
In-Reply-To: <20120405181600.GA6340@burratino>

On Thu, Apr 05, 2012 at 01:16:01PM -0500, Jonathan Nieder wrote:

Show 13 quoted lines

> Ramsay Jones wrote:
> 
> >         CC run-command.o
> >     run-command.c: In function 'sane_execvp':
> >     run-command.c:124: error: invalid use of void expression
> >     make: *** [run-command.o] Error 1
> >
> > My first reaction was to simply remove the conditional since, if execvp()
> > returns at all, the result will always be -1 and so the condition will
> > always be false. ie. the conditional is pointless.
> >
> > However, I found the incorrect return type of the mingw_execv[p]() to be
> > a gratuitous incompatibility, so ... :-P

Yeah, it is just asking for trouble to #define a new execvp that does not match the declaration of the existing one, so I think Ramsay's patch is good.

Show 15 quoted lines

> My bad.  I agree that in addition to making the return type fix,
> squashing the following into jk/run-command-eacces would be a good
> idea.
> 
> diff --git i/run-command.c w/run-command.c
> index 04f0190d..fcd7e192 100644
> --- i/run-command.c
> +++ w/run-command.c
> @@ -59,8 +59,7 @@ static int exists_in_PATH(const char *file)
>  
>  int sane_execvp(const char *file, char * const argv[])
>  {
> -	if (!execvp(file, argv))
> -		return 0;
> +	execvp(file, argv);

I don't have a strong opinion. The "return 0" is a little misleading, since it will never be called, but I think we should at least have a comment like:

diff --git a/run-command.c b/run-command.c
index 7123436..5b6a368 100644
--- a/run-command.c
+++ b/run-command.c
@@ -117,8 +117,11 @@ static int exists_in_PATH(const char *file)
 
 int sane_execvp(const char *file, char * const argv[])
 {
-	if (!execvp(file, argv))
-		return 0;
+	/*
+	 * No need to check the return value; if it returns at all, an error
+	 * occurred.
+	 */
+	execvp(file, argv);
 
 	/*
 	 * When a command can't be found because one of the directories