[PATCH 0/2] Compatibility fixes for non-Linux platforms

[PATCH 0/2] Compatibility fixes for non-Linux platforms

[cgit]

From: Lukas Fleischer <info at cryptocrack.de>

I just spotted these when trying to compile current master on OpenBSD.
Trivial to fix.

The stable branch doesn't seem to be affected as it doesn't contain any
of Ferry's and Lars's cgit_open_filter() changes.

Lukas Fleischer (2):
  shared.c: Remove unused "linux/limits.h" include
  shared.c: Only setenv() if value is non-null

 shared.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

-- 
1.7.6

[PATCH 1/2] shared.c: Remove unused "linux/limits.h" include

[cgit]

This isn't used anywhere and prevents the code from being compiled on
other platforms, such as *BSD.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 shared.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/shared.c b/shared.c
index 699c362..75c4b5c 100644
--- a/shared.c
+++ b/shared.c
@@ -8,7 +8,6 @@
 
 #include "cgit.h"
 #include <stdio.h>
-#include <linux/limits.h>
 
 struct cgit_repolist cgit_repolist;
 struct cgit_context ctx;
-- 
1.7.6

[PATCH 2/2] shared.c: Only setenv() if value is non-null

[cgit]

Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
segfault if we pass a NULL value. Add an additional check to avoid this.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 shared.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/shared.c b/shared.c
index 75c4b5c..0c8ce3e 100644
--- a/shared.c
+++ b/shared.c
@@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
 	p = env_vars;
 	q = p + env_var_count;
 	for (; p < q; p++)
-		if (setenv(p->name, p->value, 1))
+		if (p->value && setenv(p->name, p->value, 1))
 			fprintf(stderr, warn, p->name, p->value);
 }
 
-- 
1.7.6

[PATCH 0/2] Compatibility fixes for non-Linux platforms

[cgit]

On Fri, Jul 22, 2011 at 05:15:48PM +0200, Lukas Fleischer wrote:
> From: Lukas Fleischer <info at cryptocrack.de>

That happens if your main development box is down and you forgot to
setup Git properly on the second system *sigh*

Just ignore that...

> 
> I just spotted these when trying to compile current master on OpenBSD.
> Trivial to fix.
> 
> The stable branch doesn't seem to be affected as it doesn't contain any
> of Ferry's and Lars's cgit_open_filter() changes.
> 
> Lukas Fleischer (2):
>   shared.c: Remove unused "linux/limits.h" include
>   shared.c: Only setenv() if value is non-null
> 
>  shared.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> -- 
> 1.7.6

[PATCH 2/2] shared.c: Only setenv() if value is non-null

[mailings]

On 07/22/2011 05:15 PM, Lukas Fleischer wrote:
> Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
> segfault if we pass a NULL value. Add an additional check to avoid this.
> 
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
>  shared.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/shared.c b/shared.c
> index 75c4b5c..0c8ce3e 100644
> --- a/shared.c
> +++ b/shared.c
> @@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
>  	p = env_vars;
>  	q = p + env_var_count;
>  	for (; p < q; p++)
> -		if (setenv(p->name, p->value, 1))
> +		if (p->value && setenv(p->name, p->value, 1))
>  			fprintf(stderr, warn, p->name, p->value);
>  }
>  

Lukas,

I don't agree with this patch.
From cgitrc.5.txt, lines 501-503:

> If a setting is not defined for a repository and the corresponding global
> setting is also not defined (if applicable), then the corresponding
> environment variable will be an empty string.

So I'd like this differently, an empty string must be set.



-- 
Ferry Huberts

[PATCH 2/2] shared.c: Only setenv() if value is non-null

[cgit]

On Tue, Jul 26, 2011 at 01:18:14PM +0200, Ferry Huberts wrote:
> On 07/22/2011 05:15 PM, Lukas Fleischer wrote:
> > Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
> > segfault if we pass a NULL value. Add an additional check to avoid this.
> > 
> > Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> > ---
> >  shared.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/shared.c b/shared.c
> > index 75c4b5c..0c8ce3e 100644
> > --- a/shared.c
> > +++ b/shared.c
> > @@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
> >  	p = env_vars;
> >  	q = p + env_var_count;
> >  	for (; p < q; p++)
> > -		if (setenv(p->name, p->value, 1))
> > +		if (p->value && setenv(p->name, p->value, 1))
> >  			fprintf(stderr, warn, p->name, p->value);
> >  }
> >  
> 
> Lukas,
> 
> I don't agree with this patch.
> >From cgitrc.5.txt, lines 501-503:
> 
> > If a setting is not defined for a repository and the corresponding global
> > setting is also not defined (if applicable), then the corresponding
> > environment variable will be an empty string.
> 
> So I'd like this differently, an empty string must be set.

