Re: [PATCH 0/2] gitweb: patch view

[Jakub Narebski <jnareb@gmail.com>]

On Mon, 1 December 2008, Giuseppe Bilotta wrote:
> On Mon, Dec 1, 2008 at 1:45 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Sun, 30 Nov 2008, Giuseppe Bilotta wrote:
>>> On Sun, Nov 30, 2008 at 2:06 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>>> On Sat, 29 Nov 2008, Giuseppe Bilotta wrote:
>>>
>>>> By the way, we still might want to add somehow X-Git-Url and X-Git-Tag
>>>> headers later to 'patch' ('patchset') output format.
>>>
>>> Yeah, I've been thinking about it, but I couldn't find an easy and
>>> robust way to do it. Plus, should we add them for each patch, or just
>>> once for the whole patchset?
>>
>> True, that is a complication. Perhaps they should be added only for
>> single patch?
> 
> Although that's rather easy to implement technically, it also creates
> some kind of inconsistency.

Well, it is problem also from technical point of view. Currently we can
just stream (dump) git-format-patch output to browser (not forgetting
adding '--encoding=utf8' if it is not used already), and do not need
to have markers between commits. It is very simple code, which is its
own advantage.

>From theoretical point of view corrected X-Git-Tag functioning as
a kind of ref marker but for the raw (text/plain) output could be added
for each end every patch, so there would be no inconsistency for _this_
extra header.

I don't know what can be done about X-Git-URL.

>>> Considering I think commitdiff is ugly and long, you can guess my
>>> opinion on format_patch 8-P. 'patchset' might be a good candidate,
>>> considering it's what it does when both hash_parent and hash are
>>> given.
>>
>> True, 'patchset' might be even better, especially that it hints
>> what it does for a range a..b (not diff of endpoints, but series
>> of patches).
> 
> Good, I'll rename it.

I just don't know if it would be best name. Perhaps 'patches' would
be better?

[...]
>>> * diff(_plain): do what commitdiff(_plain) currently does for
>>> parent..hash views, modulo something to be discussed for commit
>>> messages (a shortlog rather maybe?)
>>
>> Equivalent of "git diff" (or "git diff-tree").
>>
>> Diffstat, or dirstat might be a good idea. Shortlog... I am not sure.
>> Diff is about endpoints, and they can be in reverse, too.
>>
>> There is a problem how to denote endpoints.
> 
> Hm? Doesn't parent..hash work? Or are you talking about something else?

Errr... I meant here for the user, not for gitweb. To somehow denote
before patch itself the endpoints. Just like for diff _for_ a commit
we have commit message denoting :-).

>>> * patch[set?][_plain?]: format-patch style output (maybe with option
>>> for HTML stuff too)
>>
>> Equivalent of "git format-patch".
>>
>> Actually the HTML format would be more like "git log -p", so perhaps
>> that could be handled simply as a version of 'log' view (perhaps via
>> @extra_options aka 'opt' parameter).
> 
> This is starting to get complicated ... I'm not sure how far in this I
> can go with this patchset, so for the time being I'll probably just
> stick to refining the (plain) patchset feature.

What I meant here is that it would be IMHO enough to have 'patch' view
(or whatever it ends up named) be raw format / plain/text format only,
and leave HTML equivalent for extra options/extra format to 'log' view.

[...]
>>>>> The second patch exposes it from commitdiff view (obviosly), but also
>>>>> from shortlog view, when less than 16 patches are begin shown.
>>>>
>>>> Why this nonconfigurable limit?
>>>
>>> Because the patch was actually a quick hack for the proof of concept
>>> 8-) I wasn't even sure the patch idea would have been worth it (as
>>> opposed to email-izing commitdiff_plain).
>>
>> Ah.
>>
>> Well, we might want to impose some limit to avoid generating and sending
>> patchset for a whole history. Perhaps to page size (100), or some similar
>> number?
> 
> The reason why I chose 16 is that (1) it's a rather commonly used
> 'small' number across gitweb and (2) it's a rather acceptable
> 'universal' upper limit for patchsets. There _are_ a few patchbombs
> that considerably overtake that limit, but observe that this limit is
> not an arbitrary limit on patchsets generated by the 'patchset' view,
> but only a condition for which a link is generated from shortlog view.

I see.

> We may want to have TWO limits here: one is the absolute maximum limit
> to the number of patches dumped in a patchset (to prevent DoS attacks
> by repeated requests of the whole history), and the other one is the
> limit for autogenerated patchset links.

A pageful (100 commits) as hard limit against DoS attacks?

[...]
-- 
Jakub Narebski
Poland