[RFCv3 0/2] gitweb: patch view

[RFCv3 0/2] gitweb: patch view

[Giuseppe Bilotta]

Another go at the patch view feature. I'm marking this as RFC because
there are still a couple of points that need to be agreed on, esp. wrt
to the patch limiting and the insertion of extra X-Git-* email headers.

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

 gitweb/gitweb.perl |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 58 insertions(+), 1 deletions(-)

[RFCv3 1/2] gitweb: add patch view

[Giuseppe Bilotta]

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@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 = (
 	'ctags' => {
 		'override' => 0,
 		'default' => [0]},
+
+	# 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]},
 );
 
 sub gitweb_get_feature {
@@ -503,6 +510,7 @@ our %actions = (
 	"heads" => \&git_heads,
 	"history" => \&git_history,
 	"log" => \&git_log,
+	"patch" => \&git_patch,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5386,6 +5394,12 @@ sub git_blobdiff_plain {
 
 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;
+	}
+
 	$hash ||= $hash_base || "HEAD";
 	my %co = parse_commit($hash)
 	    or die_error(404, "Unknown commit object");
@@ -5396,6 +5410,7 @@ sub git_commitdiff {
 	}
 	# we need to prepare $formats_nav before almost any parameter munging
 	my $formats_nav;
+
 	if ($format eq 'html') {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
@@ -5483,7 +5498,12 @@ 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", '--encoding=utf8',
+			'--stdout', $patch_max > 0 ? "-$patch_max" : (),
+			$hash_parent ? ('-n', "$hash_parent..$hash") :
+			('--root', '-1', $hash)
+			or die_error(500, "Open git-format-patch failed");
 	} else {
 		die_error(400, "Unknown commitdiff format");
 	}
@@ -5532,6 +5552,15 @@ 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" . '"');
+		# TODO add X-Git-Tag/X-Git-Url headers in a sensible way
 	}
 
 	# write patch
@@ -5553,6 +5582,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";
 	}
 }
 
@@ -5560,6 +5594,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

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

[Giuseppe Bilotta]

In shortlog view, a link to the patchset is only offered when the number
of commits shown is less than the allowed maximum number of patches.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c9abfcf..ec8fc7d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5083,6 +5083,11 @@ sub git_commit {
 			} @$parents ) .
 			')';
 	}
