David Rientjes <rientjes@xxxxxxxxxx> writes:
> On Tue, 15 Aug 2006, David Rientjes wrote:
>
> Please replace the original patch with the following.
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 9cf477c..56c5394 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();
>
(earlier)
>> 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.
If pid_t is a short then wouldn't it be promoted to "unsigned
int" just fine, except for the failure case (but this is
getpid() we are dealing with here)? More problematic is the
case where pid_t is wider than unsigned int, in which case we
can end up truncating the return value from getpid().
But for this particular case, I do not think it matters; the
code uses getpid() to seed the loop to obtain an unused
"~number" suffix; it could have used random(3) or time(2)
instead. As long as:
newpath = mkpath("%s~%u", path, nr);
does a reasonable thing, we are Ok.
I think, however, if you change the type of nr to pid_t, you
would need to cast it like this, because mkpath is defined to be
"char *mkpath(const char *, ...)":
newpath = mkpath("%s~%u", path, (unsigned int) nr);
Or use whatever matching integral type and format letter pairs;
we seem to like "%lu" format and "(unsigned long)" in other
parts of our code.
> diff --git a/fetch-clone.c b/fetch-clone.c
> index 5e84c46..c5cf477 100644
> --- a/fetch-clone.c
> +++ b/fetch-clone.c
> @@ -44,9 +44,8 @@ static int finish_pack(const char *pack_
>
> for (;;) {
> int status, code;
> - int retval = waitpid(pid, &status, 0);
>
> - if (retval < 0) {
> + if (waitpid(pid, &status, 0) < 0) {
> if (errno == EINTR)
> continue;
> error("waitpid failed (%s)", strerror(errno));
Makes sense -- if pid_t is wider than int we would be in
trouble.
> diff --git a/merge-index.c b/merge-index.c
Same.
> diff --git a/run-command.c b/run-command.c
> index ca67ee9..3bacc1b 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -25,15 +25,15 @@ int run_command_v_opt(int argc, const ch
> }
> for (;;) {
> int status, code;
> - int retval = waitpid(pid, &status, 0);
> + pid_t waiting = waitpid(pid, &status, 0);
>
> - if (retval < 0) {
> + if (waiting < 0) {
> if (errno == EINTR)
> continue;
Same.
> - error("waitpid failed (%s)", strerror(retval));
> + error("waitpid failed (%s)", strerror(waiting));
Shouldn't this be "strerror(errno)"? The original gets it wrong
already.
> diff --git a/unpack-trees.c b/unpack-trees.c
> index a20639b..e496d8c 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -278,7 +278,7 @@ static void unlink_entry(char *name)
> }
> }
>
> -static volatile int progress_update = 0;
> +static volatile sig_atomic_t progress_update = 0;
>
> static void progress_interval(int signum)
> {
This matches the other one in builtin-pack-objects.c and makes
sense.
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