-1. Is there any reason to do this? Imho, we should leave undefined
variables unset and fix the documentation here. This is way more
straightforward and allows for distinguishing between unset and empty
values (this is cleaner, even though there might not be a use case yet).

Objections?

[PATCH 2/2] shared.c: Only setenv() if value is non-null

[mailings]

On 07/26/2011 03:32 PM, Lukas Fleischer wrote:
> On Tue, Jul 26, 2011 at 01:18:14PM +0200, Ferry Huberts wrote:
>> On 07/22/2011 05:15 PM, Lukas Fleischer wrote:
>>> Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
>>> segfault if we pass a NULL value. Add an additional check to avoid this.
>>>
>>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
>>> ---
>>>  shared.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/shared.c b/shared.c
>>> index 75c4b5c..0c8ce3e 100644
>>> --- a/shared.c
>>> +++ b/shared.c
>>> @@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
>>>  	p = env_vars;
>>>  	q = p + env_var_count;
>>>  	for (; p < q; p++)
>>> -		if (setenv(p->name, p->value, 1))
>>> +		if (p->value && setenv(p->name, p->value, 1))
>>>  			fprintf(stderr, warn, p->name, p->value);
>>>  }
>>>  
>>
>> Lukas,
>>
>> I don't agree with this patch.
>> >From cgitrc.5.txt, lines 501-503:
>>
>>> If a setting is not defined for a repository and the corresponding global
>>> setting is also not defined (if applicable), then the corresponding
>>> environment variable will be an empty string.
>>
>> So I'd like this differently, an empty string must be set.
> 
> -1. Is there any reason to do this? Imho, we should leave undefined
> variables unset and fix the documentation here. This is way more
> straightforward and allows for distinguishing between unset and empty
> values (this is cleaner, even though there might not be a use case yet).

Not true, I run this code on my servers.

I agree with the fact that it is cleaner but am reluctant to have this
change.

I can live with that but let's ask Lars what he thinks about this




-- 
Ferry Huberts

[PATCH 2/2] shared.c: Only setenv() if value is non-null

[cgit]

On Tue, Jul 26, 2011 at 04:24:14PM +0200, Ferry Huberts wrote:
> On 07/26/2011 03:32 PM, Lukas Fleischer wrote:
> > On Tue, Jul 26, 2011 at 01:18:14PM +0200, Ferry Huberts wrote:
> >> On 07/22/2011 05:15 PM, Lukas Fleischer wrote:
> >>> Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
> >>> segfault if we pass a NULL value. Add an additional check to avoid this.
> >>>
> >>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> >>> ---
> >>>  shared.c |    2 +-
> >>>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/shared.c b/shared.c
> >>> index 75c4b5c..0c8ce3e 100644
> >>> --- a/shared.c
> >>> +++ b/shared.c
> >>> @@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
> >>>  	p = env_vars;
> >>>  	q = p + env_var_count;
> >>>  	for (; p < q; p++)
> >>> -		if (setenv(p->name, p->value, 1))
> >>> +		if (p->value && setenv(p->name, p->value, 1))
> >>>  			fprintf(stderr, warn, p->name, p->value);
> >>>  }
> >>>  
> >>
> >> Lukas,
> >>
> >> I don't agree with this patch.
> >> >From cgitrc.5.txt, lines 501-503:
> >>
> >>> If a setting is not defined for a repository and the corresponding global
> >>> setting is also not defined (if applicable), then the corresponding
> >>> environment variable will be an empty string.
> >>
> >> So I'd like this differently, an empty string must be set.
> > 
> > -1. Is there any reason to do this? Imho, we should leave undefined
> > variables unset and fix the documentation here. This is way more
> > straightforward and allows for distinguishing between unset and empty
> > values (this is cleaner, even though there might not be a use case yet).
> 
> Not true, I run this code on my servers.
> 
> I agree with the fact that it is cleaner but am reluctant to have this
> change.