+	if (gitweb_check_feature('patches')) {
+		$formats_nav .= " | " .
+			$cgi->a({-href => href(action=>"patch", -replay=>1)},
+				"patch");
+	}
 
 	if (!defined $parent) {
 		$parent = "--root";
@@ -5415,6 +5420,11 @@ sub git_commitdiff {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
 			        "raw");
+		if ($patch_max) {
+			$formats_nav .= " | " .
+				$cgi->a({-href => href(action=>"patch", -replay=>1)},
+					"patch");
+		}
 
 		if (defined $hash_parent &&
 		    $hash_parent ne '-c' && $hash_parent ne '--cc') {
@@ -5949,6 +5959,14 @@ sub git_shortlog {
 			$cgi->a({-href => href(-replay=>1, page=>$page+1),
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
+	my $patch_max = gitweb_check_feature('patches');
+	if ($patch_max) {
+		if ($patch_max < 0 || @commitlist <= $patch_max) {
+			$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: [RFCv3 2/2] gitweb: links to patch action in commitdiff and shortlog view

[Jakub Narebski]

On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:

> In shortlog view, a link to the patchset is only offered when the number
> of commits shown is less than the allowed maximum number of patches.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index c9abfcf..ec8fc7d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5083,6 +5083,11 @@ sub git_commit {
>  			} @$parents ) .
>  			')';
>  	}
> +	if (gitweb_check_feature('patches')) {
> +		$formats_nav .= " | " .
> +			$cgi->a({-href => href(action=>"patch", -replay=>1)},
> +				"patch");
> +	}
>  
>  	if (!defined $parent) {
>  		$parent = "--root";
> @@ -5415,6 +5420,11 @@ sub git_commitdiff {
>  		$formats_nav =
>  			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
>  			        "raw");
> +		if ($patch_max) {
> +			$formats_nav .= " | " .
> +				$cgi->a({-href => href(action=>"patch", -replay=>1)},
> +					"patch");
> +		}
>  
>  		if (defined $hash_parent &&
>  		    $hash_parent ne '-c' && $hash_parent ne '--cc') {

In the above two hunks 'patch' view functions as "git show --pretty=email"
text/plain equivalent, but this duplicates a bit 'commitdiff_plain'
functionality.  Well, 'commitdiff_plain' has currently some errors,
but...

> @@ -5949,6 +5959,14 @@ sub git_shortlog {
>  			$cgi->a({-href => href(-replay=>1, page=>$page+1),
>  			         -accesskey => "n", -title => "Alt-n"}, "next");
>  	}
> +	my $patch_max = gitweb_check_feature('patches');
> +	if ($patch_max) {
> +		if ($patch_max < 0 || @commitlist <= $patch_max) {
> +			$paging_nav .= " &sdot; " .
> +				$cgi->a({-href => href(action=>"patch", -replay=>1)},
> +					@commitlist > 1 ? "patchset" : "patch");
> +		}
> +	}

Here 'patch' view functions as "git format-patch", able to be downloaded
and fed to git-am.  Perhaps the action should also be named 'patches'
here?; it could lead to the same function.

By the way, there is subtle bug in above link. If shortlog view is less
than $patch_max commits long, but it is because the history for a given
branch (or starting from given commit) is so short, and not because
there is cutoff $hash_parent set, the 'patchset' view wouldn't display
plain text equivalent view, but only patch for top commit.

I assume that the link is only for 'shortlog' view, and not also for
'log' and 'history' views because 'shortlog' is the only log-like view
which support $hash_parent?

>  
>  	git_header_html();
>  	git_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

On Sat, Dec 6, 2008 at 1:53 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>
>> In shortlog view, a link to the patchset is only offered when the number
>> of commits shown is less than the allowed maximum number of patches.
>>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

>> +     if (gitweb_check_feature('patches')) {
>> +             $formats_nav .= " | " .
>> +                     $cgi->a({-href => href(action=>"patch", -replay=>1)},
>> +                             "patch");
>> +     }
>>
>>       if (!defined $parent) {
>>               $parent = "--root";
>> @@ -5415,6 +5420,11 @@ sub git_commitdiff {
>>               $formats_nav =
>>                       $cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
>>                               "raw");
>> +             if ($patch_max) {
>> +                     $formats_nav .= " | " .
>> +                             $cgi->a({-href => href(action=>"patch", -replay=>1)},
>> +                                     "patch");
>> +             }
>>
>>               if (defined $hash_parent &&
>>                   $hash_parent ne '-c' && $hash_parent ne '--cc') {
>
> In the above two hunks 'patch' view functions as "git show --pretty=email"
> text/plain equivalent, but this duplicates a bit 'commitdiff_plain'
> functionality.  Well, 'commitdiff_plain' has currently some errors,
> but...

All things considered, for single commit view there is (modulo bugs)
no factual difference between plain diff and patch view.

Although we could merge them, I'm thinking that the plain diff view
has somewhat too much information in it. For backwards compatibility
it's probably not wise to change it, but we should consider it for the
next major version, honestly.

>> @@ -5949,6 +5959,14 @@ sub git_shortlog {
>>                       $cgi->a({-href => href(-replay=>1, page=>$page+1),
>>                                -accesskey => "n", -title => "Alt-n"}, "next");
>>       }
>> +     my $patch_max = gitweb_check_feature('patches');
>> +     if ($patch_max) {
>> +             if ($patch_max < 0 || @commitlist <= $patch_max) {
>> +                     $paging_nav .= " &sdot; " .
>> +                             $cgi->a({-href => href(action=>"patch", -replay=>1)},
>> +                                     @commitlist > 1 ? "patchset" : "patch");
>> +             }
>> +     }
>
> Here 'patch' view functions as "git format-patch", able to be downloaded
> and fed to git-am.  Perhaps the action should also be named 'patches'
> here?; it could lead to the same function.

I had half an idea to do so. 'patches' or 'patchset'?

> By the way, there is subtle bug in above link. If shortlog view is less
> than $patch_max commits long, but it is because the history for a given
> branch (or starting from given commit) is so short, and not because
> there is cutoff $hash_parent set, the 'patchset' view wouldn't display
> plain text equivalent view, but only patch for top commit.

Ah, good point.

Hm, not easy to solve. One way could be to add the hash_parent
manually. Or we could make the 'patches' view different from the
'patch' view in the way it handles refspecs without ranges. I'm
leaning towards the latter. What's your opinion?

> I assume that the link is only for 'shortlog' view, and not also for
> 'log' and 'history' views because 'shortlog' is the only log-like view
> which support $hash_parent?

The actual reason is that I never use log nor history view, but since
they don't support hash_parent because of this (I was the one who sent
the patch to support hash_parent in shortlog view) you could
paralogistically say that's the reason ;-)

I'm not sure about history view, but for log view I'm considering
addiong also a 'patch' link next to each commit. I'll think about it.

-- 
Giuseppe "Oblomov" Bilotta

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

[Jakub Narebski]

Dnia sobota 6. grudnia 2008 14:25, Giuseppe Bilotta napisał:
> On Sat, Dec 6, 2008 at 1:53 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>>
>>> In shortlog view, a link to the patchset is only offered when the number
>>> of commits shown is less than the allowed maximum number of patches.
>>>
>>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
 
>>> +     if (gitweb_check_feature('patches')) {
>>> +             $formats_nav .= " | " .
>>> +                     $cgi->a({-href => href(action=>"patch", -replay=>1)},
>>> +                             "patch");
>>> +     }
>>>
>>>       if (!defined $parent) {
>>>               $parent = "--root";
>>> @@ -5415,6 +5420,11 @@ sub git_commitdiff {
>>>               $formats_nav =
>>>                       $cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
>>>                               "raw");
>>> +             if ($patch_max) {
>>> +                     $formats_nav .= " | " .
>>> +                             $cgi->a({-href => href(action=>"patch", -replay=>1)},
>>> +                                     "patch");
>>> +             }
>>>
>>>               if (defined $hash_parent &&
>>>                   $hash_parent ne '-c' && $hash_parent ne '--cc') {
>>
>> In the above two hunks 'patch' view functions as "git show --pretty=email"
>> text/plain equivalent, but this duplicates a bit 'commitdiff_plain'
>> functionality.  Well, 'commitdiff_plain' has currently some errors,
>> but...
> 
> All things considered, for single commit view there is (modulo bugs)
> no factual difference between plain diff and patch view.
> 
> Although we could merge them, I'm thinking that the plain diff view
> has somewhat too much information in it. For backwards compatibility
> it's probably not wise to change it, but we should consider it for the
> next major version, honestly.

I'm just wondering if we should add 'patch' link to 'commit' and
'commitdiff' views (as alternate view) at all...
 
>>> @@ -5949,6 +5959,14 @@ sub git_shortlog {
>>>                       $cgi->a({-href => href(-replay=>1, page=>$page+1),
>>>                                -accesskey => "n", -title => "Alt-n"}, "next");
>>>       }
>>> +     my $patch_max = gitweb_check_feature('patches');
>>> +     if ($patch_max) {
>>> +             if ($patch_max < 0 || @commitlist <= $patch_max) {
>>> +                     $paging_nav .= " &sdot; " .
>>> +                             $cgi->a({-href => href(action=>"patch", -replay=>1)},
>>> +                                     @commitlist> 1 ? "patchset" : "patch");
>>> +             }
>>> +     }
>>
>> Here 'patch' view functions as "git format-patch", able to be downloaded
>> and fed to git-am.  Perhaps the action should also be named 'patches'
>> here?; it could lead to the same function.
> 
> I had half an idea to do so. 'patches' or 'patchset'?

Hmmm... I think 'patches'.

>> By the way, there is subtle bug in above link. If shortlog view is less
>> than $patch_max commits long, but it is because the history for a given
>> branch (or starting from given commit) is so short, and not because
>> there is cutoff $hash_parent set, the 'patchset' view wouldn't display
>> plain text equivalent view, but only patch for top commit.
> 
> Ah, good point.
> 
> Hm, not easy to solve. One way could be to add the hash_parent
> manually. Or we could make the 'patches' view different from the
> 'patch' view in the way it handles refspecs without ranges. I'm
> leaning towards the latter. What's your opinion?

I think simplest solution would be to add $hash_parent if it is not
set from the last commit, i.e. $commitlist[-1]{'id'}

>> I assume that the link is only for 'shortlog' view, and not also for
>> 'log' and 'history' views because 'shortlog' is the only log-like view
>> which support $hash_parent?
> 
> The actual reason is that I never use log nor history view, but since
> they don't support hash_parent because of this (I was the one who sent
> the patch to support hash_parent in shortlog view) you could
> paralogistically say that's the reason ;-)
> 
> I'm not sure about history view, but for log view I'm considering
> addiong also a 'patch' link next to each commit. I'll think about it.

Well, you can add it only for 'shortlog' view, and when the code for
all log-like views would get consolidated, you will get link to 'patches'
view automatically :-)

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@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@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 = (
>  	'ctags' => {
>  		'override' => 0,
>  		'default' => [0]},
> +
> +	# 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).

>  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;
> +	}
> +

Should you have to pay overhead for the check-feature call even when
the $format is not "patch"?

> @@ -5396,6 +5410,7 @@ sub git_commitdiff {
>  	}
>  	# we need to prepare $formats_nav before almost any parameter munging
>  	my $formats_nav;
> +

Noise.

> @@ -5532,6 +5552,15 @@ 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" . '"');
> +		# TODO add X-Git-Tag/X-Git-Url headers in a sensible way

A stupid question.  Are you talking about sending these X-Foo as extra
HTTP headers?  What good would they do (iow what will they be used for by
the receiving browser/wget)?

Other than that the patch seems quite straightforward and was a pleasant
read.  Thanks.

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

[Giuseppe Bilotta]

On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@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;
>> +     }
>> +
>
> Should you have to pay overhead for the check-feature call even when
> the $format is not "patch"?

Actually I wasn't sure if I could use my within the if block, and have
the value visible outside (it's used further down when picking the
options to pass to format-patch). And since it was used in the second
patch anyway to choose whether to add the 'patch' link in html view or
not, I just put it outside the block.

>> @@ -5396,6 +5410,7 @@ sub git_commitdiff {
>>       }
>>       # we need to prepare $formats_nav before almost any parameter munging
>>       my $formats_nav;
>> +
>
> Noise.

Oopsie.

>> @@ -5532,6 +5552,15 @@ 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" . '"');
>> +             # TODO add X-Git-Tag/X-Git-Url headers in a sensible way
>
> A stupid question.  Are you talking about sending these X-Foo as extra
> HTTP headers?  What good would they do (iow what will they be used for by
> the receiving browser/wget)?

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. For sure the best way to enable these things would have to
give git format-patch some support for extra headers specified on the
command line.

> Other than that the patch seems quite straightforward and was a pleasant
> read.  Thanks.

Thank you.

-- 
Giuseppe "Oblomov" Bilotta

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

[Junio C Hamano]

"Giuseppe Bilotta" <giuseppe.bilotta@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. 

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

[Jakub Narebski]

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

>>>  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;
>>> +     }
>>> +
>>
>> Should you have to pay overhead for the check-feature call even when
>> the $format is not "patch"?
> 
> Actually I wasn't sure if I could use my within the if block, and have
> the value visible outside (it's used further down when picking the
> options to pass to format-patch). And since it was used in the second
> patch anyway to choose whether to add the 'patch' link in html view or
> not, I just put it outside the block.

You have to use _declaration_ ourside block, but assignment can happen
inside:

+     my $patch_max;
+     if ($format eq 'patch') {
+             $patch_max = gitweb_check_feature('patches');
+             die_error(403, "Patch view not allowed") unless $patch_max;
+     }

(Side note: doesn't it uses spaces instead of tabs for align?)
 
[...]
>> Other than that the patch seems quite straightforward and was a pleasant
>> read.  Thanks.

BTW. I'll try to review patch (and probably Ack) soon.
-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

On Thu, Dec 4, 2008 at 2:48 AM, Jakub Narebski <jnareb@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@pobox.com> wrote:
>>> Giuseppe Bilotta <giuseppe.bilotta@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?

>>>>  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;
>>>> +     }
>>>> +
>>>
>>> Should you have to pay overhead for the check-feature call even when
>>> the $format is not "patch"?
>>
>> Actually I wasn't sure if I could use my within the if block, and have
>> the value visible outside (it's used further down when picking the
>> options to pass to format-patch). And since it was used in the second
>> patch anyway to choose whether to add the 'patch' link in html view or
>> not, I just put it outside the block.
>
> You have to use _declaration_ ourside block, but assignment can happen
> inside:
>
> +     my $patch_max;
> +     if ($format eq 'patch') {
> +             $patch_max = gitweb_check_feature('patches');
> +             die_error(403, "Patch view not allowed") unless $patch_max;
> +     }

Right, stupid me.

> (Side note: doesn't it uses spaces instead of tabs for align?)

I'll check.

-- 
Giuseppe "Oblomov" Bilotta

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

[Jakub Narebski]

I'm sorry for the delay reviewing this patch series...

On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:

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

This is because 'commitdiff_plain' wasn't _meant_ as patch series view,
to be fed to git-am. Actually it is a bit cross between "git show"
result with '--pretty=email' format, and "git diff" between two commits,
to be fed to git-apply or GNU patch.

Nevertheless the above reasoning doesn't need to be put in a commit
message. But it explains why new 'patch' / 'patchset' view is needed:
because there was no equivalent.

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

This is independent issue which is I think worth correcting anyway,
unless we decide to scrap 'commitdiff_plain' view altogether.
But I think we would want some text/plain patch view to be applied
by GNU patch (for example in RPM .spec file).

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

Which is in my opinion a very good idea.

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

Note that this limit doesn't need to be lower than page length limit,
i.e. currently 100 commits, as new 'patch' view doesn't generate
greater load than 'log' view (which is split into 100 commits long
pages).

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@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 = (
>  	'ctags' => {
>  		'override' => 0,
>  		'default' => [0]},
> +
> +	# 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]},
>  );

You need to set "'sub' => \&feature_patches" for feature to be 
override-able at all.  Also features are usually not overridable
by default, which reduces load a tiny bit (by _possibly_ not reading
config, although that shouldn't matter much now with reading whole
commit using single call to git-config, and not one call per variable).
And I think the default might be set larger: 'log' view generates
as big if not bigger load, and it is split into 100 commits long
pages.

>  
>  sub gitweb_get_feature {
> @@ -503,6 +510,7 @@ our %actions = (
>  	"heads" => \&git_heads,
>  	"history" => \&git_history,
>  	"log" => \&git_log,
> +	"patch" => \&git_patch,
>  	"rss" => \&git_rss,
>  	"atom" => \&git_atom,
>  	"search" => \&git_search,
> @@ -5386,6 +5394,12 @@ sub git_blobdiff_plain {
>  
>  sub git_commitdiff {
>  	my $format = shift || 'html';
> +
> +	my $patch_max = gitweb_check_feature('patches');

Wouldn't it be better to name this variable $max_patchset_size, or
something like that? I'm not saying that this name is bad, but I'm
wondering if it could be better...

> +	if ($format eq 'patch') {
> +		die_error(403, "Patch view not allowed") unless $patch_max;

So undef and '' means "not allowed", beside '0'? I think it is a good
idea.

And it would be better (although now it is not as much performance hit)
to move "gitweb_check_feature('patches');" inside conditional:

+	my $patch_max;
+	if ($format eq 'patch') {
+		$patch_max = gitweb_check_feature('patches');
+		die_error(403, "Patch view not allowed") unless $patch_max;
+	}
+

> +	}
> +
>  	$hash ||= $hash_base || "HEAD";
>  	my %co = parse_commit($hash)
>  	    or die_error(404, "Unknown commit object");
> @@ -5396,6 +5410,7 @@ sub git_commitdiff {
>  	}
>  	# we need to prepare $formats_nav before almost any parameter munging
>  	my $formats_nav;
> +

Hmmm... some accidental change, it looks like.

>  	if ($format eq 'html') {
>  		$formats_nav =
>  			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
> @@ -5483,7 +5498,12 @@ 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", '--encoding=utf8',
> +			'--stdout', $patch_max > 0 ? "-$patch_max" : (),

> +			$hash_parent ? ('-n', "$hash_parent..$hash") :
> +			('--root', '-1', $hash)

This bit makes 'patch' view behave bit differently than git-format-patch,
which I think is a good idea: namely it shows single patch if there is
no cutoff.  This should be mentioned in commit message, and perhaps
even in a comment in code.

Beside, if you show only single patch because $hash_parent is not set,
you don't need to examine $patch_max nor set limit, because you use '-1'.
Currently if $patch_max > 0 and !$hash_parent, you pass limit for the
number of patches twice.  This I think is harmless but...

> +			or die_error(500, "Open git-format-patch failed");
>  	} else {
>  		die_error(400, "Unknown commitdiff format");
>  	}
> @@ -5532,6 +5552,15 @@ 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" . '"');

Good.

> +		# TODO add X-Git-Tag/X-Git-Url headers in a sensible way

Sensible way would mean modifying git-format-patch to be able to add
extra headers via command option, just like it is now possible via
config variable format.headers, I think.  Because there are no surefire
markers of where one patch ends and another begins: commit message is
free text, and can contain diff... although if it contains diff
separator '---' then git-am would have problem with patch; or at least
even assuming sane commit messages it is not easy.

Also I think that only X-Git-Url makes sense, and it is per whole
patchset (whole 'patch' view output) and not for each individual
patch.

>  	}
>  
>  	# write patch
> @@ -5553,6 +5582,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";
>  	}
>  }
>  
> @@ -5560,6 +5594,11 @@ sub git_commitdiff_plain {
>  	git_commitdiff('plain');
>  }
>  
> +# format-patch-style patches
> +sub git_patch {
> +	git_commitdiff('patch');
> +}
> +

O.K.

>  sub git_history {
>  	if (!defined $hash_base) {
>  		$hash_base = git_get_head_hash($project);
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@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]},
>>  );
>
> You need to set "'sub' => \&feature_patches" for feature to be 
> override-able at all.  Also features are usually not overridable
> by default, which reduces load a tiny bit (by _possibly_ not reading
> config, although that shouldn't matter much now with reading whole
> commit using single call to git-config, and not one call per variable).
> And I think the default might be set larger: 'log' view generates
> as big if not bigger load, and it is split into 100 commits long
> pages.

I do not think defaulting to 'no' for overridability nor defaulting a new
feature to 'disabled' have much to do with the load, but they are more
about the principle of least surprise.  Somebody who runs gitweb in the
playpen he was given on the server shouldn't be getting a phone call from
his users late at night complaining that the page his gitweb serves look
different and has one extra link per each line, only because the sysadmin
of the server decided to update git to 1.6.1 without telling him.

Once a new version capable of serving a new feature is introduced, he can
plan, announce and deploy by switching the feature on in his gitweb
configuration file.

Some things, like sitewide default css changes, cannot be made disabled
by default.  But a new feature can easily be kept disabled by default not
to cause needless surprises.

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

[Jakub Narebski]

Dnia sobota 6. grudnia 2008 01:46, Junio C Hamano napisał:
> Jakub Narebski <jnareb@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]},
>>>  );
> >
> > [...]  Also features are usually not overridable
> > by default, which reduces load a tiny bit (by _possibly_ not reading
> > config, although that shouldn't matter much now with reading whole
> > commit using single call to git-config, and not one call per variable).
> > And I think the default might be set larger: 'log' view generates
> > as big if not bigger load, and it is split into 100 commits long
> > pages.
> 
> I do not think defaulting to 'no' for overridability nor defaulting a new
> feature to 'disabled' have much to do with the load, but they are more
> about the principle of least surprise.  Somebody who runs gitweb in the
> playpen he was given on the server shouldn't be getting a phone call from
> his users late at night complaining that the page his gitweb serves look
> different and has one extra link per each line, only because the sysadmin
> of the server decided to update git to 1.6.1 without telling him.
> 
> Once a new version capable of serving a new feature is introduced, he can
> plan, announce and deploy by switching the feature on in his gitweb
> configuration file.
> 
> Some things, like sitewide default css changes, cannot be made disabled
> by default.  But a new feature can easily be kept disabled by default not
> to cause needless surprises.

Well, 'search', 'grep' and 'pickaxe' features are enabled by default,
but I think it is cause by the fact that they predate %features.

But we have also 'snapshot' feature, which like 'patches' is not simply
on/off but is configurable feature, like 'patches' adds new action and
does modify existing actions only by adding extra links... and which is
enabled by default.

So there... ;-)
-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> But we have also 'snapshot' feature, which like 'patches' is not simply
> on/off but is configurable feature, like 'patches' adds new action and
> does modify existing actions only by adding extra links... and which is
> enabled by default.

If my "git log" is telling the true story, "snapshot" was introduced in
cb9c6e5 (gitweb: Support for snapshot, 2006-08-17) and it was disabled by
default.

The "feature" mechanism was introduced after that, with ddb8d90 (gitweb:
Make blame and snapshot a feature., 2006-08-20).  To avoid surprises,
"blame" were marked disabled by default to match the then-current default.
"snapshot" somehow ended up enabled with that change, though, which might
have been a mistake.

But "we erred before" is not a good excuse to deliberately err again, is
it?

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

[Giuseppe Bilotta]

On Sat, Dec 6, 2008 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@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]},
>>>  );
>>
>> You need to set "'sub' => \&feature_patches" for feature to be
>> override-able at all.  Also features are usually not overridable
>> by default, which reduces load a tiny bit (by _possibly_ not reading
>> config, although that shouldn't matter much now with reading whole
>> commit using single call to git-config, and not one call per variable).
>> And I think the default might be set larger: 'log' view generates
>> as big if not bigger load, and it is split into 100 commits long
>> pages.
>
> I do not think defaulting to 'no' for overridability nor defaulting a new
> feature to 'disabled' have much to do with the load, but they are more
> about the principle of least surprise.  Somebody who runs gitweb in the
> playpen he was given on the server shouldn't be getting a phone call from
> his users late at night complaining that the page his gitweb serves look
> different and has one extra link per each line, only because the sysadmin
> of the server decided to update git to 1.6.1 without telling him.
>
> Once a new version capable of serving a new feature is introduced, he can
> plan, announce and deploy by switching the feature on in his gitweb
> configuration file.
>
> Some things, like sitewide default css changes, cannot be made disabled
> by default.  But a new feature can easily be kept disabled by default not
> to cause needless surprises.

