Re: [RFCv3 1/2] gitweb: add patch view

["Giuseppe Bilotta" <giuseppe.bilotta@gmail.com>]

On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>> +
>> +     # The maximum number of patches in a patchset generated in patch
>> +     # view. Set this to 0 or undef to disable patch view, or to a
>> +     # negative number to remove any limit.
>> +     'patches' => {
>> +             'override' => 1,
>> +             'default' => [16]},
>>  );
>
> Looking at the existing entries in the %feature hash, it seems that it is
> our tradition that a new feature starts as disabled and not overridable
> (see 'ctags' in the context above).

I always assumed that the disabled default was related to how invasive
the changes would be (to the UI or computationally-wise). As for the
overridability, that's actually the only reason why it would make
sense to put in the %feature hash ... otherwise a conf-settable
$patch_max (as in v2) would have been enough.

>>  sub git_commitdiff {
>>       my $format = shift || 'html';
>> +
>> +     my $patch_max = gitweb_check_feature('patches');
>> +     if ($format eq 'patch') {
>> +             die_error(403, "Patch view not allowed") unless $patch_max;
>> +     }
>> +
>
> Should you have to pay overhead for the check-feature call even when
> the $format is not "patch"?

Actually I wasn't sure if I could use my within the if block, and have
the value visible outside (it's used further down when picking the
options to pass to format-patch). And since it was used in the second
patch anyway to choose whether to add the 'patch' link in html view or
not, I just put it outside the block.

>> @@ -5396,6 +5410,7 @@ sub git_commitdiff {
>>       }
>>       # we need to prepare $formats_nav before almost any parameter munging
>>       my $formats_nav;
>> +
>
> Noise.

Oopsie.

>> @@ -5532,6 +5552,15 @@ sub git_commitdiff {
>>                       print to_utf8($line) . "\n";
>>               }
>>               print "---\n\n";
>> +     } elsif ($format eq 'patch') {
>> +             my $filename = basename($project) . "-$hash.patch";
>> +
>> +             print $cgi->header(
>> +                     -type => 'text/plain',
>> +                     -charset => 'utf-8',
>> +                     -expires => $expires,
>> +                     -content_disposition => 'inline; filename="' . "$filename" . '"');
>> +             # TODO add X-Git-Tag/X-Git-Url headers in a sensible way
>
> A stupid question.  Are you talking about sending these X-Foo as extra
> HTTP headers?  What good would they do (iow what will they be used for by
> the receiving browser/wget)?

No, as extra 'email' headers, similarly to what commitdiff_plain does.
It might or might not make sense, and might or might not be worth the
effort. For sure the best way to enable these things would have to
give git format-patch some support for extra headers specified on the
command line.

> Other than that the patch seems quite straightforward and was a pleasant
> read.  Thanks.

Thank you.

-- 
Giuseppe "Oblomov" Bilotta