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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 27, 2013, 20:55 UTC
Message-ID: <xmqqob55sgww.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPig+cR3hoWmDPJyWP6HbpuDvRwgdNj5VwX8A20DCfRBK5OuSQ@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 6 quoted lines

> On Wed, Nov 27, 2013 at 3:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Nov 27, 2013 at 10:30 AM, Krzesimir Nowak
>> <krzesimir@endocode.com> wrote:
>>> Overriding an @additional_branch_refs configuration variable with
>>> value ('wip') will make gitweb to show branches that appear in
>>> refs/heads and refs/wip (refs/heads is hardcoded).

"branches" are by definition what are in refs/heads/ hierarchy, so

  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.

would be more appropriate and sufficient.

Show 14 quoted lines

>> Mentioning $ref in the error message would help the user resolve the
>> problem more quickly.
>>
>>> +               die_error(500, '"heads" specified in @additional_branch_refs') if ($ref eq 'heads');
>>
>> Rephrasing this as
>>
>>     "heads" disallowed in @additional_branch_refs
>>
>> would better explain the problem to a user who has only made a cursory
>> read of the documentation.
>
> The program could easily filter out the redundant 'heads', so does
> this really deserve a diagnostic?

True.

I was primarily worried about metacharacters in the specified strings getting in the way of regexp matches the new code allows on them, but that has been resolved with the use of \Q..\E; if that automatic deduping is done, I do not immediately see any remaining issues in the latest round of the patch.

Thanks.