patchUse a default for a bad env config file variable

Hi git list, my name is James, and this is my first patch.

It's quite trivial really, all that changes is that if someone sets the environment variable: $GITWEB_CONFIG_SYSTEM, and this points to something like: /srv/gitosis/gitweb.conf, which doesn't actually exist, then gitweb will default to trying out the built in default of /etc/gitweb.conf (if it was built with that var).

This patch should make it easier for people who are configuring gitweb+gitosis, so that a separate gitweb.conf config file can be used to call the main config, but which doesn't null out the system defaults if it is missing.

Thank you in advance, _James

From d29adf8c788b8a747bfd38dd7e10f684de9aa8e9 Mon Sep 17 00:00:00 2001
From: James Shubin <purpleidea@gmail.com>
Date: Tue, 10 Aug 2010 10:30:22 -0400
Subject: [PATCH] Use a default for a bad env config file variable.

---
 gitweb/gitweb.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Hi,

James <purpleidea@gmail.com> writes:

Then, I'll have to be the first person directing you to Documentation/SubmittingPatches ;-).

The custom here is to send patches inline (git send-email can help), not attached. Read about Signed-off-by too.

(Don't know perl and gitweb enough to judge on the content unfortunately, sorry)

Sorry about that, I guess I had only read the README. Hope this is better:

From: James Shubin <purpleidea@gmail.com>
Date: Tue, 10 Aug 2010 10:30:22 -0400
Subject: [PATCH] Use a default for a bad env config file variable.

Signed-off-by: James Shubin <purpleidea@gmail.com>
---
 gitweb/gitweb.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

On Tue, Aug 10, 2010 at 15:54, James <purpleidea@gmail.com> wrote:

Not really, no. You should send the patches you produce with git-format-patch with git-send-email, and try sending to yourself first and apply it with git-am (this is all mentioned in SubmittingPatches).

This is what your new patch looks like after being applied with git-am:

    commit 7be6207e8923cd7c4c48243f5257a0fdba6bfa0a
    Author: James <purpleidea@gmail.com>
    Date:   Tue Aug 10 11:54:43 2010 -0400

        Use a default for a bad env config file variable

        Sorry about that,
        I guess I had only read the README.
        Hope this is better:

        From d29adf8c788b8a747bfd38dd7e10f684de9aa8e9 Mon Sep 17 00:00:00 2001
        From: James Shubin <purpleidea@gmail.com>
        Date: Tue, 10 Aug 2010 10:30:22 -0400
        Subject: [PATCH] Use a default for a bad env config file variable.

        Signed-off-by: James Shubin <purpleidea@gmail.com>

    diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
    index 4efeebc..43294e1 100755
    --- a/gitweb/gitweb.perl
    +++ b/gitweb/gitweb.perl
    @@ -605,6 +605,10 @@ sub evaluate_gitweb_config {
            } elsif (-e $GITWEB_CONFIG_SYSTEM) {
                    do $GITWEB_CONFIG_SYSTEM;
                    die $@ if $@;
    +       # if config file from env is missing, then try the default anyways
    +       } elsif (-e "++GITWEB_CONFIG_SYSTEM++") {
    +               do "++GITWEB_CONFIG_SYSTEM++";
    +               die $@ if $@;
            }
     }

I.e. your message has become part of the patch. To include commentary on resend add it after -- and before the diffstat (also in SubmittingPatches).

Thanks.

From: James Shubin <purpleidea@gmail.com>
Signed-off-by: James Shubin <purpleidea@gmail.com>
---
It's quite trivial really, all that changes is that if someone sets
the environment variable: $GITWEB_CONFIG_SYSTEM, and this points to
something like: /srv/gitosis/gitweb.conf, which doesn't actually
exist, then gitweb will default to trying out the built in default of
/etc/gitweb.conf (if it was built with that value).

This patch should make it easier for people who are configuring gitweb+gitosis, so that a separate gitweb.conf config file can be used to call the main config, but which doesn't null out the system defaults if it is missing.

PS: thanks to everyone for their patience with my first patch.
This applied cleanly with git am, let me know if I should do anything
else differently.

 gitweb/gitweb.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

James Shubin <purpleidea@gmail.com> writes:

Small nitpick: you need line like the above only if the From: header in your email is diferent from the authorship you want to have in commit, i.e. when you are sending email from other email account, or when you are (re)sending someone's else patches.

In this patch situation it is not, I think, necessary.

First, why it is needed? Why can't you just have GITWEB_CONFIG_SYSTEM (or GITWEB_CONFIG) environment variable visible to gitweb.cgi that points to existing file?

Second, is there any history behind providing this fallback only for $GITWEB_CONFIG_SYSTEM variable, and not for $GITWEB_CONFIG? Currently gitweb use environment variable if it exists, falling back to build-time value (might be default), and using first of $GITWEB_CONFIG and $GITWEB_CONFIG_SYSTEM that exists.

Minor nitpick: Actually that is not default, but build-time value, which has a default.

Thanks for all the "sending patches" input; it's useful, even if my patch isn't ;) I suppose I was trying to provide more defaults to work around some weirdly setup servers that I have inherited. Upon reconsideration, I'll withdraw the patch. Thanks for your time, and thanks for your nitpicks.

_James

-----Original Message-----

Second, is there any history behind providing this fallback only for $GITWEB_CONFIG_SYSTEM variable, and not for $GITWEB_CONFIG? Currently gitweb use environment variable if it exists, falling back to build-time value (might be default), and using first of $GITWEB_CONFIG and $GITWEB_CONFIG_SYSTEM that exists.

Minor nitpick: Actually that is not default, but build-time value, which has a default.

Hi James,

James Shubin wrote:

[...]

[...]
|	our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
|	our $GITWEB_CONFIG_SYSTEM = $ENV{'GITWEB_CONFIG_SYSTEM'} || "++GITWEB_CONFIG_SYSTEM++"; # die if there are errors parsing config file
|	if (-e $GITWEB_CONFIG) {
|		do $GITWEB_CONFIG;
|		die $@ if $@;
|	} elsif (-e $GITWEB_CONFIG_SYSTEM) {
|		do $GITWEB_CONFIG_SYSTEM;
|		die $@ if $@;
|	} elsif (-e "++GITWEB_CONFIG_SYSTEM++") {
|		...

Interesting. I am a bit nervous that this might be confusing. cc-ing Jakub for input.

James venit, vidit, dixit 10.08.2010 17:54:

Now, make that commit subject

gitweb: Use a default for a bad env config file variable

(i.e. insert "gitweb: ", delete the stop) and you're good to go ;)

Michael