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

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

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.

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

>> We can probably consider deprecating commitdiff(_plain) and have the
>> following three views:
>>
>> * commit(_plain): do what commitdiff(_plain) currently does for a single commit
>
> Equivalent of "git show" (and not merely "git cat-file -t commit").
>
>> * 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?

>> * 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 'patch' view does, what might be not obvious from this description
>>> and from first patch in series, is to show diffs for _series_ of
>>> commits. It means equivalent of "git log -p" or "git whatchanged".
>>> It might make more sense to have plain git-format-patch output, but it
>>> could be useful to have some kind of 'git log -p' HTML output.
>>>
>>> So even if 'commitdiff' / 'commitdiff_plain' is fixed, 'patch' whould
>>> still have its place.
>>
>> Nice to know. Do consider the current version more of a
>> proof-of-concept that some definitive code.
>
> Ah. O.K. It would be nice if this patch was marked as RFC (well, lack
> of signoff hints at this), or as WIP, or as PoC,...

Damn,  I always forget about that.

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

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.

BTW, autogenerated patchset links probably make sense taking some
previous branch name as point of reference. e.g., if I have branch1
within the history of branch2, we probably want some (semi)automatic
way of getting a patchset for branch1..branch2 --of course, we also
want to do a shortlog between them, so that's a more general feature
we should think about.


-- 
Giuseppe "Oblomov" Bilotta