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

["J.H." <warthog9@xxxxxxxxxx>]

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

> > Signed-off-by: John 'Warthog9' Hawley <warthog9@xxxxxxxxxxxxxx>
>
> 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.

> >  	$(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>

> > +# 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.

> [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@xxxxxxxx>
> > +# (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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html