Re: [PATCH v3] gitweb: Add an option for adding more branch refs

[Krzesimir Nowak <krzesimir@endocode.com>]

On Mon, 2013-12-02 at 18:34 +0100, Jakub Narębski wrote:
> W dniu 2013-12-02 13:06, Krzesimir Nowak pisze:
> > On Mon, 2013-12-02 at 01:21 +0100, Jakub Narębski wrote:
> >> On Thu, Nov 28, 2013 at 12:44 PM, Krzesimir Nowak
> >> <krzesimir@endocode.com>  wrote:
> >>
> >>> Allow @additional_branch_refs configuration variable to tell gitweb to
> >>> show refs from additional hierarchies in addition to branches in the
> >>> list-of-branches view.
> >>>
> >>> Signed-off-by: Krzesimir Nowak<krzesimir@endocode.com>
> >>
> >> Why not use %feature hash instead of adding new configuration variable?
> >> I think that this option is similar enough to 'remote_heads' feature
> >> (which BTW should be 'remote-heads'), and could conceivably enabled
> >> on a per-repository basis, i.e. with repository configuration override,
> >> isn't it?
> >
> > I'd like to see some consensus on it before I start changing the patch
> > again.
> 
> %feature hash is mainly (but not only) about options that can be
> configured on per-repository basis.  Configuration variables are
> about options that are per-instance (per gitweb).

Well, I am mostly interested in per-instance configuration in this case,
but if that is also possible with %feature hash, then ok, I'll try to
make it work.

From what I've seen (correct me please if I got it wrong) feature
settings is taken from per-repository config file from [gitweb] section.
If there's nothing then some default value is taken. That default value
can be overriden with per-instance perl config file.

So it is easy to override it from per-instance perl config by typing:
$feature{'additional-branch-refs'}{'default'} = ['wip', 'no|tf"un,ny'];
$feature{'additional-branch-refs'}{'override'} = 1;

(Note the edge case of refs/no|tf"un,ny, which passes the git
check-ref-format scrutiny.)

But for now, most of features are quite simple - either booleans,
integers or list of simple strings (in snapshot feature). What I need
here is a list of strings, like CSV in following example:
[gitweb]
	additional_branch_refs = wip,"no|tf""un,ny"

Is dependency on external module like Text::CSV or Text::CSV_XS ok? If
not, I can hack some CSV reading code.

> 
> >> Usually %feature hash is preferred over adding new configuration variable
> >> but this is not some hard rule. Note however that patches adding new config
> >> are met with more scrutiny, as it is harder to fix mistakes because of
> >> requirement of backwards compatibility of configuration files.
> >>
> >
> > I don't know what kind of backwards compatibility you mention. Whether
> > you want gitweb to survive reading old config file or to honor
> > deprecated/old config variables.
> 
> I meant here honoring deprecated/old variables, i.e. honoring existing
> configuration files.  See for example backward compatibility for old
> $stylesheet variable vs new @stylesheets in print_header_links().
> 
> Though in this case it shouldn't be much of a problem; it would be
> easy to honor @additional_branch_refs by setting 'default' for
> 'extra-branch-refs' feature to it.

extra-branch-refs is nicer than additional-branch-refs, I'll use it.

> 
> >> BTW. there really should be gitweb/CodingGuidelines...
> >>
> >
> > Yes, would be useful. As in every other project. :)
> 
> Well, Git itself *has* Documentation/CodingGuidelines, but perhaps
> gitweb subsystem should have it's own...
> 
> [...]
> >>> @@ -3662,7 +3701,8 @@ sub git_get_heads_list {
> >>>                  my ($committer, $epoch, $tz) =
> >>>                          ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
> >>>                  $ref_item{'fullname'}  = $name;
> >>> -               $name =~ s!^refs/(?:head|remote)s/!!;
> >>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
> >>> +               $name =~ s!^refs/(?:$strip_refs|remotes)/!!;
> >>>
> >>>                  $ref_item{'name'}  = $name;
> >>>                  $ref_item{'id'}    = $hash;
> >>> @@ -7179,7 +7219,8 @@ sub snapshot_name {
> >>>                  $ver = $1;
> >>>          } else {
> >>>                  # branches and other need shortened SHA-1 hash
> >>> -               if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {
> >>> +               my $strip_refs = join '|', map { quotemeta } get_branch_refs();
> >>> +               if ($hash =~ m!^refs/(?:$strip_refs|remotes)/(.*)$!) {
> >>>                          $ver = $1;
> >>>                  }
> >>>                  $ver .= '-' . git_get_short_hash($project, $hash);
> >>
> >> One one hand, it is about threating extra branch refs the same way as 'head'.
> >> On the other hand we loose distinction between 'refs/heads/foo' and e.g.
> >> 'refs/wip/foo'. But maybe that's all right...
> >>
> >
> > In git_get_heads_list sub I could append a " ($ref_dir)" to refs which
> > are in neither 'heads' nor 'remotes', so heads view would look like:
> > master
> > old-stable
> > some-work-in-progress (wip)
> > some-other-branch (other)
> >
> > where both master and old-stable are in refs/heads/,
> > some-work-in-progress in refs/wip/ and some-other-branch in refs/other/.
> >
> > In case of branch snapshot names (snapshot_name sub) I could change it,
> > so names for branches mentioned above would be
> > "Project-master-<short-hash>.tgz",
> > "Project-old_stable-<short-hash>.tgz",
> > "Project-wip-some-work-in-progress-<short-hash>.tgz"
> > "Project-other-some-other-branch-<short-hash>.tgz"
> >
> > What do you think?
> 
> That is, I think, a very good idea.  Though perhaps it would be more 
> readable to add this extra feature as a separate patch, on top of main one.
> 

Right, I suppose this patch is going to end up being several patches.

-- 
Krzesimir Nowak
Software Developer
Endocode AG

krzesimir@endocode.com

------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B