In the eternal war between making feature easily available and not
disturbing the user too much between upgrades, I'm more on the 'making
available' field, especially when the features are not particularly
invasive (e.g., the patch view only adds one single link, on the
navigation bar, and only in some views).

It should also be noted that if the sysadmin deploys a new software
version without telling its users, there's an obvious communication
problem between the sysadmin and its users, but that's not something
the tool developers should try to work around. Otherwise we'd still be
offering the dash version of the commands by default.

(Plus, weren't you the one suggesting that the remote heads feature
should be enabled by default? And that's an even more invasive change,
if you ask me.)


-- 
Giuseppe "Oblomov" Bilotta

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

[Petr Baudis]

On Sat, Dec 06, 2008 at 02:01:09PM +0100, Giuseppe Bilotta wrote:
> On Sat, Dec 6, 2008 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Some things, like sitewide default css changes, cannot be made disabled
> > by default.  But a new feature can easily be kept disabled by default not
> > to cause needless surprises.
> 
> In the eternal war between making feature easily available and not
> disturbing the user too much between upgrades, I'm more on the 'making
> available' field, especially when the features are not particularly
> invasive (e.g., the patch view only adds one single link, on the
> navigation bar, and only in some views).
> 
> It should also be noted that if the sysadmin deploys a new software
> version without telling its users, there's an obvious communication
> problem between the sysadmin and its users, but that's not something
> the tool developers should try to work around. Otherwise we'd still be
> offering the dash version of the commands by default.

