[PATCH 0/2] gitweb: patch view

[PATCH 0/2] gitweb: patch view

[Giuseppe Bilotta]

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

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.

The second patch exposes it from commitdiff view (obviosly), but also
from shortlog view, when less than 16 patches are begin shown.

Giuseppe Bilotta (2):
  gitweb: add patch view
  gitweb: links to patch action in commitdiff and shortlog view

 gitweb/gitweb.perl |   35 +++++++++++++++++++++++++++++++++--
 1 files changed, 33 insertions(+), 2 deletions(-)

[PATCH 1/2] gitweb: add patch view

[Giuseppe Bilotta]

Trying to use 'commitdiff_plain' output as input to git am results in
some annoying results such as doubled subject lines. We thus offer a new
'patch' view that exposes format-patch output directly. This makes it
easier to offer patches by linking to gitweb repositories.
---
 gitweb/gitweb.perl |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 933e137..befe6b6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -485,6 +485,7 @@ our %actions = (
 	"heads" => \&git_heads,
 	"history" => \&git_history,
 	"log" => \&git_log,
+	"patch" => \&git_patch,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5465,7 +5466,11 @@ sub git_commitdiff {
 		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
 			'-p', $hash_parent_param, $hash, "--"
 			or die_error(500, "Open git-diff-tree failed");
-
+	} elsif ($format eq 'patch') {
+		open $fd, "-|", git_cmd(), "format-patch", '--stdout',
+			$hash_parent ? "$hash_parent..$hash" :
+			('--root', '-1', $hash)
+			or die_error(500, "Open git-format-patch failed");
 	} else {
 		die_error(400, "Unknown commitdiff format");
 	}
@@ -5514,6 +5519,14 @@ 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" . '"');
 	}
 
 	# write patch
@@ -5535,6 +5548,11 @@ sub git_commitdiff {
 		print <$fd>;
 		close $fd
 			or print "Reading git-diff-tree failed\n";
+	} elsif ($format eq 'patch') {
+		local $/ = undef;
+		print <$fd>;
+		close $fd
+			or print "Reading git-format-patch failed\n";
 	}
 }
 
@@ -5542,6 +5560,11 @@ sub git_commitdiff_plain {
 	git_commitdiff('plain');
 }
 