Wait, do you actually depend on having these environment variables
defined? Leaving environment variables unset instead of initializing
them to an empty string shouldn't make any difference if you use a shell
script (unless you check for it explicitly or do some fancy stuff).

If you use C (other any other programming language that uses getenv() or
behaves similarly), making your code compatible is as easy as modifying
the NULL check branch after getenv() to set the result to an empty
string instead of bailing out. It literally is a one-line diff.

Given that this is a real improvement and we only break backwards
compatibility for a few corner cases, +1 to changing this here.

> 
> I can live with that but let's ask Lars what he thinks about this

Ack.

[PATCH 2/2] shared.c: Only setenv() if value is non-null

[mailings]

On 07/26/2011 04:48 PM, Lukas Fleischer wrote:
> On Tue, Jul 26, 2011 at 04:24:14PM +0200, Ferry Huberts wrote:
>> On 07/26/2011 03:32 PM, Lukas Fleischer wrote:
>>> On Tue, Jul 26, 2011 at 01:18:14PM +0200, Ferry Huberts wrote:
>>>> On 07/22/2011 05:15 PM, Lukas Fleischer wrote:
>>>>> Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
>>>>> segfault if we pass a NULL value. Add an additional check to avoid this.
>>>>>
>>>>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
>>>>> ---
>>>>>  shared.c |    2 +-
>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/shared.c b/shared.c
>>>>> index 75c4b5c..0c8ce3e 100644
>>>>> --- a/shared.c
>>>>> +++ b/shared.c
>>>>> @@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
>>>>>  	p = env_vars;
>>>>>  	q = p + env_var_count;
>>>>>  	for (; p < q; p++)
>>>>> -		if (setenv(p->name, p->value, 1))
>>>>> +		if (p->value && setenv(p->name, p->value, 1))
>>>>>  			fprintf(stderr, warn, p->name, p->value);
>>>>>  }
>>>>>  
>>>>
>>>> Lukas,
>>>>
>>>> I don't agree with this patch.
>>>> >From cgitrc.5.txt, lines 501-503:
>>>>
>>>>> If a setting is not defined for a repository and the corresponding global
>>>>> setting is also not defined (if applicable), then the corresponding
>>>>> environment variable will be an empty string.
>>>>
>>>> So I'd like this differently, an empty string must be set.
>>>
>>> -1. Is there any reason to do this? Imho, we should leave undefined
>>> variables unset and fix the documentation here. This is way more
>>> straightforward and allows for distinguishing between unset and empty
>>> values (this is cleaner, even though there might not be a use case yet).
>>
>> Not true, I run this code on my servers.
>>
>> I agree with the fact that it is cleaner but am reluctant to have this
>> change.
> 
> Wait, do you actually depend on having these environment variables
> defined? Leaving environment variables unset instead of initializing
> them to an empty string shouldn't make any difference if you use a shell
> script (unless you check for it explicitly or do some fancy stuff).
> 

unless you use 'set -u', which I do.

> If you use C (other any other programming language that uses getenv() or
> behaves similarly), making your code compatible is as easy as modifying
> the NULL check branch after getenv() to set the result to an empty
> string instead of bailing out. It literally is a one-line diff.
> 
> Given that this is a real improvement and we only break backwards
> compatibility for a few corner cases, +1 to changing this here.

From my point of view it is an ABI change...

Granted, there don't seem to be many users of this yet but an ABI change
nonetheless



-- 
Ferry Huberts

[PATCH 2/2] shared.c: Only setenv() if value is non-null

[cgit]

