[PATCH] Add compat/setenv.c, use in git.c.

[PATCH] Add compat/setenv.c, use in git.c.

[Jason Riedy]

There is no setenv() in Solaris 5.8.  The trivial calls to
setenv() were replaced by putenv() in a much earlier patch,
but setenv() was used again in git.c.  This patch just adds
a compat/setenv.c.

The rule for building git$(X) also needs to include compat.
objects and compiler flags.  Those are now in makefile vars
COMPAT_OBJS and COMPAT_CFLAGS.

Signed-off-by: E. Jason Riedy <ejr@cs.berkeley.edu>

---

 Makefile        |   27 ++++++++++++++++++---------
 compat/setenv.c |   31 +++++++++++++++++++++++++++++++
 git.c           |    4 ++++
 3 files changed, 53 insertions(+), 9 deletions(-)
 create mode 100644 compat/setenv.c

applies-to: fc206c1ad60cfb6481cc51f6737ce728d1d8d83f
4bbc94c80d34b1ef859dd92254284198b0c3c2b6
diff --git a/Makefile b/Makefile
index 45db357..df3c6eb 100644
--- a/Makefile
+++ b/Makefile
@@ -21,6 +21,8 @@ all:
 #
 # Define NO_STRCASESTR if you don't have strcasestr.
 #
+# Define NO_SETENV if you don't have setenv in the C library.
+#
 # Define PPC_SHA1 environment variable when running make to make use of
 # a bundled SHA1 routine optimized for PowerPC.
 #
@@ -194,6 +196,7 @@ shellquote = '$(call shq,$(1))'
 uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
 uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')
 uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')
+uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
 
 ifeq ($(uname_S),Darwin)
 	NEEDS_SSL_WITH_CRYPTO = YesPlease
@@ -211,6 +214,9 @@ ifeq ($(uname_S),SunOS)
 	NEEDS_LIBICONV = YesPlease
 	SHELL_PATH = /bin/bash
 	NO_STRCASESTR = YesPlease
+	ifeq ($(uname_R),5.8)
+		NO_SETENV = YesPlease
+	endif
 	INSTALL = ginstall
 	TAR = gtar
 	ALL_CFLAGS += -D__EXTENSIONS__
@@ -314,12 +320,16 @@ ifdef NEEDS_NSL
 	SIMPLE_LIB += -lnsl
 endif
 ifdef NO_STRCASESTR
-	ALL_CFLAGS += -Dstrcasestr=gitstrcasestr -DNO_STRCASESTR=1
-	LIB_OBJS += compat/strcasestr.o
+	COMPAT_CFLAGS += -Dstrcasestr=gitstrcasestr -DNO_STRCASESTR=1
+	COMPAT_OBJS += compat/strcasestr.o
+endif
+ifdef NO_SETENV
+	COMPAT_CFLAGS += -Dsetenv=gitsetenv -DNO_SETENV=1
+	COMPAT_OBJS += compat/setenv.o
 endif
 ifdef NO_MMAP
-	ALL_CFLAGS += -Dmmap=gitfakemmap -Dmunmap=gitfakemunmap -DNO_MMAP
-	LIB_OBJS += compat/mmap.o
+	COMPAT_CFLAGS += -Dmmap=gitfakemmap -Dmunmap=gitfakemunmap -DNO_MMAP
+	COMPAT_OBJS += compat/mmap.o
 endif
 ifdef NO_IPV6
 	ALL_CFLAGS += -DNO_IPV6 -Dsockaddr_storage=sockaddr_in
@@ -343,8 +353,8 @@ endif
 endif
 endif
 
-ALL_CFLAGS += -DSHA1_HEADER=$(call shellquote,$(SHA1_HEADER))
-
+ALL_CFLAGS += -DSHA1_HEADER=$(call shellquote,$(SHA1_HEADER)) $(COMPAT_CFLAGS)
+LIB_OBJS += $(COMPAT_OBJS)
 export prefix TAR INSTALL DESTDIR SHELL_PATH template_dir
 ### Build rules
 
