Re: [PATCH] use appropriate typedefs
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:37
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] use appropriate typedefs · David Rientjes <rientjes@google.com>
- 2016-06-15 · Re: [PATCH] use appropriate typedefs · Johannes Schindelin <hidden>
- 2016-06-15 · Re: [PATCH] use appropriate typedefs · David Rientjes <rientjes@google.com>
David Rientjes [off-list ref] writes:
quoted hunk
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?
quoted hunk
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.
quoted hunk
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...
quoted hunk
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 ;-)?
quoted hunk
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.