I feel exactly the same.

Acked-by: Petr Baudis <pasky@suse.cz>

Same goes for the patches, I haven't had time to examine them in detail
but the general shape looks fine.

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

[Giuseppe Bilotta]

On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>
>> 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,
>
> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,
> to be fed to git-am. Actually it is a bit cross between "git show"
> result with '--pretty=email' format, and "git diff" between two commits,
> to be fed to git-apply or GNU patch.
>
> Nevertheless the above reasoning doesn't need to be put in a commit
> message. But it explains why new 'patch' / 'patchset' view is needed:
> because there was no equivalent.

I'll remove it.

>>  * 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.
>
> This is independent issue which is I think worth correcting anyway,
> unless we decide to scrap 'commitdiff_plain' view altogether.
> But I think we would want some text/plain patch view to be applied
> by GNU patch (for example in RPM .spec file).

I don't think we should scrap commitdiff either, but the subject
replication is not really an issue if the view is not fed to git am.

>> +     # 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]},
>>  );
>
> You need to set "'sub' => \&feature_patches" for feature to be
> override-able at all.  Also features are usually not overridable
> by default, which reduces load a tiny bit (by _possibly_ not reading
> config, although that shouldn't matter much now with reading whole
> commit using single call to git-config, and not one call per variable).

I think I'll make the feature non-overridable. I'll also make it
default to disabled, although I'm not particularly happy with the
choice.