On Tue, Jul 26, 2011 at 06:15:07PM +0200, Ferry Huberts wrote:
> On 07/26/2011 04:48 PM, Lukas Fleischer wrote:
> > On Tue, Jul 26, 2011 at 04:24:14PM +0200, Ferry Huberts wrote:
> >> On 07/26/2011 03:32 PM, Lukas Fleischer wrote:
> >>> On Tue, Jul 26, 2011 at 01:18:14PM +0200, Ferry Huberts wrote:
> >>>> On 07/22/2011 05:15 PM, Lukas Fleischer wrote:
> >>>>> Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
> >>>>> segfault if we pass a NULL value. Add an additional check to avoid this.
> >>>>>
> >>>>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> >>>>> ---
> >>>>>  shared.c |    2 +-
> >>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/shared.c b/shared.c
> >>>>> index 75c4b5c..0c8ce3e 100644
> >>>>> --- a/shared.c
> >>>>> +++ b/shared.c
> >>>>> @@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
> >>>>>  	p = env_vars;
> >>>>>  	q = p + env_var_count;
> >>>>>  	for (; p < q; p++)
> >>>>> -		if (setenv(p->name, p->value, 1))
> >>>>> +		if (p->value && setenv(p->name, p->value, 1))
> >>>>>  			fprintf(stderr, warn, p->name, p->value);
> >>>>>  }
> >>>>>  
> >>>>
> >>>> Lukas,
> >>>>
> >>>> I don't agree with this patch.
> >>>> >From cgitrc.5.txt, lines 501-503:
> >>>>
> >>>>> If a setting is not defined for a repository and the corresponding global
> >>>>> setting is also not defined (if applicable), then the corresponding
> >>>>> environment variable will be an empty string.
> >>>>
> >>>> So I'd like this differently, an empty string must be set.
> >>>
> >>> -1. Is there any reason to do this? Imho, we should leave undefined
> >>> variables unset and fix the documentation here. This is way more
> >>> straightforward and allows for distinguishing between unset and empty
> >>> values (this is cleaner, even though there might not be a use case yet).
> >>
> >> Not true, I run this code on my servers.
> >>
> >> I agree with the fact that it is cleaner but am reluctant to have this
> >> change.
> > 
> > Wait, do you actually depend on having these environment variables
> > defined? Leaving environment variables unset instead of initializing
> > them to an empty string shouldn't make any difference if you use a shell
> > script (unless you check for it explicitly or do some fancy stuff).
> > 
> 
> unless you use 'set -u', which I do.

In that case just use "${CGIT_*-}" or do some manual checking. Easy
enough to fix.

> > If you use C (other any other programming language that uses getenv() or
> > behaves similarly), making your code compatible is as easy as modifying
> > the NULL check branch after getenv() to set the result to an empty
> > string instead of bailing out. It literally is a one-line diff.
> > 
> > Given that this is a real improvement and we only break backwards
> > compatibility for a few corner cases, +1 to changing this here.
> 
> >From my point of view it is an ABI change...
> 
> Granted, there don't seem to be many users of this yet but an ABI change
> nonetheless

It is a minor ABI change, yeah. Well, let's let Lars decide. If he says
we're not gonna change this now, I'll send an amended patch.

[PATCH 2/2] shared.c: Only setenv() if value is non-null

[hjemli]

[My apologies for the extremly late reply]

