threads / discuss / 16750

[PATCHv5 1/3] gitweb: add patch view

Subject: [PATCHv5 1/3] gitweb: add patch view

## tl;dr

12 messages between Dec 16, 2008 and Dec 18, 2008.

replies: 11people: 3as markdown or json

Giuseppe Bilotta· Dec 16, 2008, 10:11 UTC · lore

[PATCHv5 0/3] gitweb: patch view

Fifth iteration of the patch view in gitweb, that exposes the git-format-patch output directly, allowing patchset exchange via gitweb.

No new functionality, just cleanups suggested by Jakub.
Giuseppe Bilotta (3):
  gitweb: add patch view
  gitweb: add patches view
  gitweb: link to patch(es) view in commit(diff) and (short)log view
 gitweb/gitweb.perl |  109 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 108 insertions(+), 1 deletions(-)
Giuseppe Bilotta· Dec 16, 2008, 10:11 UTC · re: Giuseppe Bilotta · lore
The output of commitdiff_plain is not intended for git-am:
 * when given a range of commits, commitdiff_plain publishes a single
   patch with the message from the first commit, instead of a patchset
 * the hand-built email format replicates the commit summary both as
   email subject and as first line of the email itself, resulting in
   a duplication if the output is used with git-am.

We thus create a new view that can be fed to git-am directly, allowing patch exchange via gitweb. The new view exposes the output of git format-patch directly, limiting it to a single patch in the case of a single commit.

A configurable upper limit defaulting to 16 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 |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 68 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6eb370d..1006dfe 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -330,6 +330,21 @@ 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.
+
+	# To disable system wide have in $GITWEB_CONFIG
+	# $feature{'patches'}{'default'} = [0];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'patches'}{'override'} = 1;
+	# and in project config gitweb.patches = 0|n;
+	# where n is the maximum number of patches allowed in a patchset.
+	'patches' => {
+		'sub' => \&feature_patches,
+		'override' => 0,
+		'default' => [16]},
 );
 
 sub gitweb_get_feature {
@@ -411,6 +426,16 @@ sub feature_pickaxe {
 	return ($_[0]);
 }
 
+sub feature_patches {
+	my @val = (git_get_project_config('patches', '--int'));
+
+	if (@val) {
+		return @val;
+	}
+
+	return ($_[0]);
+}
+
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.
@@ -504,6 +529,7 @@ our %actions = (
 	"heads" => \&git_heads,
 	"history" => \&git_history,
 	"log" => \&git_log,
+	"patch" => \&git_patch,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5387,6 +5413,13 @@ sub git_blobdiff_plain {
 
 sub git_commitdiff {
 	my $format = shift || 'html';
+
+	my $patch_max;
+	if ($format eq 'patch') {
+		($patch_max) = gitweb_get_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");
@@ -5484,7 +5517,23 @@ 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') {
+		# For commit ranges, we limit the output to the number of
+		# patches specified in the 'patches' feature.
+		# For single commits, we limit the output to a single patch,
+		# diverging from the git-format-patch default.
+		my @commit_spec = ();
+		if ($hash_parent) {
+			if ($patch_max > 0) {
+				push @commit_spec, "-$patch_max";
+			}
+			push @commit_spec, '-n', "$hash_parent..$hash";
+		} else {
+			push @commit_spec, '-1', '--root', $hash;
+		}
+		open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
+			'--stdout', @commit_spec
+			or die_error(500, "Open git-format-patch failed");
 	} else {
 		die_error(400, "Unknown commitdiff format");
 	}
@@ -5533,6 +5582,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
@@ -5554,6 +5611,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";
 	}
 }
 
@@ -5561,6 +5623,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
Giuseppe Bilotta· Dec 16, 2008, 10:11 UTC · re: Giuseppe Bilotta · lore

[PATCHv5 2/3] gitweb: add patches view

The only difference between patch and patches view is in the treatement of single commits: the former only displays a single patch, whereas the latter displays a patchset leading to the specified commit.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1006dfe..e7a6477 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -530,6 +530,7 @@ our %actions = (
 	"history" => \&git_history,
 	"log" => \&git_log,
 	"patch" => \&git_patch,
+	"patches" => \&git_patches,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -5413,6 +5414,7 @@ sub git_blobdiff_plain {
 
 sub git_commitdiff {
 	my $format = shift || 'html';
+	my %params = @_;
 
 	my $patch_max;
 	if ($format eq 'patch') {
@@ -5529,7 +5531,15 @@ sub git_commitdiff {
 			}
 			push @commit_spec, '-n', "$hash_parent..$hash";
 		} else {
-			push @commit_spec, '-1', '--root', $hash;
+			if ($params{-single}) {
+				push @commit_spec, '-1';
+			} else {
+				if ($patch_max > 0) {
+					push @commit_spec, "-$patch_max";
+				}
+				push @commit_spec, "-n";
+			}
+			push @commit_spec, '--root', $hash;
 		}
 		open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
 			'--stdout', @commit_spec
@@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
 
 # format-patch-style patches
 sub git_patch {
+	git_commitdiff('patch', -single=> 1);
+}
+
+sub git_patches {
 	git_commitdiff('patch');
 }
 
-- 
1.5.6.5
Giuseppe Bilotta· Dec 16, 2008, 10:11 UTC · re: Giuseppe Bilotta · lore

[PATCHv5 3/3] gitweb: link to patch(es) view in commit(diff) and (short)log view

We link to patch view in commit and commitdiff view, and to patches view in log and shortlog view.

In (short)log view, the link is only offered when the number of commits shown is no more than the allowed maximum number of patches.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e7a6477..168deb7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5024,6 +5024,15 @@ sub git_log {
 
 	my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);
 
+	my ($patch_max) = gitweb_get_feature('patches');
+	if ($patch_max) {
+		if ($patch_max < 0 || @commitlist <= $patch_max) {
+			$paging_nav .= " &sdot; " .
+				$cgi->a({-href => href(action=>"patches", -replay=>1)},
+					"patches");
+		}
+	}
+
 	git_header_html();
 	git_print_page_nav('log','', $hash,undef,undef, $paging_nav);
 
@@ -5103,6 +5112,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";
@@ -5416,9 +5430,8 @@ sub git_commitdiff {
 	my $format = shift || 'html';
 	my %params = @_;
 
-	my $patch_max;
+	my ($patch_max) = gitweb_get_feature('patches');
 	if ($format eq 'patch') {
-		($patch_max) = gitweb_get_feature('patches');
 		die_error(403, "Patch view not allowed") unless $patch_max;
 	}
 
@@ -5436,6 +5449,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') {
@@ -5992,6 +6010,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=>"patches", -replay=>1)},
+					"patches");
+		}
+	}
 
 	git_header_html();
 	git_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);
