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