> And I think the default might be set larger: 'log' view generates
> as big if not bigger load, and it is split into 100 commits long
> pages.

Hm, I would say the load of patch view is much higher than the load of
log view, both in terms of bandwidth and in terms of load on the
server, because of the diffs.

>>  sub git_commitdiff {
>>       my $format = shift || 'html';
>> +
>> +     my $patch_max = gitweb_check_feature('patches');
>
> Wouldn't it be better to name this variable $max_patchset_size, or
> something like that? I'm not saying that this name is bad, but I'm
> wondering if it could be better...

max_patchset_size sounds much worse than patch_max to me, which is why
I went for the latter 8-)

>>       if ($format eq 'html') {
>>               $formats_nav =
>>                       $cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
>> @@ -5483,7 +5498,12 @@ 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", '--encoding=utf8',
>> +                     '--stdout', $patch_max > 0 ? "-$patch_max" : (),
>
>> +                     $hash_parent ? ('-n', "$hash_parent..$hash") :
>> +                     ('--root', '-1', $hash)
>
> This bit makes 'patch' view behave bit differently than git-format-patch,
> which I think is a good idea: namely it shows single patch if there is
> no cutoff.  This should be mentioned in commit message, and perhaps
> even in a comment in code.
>
> Beside, if you show only single patch because $hash_parent is not set,
> you don't need to examine $patch_max nor set limit, because you use '-1'.
> Currently if $patch_max > 0 and !$hash_parent, you pass limit for the
> number of patches twice.  This I think is harmless but...

I've reworked the code a bit, making the commit spec an array computed
before passing it to the command line. The code is cleaner but
obviously longer 8-)

