headers
Junio C Hamano | 4 Dec 00:55
Favicon
Gravatar

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

Giuseppe Bilotta <giuseppe.bilotta <at> gmail.com> writes:

> The manually-built email format in commitdiff_plain output is not
> appropriate for feeding git-am, because of two limitations:
>  * when a range of commits is specified, commitdiff_plain publishes a
>    single patch with the message from the first commit, instead of a
>    patchset,
>  * in either case, the patch summary is replicated both as email subject
>    and as first line of the email itself, resulting in a doubled summary
>    if the output is fed to git-am.
>
> We thus create a new view that can be fed to git-am directly by exposing
> the output of git format-patch directly. This allows patch exchange and
> submission via gitweb.
>
> A configurable limit is imposed on the number of commits which will be
> included in a patchset, to prevent DoS attacks on the server. Setting
> the limit to 0 will disable the patch view, setting it to a negative
> number will remove the limit.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta <at> gmail.com>
> ---
>  gitweb/gitweb.perl |   41 ++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 40 insertions(+), 1 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 2738643..c9abfcf 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -329,6 +329,13 @@ our %feature = (
(Continue reading)

Permalink | Reply |
headers
Giuseppe Bilotta | 4 Dec 01:20
Gravatar

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

On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano <gitster <at> pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta <at> 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;
>> +     }
>> +
>
(Continue reading)

Permalink | Reply |
headers
Junio C Hamano | 4 Dec 01:40
Favicon
Gravatar

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

"Giuseppe Bilotta" <giuseppe.bilotta <at> gmail.com> writes:

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

I tend to agree; the point of this 'patch' feature is to give something
you can feed "am" with, and "am" certainly would discard such extra
garbage headers as uninteresting, so there is no point to add such
headers.
Permalink | Reply |
headers
Jakub Narebski | 4 Dec 02:48
Gravatar

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

On Thu, Dec 4, 2008 at 01:20, Giuseppe Bilotta wrote:
> On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano <gitster <at> pobox.com> wrote:
>> Giuseppe Bilotta <giuseppe.bilotta <at> 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]},

Errr... you need something like 'sub' => \&feature_patches for override
to actually work, I think.

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

Add to that the fact that this patch just adds the new view, like for
example in the case of 'snapshot' link, which was turned on... but fact,
it was by default not overridable. I would agree that it can be turned
on with low limit but not overridable in introductory patch.
(Continue reading)

Permalink | Reply |
headers
Giuseppe Bilotta | 4 Dec 08:24
Gravatar

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

On Thu, Dec 4, 2008 at 2:48 AM, Jakub Narebski <jnareb <at> gmail.com> wrote:
> On Thu, Dec 4, 2008 at 01:20, Giuseppe Bilotta wrote:
>> On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano <gitster <at> pobox.com> wrote:
>>> Giuseppe Bilotta <giuseppe.bilotta <at> 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]},
>
> Errr... you need something like 'sub' => \&feature_patches for override
> to actually work, I think.

Oops, right.

>> 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.
>
> Add to that the fact that this patch just adds the new view, like for
> example in the case of 'snapshot' link, which was turned on... but fact,
> it was by default not overridable. I would agree that it can be turned
> on with low limit but not overridable in introductory patch.

Ok, I'll make it non-overridable, and keep this 16 limit for starters.
Or would you suggest even lower?
(Continue reading)

Permalink | Reply |

Gmane