On Tue, 15 Aug 2006, Junio C Hamano wrote:
> > 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?
>
No, it is not necessary in the sense that any of these changes in this patch are
necessary. But since getpid() returns pid_t, every assignment should be cast as
such. pid_t can be typed as a short.
> > 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...
>
I did make finish_connect return pid_t, so it is consistent. Changing the use
of ret is beyond the scope of a patch that changes types to typedefs.
> > 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.
>
Please cite where this function is specified to return zero on success and not
the return value of waitpid which, after all, is the only assignment to the
return value. waitpid only returns when the status of the child is available or
an error has occurred as a result of an interrupt. The correct interface, in my
opinion, for this function is to return what waitpid returns and allow it to
indicate the pid of the child or interrupt to the caller. The signature now
suggests that. If Linus did indeed write this, he did so to spin until the
status of the child was known.
David
-
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