@@ -353,10 +363,9 @@ all: $(ALL_PROGRAMS)
 all:
 	$(MAKE) -C templates
 
-# Only use $(CFLAGS). We don't need anything else.
-git$(X): git.c Makefile
+git$(X): git.c $(COMPAT_OBJS) Makefile
 	$(CC) -DGIT_EXEC_PATH='"$(bindir)"' -DGIT_VERSION='"$(GIT_VERSION)"' \
-		$(CFLAGS) $< -o $@
+		$(CFLAGS) $(COMPAT_CFLAGS) -o $@ $(filter %.c,$^) $(filter %.o,$^)
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 	rm -f $@
diff --git a/compat/setenv.c b/compat/setenv.c
new file mode 100644
index 0000000..94acd2d
--- /dev/null
+++ b/compat/setenv.c
@@ -0,0 +1,31 @@
+#include <stdlib.h>
+#include <string.h>
+
+int gitsetenv(const char *name, const char *value, int replace)
+{
+	int out;
+	size_t namelen, valuelen;
+	char *envstr;
+
+	if (!name || !value) return -1;
+	if (!replace) {
+		char *oldval = NULL;
+		oldval = getenv(name);
+		if (oldval) return 0;
+	}
+
+	namelen = strlen(name);
+	valuelen = strlen(value);
+	envstr = malloc((namelen + valuelen + 2) * sizeof(char));
+	if (!envstr) return -1;
+
+	memcpy(envstr, name, namelen);
+	envstr[namelen] = '=';
+	memcpy(envstr + namelen + 1, value, valuelen);
+	envstr[namelen + valuelen + 1] = 0;
+
+	out = putenv(envstr);
+
+	free(envstr);
+	return out;
+}
diff --git a/git.c b/git.c
index 878c359..619f25a 100644
--- a/git.c
+++ b/git.c
@@ -13,6 +13,10 @@
 # define PATH_MAX 4096
 #endif
 
+#ifdef NO_SETENV
+extern int gitsetenv(char *name, char *value, int overwrite);
+#endif
+
 static const char git_usage[] =
 	"Usage: git [--version] [--exec-path[=GIT_EXEC_PATH]] [--help] COMMAND [ ARGS ]";
 
---
0.99.9h

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Junio C Hamano]

Jason Riedy <ejr@EECS.Berkeley.EDU> writes:

> The rule for building git$(X) also needs to include compat.
> objects and compiler flags.  Those are now in makefile vars
> COMPAT_OBJS and COMPAT_CFLAGS.

Thanks for the cleanup.  Looks much saner.

Re: [PATCH] Add compat/setenv.c, use in git.c.

[H. Peter Anvin]

Jason Riedy wrote:
> +
> +int gitsetenv(const char *name, const char *value, int replace)
> +{
> +	int out;
> +	size_t namelen, valuelen;
> +	char *envstr;
> +
> +	if (!name || !value) return -1;
> +	if (!replace) {
> +		char *oldval = NULL;
> +		oldval = getenv(name);
> +		if (oldval) return 0;
> +	}
> +
> +	namelen = strlen(name);
> +	valuelen = strlen(value);
> +	envstr = malloc((namelen + valuelen + 2) * sizeof(char));
> +	if (!envstr) return -1;
> +
> +	memcpy(envstr, name, namelen);
> +	envstr[namelen] = '=';
> +	memcpy(envstr + namelen + 1, value, valuelen);
> +	envstr[namelen + valuelen + 1] = 0;
> +
> +	out = putenv(envstr);
> +
> +	free(envstr);
> +	return out;
> +}

Wouldn't this be a good case for using alloca()?

	-hpa

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Wouldn't this be a good case for using alloca()?

Perhaps, but considering that (1) this function is not something
frequently called anyway, and (2) the proposed change would make
it the first alloca() user, and (3) this is compatibility
replacement function, I'd rather choose to keep it "old, known
to work at more places" malloc/free pair, and not having to
worry about it.