On Tue, Jul 26, 2011 at 18:39, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> On Tue, Jul 26, 2011 at 06:15:07PM +0200, Ferry Huberts wrote:
>> On 07/26/2011 04:48 PM, Lukas Fleischer wrote:
>> > On Tue, Jul 26, 2011 at 04:24:14PM +0200, Ferry Huberts wrote:
>> >> On 07/26/2011 03:32 PM, Lukas Fleischer wrote:
>> >>> On Tue, Jul 26, 2011 at 01:18:14PM +0200, Ferry Huberts wrote:
>> >>>> On 07/22/2011 05:15 PM, Lukas Fleischer wrote:
>> >>>>> Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
>> >>>>> segfault if we pass a NULL value. Add an additional check to avoid this.
>> >>>>>
>> >>>>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
>> >>>>> ---
>> >>>>> ?shared.c | ? ?2 +-
>> >>>>> ?1 files changed, 1 insertions(+), 1 deletions(-)
>> >>>>>
>> >>>>> diff --git a/shared.c b/shared.c
>> >>>>> index 75c4b5c..0c8ce3e 100644
>> >>>>> --- a/shared.c
>> >>>>> +++ b/shared.c
>> >>>>> @@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
>> >>>>> ? ? ? ? p = env_vars;
>> >>>>> ? ? ? ? q = p + env_var_count;
>> >>>>> ? ? ? ? for (; p < q; p++)
>> >>>>> - ? ? ? ? ? ? ? if (setenv(p->name, p->value, 1))
>> >>>>> + ? ? ? ? ? ? ? if (p->value && setenv(p->name, p->value, 1))
>> >>>>> ? ? ? ? ? ? ? ? ? ? ? ? fprintf(stderr, warn, p->name, p->value);
>> >>>>> ?}
>> >>>>>
>> >>>>
>> >>>> Lukas,
>> >>>>
>> >>>> I don't agree with this patch.
>> >>>> >From cgitrc.5.txt, lines 501-503:
>> >>>>
>> >>>>> If a setting is not defined for a repository and the corresponding global
>> >>>>> setting is also not defined (if applicable), then the corresponding
>> >>>>> environment variable will be an empty string.
>> >>>>
>> >>>> So I'd like this differently, an empty string must be set.
>> >>>
>> >>> -1. Is there any reason to do this? Imho, we should leave undefined
>> >>> variables unset and fix the documentation here. This is way more
>> >>> straightforward and allows for distinguishing between unset and empty
>> >>> values (this is cleaner, even though there might not be a use case yet).
>> >>
>> >> Not true, I run this code on my servers.
>> >>
>> >> I agree with the fact that it is cleaner but am reluctant to have this
>> >> change.
>> >
>> > Wait, do you actually depend on having these environment variables
>> > defined? Leaving environment variables unset instead of initializing
>> > them to an empty string shouldn't make any difference if you use a shell
>> > script (unless you check for it explicitly or do some fancy stuff).
>> >
>>
>> unless you use 'set -u', which I do.
>
> In that case just use "${CGIT_*-}" or do some manual checking. Easy
> enough to fix.
>
>> > If you use C (other any other programming language that uses getenv() or
>> > behaves similarly), making your code compatible is as easy as modifying
>> > the NULL check branch after getenv() to set the result to an empty
>> > string instead of bailing out. It literally is a one-line diff.
>> >
>> > Given that this is a real improvement and we only break backwards
>> > compatibility for a few corner cases, +1 to changing this here.
>>
>> >From my point of view it is an ABI change...
>>
>> Granted, there don't seem to be many users of this yet but an ABI change
>> nonetheless
>
> It is a minor ABI change, yeah. Well, let's let Lars decide. If he says
> we're not gonna change this now, I'll send an amended patch.
>

Since the feature which this patch fixes hasn't been included in any
released version yet, I don't think the ABI-argument is very important
(no disrespect intended). So, we're free to tweak the feature into
perfection and I think Lukas' patch is a nice step in that direction.
But cgitrc.5 also needs updating...

-- 
larsh

[PATCH 2/2] shared.c: Only setenv() if value is non-null

[mailings]

On 09/14/2011 09:09 AM, Lars Hjemli wrote:
> [My apologies for the extremly late reply]
> 
> On Tue, Jul 26, 2011 at 18:39, Lukas Fleischer <cgit at cryptocrack.de> wrote:
>> On Tue, Jul 26, 2011 at 06:15:07PM +0200, Ferry Huberts wrote:
>>> On 07/26/2011 04:48 PM, Lukas Fleischer wrote:
>>>> On Tue, Jul 26, 2011 at 04:24:14PM +0200, Ferry Huberts wrote:
>>>>> On 07/26/2011 03:32 PM, Lukas Fleischer wrote:
>>>>>> On Tue, Jul 26, 2011 at 01:18:14PM +0200, Ferry Huberts wrote:
>>>>>>> On 07/22/2011 05:15 PM, Lukas Fleischer wrote:
>>>>>>>> Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
>>>>>>>> segfault if we pass a NULL value. Add an additional check to avoid this.
>>>>>>>>
>>>>>>>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
>>>>>>>> ---
>>>>>>>>  shared.c |    2 +-
>>>>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/shared.c b/shared.c
>>>>>>>> index 75c4b5c..0c8ce3e 100644
>>>>>>>> --- a/shared.c
>>>>>>>> +++ b/shared.c
>>>>>>>> @@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
>>>>>>>>         p = env_vars;
>>>>>>>>         q = p + env_var_count;
>>>>>>>>         for (; p < q; p++)
>>>>>>>> -               if (setenv(p->name, p->value, 1))
>>>>>>>> +               if (p->value && setenv(p->name, p->value, 1))
>>>>>>>>                         fprintf(stderr, warn, p->name, p->value);
>>>>>>>>  }
>>>>>>>>
>>>>>>>
>>>>>>> Lukas,
>>>>>>>
>>>>>>> I don't agree with this patch.
>>>>>>> >From cgitrc.5.txt, lines 501-503:
>>>>>>>
>>>>>>>> If a setting is not defined for a repository and the corresponding global
>>>>>>>> setting is also not defined (if applicable), then the corresponding
>>>>>>>> environment variable will be an empty string.
>>>>>>>
>>>>>>> So I'd like this differently, an empty string must be set.
>>>>>>
>>>>>> -1. Is there any reason to do this? Imho, we should leave undefined
>>>>>> variables unset and fix the documentation here. This is way more
>>>>>> straightforward and allows for distinguishing between unset and empty
>>>>>> values (this is cleaner, even though there might not be a use case yet).
>>>>>
>>>>> Not true, I run this code on my servers.
>>>>>
>>>>> I agree with the fact that it is cleaner but am reluctant to have this
>>>>> change.
>>>>
>>>> Wait, do you actually depend on having these environment variables
>>>> defined? Leaving environment variables unset instead of initializing
>>>> them to an empty string shouldn't make any difference if you use a shell
>>>> script (unless you check for it explicitly or do some fancy stuff).
>>>>
>>>
>>> unless you use 'set -u', which I do.
>>
>> In that case just use "${CGIT_*-}" or do some manual checking. Easy
>> enough to fix.
>>
>>>> If you use C (other any other programming language that uses getenv() or
>>>> behaves similarly), making your code compatible is as easy as modifying
>>>> the NULL check branch after getenv() to set the result to an empty
>>>> string instead of bailing out. It literally is a one-line diff.
>>>>
>>>> Given that this is a real improvement and we only break backwards
>>>> compatibility for a few corner cases, +1 to changing this here.
>>>
>>> >From my point of view it is an ABI change...
>>>
>>> Granted, there don't seem to be many users of this yet but an ABI change
>>> nonetheless
>>
>> It is a minor ABI change, yeah. Well, let's let Lars decide. If he says
>> we're not gonna change this now, I'll send an amended patch.
>>
> 
> Since the feature which this patch fixes hasn't been included in any
> released version yet, I don't think the ABI-argument is very important
> (no disrespect intended). So, we're free to tweak the feature into
> perfection and I think Lukas' patch is a nice step in that direction.
> But cgitrc.5 also needs updating...
> 

