From: Jakub Narębski <jnareb@gmail.com>
Date: Mon, 02 Dec 2013 17:34:04 GMT
Subject: Re: [PATCH v3] gitweb: Add an option for adding more branch refs
Message-ID: <529CC48C.5080902@gmail.com>
In-Reply-To: <1385985997.2054.27.camel@localhost.localdomain>

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.

-- 
Jakub Narebski