But now you quote the patch, sizeof(char) looks funny.  Isn't it
always 1 by definition?

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Jason Riedy wrote:
>> +
>> +int gitsetenv(const char *name, const char *value, int replace)
>> +{
>> +...
>> +	envstr = malloc((namelen + valuelen + 2) * sizeof(char));
>> +...
>> +	out = putenv(envstr);
>> +
>> +	free(envstr);
>> +	return out;
>> +}
>
> Wouldn't this be a good case for using alloca()?

Oops.  Isn't the patch itself wrong, so is using alloca()?

putenv(3) says

	int putenv(char *string);

	The string pointed to by string becomes part of the environment,
	so altering the string changes the environment.

which tell sme that whatever we pass to putenv() we should *not*
free.

[PATCH] compat/setenv: do not free what we fed putenv(3).

[Junio C Hamano]

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 compat/setenv.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

3a2674337c12e958f8c697af991a0ef6c06ddd4d
diff --git a/compat/setenv.c b/compat/setenv.c
index 94acd2d..b7d7678 100644
--- a/compat/setenv.c
+++ b/compat/setenv.c
@@ -16,7 +16,7 @@ int gitsetenv(const char *name, const ch
 
 	namelen = strlen(name);
 	valuelen = strlen(value);
-	envstr = malloc((namelen + valuelen + 2) * sizeof(char));
+	envstr = malloc((namelen + valuelen + 2));
 	if (!envstr) return -1;
 
 	memcpy(envstr, name, namelen);
@@ -25,7 +25,11 @@ int gitsetenv(const char *name, const ch
 	envstr[namelen + valuelen + 1] = 0;
 
 	out = putenv(envstr);
+	/* putenv(3) makes the argument string part of the environment,
+	 * and changing that string modifies the environment --- which
+	 * means we do not own that storage anymore.  Do not free
+	 * envstr.
+	 */
 
-	free(envstr);
 	return out;
 }
-- 
0.99.9.GIT

Re: [PATCH] Add compat/setenv.c, use in git.c.

[H. Peter Anvin]

Junio C Hamano wrote:
> 
> Oops.  Isn't the patch itself wrong, so is using alloca()?
> 
> putenv(3) says
> 
> 	int putenv(char *string);
> 
> 	The string pointed to by string becomes part of the environment,
> 	so altering the string changes the environment.
> 
> which tell sme that whatever we pass to putenv() we should *not*
> free.

Indeed.

	-hpa

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Jason Riedy]

And Junio C Hamano writes:
 - putenv(3) says
 - 	The string pointed to by string becomes part of the environment,
 - 	so altering the string changes the environment.

Good catch, thanks.  The Solaris man page first says the 
string space is "no longer used", but at the very end warns 
against using an automatic variable.  Chalk one up for lousy 
docs.

(And sizeof(char) is just habit; 99% of my mallocs aren't of 
char...  Sorry.)

Jason

Re: [PATCH] Add compat/setenv.c, use in git.c.

[H. Peter Anvin]

Jason Riedy wrote:
> And Junio C Hamano writes:
>  - putenv(3) says
>  - 	The string pointed to by string becomes part of the environment,
>  - 	so altering the string changes the environment.
> 
> Good catch, thanks.  The Solaris man page first says the 
> string space is "no longer used", but at the very end warns 
> against using an automatic variable.  Chalk one up for lousy 
> docs.
> 
> (And sizeof(char) is just habit; 99% of my mallocs aren't of 
> char...  Sorry.)
> 

Personally I consider it a serious bug in the C language that 
sizeof(char) == 1 by definition.  It basically prohibits a whole lot of 
useful machine models.

	-hpa

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Junio C Hamano]

Jason Riedy <ejr@EECS.Berkeley.EDU> writes:

> And Junio C Hamano writes:
>  - putenv(3) says
>  - 	The string pointed to by string becomes part of the environment,
>  - 	so altering the string changes the environment.
>
> Good catch, thanks.  The Solaris man page first says the 
> string space is "no longer used", but at the very end warns 
> against using an automatic variable.  Chalk one up for lousy 
> docs.

Same thing for 5.9.

With the "compat update" patch from last night, I managed to
build on a borrowed sparc with Solaris 5.9, but I needed
NO_SETENV myself.  I'd like to throw in another Makefile patch
to catch both 5.8 and 5.9 for this, but would appreciate if
people with various vintage of Solaris boxes can give some
inputs before doing that.

This was done with somewhat stripped down configuration.  I had
to say NO_EXPAT, libiconv was needed but iconv and openssl were
installed at nonstandard places, python was 2.3 so I also said
PYTHON_PATH and WITH_OWN_SUBPROCESS_PY from the make command
line.  But the borrowed box is not what I administer myself, so
I declare victory for now.

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Jason Riedy]

And Junio C Hamano writes:
 - I'd like to throw in another Makefile patch to catch both 5.8 and 
 - 5.9 for this, but would appreciate if people with various vintage 
 - of Solaris boxes can give some inputs before doing that.

Anyone else seeing the qsort in object.c trash memory?  The
input _looks_ correct.  I only see this on Solaris (8), but 
I've never had a problem with qsort there.  I think we have 
a Solaris 9 box around here somewhere, but probably only
partially 9.  The admins here are too busy fixing Windows to
maintain anything else properly.

 - This was done with somewhat stripped down configuration.

Once I replace the qsort (with an insertion sort), almost all 
the tests work with my odd-ball but full configuration.  I'm 
having some problems with cpio in the push tests, but it's 
just some path issue for my setup.  When I run the same 
commands by hand, they work.

I'll test this on AIX again soon.

Jason

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Morten Welinder]

The code looks wrong.  It assumes that pointers are no larger than ints.
If pointers are larger than ints, the code does not necessarily compute
a consistent ordering and qsort is allowed to do whatever it wants.

Morten



static int compare_object_pointers(const void *a, const void *b)
{
	const struct object * const *pa = a;
	const struct object * const *pb = b;
	return *pa - *pb;
}

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Junio C Hamano]

Morten Welinder <mwelinder@gmail.com> writes:

> The code looks wrong.  It assumes that pointers are no larger than ints.
> If pointers are larger than ints, the code does not necessarily compute
> a consistent ordering and qsort is allowed to do whatever it wants.
>
> Morten
>
>
>
> static int compare_object_pointers(const void *a, const void *b)
> {
> 	const struct object * const *pa = a;
> 	const struct object * const *pb = b;
> 	return *pa - *pb;
> }

Are you suggesting it to be done like this?

---
git diff
diff --git a/object.c b/object.c
index 427e14c..dac5c92 100644
--- a/object.c
+++ b/object.c
@@ -82,7 +82,12 @@ static int compare_object_pointers(const
 {
 	const struct object * const *pa = a;
 	const struct object * const *pb = b;
-	return *pa - *pb;
+	if (*pa == *pb)
+		return 0;
+	else if (*pa < *pb)
+		return -1;
+	else
+		return 1;
 }
 
 void set_object_refs(struct object *obj, struct object_refs *refs)

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Jason Riedy]

And Morten Welinder writes:
 - The code looks wrong.  It assumes that pointers are no larger than ints.

The failure's in 32-bit mode, so they _are_ the same size.
The difference is no more than 0x80 in the tests.

But you're right; the result is implementation defined in
C99.

And Junio C Hamano writes:
 - Are you suggesting it to be done like this?

Your change fixed it.  Oddly enough, using arithmetic in
ptrdiff_t then testing against zero didn't...  I don't
have time to dig through assembly or try different gcc
versions to figure out the real problem.  ugh.

Jason

Re: [PATCH] Add compat/setenv.c, use in git.c.

[Morten Welinder]

> Are you suggesting it to be done like this?

Precisely.

Morten