[PATCH 0/2] Improve portability for OpenSolaris

[PATCH 0/2] Improve portability for OpenSolaris

[David Soria Parra]

This small patch series fixes some compile warnings on OpenSolaris/Solaris.
As pid_t is a long on Solaris, I changed the output of fprintf&co to longs
and cast pids to long, so that we are safe on the solaris

[PATCH 1/2] Improvate portability: Display pid_t's always as long

[David Soria Parra]

From: David Soria Parra <dsp@php.net>

Some systems (like e.g. OpenSolaris) define pid_t as long,
therefore all our sprintf that use %i cause a compiler warning
beacuse if the implicit long->int cast. So to make sure that
we fit the limits we display pids as longs and cast them explicitly.

Signed-off-by: David Soria Parra <dsp@php.net>
---
 builtin-commit.c     |    2 +-
 builtin-fetch-pack.c |    2 +-
 daemon.c             |    2 +-
 fast-import.c        |    6 +++---
 receive-pack.c       |    2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 649c8be..b4c940f 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -320,7 +320,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 		die("unable to write new_index file");
 
 	fd = hold_lock_file_for_update(&false_lock,
-				       git_path("next-index-%d", getpid()), 1);
+				       git_path("next-index-%ld", (long) getpid()), 1);
 
 	create_base_index();
 	add_remove_files(&partial);
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 273239a..46bb3e2 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -540,7 +540,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
 			*av++ = "--fix-thin";
 		if (args.lock_pack || unpack_limit) {
 			int s = sprintf(keep_arg,
-					"--keep=fetch-pack %d on ", getpid());
+					"--keep=fetch-pack %ld on ", (long) getpid());
 			if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
 				strcpy(keep_arg + s, "localhost");
 			*av++ = keep_arg;
diff --git a/daemon.c b/daemon.c
index 23278e2..1e5cb58 100644
--- a/daemon.c
+++ b/daemon.c
@@ -923,7 +923,7 @@ static void store_pid(const char *path)
 	FILE *f = fopen(path, "w");
 	if (!f)
 		die("cannot open pid file %s: %s", path, strerror(errno));
-	if (fprintf(f, "%d\n", getpid()) < 0 || fclose(f) != 0)
+	if (fprintf(f, "%ld\n", (long) getpid()) < 0 || fclose(f) != 0)
 		die("failed to write pid file %s: %s", path, strerror(errno));
 }
 
