git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 6/6] GITWEB - Separate defaults from main file

From
Jakub Narebski <jnareb@gmail.com>
Date
Dec 11, 2009, 22:53 UTC
Message-ID
<200912112353.11034.jnareb@gmail.com>
In-Reply-To
<4B226C0F.2070407@kernel.org>
On Fri, 11 Dec 2009, J.H. wrote:
Show 12 quoted lines
>>> This is also a not-so-subtle start of trying to break up gitweb into
>>> separate files for easier maintainability, having everything in a
>>> single file is just a mess and makes the whole thing more complicated
>>> than it needs to be.  This is a bit of a baby step towards breaking it
>>> up for easier maintenance.
>> 
>> The question is if easier maintenance and development by spliting
>> gitweb for developers offsets ease of install for users.
> 
> This would just get dropped into the same location that gitweb.cgi 
> exists in, there is no real difference in installation, and thus I can't 
> see this as an issue for users.

To be more exact you have to know that you have to drop _generated files_, which means (for this version of patch) gitweb.cgi and gitweb_defaults.pl (or whatever the generated file with config variables would be named).

ATTENTION!

Your changes would make stop working all gitweb tests. Currently they do some magic with generated gitweb config file "$(pwd)/gitweb_config.perl" set via GITWEB_CONFIG configuration variable to be able to run _unprocessed_ gitweb/gitweb.perl (without any substitutions). This allow to run tests on "live" version of gitweb.

After your changes it would be no longer possible, at least not if we want to be sure that we test the same version of gitweb as gitweb_defaults.

It would probably mean that we need to move to testing built version, i.e. gitweb.cgi, not gitweb.perl

Show 28 quoted lines
>>> ---
>>>  .gitignore                  |    1 +
>>>  Makefile                    |   15 +-
>>>  gitweb/Makefile             |    2 +-
>>>  gitweb/gitweb.perl          |  515 +++++--------------------------------------
>>>  gitweb/gitweb_defaults.perl |  468 +++++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 537 insertions(+), 464 deletions(-)
>>>  create mode 100644 gitweb/gitweb_defaults.perl
>>>
>>>
>>> diff --git a/.gitignore b/.gitignore
>>> index ac02a58..5e48102 100644
>>> --- a/.gitignore
>>> +++ b/.gitignore
>>> @@ -151,6 +151,7 @@
>>>  /git-core-*/?*
>>>  /gitk-git/gitk-wish
>>>  /gitweb/gitweb.cgi
>>> +/gitweb/gitweb_defaults.pl
>> 
>> Hmmm... gitweb/gitweb_defaults.perl as source file, and
>> gitweb/gitweb_defaults.pl as generated file?  Wouldn't it be better to
>> go with the convention used elsewhere in gitweb and use
>> gitweb/gitweb_defaults.perl.in or gitweb/gitweb_defaults.pl.in as
>> source file?
> 
> I think you got confused, the committed file is .perl the generated file 
> is .pl.

Maybe I wasn't entirely clean. I meant that the committed source file should perhaps have *.in extension to denote that it is to be processed via variable substitution, so it would be

  committed file: gitweb/gitweb_defaults.pl.in
  generated file: gitweb/gitweb_defaults.pl
 
or whatever name (*.pm?) we agree on.
Show 18 quoted lines
>>>  	$(QUIET_GEN)$(RM) $@ $@+ && \
>>>  	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
>>>  	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
>>> @@ -1539,7 +1541,7 @@ endif
>>>  	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
>>>  	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
>>>  	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
>>> -	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
>>> +	    $(patsubst %.cgi,%.perl,$(patsubst %.pl, %.perl, $@)) >$@+ && \
>> 
>> Why the slightly inconsistent style ("%.cgi,%perl" vs "%.pl, %perl")?
> 
> Considering that the defaults is more of an include vs. a cgi it 
> probably shouldn't share the standard expected executable suffix, thus I 
> used .pl.  Could just as easily change it to .pm, or something else but 
> I think it would make the most sense to leave things we are expecting 
> the webserver to directly execute as .cgi, and includes as a different 
> suffix.
I was not asking about that, but about
+	    $(patsubst %.cgi,%.perl,$(patsubst %.pl, %.perl, $@)) >$@+ && \
vs
+	    $(patsubst %.cgi,%.perl,$(patsubst %.pl,%.perl,$@)) >$@+ && \

But after thinking about it a bit, and consulting make documentation (in particular definition of $@ variable) this rule shouldn't work at all.

