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

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

On Sun, Nov 30, 2008 at 2:06 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 29 Nov 2008, Giuseppe Bilotta wrote:
>
>> I recently discovered that the commitdiff_plain view is not exactly
>> something that can be used by git am directly (for example, the subject
>> line gets duplicated in the commit message body after using git am).
>
> That's because gitweb generates email-like format "by hand", instead
> of using '--format=email' or git-format-patch like in your series. On
> the other hand that allows us to add extra headers, namely X-Git-Tag:
> (which hasn't best implementation, anyway) and X-Git-Url: with URL
> for given output.

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

>> Since I'm not sure if it was the case to fix the plain view because I
>> don't know what its intended usage was, I prepared a new view,
>> uncreatively called 'patch', that exposes git format-patch output
>> directly.
>
> Perhaps 'format_patch' would be better... hmmm... ?

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.

> Actually IMHO both 'commitdiff' and 'commitdiff_plain' try to do two
> things at once. First to show diff _for_ a commit, i.e. equivalent of
> "git show" or "git show --pretty=email", perhaps choosing one of
> parents for a merge commit. Then showing commit message for $hash has
> sense. The fact that 'commit' view doesn't show patchset, while
> 'commitdiff' does might be result of historical situation.
>
> Second, to show diff _between_ commits, i.e. equivalent of
> "git diff branch master". Then there doesn't make much sense to show
> full commit message _only_ for one side of diff. IMHO that should be
> main purpose of 'commitdiff' and 'commitdiff_plain' views, or simply
> 'diff' / 'diff_plain' future views.

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
* diff(_plain): do what commitdiff(_plain) currently does for
parent..hash views, modulo something to be discussed for commit
messages (a shortlog rather maybe?)
* patch[set?][_plain?]: format-patch style output (maybe with option
for HTML stuff too)

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

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

-- 
Giuseppe "Oblomov" Bilotta