diff --git a/fast-import.c b/fast-import.c
index 7089e6f..e04ed94 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -376,7 +376,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);
 
 static void write_crash_report(const char *err)
 {
-	char *loc = git_path("fast_import_crash_%d", getpid());
+	char *loc = git_path("fast_import_crash_%ld", (long) getpid());
 	FILE *rpt = fopen(loc, "w");
 	struct branch *b;
 	unsigned long lu;
@@ -390,8 +390,8 @@ static void write_crash_report(const char *err)
 	fprintf(stderr, "fast-import: dumping crash report to %s\n", loc);
 
 	fprintf(rpt, "fast-import crash report:\n");
-	fprintf(rpt, "    fast-import process: %d\n", getpid());
-	fprintf(rpt, "    parent process     : %d\n", getppid());
+	fprintf(rpt, "    fast-import process: %ld\n", (long) getpid());
+	fprintf(rpt, "    parent process     : %ld\n", (long) getppid());
 	fprintf(rpt, "    at %s\n", show_date(time(NULL), 0, DATE_LOCAL));
 	fputc('\n', rpt);
 
diff --git a/receive-pack.c b/receive-pack.c
index d44c19e..0a82f73 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -407,7 +407,7 @@ static const char *unpack(void)
 		char keep_arg[256];
 		struct child_process ip;
 
-		s = sprintf(keep_arg, "--keep=receive-pack %i on ", getpid());
+		s = sprintf(keep_arg, "--keep=receive-pack %li on ", (long) getpid());
 		if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
 			strcpy(keep_arg + s, "localhost");
 
-- 
1.6.0.174.gd789c

[PATCH 2/2] Improve portability: Avoid SS constant as it is already defined

[David Soria Parra]

From: David Soria Parra <dsp@php.net>

Constants that have the names of CPU registers are already defined
in OpenSolaris's sys/regset.h. This causes a warning as we try to
(re)define SS in ctype.c. So we just use another name.

Signed-off-by: David Soria Parra <dsp@php.net>
---
 ctype.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ctype.c b/ctype.c
index ee06eb7..6ed2ba2 100644
--- a/ctype.c
+++ b/ctype.c
@@ -5,14 +5,14 @@
  */
 #include "cache.h"
 
-#define SS GIT_SPACE
+#define SP GIT_SPACE
 #define AA GIT_ALPHA
 #define DD GIT_DIGIT
 
 unsigned char sane_ctype[256] = {
-	 0,  0,  0,  0,  0,  0,  0,  0,  0, SS, SS,  0,  0, SS,  0,  0,		/* 0-15 */
+	 0,  0,  0,  0,  0,  0,  0,  0,  0, SP, SP,  0,  0, SP,  0,  0,		/* 0-15 */
 	 0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,		/* 16-15 */
-	SS,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,		/* 32-15 */
+	SP,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,		/* 32-15 */
 	DD, DD, DD, DD, DD, DD, DD, DD, DD, DD,  0,  0,  0,  0,  0,  0,		/* 48-15 */
 	 0, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,		/* 64-15 */
 	AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,  0,  0,  0,  0,  0,		/* 80-15 */
-- 
1.6.0.174.gd789c

Re: [PATCH 2/2] Improve portability: Avoid SS constant as it is already defined

[Junio C Hamano]

David Soria Parra <sn_@gmx.net> writes:

> From: David Soria Parra <dsp@php.net>
>
> Constants that have the names of CPU registers are already defined
> in OpenSolaris's sys/regset.h. This causes a warning as we try to
> (re)define SS in ctype.c. So we just use another name.

I do not mind this _particular_ workaround per-se, but I have to wonder
what happens the next time some random other platform has "SP" defined in
a random header file.

First of all, why are you including <sys/regset.h>?  We certainly don't
include from any of our header or source files.  And second of all, why is
the indirect inclusion of that header file by some standard header file we
do include cause the namespace to get poluted with "SS" symbol?

Re: [PATCH 2/2] Improve portability: Avoid SS constant as it is already defined

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> David Soria Parra <sn_@gmx.net> writes:
>
>> From: David Soria Parra <dsp@php.net>
>>
>> Constants that have the names of CPU registers are already defined
>> in OpenSolaris's sys/regset.h. This causes a warning as we try to
>> (re)define SS in ctype.c. So we just use another name.
>
> I do not mind this _particular_ workaround per-se, but I have to wonder
> what happens the next time some random other platform has "SP" defined in
> a random header file.

If we are doing an workaround, how about doing it this way instead?

 ctype.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git i/ctype.c w/ctype.c
index ee06eb7..d2bd38e 100644
--- i/ctype.c
+++ w/ctype.c
@@ -5,6 +5,11 @@
  */
 #include "cache.h"
 
+/* Just so that no insane platform contaminates the namespace with these symbols */
+#undef SS
+#undef AA
+#undef DD
+
 #define SS GIT_SPACE
 #define AA GIT_ALPHA
 #define DD GIT_DIGIT

Re: [PATCH 2/2] Improve portability: Avoid SS constant as it is already defined

[David Soria Parra]

Should be fine as well.

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> David Soria Parra <sn_@gmx.net> writes:
>>
>>> From: David Soria Parra <dsp@php.net>
>>>
>>> Constants that have the names of CPU registers are already defined
>>> in OpenSolaris's sys/regset.h. This causes a warning as we try to
>>> (re)define SS in ctype.c. So we just use another name.
>> I do not mind this _particular_ workaround per-se, but I have to wonder
>> what happens the next time some random other platform has "SP" defined in
>> a random header file.
> 
> If we are doing an workaround, how about doing it this way instead?
> 
>  ctype.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git i/ctype.c w/ctype.c
> index ee06eb7..d2bd38e 100644
> --- i/ctype.c
> +++ w/ctype.c
> @@ -5,6 +5,11 @@
>   */
>  #include "cache.h"
>  
> +/* Just so that no insane platform contaminates the namespace with these symbols */
> +#undef SS
> +#undef AA
> +#undef DD
> +
>  #define SS GIT_SPACE
>  #define AA GIT_ALPHA
>  #define DD GIT_DIGIT

Re: [PATCH 1/2] Improvate portability: Display pid_t's always as long

[Junio C Hamano]

David Soria Parra <sn_@gmx.net> writes:

> Some systems (like e.g. OpenSolaris) define pid_t as long,
> therefore all our sprintf that use %i cause a compiler warning
> beacuse if the implicit long->int cast. So to make sure that
> we fit the limits we display pids as longs and cast them explicitly.

This patch just makes one wonder what needs to happen when the next random
platform has pid_t as long long or int32_t or whatever signed integral
type that was picked arbitrarily by the platform.

I think these *printf()s are mostly for informational purposes and if you
favor minimum change, you might be better off casting it to "int" without
changing the format specifiers.

On the other hand, if you are shooting for maximum compatibility perhaps
you may want to cast it to "intmax_t" and format as such.

Casting to "long" does not make much sense from either perspective, does
it?

Re: [PATCH 1/2] Improvate portability: Display pid_t's always as long

[David Soria Parra]

 > On the other hand, if you are shooting for maximum compatibility perhaps
 > you may want to cast it to "intmax_t" and format as such.
Yes, good point, casting to long isn't enough. I think it's a good approach to cast the pids to intmax_t as pids
are also used in git_path() and therefore might result in equal paths for separate processes if
the pid is higher than int.

so here is an updated patch:

 From da5519b3ae5ce84c703aeaab2bc4ea363897c334 Mon Sep 17 00:00:00 2001
From: David Soria Parra <dsp at>
Date: Fri, 29 Aug 2008 01:19:43 +0200
Subject: [PATCH] Improvate portability: Cast pid_t's to intmax_t

Some systems (like e.g. OpenSolaris) define pid_t as long,
therefore all our sprintf that use %i cause a compiler warning
beacuse if the implicit long->int cast. So to make sure that
we fit the limits we display pids as intmax_t and cast them explicitly.

Signed-off-by: David Soria Parra <dsp@php.net>
---
  builtin-commit.c     |    2 +-
  builtin-fetch-pack.c |    2 +-
  daemon.c             |    6 +++---
  fast-import.c        |    6 +++---
  receive-pack.c       |    2 +-
  5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index c870037..90ef3d5 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -320,7 +320,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
  		die("unable to write new_index file");

  	fd = hold_lock_file_for_update(&false_lock,
-				       git_path("next-index-%d", getpid()), 1);
+				       git_path("next-index-%jd", (intmax_t) getpid()), 1);

  	create_base_index();
  	add_remove_files(&partial);
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 273239a..91616e7 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -540,7 +540,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
  			*av++ = "--fix-thin";
  		if (args.lock_pack || unpack_limit) {
  			int s = sprintf(keep_arg,
-					"--keep=fetch-pack %d on ", getpid());
+					"--keep=fetch-pack %jd on ", (intmax_t) getpid());
  			if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
  				strcpy(keep_arg + s, "localhost");
  			*av++ = keep_arg;
diff --git a/daemon.c b/daemon.c
index 23278e2..6081986 100644
--- a/daemon.c
+++ b/daemon.c
@@ -86,7 +86,7 @@ static void logreport(int priority, const char *err, va_list params)
  		 * Since stderr is set to linebuffered mode, the
  		 * logging of different processes will not overlap
  		 */
-		fprintf(stderr, "[%d] ", (int)getpid());
+		fprintf(stderr, "[%jd] ", (intmax_t)getpid());
  		vfprintf(stderr, err, params);
  		fputc('\n', stderr);
  	}