I already anticipated your reply and updated my server scripting :-)

I'll let Lukas send in a patch updating the documentation (don't forget
the example scripts) ;-)

-- 
Ferry Huberts

[PATCH] shared.c: Only setenv() if value is non-null

[cgit]

Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
segfault if we pass a NULL value. Only set environment variables if the
corresponding settings are defined to avoid this.

Note that this is a minor behaviour change as environment variables were
supposed to be set to an empty string if a setting was undefined. Given
that this feature isn't part of any official release yet, there's no
need to worry about backwards compatibility, really. Change the
documentation accordingly.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 cgitrc.5.txt |    2 +-
 shared.c     |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 4721c1e..a22423b 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -500,7 +500,7 @@ Also, all filters are handed the following environment variables:
 
 If a setting is not defined for a repository and the corresponding global
 setting is also not defined (if applicable), then the corresponding
-environment variable will be an empty string.
+environment variable will be unset.
 
 
 MACRO EXPANSION
diff --git a/shared.c b/shared.c
index 75c4b5c..0c8ce3e 100644
--- a/shared.c
+++ b/shared.c
@@ -392,7 +392,7 @@ void cgit_prepare_repo_env(struct cgit_repo * repo)
 	p = env_vars;
 	q = p + env_var_count;
 	for (; p < q; p++)
-		if (setenv(p->name, p->value, 1))
+		if (p->value && setenv(p->name, p->value, 1))
 			fprintf(stderr, warn, p->name, p->value);
 }
 
-- 
1.7.6.1

[PATCH 1/2] shared.c: Remove unused "linux/limits.h" include

[hjemli]

On Fri, Jul 22, 2011 at 17:15, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> This isn't used anywhere and prevents the code from being compiled on
> other platforms, such as *BSD.

Thanks, this has finally been applied.

--
larsh

[PATCH] shared.c: Only setenv() if value is non-null

[hjemli]

On Wed, Sep 14, 2011 at 11:52, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> Some setenv() implementations (e.g. the one in OpenBSD's stdlib)
> segfault if we pass a NULL value. Only set environment variables if the
> corresponding settings are defined to avoid this.

Thanks, also finally applied.

--
larsh

[PATCH] shared.c: Only setenv() if value is non-null

[mailings]

Lars!

Happy you're back!


Ferry