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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxx
------
Endocode AG, Johannisstraße 20, 10117 Berlin
info@xxxxxxxxxxxx | 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
--
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