@@ -658,7 +658,7 @@ static void check_dead_children(void)
  		remove_child(pid);
  		if (!WIFEXITED(status) || (WEXITSTATUS(status) > 0))
  			dead = " (with error)";
-		loginfo("[%d] Disconnected%s", (int)pid, dead);
+		loginfo("[%jd] Disconnected%s", (intmax_t)pid, dead);
  	}
  }

@@ -923,7 +923,7 @@ static void store_pid(const char *path)
  	FILE *f = fopen(path, "w");
  	if (!f)
  		die("cannot open pid file %s: %s", path, strerror(errno));
-	if (fprintf(f, "%d\n", getpid()) < 0 || fclose(f) != 0)
+	if (fprintf(f, "%jd\n", (intmax_t) getpid()) < 0 || fclose(f) != 0)
  		die("failed to write pid file %s: %s", path, strerror(errno));
  }

diff --git a/fast-import.c b/fast-import.c
index 7089e6f..e3a6510 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -376,7 +376,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);

  static void write_crash_report(const char *err)
  {
-	char *loc = git_path("fast_import_crash_%d", getpid());
+	char *loc = git_path("fast_import_crash_%jd", (intmax_t) getpid());
  	FILE *rpt = fopen(loc, "w");
  	struct branch *b;
  	unsigned long lu;
@@ -390,8 +390,8 @@ static void write_crash_report(const char *err)
  	fprintf(stderr, "fast-import: dumping crash report to %s\n", loc);

  	fprintf(rpt, "fast-import crash report:\n");
-	fprintf(rpt, "    fast-import process: %d\n", getpid());
-	fprintf(rpt, "    parent process     : %d\n", getppid());
+	fprintf(rpt, "    fast-import process: %jd\n", (intmax_t) getpid());
+	fprintf(rpt, "    parent process     : %jd\n", (intmax_t) getppid());
  	fprintf(rpt, "    at %s\n", show_date(time(NULL), 0, DATE_LOCAL));
  	fputc('\n', rpt);

diff --git a/receive-pack.c b/receive-pack.c
index d44c19e..ec770d0 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -407,7 +407,7 @@ static const char *unpack(void)
  		char keep_arg[256];
  		struct child_process ip;

-		s = sprintf(keep_arg, "--keep=receive-pack %i on ", getpid());
+		s = sprintf(keep_arg, "--keep=receive-pack %ji on ", (intmax_t) getpid());
  		if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
  			strcpy(keep_arg + s, "localhost");

-- 
1.6.0.174.gd789c

Re: [PATCH 1/2] Improvate portability: Display pid_t's always as long

[Junio C Hamano]

David Soria Parra <dsoria@gmx.net> writes:

>> On the other hand, if you are shooting for maximum compatibility perhaps
>> you may want to cast it to "intmax_t" and format as such.
> Yes, good point, casting to long isn't enough. I think it's a good approach to cast the pids to intmax_t as pids
> are also used in git_path() and therefore might result in equal paths for separate processes if
> the pid is higher than int.
>
> so here is an updated patch:

Please wrap lines to reasonable length (e.g. 70-76 cols).

Please move commentary like this that clarifies context of the patch
submission to after three-dashes (emulate patches from people with good
manners).

> From da5519b3ae5ce84c703aeaab2bc4ea363897c334 Mon Sep 17 00:00:00 2001

Especially, don't paste this line.

> From: David Soria Parra <dsp at>
> Date: Fri, 29 Aug 2008 01:19:43 +0200
> Subject: [PATCH] Improvate portability: Cast pid_t's to intmax_t

"Improvate"?

Including these in your message is not very useful.  These in-body headers
are used to override what can be read from the real headers of the e-mail
message, but you do not have a valid e-mail address here!

> Some systems (like e.g. OpenSolaris) define pid_t as long,
> ...
> diff --git a/builtin-commit.c b/builtin-commit.c
> index c870037..90ef3d5 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -320,7 +320,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
>  		die("unable to write new_index file");
>
>  	fd = hold_lock_file_for_update(&false_lock,
> -				       git_path("next-index-%d", getpid()), 1);
> +				       git_path("next-index-%jd", (intmax_t) getpid()), 1);

Some systems we support do not have %j width specifier.  I'd suggest
casting up to uintmax_t and format with PRIuMAX, which we do define
a substitute for portability.