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

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

From
J.H. <warthog9@kernel.org>
Date
Dec 11, 2009, 15:58 UTC
Message-ID
<4B226C0F.2070407@kernel.org>
In-Reply-To
<m3ljh9cy3b.fsf@localhost.localdomain>
Show 8 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.

Show 30 quoted lines
> 
>> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> 
> Signoff mismatch.
> 
>> ---
>>  .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.

>> +	#$(QUIET_GEN)$(RM) $@ $@+ &&
> 
> What this line is about?

Cruft, thought I had deleted and excluded it, won't be there in next version.

Show 12 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.

> 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>
Show 61 quoted lines
>> +# Define and than setup our configuration 
>> +#
>> +our(
>> +	$VERSION,
>> +	$path_info,
>> +	$GIT,
>> +	$projectroot,
>> +	$project_maxdepth,
>> +	$home_link,
>> +	$home_link_str,
>> +	$site_name,
>> +	$site_header,
>> +	$home_text,
>> +	$site_footer,
>> +	@stylesheets,
>> +	$stylesheet,
>> +	$logo,
>> +	$favicon,
>> +	$javascript,
>> +	$logo_url,
>> +	$logo_label,
>> +	$projects_list,
>> +	$projects_list_description_width,
>> +	$default_projects_order,
>> +	$export_ok,
>> +	$export_auth_hook,
>> +	$strict_export,
>> +	@git_base_url_list,
>> +	$default_blob_plain_mimetype,
>> +	$default_text_plain_charset,
>> +	$mimetypes_file,
>> +	$missmatch_git,
>> +	$gitlinkurl,
>> +	$maxload,
>> +	$cache_enable,
>> +	$minCacheTime,
>> +	$maxCacheTime,
>> +	$cachedir,
>> +	$backgroundCache,
>> +	$nocachedata,
>> +	$nocachedatabin,
>> +	$fullhashpath,
>> +	$fullhashbinpath,
>> +	$export_auth_hook,
>> +	%known_snapshot_format_aliases,
>> +	%known_snapshot_formats,
>> +	$path_info,
>> +	$fallback_encoding,
>> +	%avatar_size,
>> +	$project_maxdepth,
>> +	$headerRefresh,
>> +	$base_url,
>> +	$projects_list_description_width,
>> +	$default_projects_order,
>> +	$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.

Show 25 quoted lines
> 
> [cut deletion]  
> 
>> +do 'gitweb_defaults.pl';
>>  
>>  sub gitweb_get_feature {
>>  	my ($name) = @_;
>> 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
>> +
>> +# 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, though why this ended up as a separate patch vs. the rest of the file I don't know.

- John 'Warthog9' Hawley
Previous: Jakub NarebskiNext: Jakub Narebski
Message 31 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.