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 <[email protected]> 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 <[email protected]> >>>>>>>> --- >>>>>>>> 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 _______________________________________________ cgit mailing list [email protected] http://hjemli.net/mailman/listinfo/cgit