`$@'
     The file name of the target of the rule.  If the target is an
     archive member, then `$@' is the name of the archive file.  In a
     pattern rule that has multiple targets (*note Introduction to
     Pattern Rules: Pattern Intro.), `$@' is the name of whichever
     target caused the rule's commands to be run.
 
What we need is to run pattern substitution for _two_ files, perhaps
using the for loop.
Also I think the order of substitutions would be better to be reversed:
    $(patsubst %.pl,%.perl,$(patsubst %.cgi,%.perl,$FILE)) >$FILE+
This way the gitweb_defaults file can even have *.perl extension
Show 11 quoted lines
>> Also wouldn't all replacements be in the new gitweb_defaults file, so
>> there would be no need then to do replacements for gitweb.cgi?
> 
> Not all replacements are done in one or the other, and since it's 
> basically a NOP to perform the full set of replacements on both files 
> that seemed the easiest way to ensure they were done in both places.
> 
>> Oh, I see there is at least one that stayed in gitweb.perl: $version
>> 
> 
> <snip>
O.K.
But Makefile would be (slightly) simpler if replacements were needed only
for single file of two.
Show 6 quoted lines
>>> +# Define and than setup our configuration 
>>> +#
>>> +our(
>>> +	$VERSION,
>>> +	$path_info,
>>> +	$GIT,
[...]
Show 10 quoted lines
>>> +	$prevent_xss,
>>> +	@diff_opts,
>>> +	%feature
>>>  );
>> 
>> Why this block is required?  Why not have variables defined (using
>> "our") in gitweb_defaults file?
> 
> Wanted to make sure things were properly defined, if in an unexpected 
> state, should a user have gitweb.cgi in place but not the defaults.
In my opinion it actually *makes worse*.  I am not sure if gitweb would
work if the variables are undefined, and you would lose 'undeclared 
variable' warning.
Show 12 quoted lines
>>> diff --git a/gitweb/gitweb_defaults.perl b/gitweb/gitweb_defaults.perl
>>> new file mode 100644
>>> index 0000000..ede0daf
>>> --- /dev/null
>>> +++ b/gitweb/gitweb_defaults.perl
>>> @@ -0,0 +1,468 @@
>>> +# gitweb - simple web interface to track changes in git repositories
>>> +#
>>> +# (C) 2005-2006, Kay Sievers <kay.sievers@vrfy.org>
>>> +# (C) 2005, Christian Gierke
>>> +#
>>> +# This program is licensed under the GPLv2

This header should probably be modified, at least stating what the file is for.

Show 8 quoted lines
>>> +
>>> +# Base URL for relative URLs in gitweb ($logo, $favicon, ...),
>>> +# needed and used only for URLs with nonempty PATH_INFO
>>> +$base_url = $my_url;
>> 
>> Why not "our $base_url = $my_url;"?
> 
> same reason as the other 'our' includes above,

See comment above about pre-declaring variables actually making it worse wrt checking.

> though why this ended up  
> as a separate patch vs. the rest of the file I don't know.
Errr... what you are talking about here?
-- 
Jakub Narebski
Poland
Previous: J.H.Next: Junio C Hamano
Message 32 of 40 in “Gitweb caching changes v2”
  1. 0/6 Gitweb caching changes v2John 'Warthog9' Hawley, Dec 10, 2009
  2. 1/6 GITWEB - Load CheckingJohn 'Warthog9' Hawley, Dec 10, 2009
  3. 2/6 GITWEB - Missmatching git w/ gitwebJohn 'Warthog9' Hawley, Dec 10, 2009
  4. 3/6 GITWEB - Add git:// link to summary pagesJohn 'Warthog9' Hawley, Dec 10, 2009
  5. 4/6 GITWEB - Makefile changesJohn 'Warthog9' Hawley, Dec 10, 2009
  6. Jakub NarebskiDec 11, 2009
  7. J.H.Dec 11, 2009
  8. Jakub NarebskiDec 11, 2009
  9. 4/6 gitweb: Makefile improvementsJakub Narebski, Dec 19, 2009
  10. Johannes SchindelinDec 11, 2009
  11. Jakub NarebskiDec 11, 2009
  12. 3/6 gitweb: Optionally add "git" links in project list pageJakub Narebski, Dec 18, 2009
  13. Jakub NarebskiDec 11, 2009
  14. 2/6 gitweb: Add option to force version matchJakub Narebski, Dec 18, 2009
  15. Johannes SchindelinDec 11, 2009
  16. Sverre RabbelierDec 10, 2009
  17. Jakub NarebskiDec 11, 2009
  18. Junio C HamanoDec 11, 2009
  19. J.H.Dec 11, 2009
  20. Junio C HamanoDec 11, 2009
  21. J.H.Dec 11, 2009
  22. J.H.Dec 11, 2009
  23. Junio C HamanoDec 11, 2009
  24. Jakub NarebskiDec 11, 2009
  25. 1/6 gitweb: Load checkingJakub Narebski, Dec 18, 2009
  26. Mihamina RakotomandimbyDec 11, 2009
  27. Sverre RabbelierDec 10, 2009
  28. Jakub NarebskiDec 11, 2009
  29. 6/6 GITWEB - Separate defaults from main fileJohn 'Warthog9' Hawley, Dec 10, 2009
  30. Jakub NarebskiDec 11, 2009
  31. J.H.Dec 11, 2009
  32. Jakub NarebskiDec 11, 2009
  33. Junio C HamanoDec 16, 2009
  34. J.H.Dec 16, 2009
  35. Jakub NarebskiDec 16, 2009
  36. J.H.Dec 16, 2009
  37. Jakub NarebskiDec 16, 2009
  38. Jakub NarebskiDec 11, 2009
  39. J.H.Dec 11, 2009
  40. Jakub NarebskiDec 12, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.