The double limit worked properly, btw.

>> +             # TODO add X-Git-Tag/X-Git-Url headers in a sensible way
>
> Sensible way would mean modifying git-format-patch to be able to add
> extra headers via command option, just like it is now possible via
> config variable format.headers, I think.  Because there are no surefire
> markers of where one patch ends and another begins: commit message is
> free text, and can contain diff... although if it contains diff
> separator '---' then git-am would have problem with patch; or at least
> even assuming sane commit messages it is not easy.
>
> Also I think that only X-Git-Url makes sense, and it is per whole
> patchset (whole 'patch' view output) and not for each individual
> patch.

I've stripped this commet for the time being. I'm not sure even
X-Git-Url makes sense, and the fact that it should only attached to
the first email makes it an oddball.

-- 
Giuseppe "Oblomov" Bilotta

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

[Jakub Narebski]

On Sat, Dec 6, 2008 at 13:34, Giuseppe Bilotta wrote:
> On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>>
>>> 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,
>>
>> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,
>> to be fed to git-am. Actually it is a bit cross between "git show"
>> result with '--pretty=email' format, and "git diff" between two commits,
>> to be fed to git-apply or GNU patch.
>>
>> Nevertheless the above reasoning doesn't need to be put in a commit
>> message. But it explains why new 'patch' / 'patchset' view is needed:
>> because there was no equivalent.
> 
> I'll remove it.

