David Rientjes <rientjes@xxxxxxxxxx> writes:
> diff --git a/builtin-apply.c b/builtin-apply.c
> index be2c715..2862eb1 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -2097,7 +2097,7 @@ static void create_one_file(char *path,
> }
>
> if (errno == EEXIST) {
> - unsigned int nr = getpid();
> + pid_t nr = getpid();
Since mkpath() is vararg, doesn't this make it necessary to cast
its parameter several lines down?
> diff --git a/builtin-read-tree.c b/builtin-read-tree.c
> index b30160a..f902fee 100644
> --- a/builtin-read-tree.c
> +++ b/builtin-read-tree.c
> @@ -23,7 +23,7 @@ static int nontrivial_merge = 0;
>...
> -static volatile int progress_update = 0;
> +static volatile sig_atomic_t progress_update = 0;
This is good. Thanks.
> diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c
> index 215892b..6fed919 100644
> --- a/builtin-tar-tree.c
> +++ b/builtin-tar-tree.c
> @@ -361,8 +361,8 @@ static const char *exec = "git-upload-ta
>
> static int remote_tar(int argc, const char **argv)
> {
> - int fd[2], ret, len;
> - pid_t pid;
> + int fd[2], len;
> + pid_t pid, ret;
Hmph. You might have made finish_connect() to return pid_t so
making "ret" of that type is consistent with that change, but it
also gets return value from copy_fd() -- which is "did we error
out?" This part smells funny...
> diff --git a/connect.c b/connect.c
> index 4422a0d..a4c02d1 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -735,9 +735,9 @@ int git_connect(int fd[2], char *url, co
> return pid;
> }
>
> -int finish_connect(pid_t pid)
> +pid_t finish_connect(pid_t pid)
> {
> - int ret;
> + pid_t ret;
This function wants to wait for the given process and return
zero on success otherwise the caller takes it as a sign for
failure. Most existing callers do not check the return value,
which should be cleaned up, but we always call it with specific
pid, not wildcard values like 0 or -1, so returning pid_t to say
which child exited does not add value to the interface. Please
leave its function signature (and one of the callers,
remote_tar() you changed above) as it is.
Having said that, I suspect the existing implementation is quite
buggy. It says:
for (;;) {
ret = waitpid(pid, NULL, 0);
if (!ret)
break;
if (errno != EINTR)
break;
}
return ret;
But it probably should read:
for (;;) {
pid_t ret = waitpid(pid, NULL, 0);
if (ret < 0 && errno == EINTR)
continue;
if (ret == pid)
return 0;
return -1;
}
I do not remember what I was smoking when I wrote that code, but
I suspect somehow I incorrectly thought waitpid() would return
zero for success.
I then dig the history and find out that it was not me but Linus
who did this with commit f719259 on July 4th 2005. Maybe this
was a program under influence, judging from the date of the
commit ;-)?
> diff --git a/fetch-clone.c b/fetch-clone.c
> index 5e84c46..c5cf477 100644
> --- a/fetch-clone.c
> +++ b/fetch-clone.c
>...
> diff --git a/merge-index.c b/merge-index.c
> index 0498a6f..a9c8cc1 100644
> --- a/merge-index.c
> +++ b/merge-index.c
>...
> diff --git a/run-command.c b/run-command.c
> index ca67ee9..3bacc1b 100644
> --- a/run-command.c
> +++ b/run-command.c
>...
These all look good, thanks.
-
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