+# format-patch-style patches
+sub git_patch {
+	git_commitdiff('patch');
+}
+
 sub git_history {
 	if (!defined $hash_base) {
 		$hash_base = git_get_head_hash($project);
-- 
1.5.6.5

[PATCH 2/2] gitweb: links to patch action in commitdiff and shortlog view

[Giuseppe Bilotta]

The link from commitdiff view is an obviously needed one, but we also
offer the option to link to the patchset in shortlog view, when there
are less than 15 commits being shown.
---
 gitweb/gitweb.perl |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index befe6b6..5b18fdf 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5382,7 +5382,9 @@ sub git_commitdiff {
 	if ($format eq 'html') {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
-			        "raw");
+			        "raw") . " | " .
+			$cgi->a({-href => href(action=>"patch", -replay=>1)},
+			        "patch");
 
 		if (defined $hash_parent &&
 		    $hash_parent ne '-c' && $hash_parent ne '--cc') {
@@ -5915,6 +5917,12 @@ sub git_shortlog {
 			$cgi->a({-href => href(-replay=>1, page=>$page+1),
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
+	# TODO this should be configurable
+	if ($#commitlist <= 15) {
+		$paging_nav .= " &sdot; " .
+			$cgi->a({-href => href(action=>"patch", -replay=>1)},
+			        $#commitlist > 1 ? "patchset" : "patch");
+	}
 
 	git_header_html();
 	git_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);
-- 
1.5.6.5

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

[Sverre Rabbelier]

On Sat, Nov 29, 2008 at 14:41, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> Trying to use 'commitdiff_plain' output as input to git am results in
> some annoying results such as doubled subject lines. We thus offer a new
> 'patch' view that exposes format-patch output directly. This makes it
> easier to offer patches by linking to gitweb repositories.

If this does what I think it does I would be very happy with this
feature :). Only yesterday I wanted to link someone to a patch I put
up on repo.or.cz, but instead ended up telling them to download the
snapshot.

As an additional feature request; would it be possible to have a view
that exposes a patch that is directly applyable by the patch command?

-- 
Cheers,

Sverre Rabbelier

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

[Jakub Narebski]

On Sat, 29 Nov 2008, Sverre Rabbelier wrote:
> On Sat, Nov 29, 2008 at 14:41, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:

> > Trying to use 'commitdiff_plain' output as input to git am results in
> > some annoying results such as doubled subject lines. We thus offer a new
> > 'patch' view that exposes format-patch output directly. This makes it
> > easier to offer patches by linking to gitweb repositories.
> 
> If this does what I think it does I would be very happy with this
> feature :). Only yesterday I wanted to link someone to a patch I put
> up on repo.or.cz, but instead ended up telling them to download the
> snapshot.

True. 'commitdiff_plain' wasn't good enough; what's more it suffers
from the same ambiguity as 'commitdiff', i.e. it is both means to
show diff _for_ a commit (perhaps selecting one of parents), and
showing diff _between_ two commits; the new 'patch' always shows
diff for a commit, or for a series of commits.

>
> As an additional feature request; would it be possible to have a view
> that exposes a patch that is directly applyable by the patch command?

Both new 'patch' ('patchset') and old 'blobdiff_plain' should be
directly applicable by patch and by git-apply.

-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

Wow, I honestly didn't expect this idea to be so successful. I thought
I was the only one using gitweb to send patches around, honestly 8-D

On Sat, Nov 29, 2008 at 5:10 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 29 Nov 2008, Sverre Rabbelier wrote:
>>
>> If this does what I think it does I would be very happy with this
>> feature :). Only yesterday I wanted to link someone to a patch I put
>> up on repo.or.cz, but instead ended up telling them to download the
>> snapshot.
>
> True. 'commitdiff_plain' wasn't good enough; what's more it suffers
> from the same ambiguity as 'commitdiff', i.e. it is both means to
> show diff _for_ a commit (perhaps selecting one of parents), and
> showing diff _between_ two commits; the new 'patch' always shows
> diff for a commit, or for a series of commits.

Maybe commitdiff should me renamed to just be 'diff'.

Also, I was in doubt about the name for the new view, and I did
consider 'patchset' (which you mention in your email). I chose to
stick with the shorter form in the end, since many people complain
that gitweb already produces paths that are too long.

-- 
Giuseppe "Oblomov" Bilotta

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

[Sverre Rabbelier]

On Sat, Nov 29, 2008 at 17:48, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> Also, I was in doubt about the name for the new view, and I did
> consider 'patchset' (which you mention in your email). I chose to
> stick with the shorter form in the end, since many people complain
> that gitweb already produces paths that are too long.

Heh, what do I care about the length of the url, there's so many url
shorteners out there, that's not really a problem to me :).

-- 
Cheers,

Sverre Rabbelier

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

[Jakub Narebski]

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

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.


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.


By the way, we still might want to add somehow X-Git-Url and X-Git-Tag
headers later to 'patch' ('patchset') output format.

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

> 
> Giuseppe Bilotta (2):
>   gitweb: add patch view
>   gitweb: links to patch action in commitdiff and shortlog view
> 
>  gitweb/gitweb.perl |   35 +++++++++++++++++++++++++++++++++--
>  1 files changed, 33 insertions(+), 2 deletions(-)

Thank you for your work on gitweb
-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

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

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

[Jakub Narebski]

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

True, that is a complication. Perhaps they should be added only for
single patch?

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

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

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

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.

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

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

>>> 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?
-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

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

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

[Jakub Narebski]

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

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

[Giuseppe Bilotta]

On Mon, Dec 1, 2008 at 12:02 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> 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.

I'm thinking that the best way to achieve these results would be to
have some way to specify extra headers to git format-patch from the
command line and not just from a config file. Plus, we want to make
them 'dynamic' in the sense that we want to be able to put hash or ref
names etc in them. For the moment I'll mark this 'TODO' in the file.

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

The only thing I don't like about 'patches' is that if you ask for a
single commit you get a single patch. I'd rather stick to 'patch'
then, maybe make 'patches' a synonym?

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

Ah, in the sense that you have to specify parent..hash manually in the
URL presently? I've seen some patches to non-main gitweb doing this
kind of thing. If it got merged with upstream we could use that as
well.

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

Ah, ok. I'll resubmit a cleaned up version of these two patches for
the time being then.

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

I suspect 100 is too hight already, but I guess we can tune it later.

-- 
Giuseppe "Oblomov" Bilotta