Errr... sorry, I haven't made myself clear. I meant here that *my*
comment should not be added; your explanation about adding 'patch'
view should IMHO stay, perhaps reworked a bit: commitdiff is not
about generating patch series.

>>>  * 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.
>>
>> This is independent issue which is I think worth correcting anyway,
>> unless we decide to scrap 'commitdiff_plain' view altogether.
>> But I think we would want some text/plain patch view to be applied
>> by GNU patch (for example in RPM .spec file).
> 
> I don't think we should scrap commitdiff either, but the subject
> replication is not really an issue if the view is not fed to git am.

Well, there is it.
 
>>> +     # 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]},
>>>  );
>>
>> You need to set "'sub' => \&feature_patches" for feature to be
>> override-able at all.  Also features are usually not overridable
>> by default, which reduces load a tiny bit (by _possibly_ not reading
>> config, although that shouldn't matter much now with reading whole
>> commit using single call to git-config, and not one call per variable).
> 
> I think I'll make the feature non-overridable. I'll also make it
> default to disabled, although I'm not particularly happy with the
> choice.

I think that having 'patches' feature to be able to override is a good
idea, as limit on number of patches might depend on _repository_, for
example being lower for repository with large commits (large in sense
of diff size).

Default with override unset seems to be precedence... as to having it
disabled by default: having feature which require configuration enabled
by default serves as an example of configuration; on the other hand the
example might be in comments, like for 'actions' %feature.
 
