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

[Ferry Huberts]

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