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

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

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

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