{"thread":{"id":"5279","subject":"[PATCH] use appropriate typedefs","startedAt":"2006-08-15T06:07:28Z","lastAt":"2006-08-16T06:40:45Z","messageCount":7,"participants":["David Rientjes","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"25336","messageId":"Pine.LNX.4.63.0608142305290.23445@chino.corp.google.com","threadId":"5279","inReplyTo":null,"subject":"[PATCH] use appropriate typedefs","fromName":"David Rientjes","fromEmail":"rientjes@google.com","sentAt":"2006-08-15T06:07:28Z","receivedAt":"2006-08-15T06:07:28Z","isPatch":true,"sender":{"key":"rientjes@google.com","avatar":null},"body":"Replaces int types with the appropriate definition in atomic and PID instances.\n\n\t\tDavid\n\nSigned-off-by: David Rientjes <rientjes@google.com>\n---\n builtin-apply.c     |    2 +-\n builtin-read-tree.c |    2 +-\n builtin-tar-tree.c  |    4 ++--\n connect.c           |    4 ++--\n fetch-clone.c       |    3 +--\n merge-index.c       |    3 ++-\n run-command.c       |    8 ++++----\n 7 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex be2c715..2862eb1 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2097,7 +2097,7 @@ static void create_one_file(char *path, \n \t}\n \n \tif (errno == EEXIST) {\n-\t\tunsigned int nr = getpid();\n+\t\tpid_t nr = getpid();\n \n \t\tfor (;;) {\n \t\t\tconst char *newpath;\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex b30160a..f902fee 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -23,7 +23,7 @@ static int nontrivial_merge = 0;\n static int trivial_merges_only = 0;\n static int aggressive = 0;\n static int verbose_update = 0;\n-static volatile int progress_update = 0;\n+static volatile sig_atomic_t progress_update = 0;\n static const char *prefix = NULL;\n \n static int head_idx = -1;\ndiff --git a/builtin-tar-tree.c b/builtin-tar-tree.c\nindex 215892b..6fed919 100644\n--- a/builtin-tar-tree.c\n+++ b/builtin-tar-tree.c\n@@ -361,8 +361,8 @@ static const char *exec = \"git-upload-ta\n \n static int remote_tar(int argc, const char **argv)\n {\n-\tint fd[2], ret, len;\n-\tpid_t pid;\n+\tint fd[2], len;\n+\tpid_t pid, ret;\n \tchar buf[1024];\n \tchar *url;\n \ndiff --git a/connect.c b/connect.c\nindex 4422a0d..a4c02d1 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -735,9 +735,9 @@ int git_connect(int fd[2], char *url, co\n \treturn pid;\n }\n \n-int finish_connect(pid_t pid)\n+pid_t finish_connect(pid_t pid)\n {\n-\tint ret;\n+\tpid_t ret;\n \n \tfor (;;) {\n \t\tret = waitpid(pid, NULL, 0);\ndiff --git a/fetch-clone.c b/fetch-clone.c\nindex 5e84c46..c5cf477 100644\n--- a/fetch-clone.c\n+++ b/fetch-clone.c\n@@ -44,9 +44,8 @@ static int finish_pack(const char *pack_\n \n \tfor (;;) {\n \t\tint status, code;\n-\t\tint retval = waitpid(pid, &status, 0);\n \n-\t\tif (retval < 0) {\n+\t\tif (waitpid(pid, &status, 0) < 0) {\n \t\t\tif (errno == EINTR)\n \t\t\t\tcontinue;\n \t\t\terror(\"waitpid failed (%s)\", strerror(errno));\ndiff --git a/merge-index.c b/merge-index.c\nindex 0498a6f..a9c8cc1 100644\n--- a/merge-index.c\n+++ b/merge-index.c\n@@ -11,7 +11,8 @@ static int err;\n \n static void run_program(void)\n {\n-\tint pid = fork(), status;\n+\tpid_t pid = fork();\n+\tint status;\n \n \tif (pid < 0)\n \t\tdie(\"unable to fork\");\ndiff --git a/run-command.c b/run-command.c\nindex ca67ee9..3bacc1b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -25,15 +25,15 @@ int run_command_v_opt(int argc, const ch\n \t}\n \tfor (;;) {\n \t\tint status, code;\n-\t\tint retval = waitpid(pid, &status, 0);\n+\t\tpid_t waiting = waitpid(pid, &status, 0);\n \n-\t\tif (retval < 0) {\n+\t\tif (waiting < 0) {\n \t\t\tif (errno == EINTR)\n \t\t\t\tcontinue;\n-\t\t\terror(\"waitpid failed (%s)\", strerror(retval));\n+\t\t\terror(\"waitpid failed (%s)\", strerror(waiting));\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID;\n \t\t}\n-\t\tif (retval != pid)\n+\t\tif (waiting != pid)\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_WRONG_PID;\n \t\tif (WIFSIGNALED(status))\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_SIGNAL;\n-- \n1.4.2.GIT\n"},{"id":"25342","messageId":"7vveou8myg.fsf@assigned-by-dhcp.cox.net","threadId":"5279","inReplyTo":"Pine.LNX.4.63.0608142305290.23445@chino.corp.google.com","subject":"Re: [PATCH] use appropriate typedefs","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-15T07:41:59Z","receivedAt":"2006-08-15T07:41:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Rientjes <rientjes@google.com> writes:\n\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index be2c715..2862eb1 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -2097,7 +2097,7 @@ static void create_one_file(char *path, \n>  \t}\n>  \n>  \tif (errno == EEXIST) {\n> -\t\tunsigned int nr = getpid();\n> +\t\tpid_t nr = getpid();\n\nSince mkpath() is vararg, doesn't this make it necessary to cast\nits parameter several lines down?\n\n> diff --git a/builtin-read-tree.c b/builtin-read-tree.c\n> index b30160a..f902fee 100644\n> --- a/builtin-read-tree.c\n> +++ b/builtin-read-tree.c\n> @@ -23,7 +23,7 @@ static int nontrivial_merge = 0;\n>...\n> -static volatile int progress_update = 0;\n> +static volatile sig_atomic_t progress_update = 0;\n\nThis is good.  Thanks.\n\n> diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c\n> index 215892b..6fed919 100644\n> --- a/builtin-tar-tree.c\n> +++ b/builtin-tar-tree.c\n> @@ -361,8 +361,8 @@ static const char *exec = \"git-upload-ta\n>  \n>  static int remote_tar(int argc, const char **argv)\n>  {\n> -\tint fd[2], ret, len;\n> -\tpid_t pid;\n> +\tint fd[2], len;\n> +\tpid_t pid, ret;\n\nHmph.  You might have made finish_connect() to return pid_t so\nmaking \"ret\" of that type is consistent with that change, but it\nalso gets return value from copy_fd() -- which is \"did we error\nout?\"  This part smells funny...\n\n> diff --git a/connect.c b/connect.c\n> index 4422a0d..a4c02d1 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -735,9 +735,9 @@ int git_connect(int fd[2], char *url, co\n>  \treturn pid;\n>  }\n>  \n> -int finish_connect(pid_t pid)\n> +pid_t finish_connect(pid_t pid)\n>  {\n> -\tint ret;\n> +\tpid_t ret;\n\nThis function wants to wait for the given process and return\nzero on success otherwise the caller takes it as a sign for\nfailure.  Most existing callers do not check the return value,\nwhich should be cleaned up, but we always call it with specific\npid, not wildcard values like 0 or -1, so returning pid_t to say\nwhich child exited does not add value to the interface.  Please\nleave its function signature (and one of the callers,\nremote_tar() you changed above) as it is.\n\nHaving said that, I suspect the existing implementation is quite\nbuggy.  It says:\n\n\tfor (;;) {\n\t\tret = waitpid(pid, NULL, 0);\n\t        if (!ret)\n                \tbreak;\n\t\tif (errno != EINTR)\n                \tbreak;\n\t}\n        return ret;\n\nBut it probably should read:\n\n\tfor (;;) {\n\t\tpid_t ret = waitpid(pid, NULL, 0);\n\t\tif (ret < 0 && errno == EINTR)\n\t\t\tcontinue;\n\t        if (ret == pid)\n\t\t\treturn 0;\n\t\treturn -1;\n\t}\n\nI do not remember what I was smoking when I wrote that code, but\nI suspect somehow I incorrectly thought waitpid() would return\nzero for success.\n\nI then dig the history and find out that it was not me but Linus\nwho did this with commit f719259 on July 4th 2005.  Maybe this\nwas a program under influence, judging from the date of the\ncommit ;-)?\n\n> diff --git a/fetch-clone.c b/fetch-clone.c\n> index 5e84c46..c5cf477 100644\n> --- a/fetch-clone.c\n> +++ b/fetch-clone.c\n>...\n> diff --git a/merge-index.c b/merge-index.c\n> index 0498a6f..a9c8cc1 100644\n> --- a/merge-index.c\n> +++ b/merge-index.c\n>...\n> diff --git a/run-command.c b/run-command.c\n> index ca67ee9..3bacc1b 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n>...\n\nThese all look good, thanks.\n"},{"id":"25353","messageId":"Pine.LNX.4.63.0608151204540.28360@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5279","inReplyTo":"Pine.LNX.4.63.0608142305290.23445@chino.corp.google.com","subject":"Re: [PATCH] use appropriate typedefs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-08-15T10:05:56Z","receivedAt":"2006-08-15T10:05:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 14 Aug 2006, David Rientjes wrote:\n\n> Replaces int types with the appropriate definition in atomic and PID instances.\n\nI was looking forward to the performance boosts you hinted at. But this \npatch, and the proposed static variables patch do nothing for it.\n\nCiao,\nDscho\n"},{"id":"25366","messageId":"Pine.LNX.4.63.0608151000350.26891@chino.corp.google.com","threadId":"5279","inReplyTo":"7vveou8myg.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] use appropriate typedefs","fromName":"David Rientjes","fromEmail":"rientjes@google.com","sentAt":"2006-08-15T17:19:16Z","receivedAt":"2006-08-15T17:19:16Z","isPatch":true,"sender":{"key":"rientjes@google.com","avatar":null},"body":"On Tue, 15 Aug 2006, Junio C Hamano wrote:\n> > diff --git a/builtin-apply.c b/builtin-apply.c\n> > index be2c715..2862eb1 100644\n> > --- a/builtin-apply.c\n> > +++ b/builtin-apply.c\n> > @@ -2097,7 +2097,7 @@ static void create_one_file(char *path, \n> >  \t}\n> >  \n> >  \tif (errno == EEXIST) {\n> > -\t\tunsigned int nr = getpid();\n> > +\t\tpid_t nr = getpid();\n> \n> Since mkpath() is vararg, doesn't this make it necessary to cast\n> its parameter several lines down?\n> \n\nNo, it is not necessary in the sense that any of these changes in this patch are \nnecessary.  But since getpid() returns pid_t, every assignment should be cast as \nsuch.  pid_t can be typed as a short.\n\n> > diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c\n> > index 215892b..6fed919 100644\n> > --- a/builtin-tar-tree.c\n> > +++ b/builtin-tar-tree.c\n> > @@ -361,8 +361,8 @@ static const char *exec = \"git-upload-ta\n> >  \n> >  static int remote_tar(int argc, const char **argv)\n> >  {\n> > -\tint fd[2], ret, len;\n> > -\tpid_t pid;\n> > +\tint fd[2], len;\n> > +\tpid_t pid, ret;\n> \n> Hmph.  You might have made finish_connect() to return pid_t so\n> making \"ret\" of that type is consistent with that change, but it\n> also gets return value from copy_fd() -- which is \"did we error\n> out?\"  This part smells funny...\n> \n\nI did make finish_connect return pid_t, so it is consistent.  Changing the use \nof ret is beyond the scope of a patch that changes types to typedefs.\n\n> > diff --git a/connect.c b/connect.c\n> > index 4422a0d..a4c02d1 100644\n> > --- a/connect.c\n> > +++ b/connect.c\n> > @@ -735,9 +735,9 @@ int git_connect(int fd[2], char *url, co\n> >  \treturn pid;\n> >  }\n> >  \n> > -int finish_connect(pid_t pid)\n> > +pid_t finish_connect(pid_t pid)\n> >  {\n> > -\tint ret;\n> > +\tpid_t ret;\n> \n> This function wants to wait for the given process and return\n> zero on success otherwise the caller takes it as a sign for\n> failure.  Most existing callers do not check the return value,\n> which should be cleaned up, but we always call it with specific\n> pid, not wildcard values like 0 or -1, so returning pid_t to say\n> which child exited does not add value to the interface.  Please\n> leave its function signature (and one of the callers,\n> remote_tar() you changed above) as it is.\n> \n\nPlease cite where this function is specified to return zero on success and not \nthe return value of waitpid which, after all, is the only assignment to the \nreturn value.  waitpid only returns when the status of the child is available or \nan error has occurred as a result of an interrupt.  The correct interface, in my \nopinion, for this function is to return what waitpid returns and allow it to \nindicate the pid of the child or interrupt to the caller.  The signature now \nsuggests that.  If Linus did indeed write this, he did so to spin until the \nstatus of the child was known.\n\n\t\tDavid\n"},{"id":"25367","messageId":"Pine.LNX.4.63.0608151019390.26891@chino.corp.google.com","threadId":"5279","inReplyTo":"Pine.LNX.4.63.0608151204540.28360@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] use appropriate typedefs","fromName":"David Rientjes","fromEmail":"rientjes@google.com","sentAt":"2006-08-15T17:21:52Z","receivedAt":"2006-08-15T17:21:52Z","isPatch":true,"sender":{"key":"rientjes@google.com","avatar":null},"body":"On Tue, 15 Aug 2006, Johannes Schindelin wrote:\n> \n> I was looking forward to the performance boosts you hinted at. But this \n> patch, and the proposed static variables patch do nothing for it.\n> \n\nMy optimization for speed is mostly architecture specific so it's not useful for \nyour project and a lot of the functionality is actually stripped because it's \nhandled by a wrapper.  My use of register variables were actually stripped out \nby hand before I submitted these patches here.\n\n\t\tDavid\n"},{"id":"25371","messageId":"Pine.LNX.4.63.0608151037570.28175@chino.corp.google.com","threadId":"5279","inReplyTo":"Pine.LNX.4.63.0608151000350.26891@chino.corp.google.com","subject":"Re: [PATCH] use appropriate typedefs","fromName":"David Rientjes","fromEmail":"rientjes@google.com","sentAt":"2006-08-15T17:40:06Z","receivedAt":"2006-08-15T17:40:06Z","isPatch":true,"sender":{"key":"rientjes@google.com","avatar":null},"body":"On Tue, 15 Aug 2006, David Rientjes wrote:\n> \n> Please cite where this function is specified to return zero on success and not \n> the return value of waitpid which, after all, is the only assignment to the \n> return value.  waitpid only returns when the status of the child is available or \n> an error has occurred as a result of an interrupt.  The correct interface, in my \n> opinion, for this function is to return what waitpid returns and allow it to \n> indicate the pid of the child or interrupt to the caller.  The signature now \n> suggests that.  If Linus did indeed write this, he did so to spin until the \n> status of the child was known.\n> \n\nForget this, the function is correct as is because EINTR is only returned on \nsignal interrupt and ret is set to -1 (by the waitpid spec) implicitly.\n\nPlease replace the original patch with the following.\n\n\nReplaces types with appropriate typedefs.\n\n\t\tDavid\n\nSigned-off-by: David Rientjes <rientjes@google.com>\n---\n builtin-apply.c |    2 +-\n fetch-clone.c   |    3 +--\n merge-index.c   |    3 ++-\n run-command.c   |    8 ++++----\n unpack-trees.c  |    2 +-\n 5 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 9cf477c..56c5394 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2097,7 +2097,7 @@ static void create_one_file(char *path, \n \t}\n \n \tif (errno == EEXIST) {\n-\t\tunsigned int nr = getpid();\n+\t\tpid_t nr = getpid();\n \n \t\tfor (;;) {\n \t\t\tconst char *newpath;\ndiff --git a/fetch-clone.c b/fetch-clone.c\nindex 5e84c46..c5cf477 100644\n--- a/fetch-clone.c\n+++ b/fetch-clone.c\n@@ -44,9 +44,8 @@ static int finish_pack(const char *pack_\n \n \tfor (;;) {\n \t\tint status, code;\n-\t\tint retval = waitpid(pid, &status, 0);\n \n-\t\tif (retval < 0) {\n+\t\tif (waitpid(pid, &status, 0) < 0) {\n \t\t\tif (errno == EINTR)\n \t\t\t\tcontinue;\n \t\t\terror(\"waitpid failed (%s)\", strerror(errno));\ndiff --git a/merge-index.c b/merge-index.c\nindex 0498a6f..a9c8cc1 100644\n--- a/merge-index.c\n+++ b/merge-index.c\n@@ -11,7 +11,8 @@ static int err;\n \n static void run_program(void)\n {\n-\tint pid = fork(), status;\n+\tpid_t pid = fork();\n+\tint status;\n \n \tif (pid < 0)\n \t\tdie(\"unable to fork\");\ndiff --git a/run-command.c b/run-command.c\nindex ca67ee9..3bacc1b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -25,15 +25,15 @@ int run_command_v_opt(int argc, const ch\n \t}\n \tfor (;;) {\n \t\tint status, code;\n-\t\tint retval = waitpid(pid, &status, 0);\n+\t\tpid_t waiting = waitpid(pid, &status, 0);\n \n-\t\tif (retval < 0) {\n+\t\tif (waiting < 0) {\n \t\t\tif (errno == EINTR)\n \t\t\t\tcontinue;\n-\t\t\terror(\"waitpid failed (%s)\", strerror(retval));\n+\t\t\terror(\"waitpid failed (%s)\", strerror(waiting));\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID;\n \t\t}\n-\t\tif (retval != pid)\n+\t\tif (waiting != pid)\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_WRONG_PID;\n \t\tif (WIFSIGNALED(status))\n \t\t\treturn -ERR_RUN_COMMAND_WAITPID_SIGNAL;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a20639b..e496d8c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -278,7 +278,7 @@ static void unlink_entry(char *name)\n \t}\n }\n \n-static volatile int progress_update = 0;\n+static volatile sig_atomic_t progress_update = 0;\n \n static void progress_interval(int signum)\n {\n-- \n1.4.2.g460c-dirty\n"},{"id":"25401","messageId":"7vr6zh41zm.fsf@assigned-by-dhcp.cox.net","threadId":"5279","inReplyTo":"Pine.LNX.4.63.0608151037570.28175@chino.corp.google.com","subject":"Re: [PATCH] use appropriate typedefs","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-16T06:40:45Z","receivedAt":"2006-08-16T06:40:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Rientjes <rientjes@google.com> writes:\n\n> On Tue, 15 Aug 2006, David Rientjes wrote:\n>\n> Please replace the original patch with the following.\n\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index 9cf477c..56c5394 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -2097,7 +2097,7 @@ static void create_one_file(char *path, \n>  \t}\n>  \n>  \tif (errno == EEXIST) {\n> -\t\tunsigned int nr = getpid();\n> +\t\tpid_t nr = getpid();\n>  \n\n(earlier)\n\n>> Since mkpath() is vararg, doesn't this make it necessary to cast\n>> its parameter several lines down?\n>\n> No, it is not necessary in the sense that any of these changes\n> in this patch are necessary.  But since getpid() returns\n> pid_t, every assignment should be cast as such.  pid_t can be\n> typed as a short.\n\nIf pid_t is a short then wouldn't it be promoted to \"unsigned\nint\" just fine, except for the failure case (but this is\ngetpid() we are dealing with here)?  More problematic is the\ncase where pid_t is wider than unsigned int, in which case we\ncan end up truncating the return value from getpid().\n\nBut for this particular case, I do not think it matters; the\ncode uses getpid() to seed the loop to obtain an unused\n\"~number\" suffix; it could have used random(3) or time(2)\ninstead.  As long as:\n\n\tnewpath = mkpath(\"%s~%u\", path, nr);\n\ndoes a reasonable thing, we are Ok.\n\nI think, however, if you change the type of nr to pid_t, you\nwould need to cast it like this, because mkpath is defined to be\n\"char *mkpath(const char *, ...)\":\n\n\tnewpath = mkpath(\"%s~%u\", path, (unsigned int) nr);\n\nOr use whatever matching integral type and format letter pairs;\nwe seem to like \"%lu\" format and \"(unsigned long)\" in other\nparts of our code.\n\n> diff --git a/fetch-clone.c b/fetch-clone.c\n> index 5e84c46..c5cf477 100644\n> --- a/fetch-clone.c\n> +++ b/fetch-clone.c\n> @@ -44,9 +44,8 @@ static int finish_pack(const char *pack_\n>  \n>  \tfor (;;) {\n>  \t\tint status, code;\n> -\t\tint retval = waitpid(pid, &status, 0);\n>  \n> -\t\tif (retval < 0) {\n> +\t\tif (waitpid(pid, &status, 0) < 0) {\n>  \t\t\tif (errno == EINTR)\n>  \t\t\t\tcontinue;\n>  \t\t\terror(\"waitpid failed (%s)\", strerror(errno));\n\nMakes sense -- if pid_t is wider than int we would be in\ntrouble.\n\n> diff --git a/merge-index.c b/merge-index.c\n\nSame.\n\n> diff --git a/run-command.c b/run-command.c\n> index ca67ee9..3bacc1b 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -25,15 +25,15 @@ int run_command_v_opt(int argc, const ch\n>  \t}\n>  \tfor (;;) {\n>  \t\tint status, code;\n> -\t\tint retval = waitpid(pid, &status, 0);\n> +\t\tpid_t waiting = waitpid(pid, &status, 0);\n>  \n> -\t\tif (retval < 0) {\n> +\t\tif (waiting < 0) {\n>  \t\t\tif (errno == EINTR)\n>  \t\t\t\tcontinue;\n\nSame.\n\n> -\t\t\terror(\"waitpid failed (%s)\", strerror(retval));\n> +\t\t\terror(\"waitpid failed (%s)\", strerror(waiting));\n\nShouldn't this be \"strerror(errno)\"?  The original gets it wrong\nalready.\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index a20639b..e496d8c 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -278,7 +278,7 @@ static void unlink_entry(char *name)\n>  \t}\n>  }\n>  \n> -static volatile int progress_update = 0;\n> +static volatile sig_atomic_t progress_update = 0;\n>  \n>  static void progress_interval(int signum)\n>  {\n\nThis matches the other one in builtin-pack-objects.c and makes\nsense.\n\nThanks.\n"}]}