>> And I think the default might be set larger: 'log' view generates
>> as big if not bigger load, and it is split into 100 commits long
>> pages.
> 
> Hm, I would say the load of patch view is much higher than the load of
> log view, both in terms of bandwidth and in terms of load on the
> server, because of the diffs.

Ah, I forgot about that. Limit to 16-25 seems to be reasonable, then.
 
>>> @@ -5483,7 +5498,12 @@ 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", '--encoding=utf8',
>>> +                     '--stdout', $patch_max > 0 ? "-$patch_max" : (),
>>> +                     $hash_parent ? ('-n', "$hash_parent..$hash") :
>>> +                     ('--root', '-1', $hash)
>>
>> This bit makes 'patch' view behave bit differently than git-format-patch,
>> which I think is a good idea: namely it shows single patch if there is
>> no cutoff.  This should be mentioned in commit message, and perhaps
>> even in a comment in code.
>>
>> Beside, if you show only single patch because $hash_parent is not set,
>> you don't need to examine $patch_max nor set limit, because you use '-1'.
>> Currently if $patch_max > 0 and !$hash_parent, you pass limit for the
>> number of patches twice.  This I think is harmless but...
> 
> I've reworked the code a bit, making the commit spec an array computed
> before passing it to the command line. The code is cleaner but
> obviously longer 8-)

Cleaner is good.
 
> The double limit worked properly, btw.

Nice to know. But should we rely on corner cases handling?

-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

On Sat, Dec 6, 2008 at 2:09 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, Dec 6, 2008 at 13:34, Giuseppe Bilotta wrote:
>> On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>>>
>>>> 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,
>>>
>>> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,
>>> to be fed to git-am. Actually it is a bit cross between "git show"
>>> result with '--pretty=email' format, and "git diff" between two commits,
>>> to be fed to git-apply or GNU patch.
>>>
>>> Nevertheless the above reasoning doesn't need to be put in a commit
>>> message. But it explains why new 'patch' / 'patchset' view is needed:
>>> because there was no equivalent.
>>
>> I'll remove it.
>
> Errr... sorry, I haven't made myself clear. I meant here that *my*
> comment should not be added; your explanation about adding 'patch'
> view should IMHO stay, perhaps reworked a bit: commitdiff is not
> about generating patch series.

Oops, ok, I'll just rewrite it better 8-)

>>>>  * 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.
>>>
>>> This is independent issue which is I think worth correcting anyway,
>>> unless we decide to scrap 'commitdiff_plain' view altogether.
>>> But I think we would want some text/plain patch view to be applied
>>> by GNU patch (for example in RPM .spec file).
>>
>> I don't think we should scrap commitdiff either, but the subject
>> replication is not really an issue if the view is not fed to git am.
>
> Well, there is it.

Considering the comments on the second patch too, I've been thinking
that it might worth it to merge commitdiff_plain and patch view for
single commits. Some changes for multi-commits commitdiff_plain can be
considered too, but as I mentioned this is a major changes and should
be dealt with on its own.

>>>> +     # 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]},
>>>>  );
>>>
>>> You need to set "'sub' => \&feature_patches" for feature to be
>>> override-able at all.  Also features are usually not overridable
>>> by default, which reduces load a tiny bit (by _possibly_ not reading
>>> config, although that shouldn't matter much now with reading whole
>>> commit using single call to git-config, and not one call per variable).
>>
>> I think I'll make the feature non-overridable. I'll also make it
>> default to disabled, although I'm not particularly happy with the
>> choice.
>
> I think that having 'patches' feature to be able to override is a good
> idea, as limit on number of patches might depend on _repository_, for
> example being lower for repository with large commits (large in sense
> of diff size).
>
> Default with override unset seems to be precedence... as to having it
> disabled by default: having feature which require configuration enabled
> by default serves as an example of configuration; on the other hand the
> example might be in comments, like for 'actions' %feature.

I've added the feature_patches sub. Also, considering that there are
now basically 3 votes against 1 for enabling the feature by default,
I'll keep it enabled.

>>> And I think the default might be set larger: 'log' view generates
>>> as big if not bigger load, and it is split into 100 commits long
>>> pages.
>>
>> Hm, I would say the load of patch view is much higher than the load of
>> log view, both in terms of bandwidth and in terms of load on the
>> server, because of the diffs.
>
> Ah, I forgot about that. Limit to 16-25 seems to be reasonable, then.

I'll go with 16 for the time being, I think it's large enough to
accomodate most patchsets.

-- 
Giuseppe "Oblomov" Bilotta