-- 
1.5.6.5
Junio C Hamano· Dec 18, 2008, 06:09 UTC · re: Giuseppe Bilotta · lore

Re: [PATCHv5 2/3] gitweb: add patches view

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
Show 16 quoted lines
>  sub git_commitdiff {
>  	my $format = shift || 'html';
> +	my %params = @_;
> ...  
> +			if ($params{-single}) {
> +				push @commit_spec, '-1';
> +			} else {
> +				if ($patch_max > 0) {
> ...
> +			}
> @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
>  
>  # format-patch-style patches
>  sub git_patch {
> +	git_commitdiff('patch', -single=> 1);
> +}

Hmm, if you are changing the interface this way, wouldn't it make more sense to also do this?

	git_commitdiff(--format => 'patch', --single => 1);
	git_commitdiff(--format => 'html');
Jakub Narebski· Dec 18, 2008, 09:33 UTC · re: Junio C Hamano · lore

Re: [PATCHv5 2/3] gitweb: add patches view

On Thu, 18 Dec 2008, Junio C Hamano wrote:
Show 24 quoted lines
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> 
> >  sub git_commitdiff {
> >  	my $format = shift || 'html';
> > +	my %params = @_;
> > ...  
> > +			if ($params{-single}) {
> > +				push @commit_spec, '-1';
> > +			} else {
> > +				if ($patch_max > 0) {
> > ...
> > +			}
> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
> >  
> >  # format-patch-style patches
> >  sub git_patch {
> > +	git_commitdiff('patch', -single=> 1);
> > +}
> 
> Hmm, if you are changing the interface this way, wouldn't it make more
> sense to also do this?
> 
> 	git_commitdiff(--format => 'patch', --single => 1);
> 	git_commitdiff(--format => 'html');

The first argument (format) is _required_, second is _optional_; I'd rather use named parameters trick only for optional parameters. Because with more than one optional parameter function call begins to be cryptic; also flag (boolean) parameters are more readable when used as named parameters.

-- 
Jakub Narebski
Poland
Giuseppe Bilotta· Dec 18, 2008, 16:23 UTC · re: Jakub Narebski · lore

Re: [PATCHv5 2/3] gitweb: add patches view

On Thu, Dec 18, 2008 at 10:33 AM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 31 quoted lines
> On Thu, 18 Dec 2008, Junio C Hamano wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>
>> >  sub git_commitdiff {
>> >     my $format = shift || 'html';
>> > +   my %params = @_;
>> > ...
>> > +                   if ($params{-single}) {
>> > +                           push @commit_spec, '-1';
>> > +                   } else {
>> > +                           if ($patch_max > 0) {
>> > ...
>> > +                   }
>> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
>> >
>> >  # format-patch-style patches
>> >  sub git_patch {
>> > +   git_commitdiff('patch', -single=> 1);
>> > +}
>>
>> Hmm, if you are changing the interface this way, wouldn't it make more
>> sense to also do this?
>>
>>       git_commitdiff(--format => 'patch', --single => 1);
>>       git_commitdiff(--format => 'html');
>
> The first argument (format) is _required_, second is _optional_;
> I'd rather use named parameters trick only for optional parameters.
> Because with more than one optional parameter function call begins
> to be cryptic; also flag (boolean) parameters are more readable
> when used as named parameters.

I have mixed feelings about this: on the one hand we have href() (say) that takes all its params from a has, but on the other hand we have esc_html() (say) that takes only additional options from a hash. I'm personally more inclined towards the latter usage for git_commitdiff (i.e. this patch) but since the other alternative is straightforward I sent a v6 of the patchset which implements it.

-- 
Giuseppe "Oblomov" Bilotta
Jakub Narebski· Dec 18, 2008, 17:28 UTC · re: Giuseppe Bilotta · lore

Re: [PATCHv5 2/3] gitweb: add patches view

On Thu, Dec 18, 2008, Giuseppe Bilotta wrote:
> On Thu, Dec 18, 2008, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Thu, 18 Dec 2008, Junio C Hamano wrote:
Show 14 quoted lines
>>> [...] wouldn't it make more sense to also do this?
>>>
>>>       git_commitdiff(--format => 'patch', --single => 1);
>>>       git_commitdiff(--format => 'html');
>>
>> The first argument (format) is _required_, second is _optional_;
>> I'd rather use named parameters trick only for optional parameters.
>> Because with more than one optional parameter function call begins
>> to be cryptic; also flag (boolean) parameters are more readable
>> when used as named parameters.
> 
> I have mixed feelings about this: on the one hand we have href() (say)
> that takes all its params from a has, but on the other hand we have
> esc_html() (say) that takes only additional options from a hash. [...]

It is not "on one hand". href() is a bit special case. It has _all_ parameters optional, especially now that -replay=>1 was introduced. It has no required parameters. Therefore all parameters are named.

Beside, href() mimics a bit CGI interface...
-- 
Jakub Narebski
Poland
Junio C Hamano· Dec 18, 2008, 19:15 UTC · re: Jakub Narebski · lore

Re: [PATCHv5 2/3] gitweb: add patches view

Jakub Narebski <jnareb@gmail.com> writes:
Show 27 quoted lines
> On Thu, 18 Dec 2008, Junio C Hamano wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>> 
>> >  sub git_commitdiff {
>> >  	my $format = shift || 'html';
>> > +	my %params = @_;
>> > ...  
>> > +			if ($params{-single}) {
>> > +				push @commit_spec, '-1';
>> > +			} else {
>> > +				if ($patch_max > 0) {
>> > ...
>> > +			}
>> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
>> >  
>> >  # format-patch-style patches
>> >  sub git_patch {
>> > +	git_commitdiff('patch', -single=> 1);
>> > +}
>> 
>> Hmm, if you are changing the interface this way, wouldn't it make more
>> sense to also do this?
>> 
>> 	git_commitdiff(--format => 'patch', --single => 1);
>> 	git_commitdiff(--format => 'html');
>
> The first argument (format) is _required_, second is _optional_;

My reading of the first line of the sub seems to contradict with your statement. "If the first argument is empty, we use 'html' instead" sounds pretty optional to me.

Jakub Narebski· Dec 18, 2008, 19:44 UTC · re: Junio C Hamano · lore

Re: [PATCHv5 2/3] gitweb: add patches view

Junio C Hamano wrote:
Show 32 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
>> On Thu, 18 Dec 2008, Junio C Hamano wrote:
>>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>> 
>>>>  sub git_commitdiff {
>>>>  	my $format = shift || 'html';
>>>> +	my %params = @_;
>>>> ...  
>>>> +			if ($params{-single}) {
>>>> +				push @commit_spec, '-1';
>>>> +			} else {
>>>> +				if ($patch_max> 0) {
>>>> ...
>>>> +			}
>>>> @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
>>>>  
>>>>  # format-patch-style patches
>>>>  sub git_patch {
>>>> +	git_commitdiff('patch', -single=> 1);
>>>> +}
>>> 
>>> Hmm, if you are changing the interface this way, wouldn't it make more
>>> sense to also do this?
>>> 
>>> 	git_commitdiff(--format => 'patch', --single => 1);
>>> 	git_commitdiff(--format => 'html');
>>
>> The first argument (format) is _required_, second is _optional_;
> 
> My reading of the first line of the sub seems to contradict with your
> statement.  "If the first argument is empty, we use 'html' instead" sounds
> pretty optional to me.
 
Well, for me it is more of required argument, but with default value.
But now we argue semantics... :-)
-- 
Jakub Narebski
Poland
Jakub Narebski· Dec 18, 2008, 18:15 UTC · re: Junio C Hamano · lore

Re: [PATCHv5 2/3] gitweb: add patches view

On Thu, 18 Dec 2008, Junio C Hamano wrote:
Show 24 quoted lines
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> 
> >  sub git_commitdiff {
> >  	my $format = shift || 'html';
> > +	my %params = @_;
> > ...  
> > +			if ($params{-single}) {
> > +				push @commit_spec, '-1';
> > +			} else {
> > +				if ($patch_max > 0) {
> > ...
> > +			}
> > @@ -5625,6 +5635,10 @@ sub git_commitdiff_plain {
> >  
> >  # format-patch-style patches
> >  sub git_patch {
> > +	git_commitdiff('patch', -single=> 1);
> > +}
> 
> Hmm, if you are changing the interface this way, wouldn't it make more
> sense to also do this?
> 
> 	git_commitdiff(--format => 'patch', --single => 1);
> 	git_commitdiff(--format => 'html');

Just a though, but does it really take much sense to have "patch" and "patches" views handled in git_commitdiff? I can understand in the first version, where it was more about better 'commitdiff_plain', but I wonder about now, where patch(es) view is something between git show and log_plain format... Wouldn't it be simpler to use separate subroutine, for example git_format_patch or something?

-- 
Jakub Narebski
Poland
Giuseppe Bilotta· Dec 18, 2008, 19:57 UTC · re: Jakub Narebski · lore

Re: [PATCHv5 2/3] gitweb: add patches view

On Thu, Dec 18, 2008 at 7:15 PM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 7 quoted lines
>
> Just a though, but does it really take much sense to have "patch" and
> "patches" views handled in git_commitdiff? I can understand in the
> first version, where it was more about better 'commitdiff_plain', but
> I wonder about now, where patch(es) view is something between git show
> and log_plain format... Wouldn't it be simpler to use separate
> subroutine, for example git_format_patch or something?

I don't feel like much has changed since the beginning when IYO it made sense to have it part of commitdiff, honestly

-- 
Giuseppe "Oblomov" Bilotta

← back to recent threads