Klaver wrote:
> The interpretation of errors from execvp was rather terse. For user
> convenience communication of the nature of the error can be improved.
Could you give an example?
[...]
> --- a/run-command.c
> +++ b/run-command.c
> @@ -2,6 +2,7 @@
> #include "run-command.h"
> #include "exec_cmd.h"
> #include "argv-array.h"
> +#include "dir.h"
>
> static inline void close_pair(int fd[2])
> {
> @@ -134,6 +135,140 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
> return code;
> }
>
> +#ifndef WIN32
Not related to this patch, but I wonder if there should be a separate
run-command-unix.c file so these ifdefs would no longer be necessary.
What happens on Windows?
> +static void die_file_error(const char *file, int err)
> +{
> + die("cannot exec '%s': %s", file, strerror(err));
> +}
I suspect it might be clearer to use die() inline in the two call
sites so the reader does not have to figure out the calling
convention.
> +
> +static char *get_interpreter(const char *first_line)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + size_t start = strspn(first_line + 2, " \t") + 2;
> + size_t end = strcspn(first_line + start, " \t\r\n") + start;
> +
> + if (start >= end)
> + return NULL;
> +
> + strbuf_add(&sb, first_line + start, end - start);
> + return strbuf_detach(&sb, NULL);
> +}
What does this function do? What happens if first_line doesn't start
with "#!"? What should happen when there is a newline instead of a
command name? How about commands with quoting characters like " and
backslash --- are the semantics portable in these cases?
No need to use a strbuf here: xmemdupz would take care of the
allocation and copy more simply.
> +static void inspect_failure(const char *argv0, int silent_exec_failure)
> +{
> + int err = errno;
> + struct strbuf sb = STRBUF_INIT;
> +
> + /* errors not related to path */
> + if (errno == E2BIG || errno == ENOMEM)
> + die_file_error(argv0, err);
> +
> + if (strchr(argv0, '/')) {
> + if (file_exists(argv0)) {
> + strbuf_add(&sb, argv0, strlen(argv0));
> + inspect_file(&sb, err, argv0);
> + }
> + } else {
> + char *path, *next;
> + path = getenv("PATH");
I wonder if it's possible to rearrange this code to avoid deep
nesting. What does the function do, anyway? (If the reader has to
ask, it needs a comment or to be renamed.)
I guess the idea is to diagnose after the fact why execvp failed.
Might be simplest like this:
To diagnose execvp failure:
if filename does not contain a '/':
if we can't find it on the search path:
That's the problem, dummy!
replace filename with full path
if file does not exist:
just report strerror(errno)
if not executable:
...
if interpreter does not exist:
...
if interpreter not executable:
...
otherwise, just report strerror(errno)
with a separate function to find a command on the PATH, complaining
when it encounters an unsearchable entry.
Thanks for a fun read.
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html