[PATCH] gitweb: show no difference message

[PATCH] gitweb: show no difference message

[Martin Koegler]

Currently, gitweb shows only header and footer, if no differences are
found. This patch adds a "No differences are found" message for the html
output.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 gitweb/gitweb.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 5214050..fbadab4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2376,6 +2376,7 @@ sub git_patchset_body {
 
 	my $patch_idx = 0;
 	my $patch_line;
+	my $empty = 0;
 	my $diffinfo;
 	my (%from, %to);
 
@@ -2396,6 +2397,7 @@ sub git_patchset_body {
 		# git diff header
 		#assert($patch_line =~ m/^diff /) if DEBUG;
 		#assert($patch_line !~ m!$/$!) if DEBUG; # is chomp-ed
+		$empty++;
 		push @diff_header, $patch_line;
 
 		# extended diff header
@@ -2559,6 +2561,8 @@ sub git_patchset_body {
 		print "</div>\n"; # class="patch"
 	}
 
+	print "<div class=\"diff header\">No differences found</div>\n" if (!$empty);
+
 	print "</div>\n"; # class="patchset"
 }
 
-- 
1.5.1.rc1.51.gb08b-dirty

[PATCH] gitweb: Support comparing blobs with different names

[Martin Koegler]

Currently, blobdiff can only compare blobs with diffenrent file
names, if no hb/hpb parameters are present.

This patch adds support for comparing two blobs specified by hb/f
and hpb/fp.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 gitweb/gitweb.perl |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index fbadab4..e3c2918 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3882,9 +3882,34 @@ sub git_blobdiff {
 	my %diffinfo;
 	my $expires;
 
+	if (defined $hash_base && defined $hash_parent_base &&
+	    defined $file_name && defined $file_parent) {
+
+		$diffinfo{'from_mode'} = $diffinfo{'to_mode'} = "blob";
+		$diffinfo{'from_file'} = $file_parent;
+		$diffinfo{'to_file'}   = $file_name;
+		$diffinfo{'status'} = '2';
+
+		if (defined $hash) {
+		    $diffinfo{'to_id'}   = $hash;
+		} else {
+		    $diffinfo{'to_id'}   = git_get_hash_by_path($hash_base, $file_name);
+		}
+		if (defined $hash_parent) {
+		    $diffinfo{'from_id'}   = $hash_parent;
+		} else {
+		    $diffinfo{'from_id'}   = git_get_hash_by_path($hash_parent_base, $file_parent);
+		}
+
+		# open patch output
+		open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,
+			$hash_parent_base.":".$file_parent, $hash_base.":".$file_name, "--"
+			or die_error(undef, "Open git-diff failed");
+	}
+
 	# preparing $fd and %diffinfo for git_patchset_body
 	# new style URI
-	if (defined $hash_base && defined $hash_parent_base) {
+	if (defined $hash_base && defined $hash_parent_base && !%diffinfo) {
 		if (defined $file_name) {
 			# read raw output
 			open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-- 
1.5.1.rc1.51.gb08b-dirty

[PATCH] gitweb: link base commit (hpb) to blobdiff output

[Martin Koegler]

blobdiff only has links for the "TO" commit. This patch
adds the same links for the "FROM" commit.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 gitweb/gitweb.perl |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e3c2918..4c371b2 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4014,6 +4014,21 @@ sub git_blobdiff {
 			                       hash_base=>$hash_base, hash_parent_base=>$hash_parent_base,
 			                       file_name=>$file_name, file_parent=>$file_parent)},
 			        "raw");
+		if (defined $hash_parent_base && (my %co = parse_commit($hash_parent_base))) {
+			$formats_nav .= " | from: ".
+				$cgi->a({-href => href(action=>"commit",
+				                       hash=>$hash_parent_base)},
+				        "commit")
+				." | ".
+				$cgi->a({-href => href(action=>"commitdiff",
+				                       hash=>$hash_parent_base)},
+				        "commitdiff")
+				." | ".
+				$cgi->a({-href => href(action=>"tree",
+						       hash=>$co{'tree'},
+				                       hash_base=>$hash_parent_base)},
+				        "tree");
+		}
 		git_header_html(undef, $expires);
 		if (defined $hash_base && (my %co = parse_commit($hash_base))) {
 			git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
-- 
1.5.1.rc1.51.gb08b-dirty

[PATCH] gitweb: support filename prefix in git_patchset_body

[Martin Koegler]

git_treediff supports comparing subdirectories. As the output of
git-difftree is missing the path to the compared directories,
the links in the output would be wrong.

The patch adds two new parameters to add the missing path prefix.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 gitweb/gitweb.perl |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4c371b2..4195b1a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2372,7 +2372,7 @@ sub git_difftree_body {
 }
 
 sub git_patchset_body {
-	my ($fd, $difftree, $hash, $hash_parent) = @_;
+	my ($fd, $difftree, $hash, $hash_parent, $file_name, $file_parent) = @_;
 
 	my $patch_idx = 0;
 	my $patch_line;
@@ -2380,6 +2380,9 @@ sub git_patchset_body {
 	my $diffinfo;
 	my (%from, %to);
 
+	$file_name = (!defined $file_name)?"":($file_name."/");
+	$file_parent = (!defined $file_parent)?"":($file_parent."/");
+
 	print "<div class=\"patchset\">\n";
 
 	# skip to first patch
@@ -2439,14 +2442,14 @@ sub git_patchset_body {
 			if ($diffinfo->{'status'} ne "A") { # not new (added) file
 				$from{'href'} = href(action=>"blob", hash_base=>$hash_parent,
 				                     hash=>$diffinfo->{'from_id'},
-				                     file_name=>$from{'file'});
+				                     file_name=>$file_parent.$from{'file'});
 			} else {
 				delete $from{'href'};
 			}
 			if ($diffinfo->{'status'} ne "D") { # not deleted file
 				$to{'href'} = href(action=>"blob", hash_base=>$hash,
 				                   hash=>$diffinfo->{'to_id'},
-				                   file_name=>$to{'file'});
+				                   file_name=>$file_name.$to{'file'});
 			} else {
 				delete $to{'href'};
 			}
-- 
1.5.1.rc1.51.gb08b-dirty

[PATCH] gitweb: support filename prefix in git_difftree_body

[Martin Koegler]

git_treediff supports comparing subdirectories. As the output of
git-difftree is missing the path to the compared directories,
the links in the output would be wrong.

The patch adds two new parameters to add the missing path prefix.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 gitweb/gitweb.perl |   40 ++++++++++++++++++++++------------------
 1 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4195b1a..95723c7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2182,8 +2182,12 @@ sub git_print_tree_entry {
 ## functions printing large fragments of HTML
 
 sub git_difftree_body {
-	my ($difftree, $hash, $parent) = @_;
+	my ($difftree, $hash, $parent, $file_name, $file_parent) = @_;
 	my ($have_blame) = gitweb_check_feature('blame');
+
+	$file_name = (!defined $file_name)?"":($file_name."/");
+	$file_parent = (!defined $file_parent)?"":($file_parent."/");
+
 	print "<div class=\"list_head\">\n";
 	if ($#{$difftree} > 10) {
 		print(($#{$difftree} + 1) . " files changed:\n");
@@ -2226,7 +2230,7 @@ sub git_difftree_body {
 			$mode_chng   .= "]</span>";
 			print "<td>";
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'},
-			                             hash_base=>$hash, file_name=>$diff{'file'}),
+			                             hash_base=>$hash, file_name=>$file_name.$diff{'file'}),
 			              -class => "list"}, esc_path($diff{'file'}));
 			print "</td>\n";
 			print "<td>$mode_chng</td>\n";
@@ -2238,7 +2242,7 @@ sub git_difftree_body {
 				print " | ";
 			}
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'},
-			                             hash_base=>$hash, file_name=>$diff{'file'})},
+			                             hash_base=>$hash, file_name=>$file_name.$diff{'file'})},
 			              "blob");
 			print "</td>\n";
 
@@ -2246,7 +2250,7 @@ sub git_difftree_body {
 			my $mode_chng = "<span class=\"file_status deleted\">[deleted $from_file_type]</span>";
 			print "<td>";
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff{'from_id'},
-			                             hash_base=>$parent, file_name=>$diff{'file'}),
+			                             hash_base=>$parent, file_name=>$file_parent.$diff{'file'}),
 			               -class => "list"}, esc_path($diff{'file'}));
 			print "</td>\n";
 			print "<td>$mode_chng</td>\n";
@@ -2258,15 +2262,15 @@ sub git_difftree_body {
 				print " | ";
 			}
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff{'from_id'},
-			                             hash_base=>$parent, file_name=>$diff{'file'})},
+			                             hash_base=>$parent, file_name=>$file_parent.$diff{'file'})},
 			              "blob") . " | ";
 			if ($have_blame) {
 				print $cgi->a({-href => href(action=>"blame", hash_base=>$parent,
-				                             file_name=>$diff{'file'})},
+				                             file_name=>$file_parent.$diff{'file'})},
 				              "blame") . " | ";
 			}
 			print $cgi->a({-href => href(action=>"history", hash_base=>$parent,
-			                             file_name=>$diff{'file'})},
+			                             file_name=>$file_parent.$diff{'file'})},
 			              "history");
 			print "</td>\n";
 
@@ -2288,7 +2292,7 @@ sub git_difftree_body {
 			}
 			print "<td>";
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'},
-			                             hash_base=>$hash, file_name=>$diff{'file'}),
+			                             hash_base=>$hash, file_name=>$file_name.$diff{'file'}),
 			              -class => "list"}, esc_path($diff{'file'}));
 			print "</td>\n";
 			print "<td>$mode_chnge</td>\n";
@@ -2303,20 +2307,20 @@ sub git_difftree_body {
 				print $cgi->a({-href => href(action=>"blobdiff",
 				                             hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'},
 				                             hash_base=>$hash, hash_parent_base=>$parent,
-				                             file_name=>$diff{'file'})},
+				                             file_name=>$file_name.$diff{'file'})},
 				              "diff") .
 				      " | ";
 			}
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'},
-			                             hash_base=>$hash, file_name=>$diff{'file'})},
+			                             hash_base=>$hash, file_name=>$file_name.$diff{'file'})},
 			               "blob") . " | ";
 			if ($have_blame) {
 				print $cgi->a({-href => href(action=>"blame", hash_base=>$hash,
-				                             file_name=>$diff{'file'})},
+				                             file_name=>$file_name.$diff{'file'})},
 				              "blame") . " | ";
 			}
 			print $cgi->a({-href => href(action=>"history", hash_base=>$hash,
-			                             file_name=>$diff{'file'})},
+			                             file_name=>$file_name.$diff{'file'})},
 			              "history");
 			print "</td>\n";
 
@@ -2330,11 +2334,11 @@ sub git_difftree_body {
 			}
 			print "<td>" .
 			      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-			                             hash=>$diff{'to_id'}, file_name=>$diff{'to_file'}),
+			                             hash=>$diff{'to_id'}, file_name=>$file_name.$diff{'to_file'}),
 			              -class => "list"}, esc_path($diff{'to_file'})) . "</td>\n" .
 			      "<td><span class=\"file_status $nstatus\">[$nstatus from " .
 			      $cgi->a({-href => href(action=>"blob", hash_base=>$parent,
-			                             hash=>$diff{'from_id'}, file_name=>$diff{'from_file'}),
+			                             hash=>$diff{'from_id'}, file_name=>$file_parent.$diff{'from_file'}),
 			              -class => "list"}, esc_path($diff{'from_file'})) .
 			      " with " . (int $diff{'similarity'}) . "% similarity$mode_chng]</span></td>\n" .
 			      "<td class=\"link\">";
@@ -2348,20 +2352,20 @@ sub git_difftree_body {
 				print $cgi->a({-href => href(action=>"blobdiff",
 				                             hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'},
 				                             hash_base=>$hash, hash_parent_base=>$parent,
-				                             file_name=>$diff{'to_file'}, file_parent=>$diff{'from_file'})},
+				                             file_name=>$file_name.$diff{'to_file'}, file_parent=>$file_parent.$diff{'from_file'})},
 				              "diff") .
 				      " | ";
 			}
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'},
-			                             hash_base=>$parent, file_name=>$diff{'to_file'})},
+			                             hash_base=>$parent, file_name=>$file_name.$diff{'to_file'})},
 			              "blob") . " | ";
 			if ($have_blame) {
 				print $cgi->a({-href => href(action=>"blame", hash_base=>$hash,
-				                             file_name=>$diff{'to_file'})},
+				                             file_name=>$file_name.$diff{'to_file'})},
 				              "blame") . " | ";
 			}
 			print $cgi->a({-href => href(action=>"history", hash_base=>$hash,
-			                            file_name=>$diff{'to_file'})},
+			                            file_name=>$file_name.$diff{'to_file'})},
 			              "history");
 			print "</td>\n";
 
-- 
1.5.1.rc1.51.gb08b-dirty

[PATCH] gitweb: Add treediff

[Martin Koegler]

treediff supports comparing different trees. A tree can be specified
either as hash or as base hash and filename.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 gitweb/gitweb.perl |  131 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 131 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 95723c7..52c0c13 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -446,6 +446,8 @@ my %actions = (
 	"tag" => \&git_tag,
 	"tags" => \&git_tags,
 	"tree" => \&git_tree,
+	"treediff" => \&git_treediff,
+	"treediff_plain" => \&git_treediff_plain,
 	"snapshot" => \&git_snapshot,
 	"object" => \&git_object,
 	# those below don't need $project
@@ -4242,6 +4244,135 @@ sub git_commitdiff_plain {
 	git_commitdiff('plain');
 }
 
+
+sub git_treediff {
+	my $format = shift || 'html';
+	my $from = $file_parent || "";
+	my $to = $file_name || "";
+
+	if (!defined $hash) {
+		if (!defined $hash_base) {
+			die_error('','tree parameter missing');
+		}
+		$hash = $hash_base;
+		$hash .= ":".$file_name if (defined $file_name);
+	}
+
+	if (!defined $hash_parent) {
+		if (!defined $hash_parent_base) {
+			die_error('','tree parameter missing');
+		}
+		$hash_parent = $hash_parent_base;
+		$hash_parent .= ":".$file_parent if (defined $file_parent);
+	}
+
+	# we need to prepare $formats_nav before any parameter munging
+	my $formats_nav;
+	if ($format eq 'html') {
+		$formats_nav =
+			$cgi->a({-href => href(action=>"treediff_plain",
+			                       hash=>$hash, hash_parent=>$hash_parent,
+			                       hash_base=>$hash_base, hash_parent_base=>$hash_parent_base,
+			                       file_name=>$file_name, file_parent=>$file_parent)},
+			        "raw");
+
+		if (defined $hash_parent_base && (my %co = parse_commit($hash_parent_base))) {
+	 		$formats_nav .= " | from: ".
+				$cgi->a({-href => href(action=>"commit",
+			        	               hash=>$hash_parent_base)},
+				        "commit")
+				." | ".
+				$cgi->a({-href => href(action=>"commitdiff",
+				                       hash=>$hash_parent_base)},
+				        "commitdiff")
+				." | ".
+				$cgi->a({-href => href(action=>"tree",
+						       hash=>$co{'tree'},
+				                       hash_base=>$hash_parent_base)},
+				        "tree");
+		}
+	}
+
+	# read treediff
+	my $fd;
+	my @difftree;
+	if ($format eq 'html') {
+		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
+			"--no-commit-id", "--patch-with-raw", "--full-index",
+			$hash_parent, $hash, "--"
+			or die_error(undef, "Open git-diff-tree failed");
+
+		while (my $line = <$fd>) {
+			chomp $line;
+			# empty line ends raw part of diff-tree output
+			last unless $line;
+			push @difftree, $line;
+		}
+
+	} elsif ($format eq 'plain') {
+		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
+			'-p', $hash_parent, $hash, "--"
+			or die_error(undef, "Open git-diff-tree failed");
+
+	} else {
+		die_error(undef, "Unknown treediff format");
+	}
+
+	# non-textual hash id's can be cached
+	my $expires;
+	if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = "+1d";
+	}
+
+	# write header
+	if ($format eq 'html') {
+
+		git_header_html(undef, $expires);
+		if (defined $hash_base && (my %co = parse_commit($hash_base))) {
+			git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
+			git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
+			print "<div class=\"title\">$hash_base:$from vs $hash_parent_base:$to</div>\n";
+		} else {
+			print "<div class=\"page_nav\"><br/>$formats_nav<br/></div>\n";
+			print "<div class=\"title\">$hash vs $hash_parent</div>\n";
+		}
+		print "<div class=\"page_body\">\n";
+
+	} elsif ($format eq 'plain') {
+		my $filename = basename($project) . "-diff.patch";
+
+		print $cgi->header(
+			-type => 'text/plain',
+			-charset => 'utf-8',
+			-expires => $expires,
+			-content_disposition => 'inline; filename="' . "$filename" . '"');
+
+		print "X-Git-Url: " . $cgi->self_url() . "\n\n";
+		print "---\n\n";
+	}
+
+	# write patch
+	if ($format eq 'html') {
+		git_difftree_body(\@difftree, $hash_base, $hash_parent_base, $file_name, $file_parent);
+		print "<br/>\n";
+
+		git_patchset_body($fd, \@difftree, $hash_base, $hash_parent_base, $file_name, $file_parent);
+		close $fd;
+		print "</div>\n"; # class="page_body"
+		git_footer_html();
+
+	} elsif ($format eq 'plain') {
+		local $/ = undef;
+		print <$fd>;
+		close $fd
+			or print "Reading git-diff-tree failed\n";
+	}
+}
+
+sub git_treediff_plain {
+	git_treediff('plain');
+}
+
 sub git_history {
 	if (!defined $hash_base) {
 		$hash_base = git_get_head_hash($project);
-- 
1.5.1.rc1.51.gb08b-dirty

Re: [PATCH] gitweb: Add treediff

[Jakub Narebski]

On Sun, Mar 25, 2007, Martin Koegler wrote:

> treediff supports comparing different trees. A tree can be specified
> either as hash or as base hash and filename.

I'd use "treediff" view, or git_treediff subroutine. Just a minor nit.
 
> +sub git_treediff {
> +	my $format = shift || 'html';
> +	my $from = $file_parent || "";
> +	my $to = $file_name || "";

I'd use  $file_name || '';  here, and of course

+	my $from = $file_parent || $file_name || '';

The unwritten rule is that we use 'fp' parameter (thet $file_parent
variable is set) only 

> +
> +	if (!defined $hash) {
> +		if (!defined $hash_base) {
> +			die_error('','tree parameter missing');

This conflicts with the coding style used elsewhere in the gitweb
(the informal coding convention / guideline for gitweb).

First, we either use  undef  and not  ''  to say: use default value
of the first parameter (HTTP status) of die_error, or provide our
own value in single quotes.

Second, the second parameter, error message, is a sentence (without
final fullstop) describing error; it means starting it with capital
letter. And we use double quotes for this parameter.

Examples:

	die_error(undef, "Couldn't find base commit");
	die_error(undef, "Unknown commit object");
	die_error(undef, "No file name defined");
	die_error(undef, "Open git-diff-tree failed");

	die_error('403 Permission denied', "Permission denied");
	die_error('404 Not Found', "File name not defined");
	die_error('404 Not Found', "Not enough information to find object");
	die_error('400 Bad Request', "Object is not a blob");

> +		}
> +		$hash = $hash_base;
> +		$hash .= ":".$file_name if (defined $file_name);
> +	}
> +
> +	if (!defined $hash_parent) {
> +		if (!defined $hash_parent_base) {
> +			die_error('','tree parameter missing');
> +		}
> +		$hash_parent = $hash_parent_base;
> +		$hash_parent .= ":".$file_parent if (defined $file_parent);

The same problem as above: we do not set 'fp' parameter (and $file_parent
variable) if name does not change. So you should use instead:

+		$hash_parent .= ":".($file_parent || $file_name)
+			if (defined $file_parent || defined $file_name);


Here I think (contrary to blobdiff case) it is better to use extended
SHA-1 syntax (taken from git-rev-parse(1)):

 * A suffix `:' followed by a path; this names the blob or tree at the given
   path in the tree-ish object named by the part before the colon.

> +	}
> +
> +	# we need to prepare $formats_nav before any parameter munging
> +	my $formats_nav;
> +	if ($format eq 'html') {
> +		$formats_nav =
> +			$cgi->a({-href => href(action=>"treediff_plain",
> +			                       hash=>$hash, hash_parent=>$hash_parent,
> +			                       hash_base=>$hash_base, hash_parent_base=>$hash_parent_base,
> +			                       file_name=>$file_name, file_parent=>$file_parent)},
> +			        "raw");


I certainly agree to that.

> +		if (defined $hash_parent_base && (my %co = parse_commit($hash_parent_base))) {
> +	 		$formats_nav .= " | from: ".
> +				$cgi->a({-href => href(action=>"commit",
> +			        	               hash=>$hash_parent_base)},
> +				        "commit")
> +				." | ".
> +				$cgi->a({-href => href(action=>"commitdiff",
> +				                       hash=>$hash_parent_base)},
> +				        "commitdiff")
> +				." | ".
> +				$cgi->a({-href => href(action=>"tree",
> +						       hash=>$co{'tree'},
> +				                       hash_base=>$hash_parent_base)},
> +				        "tree");
> +		}
> +	}

This depends if the similar change (feature) for "blobdiff" view
(in git_blobdiff subroutine) is accepted. Perhaps this could be
separated into separate patch?

> +
> +	# read treediff
> +	my $fd;
> +	my @difftree;
> +	if ($format eq 'html') {
> +		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
> +			"--no-commit-id", "--patch-with-raw", "--full-index",
> +			$hash_parent, $hash, "--"
> +			or die_error(undef, "Open git-diff-tree failed");
> +
> +		while (my $line = <$fd>) {
> +			chomp $line;
> +			# empty line ends raw part of diff-tree output
> +			last unless $line;
> +			push @difftree, $line;
> +		}

This is also common with git_commitdiff. Should be it put into separate
subroutine? Or do this refactoring in another patch?

> +	} elsif ($format eq 'plain') {
> +		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
> +			'-p', $hash_parent, $hash, "--"
> +			or die_error(undef, "Open git-diff-tree failed");

For "commitdiff_plain" view the email-like format, with commit message
and the patch is I think enough. The commit message serves as summary
for this view.

I think that it would be better for "treediff_plain" to have
whatchanged-like (with shothened sha1 for example) plain difftree
before the patchset.

> +	} else {
> +		die_error(undef, "Unknown treediff format");

And here you use standard convention to call die_error.

> +	}
> +
> +	# non-textual hash id's can be cached
> +	my $expires;
> +	if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
> +		$expires = "+1d";
> +	}
> +
> +	# write header
> +	if ($format eq 'html') {
> +
> +		git_header_html(undef, $expires);
> +		if (defined $hash_base && (my %co = parse_commit($hash_base))) {
> +			git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
> +			git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
> +			print "<div class=\"title\">$hash_base:$from vs $hash_parent_base:$to</div>\n";
> +		} else {
> +			print "<div class=\"page_nav\"><br/>$formats_nav<br/></div>\n";
> +			print "<div class=\"title\">$hash vs $hash_parent</div>\n";
> +		}
> +		print "<div class=\"page_body\">\n";

Usually we use title of the $hash_base ('hb') commit as a page header,
and I think we should do the same for "treediff" view. Only if it is not
possible we use "$hash vs $hash_parent" or equivalent. Do not cause
inconsistency, please. You can propose patch changing this, but make
it separate issue.

Additionally, "blob" view has page_path, and so has "blobdiff" view (it is
for $hash / $filename / $hash_base). And "tree" view has page_path, so
I think should "treediff" have; of course if $hash_base is defined.

> +
> +	} elsif ($format eq 'plain') {
> +		my $filename = basename($project) . "-diff.patch";
> +

In "commitdiff_plain" view we use

		my $filename = basename($project) . "-$hash.patch";

Perhaps we should use the same: "-diff.patch" does not make much sense.
Is it a typo?

[...]

Some of further code could also be shared between git_treediff and
git_commitdiff, but this could wait I guess for another separate patch.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Add treediff

[Martin Koegler]

On Mon, Mar 26, 2007 at 06:12:27PM +0100, Jakub Narebski wrote:
> On Sun, Mar 25, 2007, Martin Koegler wrote:
> > treediff supports comparing different trees. A tree can be specified
> > either as hash or as base hash and filename.
> 
> I'd use "treediff" view, or git_treediff subroutine. Just a minor nit.
>  
> > +sub git_treediff {
> > +	my $format = shift || 'html';
> > +	my $from = $file_parent || "";
> > +	my $to = $file_name || "";
> 
> I'd use  $file_name || '';  here, and of course
> 
> +	my $from = $file_parent || $file_name || '';
> 
> The unwritten rule is that we use 'fp' parameter (thet $file_parent
> variable is set) only 

How do I specifiy the root tree (=tree in commit) with hpb/fp, as fp
can not be empty, but only undefined?

> > +
> > +	if (!defined $hash) {
> > +		if (!defined $hash_base) {
> > +			die_error('','tree parameter missing');
> 
> This conflicts with the coding style used elsewhere in the gitweb
> (the informal coding convention / guideline for gitweb).
> 
> First, we either use  undef  and not  ''  to say: use default value
> of the first parameter (HTTP status) of die_error, or provide our
> own value in single quotes.
> 
> Second, the second parameter, error message, is a sentence (without
> final fullstop) describing error; it means starting it with capital
> letter. And we use double quotes for this parameter.
> 
> Examples:
> 	die_error(undef, "Couldn't find base commit");
> 	die_error(undef, "Unknown commit object");
> 	die_error(undef, "No file name defined");
> 	die_error(undef, "Open git-diff-tree failed");
> 
> 	die_error('403 Permission denied', "Permission denied");
> 	die_error('404 Not Found', "File name not defined");
> 	die_error('404 Not Found', "Not enough information to find object");
> 	die_error('400 Bad Request', "Object is not a blob");

I didn't know this. I'll change this.

> > +		}
> > +		$hash = $hash_base;
> > +		$hash .= ":".$file_name if (defined $file_name);
> > +	}
> > +
> > +	if (!defined $hash_parent) {
> > +		if (!defined $hash_parent_base) {
> > +			die_error('','tree parameter missing');
> > +		}
> > +		$hash_parent = $hash_parent_base;
> > +		$hash_parent .= ":".$file_parent if (defined $file_parent);
> 
> The same problem as above: we do not set 'fp' parameter (and $file_parent
> variable) if name does not change. So you should use instead:
> 
> +		$hash_parent .= ":".($file_parent || $file_name)
> +			if (defined $file_parent || defined $file_name);
> 

Same as above: How to specifiy the root tree with hpb/fp?

> > +		if (defined $hash_parent_base && (my %co = parse_commit($hash_parent_base))) {
> > +	 		$formats_nav .= " | from: ".
> > +				$cgi->a({-href => href(action=>"commit",
> > +			        	               hash=>$hash_parent_base)},
> > +				        "commit")
> > +				." | ".
> > +				$cgi->a({-href => href(action=>"commitdiff",
> > +				                       hash=>$hash_parent_base)},
> > +				        "commitdiff")
> > +				." | ".
> > +				$cgi->a({-href => href(action=>"tree",
> > +						       hash=>$co{'tree'},
> > +				                       hash_base=>$hash_parent_base)},
> > +				        "tree");
> > +		}
> > +	}
> 
> This depends if the similar change (feature) for "blobdiff" view
> (in git_blobdiff subroutine) is accepted. Perhaps this could be
> separated into separate patch?

I'll do that.

> > +
> > +	# read treediff
> > +	my $fd;
> > +	my @difftree;
> > +	if ($format eq 'html') {
> > +		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
> > +			"--no-commit-id", "--patch-with-raw", "--full-index",
> > +			$hash_parent, $hash, "--"
> > +			or die_error(undef, "Open git-diff-tree failed");
> > +
> > +		while (my $line = <$fd>) {
> > +			chomp $line;
> > +			# empty line ends raw part of diff-tree output
> > +			last unless $line;
> > +			push @difftree, $line;
> > +		}
> 
> This is also common with git_commitdiff. Should be it put into separate
> subroutine? Or do this refactoring in another patch?

I would like to save the refactoring for later. Maybe we will need a
change, which will not work for git_commitdiff.

> > +
> > +	} elsif ($format eq 'plain') {
> > +		my $filename = basename($project) . "-diff.patch";
> > +
> 
> In "commitdiff_plain" view we use
> 
> 		my $filename = basename($project) . "-$hash.patch";
> 
> Perhaps we should use the same: "-diff.patch" does not make much sense.
> Is it a typo?

No. What unique name do you propose? It needs to include $hash and $hase_parent.
In this case, $hash could be $hash_base:$file_name, I'm not sure, how to escape 
$file_name.

mfg Martin Kögler

Re: [PATCH] gitweb: Add treediff

[Jakub Narebski]

Martin Koegler wrote:
> On Mon, Mar 26, 2007 at 06:12:27PM +0100, Jakub Narebski wrote:
>> On Sun, Mar 25, 2007, Martin Koegler wrote:

>>> +sub git_treediff {
>>> +	my $format = shift || 'html';
>>> +	my $from = $file_parent || "";
>>> +	my $to = $file_name || "";
>> 
>> I'd use  $file_name || '';  here, and of course
>> 
>> +	my $from = $file_parent || $file_name || '';
>> 
>> The unwritten rule is that we use 'fp' parameter (thet $file_parent
>> variable is set) only 
> 
> How do I specifiy the root tree (=tree in commit) with hpb/fp, as fp
> can not be empty, but only undefined?

As I said in previous message, we can simply relax check for 'f' and 'fp'
parameters, by changing

	!validate_pathname($file_name)

to

	!defined validate_pathname($file_name)
 
Still, there are some places where we assume that 'f' and 'fp' cannot be
empty, like in above proposal. It would be:

+	my $from = (defined $file_parent ? $file_parent : $file_name) || '';


Again, I don't want to loose the assumption that we generate 'fp' _only_
if it is different from 'f'. Otherwise old links would cease working,
which breaks backwards-compatibility, and is not cool. "Cool UR_is don't
change."

>>> +
>>> +	if (!defined $hash) {
>>> +		if (!defined $hash_base) {
>>> +			die_error('','tree parameter missing');
>> 
>> This conflicts with the coding style used elsewhere in the gitweb
[...]
>> Examples:
>> 	die_error(undef, "Couldn't find base commit");
[...]
> I didn't know this. I'll change this.

What's strange in other place you used die_error accoring to coding
guidelines.

>>> +
>>> +	} elsif ($format eq 'plain') {
>>> +		my $filename = basename($project) . "-diff.patch";
>>> +
>> 
>> In "commitdiff_plain" view we use
>> 
>> 		my $filename = basename($project) . "-$hash.patch";
>> 
>> Perhaps we should use the same: "-diff.patch" does not make much sense.
>> Is it a typo?
> 
> No. What unique name do you propose? It needs to include $hash and $hase_parent.
> In this case, $hash could be $hash_base:$file_name, I'm not sure, how to escape 
> $file_name.

For "commitdiff" case sole $hash is also not unique. But if not -hash,
then perhaps -treediff instead of simply -diff?

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: support filename prefix in git_patchset_body

[Jakub Narebski]

On Sun, Mar 25, 2007, Martin Koegler wrote:

> git_treediff supports comparing subdirectories. As the output of
> git-difftree is missing the path to the compared directories,
> the links in the output would be wrong.
> 
> The patch adds two new parameters to add the missing path prefix.

Wouldn't it be better to concatenate the two "path prefix" patches
together? They are about the same thing.

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 4c371b2..4195b1a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2372,7 +2372,7 @@ sub git_difftree_body {
>  }
>  
>  sub git_patchset_body {
> -	my ($fd, $difftree, $hash, $hash_parent) = @_;
> +	my ($fd, $difftree, $hash, $hash_parent, $file_name, $file_parent) = @_;
>  
>  	my $patch_idx = 0;
>  	my $patch_line;

I'd rather use $from_prefix, $to_prefix here, or $basedif_name,
$basedir_parent, or $dir_name, $dir_parent (my preference is to
$from_prefix, $to_prefix variables).

> @@ -2380,6 +2380,9 @@ sub git_patchset_body {
>  	my $diffinfo;
>  	my (%from, %to);
>  
> +	$file_name = (!defined $file_name)?"":($file_name."/");
> +	$file_parent = (!defined $file_parent)?"":($file_parent."/");
> +
>  	print "<div class=\"patchset\">\n";
>  
>  	# skip to first patch

Minor nit: I'd rather write

+	$from_prefix = !defined $from_prefix ? '' : $from_prefix.'/';
+	$to_prefix   = !defined $to_prefix   ? '' : $to_prefix . '/';
+	$to_prefix ||= $from_prefix;  # to allow to pass common prefix once
+

or something like that, or just modify $from{'file'} and $to{'file'}

	$from{'file'} = (!defined $from_prefix ? '' : $from_prefix.'/') . $from{'file'};
	$to{'file'}   = (!defined $to_prefix   ? '' : $to_prefix . '/') . $to{'file'};

just after setting $from{'file'} and $to{'file'}, although the second
solution would additionally add prefix to the shown patch body itself.

> @@ -2439,14 +2442,14 @@ sub git_patchset_body {
>  			if ($diffinfo->{'status'} ne "A") { # not new (added) file
>  				$from{'href'} = href(action=>"blob", hash_base=>$hash_parent,
>  				                     hash=>$diffinfo->{'from_id'},
> -				                     file_name=>$from{'file'});
> +				                     file_name=>$file_parent.$from{'file'});
>  			} else {
>  				delete $from{'href'};
>  			}
>  			if ($diffinfo->{'status'} ne "D") { # not deleted file
>  				$to{'href'} = href(action=>"blob", hash_base=>$hash,
>  				                   hash=>$diffinfo->{'to_id'},
> -				                   file_name=>$to{'file'});
> +				                   file_name=>$file_name.$to{'file'});
>  			} else {
>  				delete $to{'href'};
>  			}

Another solution would be to not add additional parameters to
git_difftree_body and git_patchset_body subroutines (although it is nice
touch towards completeness), but modify %diffinfo in the caller, but this
would change also patch contents (in from-file / to-file diff header, etc.)
which might not be a good thing.

I'm not sure if we should not add information somewhere that paths are
prefixed/shortened, but this might be left for later patch.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: support filename prefix in git_patchset_body

[Martin Koegler]

On Mon, Mar 26, 2007 at 06:12:18PM +0100, Jakub Narebski wrote:
> On Sun, Mar 25, 2007, Martin Koegler wrote:
> > git_treediff supports comparing subdirectories. As the output of
> > git-difftree is missing the path to the compared directories,
> > the links in the output would be wrong.
> > 
> > The patch adds two new parameters to add the missing path prefix.
> 
> Wouldn't it be better to concatenate the two "path prefix" patches
> together? They are about the same thing.

I thought, each patch would be more readable, I split them in logical
seperate units. Anyway, I'll combine them.

> >  sub git_patchset_body {
> > -	my ($fd, $difftree, $hash, $hash_parent) = @_;
> > +	my ($fd, $difftree, $hash, $hash_parent, $file_name, $file_parent) = @_;
> >  
> >  	my $patch_idx = 0;
> >  	my $patch_line;
> 
> I'd rather use $from_prefix, $to_prefix here, or $basedif_name,
> $basedir_parent, or $dir_name, $dir_parent (my preference is to
> $from_prefix, $to_prefix variables).

I'll switch to $to_prefix and $from_prefix.

> +	$from_prefix = !defined $from_prefix ? '' : $from_prefix.'/';
> +	$to_prefix   = !defined $to_prefix   ? '' : $to_prefix . '/';
> +	$to_prefix ||= $from_prefix;  # to allow to pass common prefix once

OK. But is not the 3 line useless, as $to_prefix is alway defined
after the second line and you probable want $from_prefix ||=
$to_prefix. This will cause problems, as I currently pass the root
tree (=tree in commit object) as an missing file name parameter, as
gitweb does not allow an empty file name.

With an propagation logic, comparing a root tree with an sub tree will only
work in one direction.

So I prefer to do not implement any automatic propagation between the two prefixes.

> or something like that, or just modify $from{'file'} and $to{'file'}
> 
> 	$from{'file'} = (!defined $from_prefix ? '' : $from_prefix.'/') . $from{'file'};
> 	$to{'file'}   = (!defined $to_prefix   ? '' : $to_prefix . '/') . $to{'file'};
> 
> just after setting $from{'file'} and $to{'file'}, although the second
> solution would additionally add prefix to the shown patch body itself.

Modifing the paths before generating the links is a good idea. I'll
look, where its useful.

mfg Martin Kögler

Re: [PATCH] gitweb: support filename prefix in git_patchset_body

[Jakub Narebski]

Martin Koegler wrote:
> On Mon, Mar 26, 2007 at 06:12:18PM +0100, Jakub Narebski wrote:
>> On Sun, Mar 25, 2007, Martin Koegler wrote:
>>
>>> git_treediff supports comparing subdirectories. As the output of
>>> git-difftree is missing the path to the compared directories,
>>> the links in the output would be wrong.
>>> 
>>> The patch adds two new parameters to add the missing path prefix.
>> 
>> Wouldn't it be better to concatenate the two "path prefix" patches
>> together? They are about the same thing.
> 
> I thought, each patch would be more readable, I split them in logical
> separate units. Anyway, I'll combine them.

That was just a thought. If you think that separate patches would be
more readable, by all meens keep them splitted.

>>>  sub git_patchset_body {
>>> -	my ($fd, $difftree, $hash, $hash_parent) = @_;
>>> +	my ($fd, $difftree, $hash, $hash_parent, $file_name, $file_parent) = @_;
>>>  
>>>  	my $patch_idx = 0;
>>>  	my $patch_line;
>> 
>> I'd rather use $from_prefix, $to_prefix here, or $basedif_name,
>> $basedir_parent, or $dir_name, $dir_parent (my preference is to
>> $from_prefix, $to_prefix variables).
> 
> I'll switch to $to_prefix and $from_prefix.
> 
>> +	$from_prefix = !defined $from_prefix ? '' : $from_prefix.'/';
>> +	$to_prefix   = !defined $to_prefix   ? '' : $to_prefix . '/';
>> +	$to_prefix ||= $from_prefix;  # to allow to pass common prefix once
> 
> OK. But is not the 3 line useless, as $to_prefix is alway defined
> after the second line and you probable want $from_prefix ||=
> $to_prefix. This will cause problems, as I currently pass the root
> tree (=tree in commit object) as an missing file name parameter, as
> gitweb does not allow an empty file name.

Third line is not important, as it is you who control the calling
convention. Perhaps it should read:

+	$to_prefix = $from_prefix  if (!defined $to_prefix);

And it would be fairly easy to change gitweb to allow empty file name
parameters; simply change

	!validate_pathname($file_name)

to

	!defined validate_pathname($file_name)

(and similarly for $file_parent).

But I'd rather not change _CGI parameter_ (URI) convention that we set
'fp' (file parent) parameter *only* if it is different from 'f' (file
name). Otherwise we would introduce backwards incompatibile change,
with respect to bookmarks and old URI-s. Cool URI-s don't change...


BTW. "git rev-parse <tree-ish>:" == "git rev-parse <tree-ish>^{tree}"

> With an propagation logic, comparing a root tree with an sub tree will only
> work in one direction.
> 
> So I prefer to do not implement any automatic propagation between the
> two prefixes. 

Fine by me. It is just _internal_ call convention.
 
>> or something like that, or just modify $from{'file'} and $to{'file'}
>> 
>> 	$from{'file'} = (!defined $from_prefix ? '' : $from_prefix.'/') . $from{'file'};
>> 	$to{'file'}   = (!defined $to_prefix   ? '' : $to_prefix . '/') . $to{'file'};
>> 
>> just after setting $from{'file'} and $to{'file'}, although the second
>> solution would additionally add prefix to the shown patch body itself.
> 
> Modifing the paths before generating the links is a good idea. I'll
> look, where its useful.

Please examine consequences of this, and changes in the output if you
decide to go this way (IMHO bit simpler).

-- 
Jakub Narebski
Poland

[PATCH] gitweb: Support comparing blobs (files) with different names

[Jakub Narebski]

Fix the bug that caused "blobdiff" view called with new style URI
(it means with $hash_base ('hb') and $hash_parent_base ('hpb') denoting
tree-ish, usually commit, which have blobs being compared) for
renamed files (it means with both $file_name ('f') and $file_parent ('fp')
parameters set) to be show as new (added) file diff.

It is done by adding $file_parent ('fp') to the path limiter, meaning
that diff command becomes:

	git diff-tree [options] hpb hb -- fp f

instead of finding hash of a blob using git_get_hash_by_path subroutine
or using extended SHA-1 syntax:

	git diff [options] hpb:fp hp:f

Currently code for "blobdiff" does not support well mixed style URI,
for example asking for diff between blob given by its hash only, or
by hash and filename (without hash of tree-ish / commit), and blob
given by hash base (hash of tree-ish / commit) and filename but without
hash of blob itself, and probably would never will.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Martin, sorry for the confusing suggestion about using tree-ish:path
syntax to compare (generate diff of) two file with different name.

This patch is less invasive and I think better solution.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 5214050..c79bfeb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3885,7 +3885,7 @@ sub git_blobdiff {
 			# read raw output
 			open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
 				$hash_parent_base, $hash_base,
-				"--", $file_name
+				"--", (defined $file_parent ? $file_parent : ()), $file_name
 				or die_error(undef, "Open git-diff-tree failed");
 			@difftree = map { chomp; $_ } <$fd>;
 			close $fd
@@ -3935,7 +3935,7 @@ sub git_blobdiff {
 		# open patch output
 		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
 			'-p', $hash_parent_base, $hash_base,
-			"--", $file_name
+			"--", (defined $file_parent ? $file_parent : ()), $file_name
 			or die_error(undef, "Open git-diff-tree failed");
 	}
 
-- 
1.5.0.5

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Martin Koegler]

On Mon, Mar 26, 2007 at 06:12:09PM +0100, Jakub Narebski wrote:
> It is done by adding $file_parent ('fp') to the path limiter, meaning
> that diff command becomes:
> 
> 	git diff-tree [options] hpb hb -- fp f
> 
> instead of finding hash of a blob using git_get_hash_by_path subroutine
> or using extended SHA-1 syntax:
> 
> 	git diff [options] hpb:fp hp:f
> 

As far as I tested, this only works for renames, not
for comparing different objects, eg:

git-diff -r -p 08727ea8bba8c81678e5cf15124ada23ad097bc3:grep.h bb95e19c5f1e470d2efe1c0e4e04c291019e4b25:refs.h
shows differences

git-diff-tree -r 08727ea8bba8c81678e5cf15124ada23ad097bc3 bb95e19c5f1e470d2efe1c0e4e04c291019e4b25 -- grep.h refs.h
shows no difference

As I want gitweb to be able to even do such compares (not just renames),
I'll still need a solution for this.

My idea is, that if I got hb:f and hpb:fp, the user exactly specified
the blobs to be compared. Then I don't want any guessing logic.

mfg Martin Kögler

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Jakub Narebski]

Martin Koegler wrote:
> On Mon, Mar 26, 2007 at 06:12:09PM +0100, Jakub Narebski wrote:

>> It is done by adding $file_parent ('fp') to the path limiter, meaning
>> that diff command becomes:
>> 
>> 	git diff-tree [options] hpb hb -- fp f
>> 
>> instead of finding hash of a blob using git_get_hash_by_path subroutine
>> or using extended SHA-1 syntax:
>> 
>> 	git diff [options] hpb:fp hp:f
>> 
> 
> As far as I tested, this only works for renames, not
> for comparing different objects, eg:
> 
> git-diff -r -p 08727ea8bba8c81678e5cf15124ada23ad097bc3:grep.h bb95e19c5f1e470d2efe1c0e4e04c291019e4b25:refs.h
> shows differences
> 
> git-diff-tree -r 08727ea8bba8c81678e5cf15124ada23ad097bc3 bb95e19c5f1e470d2efe1c0e4e04c291019e4b25 -- grep.h refs.h
> shows no difference
> 
> As I want gitweb to be able to even do such compares (not just renames),
> I'll still need a solution for this.

Well, I haven't thought about it. Still, it is two lines changed only,
and it fixes a bug in "blobdiff" view for rename diffs.
 
Perhaps mine patch should go to 'maint', and yours to 'master' or 'next'
branch.

> My idea is, that if I got hb:f and hpb:fp, the user exactly specified
> the blobs to be compared. Then I don't want any guessing logic.

I'd rather you reuse the "no hash_parent" code, which also hand-crafts
diffinfo. Perhaps removing "git-diff-tree hpb hb -- f" code entirely.
Besides, code dealing with "blobdiff" coming from "commit", "commitdiff"
and "history" views are tested to work as expected, not so with
arbitrary diffs.

By the way, if you call git_get_hash_by_path (which is expensive, as it
calls git command), you can use resulting hash in place of
hash_base:filename as an argument to git-diff.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Martin Koegler]

On Tue, Mar 27, 2007 at 01:56:24AM +0100, Jakub Narebski wrote:
> Martin Koegler wrote:
> > My idea is, that if I got hb:f and hpb:fp, the user exactly specified
> > the blobs to be compared. Then I don't want any guessing logic.
> 
> I'd rather you reuse the "no hash_parent" code, which also hand-crafts
> diffinfo. Perhaps removing "git-diff-tree hpb hb -- f" code entirely.
> Besides, code dealing with "blobdiff" coming from "commit", "commitdiff"
> and "history" views are tested to work as expected, not so with
> arbitrary diffs.

I don't like the whole rename detection code, so I offer to simplify
git_blobdiff. For all calls to git_blobdiff (except those from git_history),
I'm sure, that I can assume $file_parent ||= $file_name.

If you think, its safe, I can simplify git_blobdiff. I propose
doing the following way (pseudo-code):

sub git_blobdiff {
$file_parent ||= $file_name;

if (defined $hash_base && defined $file_name && !defined $hash)
$hash=git_get_hash_by_path($hash_base,$file_name);

if (defined $hash_parent_base && defined $file_parent && !defined $hash_parent)
$hash_parent=git_get_hash_by_path($hash_parent_base,$file_parent);

if (!defined $hash || ! defined $hash_parent )
die_error(....);

$diffinfo{'from_mode'} = $diffinfo{'to_mode'} = "blob";
$diffinfo{'from_file'} = $file_parent || $hash_parent;
$diffinfo{'to_file'}   = $file_name || $hash;
$diffinfo{'status'} = '2';
$diffinfo{'to_id'}   = $hash;
$diffinfo{'from_id'}   = $hash_parent;

 open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,
    $hash_parent, $hash
    or die_error(undef, "Open git-diff failed");

/* print output */

}

Else I will keep a reworked version of my patch.

> By the way, if you call git_get_hash_by_path (which is expensive, as it
> calls git command), you can use resulting hash in place of
> hash_base:filename as an argument to git-diff.
 
I must check, if we need to resolve $hash ($hash_parent) by
git_get_hash_by_path, if we construct it out of $hash_base and
$file_name. Maybe we can avoid this call.

mfg Martin Kögler
 

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Jakub Narebski]

On Thu, Mar 27, 2007, Martin Koegler wrote:
> On Tue, Mar 27, 2007 at 01:56:24AM +0100, Jakub Narebski wrote:
>> Martin Koegler wrote:
>>> My idea is, that if I got hb:f and hpb:fp, the user exactly specified
>>> the blobs to be compared. Then I don't want any guessing logic.
>> 
>> I'd rather you reuse the "no hash_parent" code, which also hand-crafts
>> diffinfo. Perhaps removing "git-diff-tree hpb hb -- f" code entirely.
>> Besides, code dealing with "blobdiff" coming from "commit", "commitdiff"
>> and "history" views are tested to work as expected, not so with
>> arbitrary diffs.
> 
> I don't like the whole rename detection code, so I offer to simplify
> git_blobdiff. For all calls to git_blobdiff (except those from git_history),
> I'm sure, that I can assume $file_parent ||= $file_name.

That was the idea. Perhaps I haven't said it clearly, but I wanted to
suggest to remove the whole git-diff-tree code, and use git-diff to
generate diff between blobs.

> If you think, its safe, I can simplify git_blobdiff. I propose
> doing the following way (pseudo-code):

> $file_parent ||= $file_name;
[...]
> $hash=git_get_hash_by_path($hash_base,$file_name);
[...]
> $hash_parent=git_get_hash_by_path($hash_parent_base,$file_parent);
[...]
>  open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,
>     $hash_parent, $hash
>     or die_error(undef, "Open git-diff failed");
[...]
> Else I will keep a reworked version of my patch.

The trouble with this is that we may lose mode change (symlink to
ordinary file etc.) because we hand-generate %diffinfo.

>> By the way, if you call git_get_hash_by_path (which is expensive, as it
>> calls git command), you can use resulting hash in place of
>> hash_base:filename as an argument to git-diff.
>  
> I must check, if we need to resolve $hash ($hash_parent) by
> git_get_hash_by_path, if we construct it out of $hash_base and
> $file_name. Maybe we can avoid this call.

We can use "$hash_base:$file_name" as second parameter to git-diff etc.,
but I don't think we want to create links with "$hash_base:$file_name"
instead of sha-1 id of a blob as 'h' parameter.

It can be first implementation, thought, and later we can try to use
"index <hash>..<hash> <mode>" lines from extended header to get $hash
and $hash_parent (with exception of pure rename, but then we need only
one invocation of git_get_hash_by_path subroutine).

But I think it is better left for later patch.
-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Martin Koegler]

On Wed, Mar 28, 2007 at 12:58:54AM +0100, Jakub Narebski wrote:
> On Thu, Mar 27, 2007, Martin Koegler wrote:
> > On Tue, Mar 27, 2007 at 01:56:24AM +0100, Jakub Narebski wrote:
> > If you think, its safe, I can simplify git_blobdiff. I propose
> > doing the following way (pseudo-code):
> 
> > $file_parent ||= $file_name;
> [...]
> > $hash=git_get_hash_by_path($hash_base,$file_name);
> [...]
> > $hash_parent=git_get_hash_by_path($hash_parent_base,$file_parent);
> [...]
> >  open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,
> >     $hash_parent, $hash
> >     or die_error(undef, "Open git-diff failed");
> [...]
> > Else I will keep a reworked version of my patch.
> 
> The trouble with this is that we may lose mode change (symlink to
> ordinary file etc.) because we hand-generate %diffinfo.

If we need the correct mode in %diffinfo, this is not difficult:

$from_mode="blob";
if (defined $hash_base && defined $file_name)
($to_mode,$hash)=git_get_hash_by_path($hash_base,$file_name);

$to_mode="blob";
if (defined $hash_parent_base && defined $file_parent)
($from_mode,$hash_parent)=git_get_hash_by_path_mode($hash_parent_base,$file_parent);

Then we set to_mode and from_mode in %diffinfo to $to_mode and $from_mode.

git_get_hash_by_path_mode is like git_get_hash_by_path, only that it
returns an array with the mode and the hash.

Blobdiff (html output) in its current version can not handle symlinks:
> diff --git a/x b/x
> deleted file mode 100644 (file)
> index 190a180..873fb8d
> --- a/x
> +++ /dev/null
> @@ -1 +0,0 @@
> -123
> diff --git a/ b/
> new file mode 120000 (symlink)
> index 190a180..873fb8d
> --- /dev/null
> +++ b/
> @@ -0,0 +1 @@
> +file3
> \ No newline at end of file

This was generated by "diff to current" in the history view of a file,
which was changed between symlink and normal file.
 
Additionally, to_mode and from_mode of %diffinfo seem to be ignored by
git_patchset_body.

> >> By the way, if you call git_get_hash_by_path (which is expensive, as it
> >> calls git command), you can use resulting hash in place of
> >> hash_base:filename as an argument to git-diff.
> >  
> > I must check, if we need to resolve $hash ($hash_parent) by
> > git_get_hash_by_path, if we construct it out of $hash_base and
> > $file_name. Maybe we can avoid this call.
> 
> We can use "$hash_base:$file_name" as second parameter to git-diff etc.,
> but I don't think we want to create links with "$hash_base:$file_name"
> instead of sha-1 id of a blob as 'h' parameter.
> 
> It can be first implementation, thought, and later we can try to use
> "index <hash>..<hash> <mode>" lines from extended header to get $hash
> and $hash_parent (with exception of pure rename, but then we need only
> one invocation of git_get_hash_by_path subroutine).

The <mode> must be discarded, as it is wrong for anything which as not
a mode of 100644, as we specify a two blob as parameter to git-diff.

> But I think it is better left for later patch.

As git_patchset_body requires an information about the compared file
as parameter, a new formating function will be needed. I'm not sure,
if the overhead of git_get_hash_by_path is this worth. Additionally,
if we have the hash passed by parameter in most cases, there is no
need to call it in these cases.

mfg Martin Kögler
PS:
I created a blob with a "strange" filename: &()=!"§$%[<>]*#+_;.
In the result of the blob view, the " is not escaped in the filename in the header
and a strage content type is returned:

$ telnet localhost 80
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET /gitweb/gitweb.cgi?p=t/.git;a=blob;f=%26()%3D%21%22%A7%24%25%5B%3C%3E%5D%2A%23%2B_%3B.;hb=7bfed2588bee66b33db544830606fa6606478fd9 HTTP/1.0

HTTP/1.1 200 OK
Date: Wed, 28 Mar 2007 19:55:36 GMT
Server: Apache
Content-disposition: inline; filename="&()=!"§$%[<>]*#+_;."
Expires: Thu, 29 Mar 2007 19:55:39 GMT
Connection: close
Content-Type: application/vnd.mif

xx

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Jakub Narebski]

On Wed, Mar 28, 2007 at 23:03 +0200, Martin Koegler wrote:
> On Wed, Mar 28, 2007 at 12:58:54AM +0100, Jakub Narebski wrote:
>> On Thu, Mar 27, 2007, Martin Koegler wrote:
>>> On Tue, Mar 27, 2007 at 01:56:24AM +0100, Jakub Narebski wrote:

>>> If you think, its safe, I can simplify git_blobdiff. I propose
>>> doing the following way (pseudo-code):
>> 
>>> $file_parent ||= $file_name;
>> [...]
>>> $hash=git_get_hash_by_path($hash_base,$file_name);
>> [...]
>>> $hash_parent=git_get_hash_by_path($hash_parent_base,$file_parent);
>> [...]
>>>  open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,
>>>     $hash_parent, $hash
>>>     or die_error(undef, "Open git-diff failed");
>> [...]
>>> Else I will keep a reworked version of my patch.
>> 
>> The trouble with this is that we may lose mode change (symlink to
>> ordinary file etc.) because we hand-generate %diffinfo.
> 
> If we need the correct mode in %diffinfo, this is not difficult:
> 
> $from_mode="blob";
> if (defined $hash_base && defined $file_name)
> ($to_mode,$hash)=git_get_hash_by_path($hash_base,$file_name);
[...]
> Then we set to_mode and from_mode in %diffinfo to $to_mode and $from_mode.
> 
> git_get_hash_by_path_mode is like git_get_hash_by_path, only that it
> returns an array with the mode and the hash.

I'd rather either have git_get_ls_tree_line_by_path, which we would then
parse using parse_ls_tree_line, or have git_get_info_by_path which would
return already parsed information (as hash reference, or as a list).

>>>> By the way, if you call git_get_hash_by_path (which is expensive, as it
>>>> calls git command), you can use resulting hash in place of
>>>> hash_base:filename as an argument to git-diff.
>>>  
>>> I must check, if we need to resolve $hash ($hash_parent) by
>>> git_get_hash_by_path, if we construct it out of $hash_base and
>>> $file_name. Maybe we can avoid this call.
>> 
>> We can use "$hash_base:$file_name" as second parameter to git-diff etc.,
>> but I don't think we want to create links with "$hash_base:$file_name"
>> instead of SHA-1 id (hash) of a blob as 'h' parameter.
>> 
>> It can be first implementation, thought, and later we can try to use
>> "index <hash>..<hash> <mode>" lines from extended header to get $hash
>> and $hash_parent (with exception of pure rename, but then we need only
>> one invocation of git_get_hash_by_path subroutine).
> 
> The <mode> must be discarded, as it is wrong for anything which as not
> a mode of 100644, as we specify a two blob as parameter to git-diff.

The <mode> information might be discarded, or might be saved. "blob"
can have 100644 mode, can have 100755 mode (executable file), or can
have 120000 mode (symbolic link).
 
>> But I think it is better left for later patch.
> 
> As git_patchset_body requires an information about the compared file
> as parameter, a new formating function will be needed. I'm not sure,
> if the overhead of git_get_hash_by_path is this worth. Additionally,
> if we have the hash passed by parameter in most cases, there is no
> need to call it in these cases.

git_patchset_body currently buffers (caches) diff header (up to 
from-file / to-file header) to catch situation where one "raw" format
line (one difftree line) correspond to two patches (like e.g. type change
situation below, from ordinary file to symlink or vice versa, and which
_should_ be handled by git_patchset_body). So I think it would not be
hard to parse extended diff header and fill values which are not present
in %diffinfo from extended diff header. This includes filling $from_hash
and $to_hash ($hash_parent and $hash) information from the 
"index <hash>..<hash> <mode>" or "index <hash>..<hash>" extended diff
header line.

The (only?) rare exception is when files (blobs) does _not_ differ, as
patch in this situation is empty, and we would have to get hash of blob
(which would be the same for from and to, for $hash_parent and $hash)
using git_get_hash_by_name or new git_get_info_by_name.

> Blobdiff (html output) in its current version can not handle symlinks:

I'd investigate that. Is "commitdiff" view correct in this situation?

>> diff --git a/x b/x
>> deleted file mode 100644 (file)
>> index 190a180..873fb8d
>> --- a/x
>> +++ /dev/null
>> @@ -1 +0,0 @@
>> -123
>> diff --git a/ b/
>> new file mode 120000 (symlink)
>> index 190a180..873fb8d
>> --- /dev/null
>> +++ b/
>> @@ -0,0 +1 @@
>> +file3
>> \ No newline at end of file
> 
> This was generated by "diff to current" in the history view of a file,
> which was changed between symlink and normal file.


> Additionally, to_mode and from_mode of %diffinfo seem to be ignored by
> git_patchset_body.

As it should, I think.

> mfg Martin Kögler
> PS:
> I created a blob with a "strange" filename: &()=!"§$%[<>]*#+_;.
> In the result of the blob view, the " is not escaped in the filename in the header
> and a strange content type is returned:
> 
> $ telnet localhost 80
> Trying 127.0.0.1...
> Connected to localhost.
> Escape character is '^]'.
> GET /gitweb/gitweb.cgi?p=t/.git;a=blob;f=%26()%3D%21%22%A7%24%25%5B%3C%3E%5D%2A%23%2B_%3B.;hb=7bfed2588bee66b33db544830606fa6606478fd9 HTTP/1.0
> 
> HTTP/1.1 200 OK
> Date: Wed, 28 Mar 2007 19:55:36 GMT
> Server: Apache
> Content-disposition: inline; filename="&()=!"§$%[<>]*#+_;."
> Expires: Thu, 29 Mar 2007 19:55:39 GMT
> Connection: close
> Content-Type: application/vnd.mif
> 
> xx

I'd try to check that

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Jakub Narebski]

On Wed, Mar 28, 2007, Martin Koegler wrote:

> I created a blob with a "strange" filename: &()=!"§$%[<>]*#+_;.
> In the result of the blob view, the " is not escaped in the filename in the header
> and a strage content type is returned:
> 
> $ telnet localhost 80
> Trying 127.0.0.1...
> Connected to localhost.
> Escape character is '^]'.
> GET /gitweb/gitweb.cgi?p=t/.git;a=blob;f=%26()%3D%21%22%A7%24%25%5B%3C%3E%5D%2A%23%2B_%3B.;hb=7bfed2588bee66b33db544830606fa6606478fd9 HTTP/1.0
> 
> HTTP/1.1 200 OK
> Date: Wed, 28 Mar 2007 19:55:36 GMT
> Server: Apache
> Content-disposition: inline; filename="&()=!"§$%[<>]*#+_;."
> Expires: Thu, 29 Mar 2007 19:55:39 GMT
> Connection: close
> Content-Type: application/vnd.mif
> 
> xx

There are two separate things. 

First is not escaped filename in HTTP header. There was some discussion
about this, and even patch by Luben Tuikov which added to_qtext 
subroutine to deal with escaping in HTTP (which has diferent rules than
escaping in HTML, or in HTML attributes)
 * gitweb: using quotemeta
   http://thread.gmane.org/gmane.comp.version-control.git/28050/
 * [PATCH] gitweb: Convert Content-Disposition filenames into qtext
   http://thread.gmane.org/gmane.comp.version-control.git/28437
But the patch was newer accepted; either lost in the noise, or in lack
of summary to the discussion.

Second is detecting file as binary (for text files we always return
HTML), and content type set. Gitweb checks mimetype of a file (based
on extension, mainly) by default using /etc/mime.magic, and if it is
neither text/* nor HTML images (png, jpg, gif) it uses "blobdiff_plain"
view. Perhaps instead of just calling git_blob_plain($mimetype) we
should do a redirect (but then how to pass detected mimetype?). It looks
like there is an error with mimetype detection: could you tell me the
line with "application/vnd.mif" in your /etc/mime.types?

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Martin Koegler]

On Sat, Mar 31, 2007 at 01:55:14AM +0200, Jakub Narebski wrote:
> Second is detecting file as binary (for text files we always return
> HTML), and content type set. Gitweb checks mimetype of a file (based
> on extension, mainly) by default using /etc/mime.magic, and if it is
> neither text/* nor HTML images (png, jpg, gif) it uses "blobdiff_plain"
> view. Perhaps instead of just calling git_blob_plain($mimetype) we
> should do a redirect (but then how to pass detected mimetype?). It looks
> like there is an error with mimetype detection: could you tell me the
> line with "application/vnd.mif" in your /etc/mime.types?

The system is a Debian system. /etc/mime.types is part of mime-support:
$dpkg -l mime-support
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Installed/Config-files/Unpacked/Failed-config/Half-installed
|/ Err?=(none)/Hold/Reinst-required/X=both-problems (Status,Err: uppercase=bad)
||/ Name                         Version                      Description
+++-============================-============================-========================================================================
ii  mime-support                 3.28-1                       MIME files 'mime.types' & 'mailcap', and support programs

$ grep vnd.mif /etc/mime.types
application/vnd.mif           mif

As it's a config file, this line could be added by an other package.

The context of this entry is:
application/vnd.koan
application/vnd.lotus-1-2-3
application/vnd.lotus-approach
application/vnd.lotus-freelance
application/vnd.lotus-notes
application/vnd.lotus-organizer
application/vnd.lotus-screencam
application/vnd.lotus-wordpro
application/vnd.mcd
application/vnd.mediastation.cdkey
application/vnd.meridian-slingshot
application/vnd.mif           mif
application/vnd.minisoft-hp3000-save
application/vnd.mitsubishi.misty-guard.trustweb
application/vnd.mobius.daf
application/vnd.mobius.dis
application/vnd.mobius.msl
application/vnd.mobius.plc
application/vnd.mobius.txf
application/vnd.motorola.flexsuite
application/vnd.motorola.flexsuite.adsi
application/vnd.motorola.flexsuite.fis
application/vnd.motorola.flexsuite.gotap

Hope, it helps.

mfg Martin Kögler

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Jakub Narebski]

On Sat, Mar 31, 2007 at 01:55 +0200, Jakub Narebski wrote:
> On Wed, Mar 28, 2007, Martin Koegler wrote:
> 
>> I created a blob with a "strange" filename: &()=!"§$%[<>]*#+_;.
>> In the result of the blob view, the " is not escaped in the filename in the header
>> and a strange content type is returned:
>> 
>> $ telnet localhost 80
>> Trying 127.0.0.1...
>> Connected to localhost.
>> Escape character is '^]'.
>> GET /gitweb/gitweb.cgi?p=t/.git;a=blob;f=%26()%3D%21%22%A7%24%25%5B%3C%3E%5D%2A%23%2B_%3B.;hb=7bfed2588bee66b33db544830606fa6606478fd9 HTTP/1.0
>> 
>> HTTP/1.1 200 OK
>> Date: Wed, 28 Mar 2007 19:55:36 GMT
>> Server: Apache
>> Content-disposition: inline; filename="&()=!"§$%[<>]*#+_;."
>> Expires: Thu, 29 Mar 2007 19:55:39 GMT
>> Connection: close
>> Content-Type: application/vnd.mif
>> 
>> xx
> 
> There are two separate things. 
> 
> First is not escaped filename in HTTP header. There was some discussion
> about this, and even patch by Luben Tuikov which added to_qtext 
> subroutine to deal with escaping in HTTP (which has diferent rules than
> escaping in HTML, or in HTML attributes)
>  * gitweb: using quotemeta
>    http://thread.gmane.org/gmane.comp.version-control.git/28050/
>  * [PATCH] gitweb: Convert Content-Disposition filenames into qtext
>    http://thread.gmane.org/gmane.comp.version-control.git/28437
> But the patch was newer accepted; either lost in the noise, or in lack
> of summary to the discussion.

Junio, do you remember by chance why this patch was dropped?

> Second is detecting file as binary (for text files we always return
> HTML), and content type set. Gitweb checks mimetype of a file (based
> on extension, mainly) by default using /etc/mime.magic, and if it is
> neither text/* nor HTML images (png, jpg, gif) it uses "blobdiff_plain"
> view. Perhaps instead of just calling git_blob_plain($mimetype) we
> should do a redirect (but then how to pass detected mimetype?). It looks
> like there is an error with mimetype detection: could you tell me the
> line with "application/vnd.mif" in your /etc/mime.types?


Unfortunately trying to find the source of this error failed (perhaps
due to name of file slightly changed by accident), and I get no errors.
WORKSFORME aka SAA#1 (Standard Answer of Admin #1: Works for me).

I use the following script to debug errors in gitweb:

-- >8 ------------------------------------
#!/bin/sh

export GATEWAY_INTERFACE="CGI/1.1"
export HTTP_ACCEPT="*/*"
export REQUEST_METHOD="GET"
export QUERY_STRING=""$1""
export PATH_INFO=""$2""

ddd "/var/www/cgi-bin/gitweb/gitweb.cgi"
-- >8 ------------------------------------

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Jakub Narebski]

On Sun, Apr 1, 2007, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>>> First is not escaped filename in HTTP header. There was some discussion
>>> about this, and even patch by Luben Tuikov which added to_qtext 
>>> subroutine to deal with escaping in HTTP (which has diferent rules than
>>> escaping in HTML, or in HTML attributes)
>>>  * gitweb: using quotemeta
>>>    http://thread.gmane.org/gmane.comp.version-control.git/28050/
>>>  * [PATCH] gitweb: Convert Content-Disposition filenames into qtext
>>>    http://thread.gmane.org/gmane.comp.version-control.git/28437
>>> But the patch was newer accepted; either lost in the noise, or in lack
>>> of summary to the discussion.
>>
>> Junio, do you remember by chance why this patch was dropped?
> 
> No, but I suspect that was because the noisiness of the thread
> around them suggested they were not ready to be applied.  I do
> not remember if people submitted the patch and commented on
> reached a consensus.

Probably not. Here is alternative proposal. It does not implement
  RFC2184: MIME Parameter Value and Encoded Word Extensions
but I'm not sure if 1) it is needed for _HTTP_ Content-Disposition
header filename, 2) all browsers implement it.

By the way, $str =~ s/[\n\r]/_/g; line (as per Junio Hamano and Petr
Baudis suggestion) is needed not only for buggy browsers, but also for
buggy CGI implementation:

  $ perl -wle \
  'use CGI; \
   our $cgi = new CGI; \
   print $cgi->header(-content_disposition => "inline; filename=\"file\nname\"");'

generates (for CGI version 3.10)

  Content-disposition="inline; filename=&quot;file
  name&quot;"

which is a bit strange. Single LF (not CRLF pair) does not need to be
quoted in the header, as per RFC822.

-- >8 --
# Generate value of Content-Disposition header field, with "inline"
# disposition type, for a given filename parameter
# Usage: $cgi->header( [...],
#          -content_disposition => content_disposition($filename))
# References: RFC 2183, RFC 822 and RFC 2045
sub content_disposition {
	my $filename = shift;

	#RFC2183: The Content-Disposition Header Field
	# parameter value containing only non-`tspecials' characters [RFC 2045]
	# SHOULD be represented as a single `token'.
	#RFC2045: MIME Part One: Format of Internet Message Bodies
	# token := 1*<any (US-ASCII) CHAR except SPACE, CTLs,
	#             or tspecials>
	if ($filename =~ m/[[:space:][:cntrl:]()<>@,;:\\"\/\[\]?=]/) {
		#RFC2183: The Content-Disposition Header Field
		# parameter value containing only ASCII characters, but including
		# `tspecials' characters, SHOULD be represented as `quoted-string'.

		# It not worth potential problems to try to carry newlines (and such)
		# in the header; it is just _suggested_ filename
		$filename =~ s/[[:cntrl:]\n\r]/_/g;

		#RFC822: Standard for the Format of ARPA Internet Text Messages
		# quoting is REQUIRED for CR and "\" and for the character(s) that
		# delimit the token (e.g., "(" and ")" for a comment).  However,
		# quoting is PERMITTED for any character.
		$filename =~ s/([\\"\r])/\\$1/g;
		$filename = '"' . $filename . '"';
	}
	return "inline; filename=$filename";
}
-- >8 --
P.S. We could probably always quote filename parameter, even if it
is not needed ("SHOULD be represented as a single `token'" part).

P.P.S. Here is an example of RFC2184 encoded header:

   Content-Type: application/x-stuff
    title*1*=us-ascii'en'This%20is%20even%20more%20
    title*2*=%2A%2A%2Afun%2A%2A%2A%20
    title*3="isn't it!"

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Jakub Narebski]

On Tue, Apr 03, 2007 at 16:57 +0200, Jakub Narebski wrote:
> On Sun, Apr 1, 2007, Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>>>> First is not escaped filename in HTTP header. There was some discussion
>>>> about this, and even patch by Luben Tuikov which added to_qtext 
>>>> subroutine to deal with escaping in HTTP [...]
>>>
>>> Junio, do you remember by chance why this patch was dropped?
>> 
>> No, but I suspect that was because the noisiness of the thread
>> around them suggested they were not ready to be applied.  I do
>> not remember if people submitted the patch and commented on
>> reached a consensus.
> 
> Probably not. Here is alternative proposal. It does not implement
>   RFC2184: MIME Parameter Value and Encoded Word Extensions
> but I'm not sure if 1) it is needed for _HTTP_ Content-Disposition
> header filename, 2) all browsers implement it.

[...]
> P.P.S. Here is an example of RFC2184 encoded header:
> 
>    Content-Type: application/x-stuff
>     title*1*=us-ascii'en'This%20is%20even%20more%20
>     title*2*=%2A%2A%2Afun%2A%2A%2A%20
>     title*3="isn't it!"

Another example:

  Content-Type: text/plain; charset=utf-8\r
  Content-Disposition: inline; filename*=utf-8'en-US'This%20is%0A%20%2A%2A%2Afun%2A%2A%2A


Although "RFC 2183: The Content-Disposition Header Field" says:

  Parameter values longer than 78 characters, or which contain non-ASCII
  characters, MUST be encoded as specified in [RFC 2184].

the limit of 78 characters is because it is was created for mail, and
some old MUA had limit on line length. It is not the case of HTTP
protocol: lines can be, and are, quite long. Besides for example
Apache 2.0.54 does not understand MUA-style continued HTTP headers
if in 'parse headers' mode: it returns server error.

As to browsers: Mozilla 1.7.12 implements RFC2183 correctly, although for
example shows %0A / \n as a strange symbol in "save as" dialog, created
file has embedded newline in filename, as it should. But both Lynx 2.8.5,
and ELinks 0.10.3 do not implement it fully and without errors.

So that is why we have:

	# It not worth potential problems to try to carry newlines
	# in the header; it is just _suggested_ filename
	$filename =~ s/[[:cntrl:]\n\r]/_/g;


P.S. If there were no objections (no discussion), I'd resend
content_disposition subroutine as patch to gitweb in about a week.

-- 
Jakub Narebski
ShadeHawk on #git
Poland

Re: [PATCH] gitweb: Support comparing blobs (files) with different names

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> As to browsers: Mozilla 1.7.12 implements RFC2183 correctly, although for
> example shows %0A / \n as a strange symbol in "save as" dialog, created
> file has embedded newline in filename, as it should. But both Lynx 2.8.5,
> and ELinks 0.10.3 do not implement it fully and without errors.
>
> So that is why we have:
>
> 	# It not worth potential problems to try to carry newlines
> 	# in the header; it is just _suggested_ filename
> 	$filename =~ s/[[:cntrl:]\n\r]/_/g;

I think the logic in the comment is very sane, although I am not
sure if saying \n \r when you say :cntrl: is necessary.

[PATCH] gitweb: Fix bug in "blobdiff" view for split (e.g. file to symlink) patches

[Jakub Narebski]

git_patchset_body needs patch generated with --full-index option to
detect split patches, meaning two patches which corresponds to single
difftree (raw diff) entry.  An example of such situation is changing
type (mode) of a file, e.g. from plain file to symbolic link.

Add, in git_blobdiff, --full-index option to patch generating git diff
invocation, for the 'html' format output ("blobdiff" view).

"blobdiff_plain" still uses shortened sha1 in the extended git diff
header "index <hash>..<hash>[ <mode>]" line.

git_patchset_body and git_commitdiff (but not git_blobdiff) were adjusted
for the split patches in commit 6d55f05576851aedc7c53f7438c1d505c22ee78f

Noticed-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Wed, Mar 28, 2007 at 23:03 +0200, Martin Koegler wrote:

> Blobdiff (html output) in its current version can not handle symlinks:
>
> > diff --git a/x b/x
> > deleted file mode 100644 (file)
> > index 190a180..873fb8d
> > --- a/x
> > +++ /dev/null
> > @@ -1 +0,0 @@
> > -123
> > diff --git a/ b/
> > new file mode 120000 (symlink)
> > index 190a180..873fb8d
> > --- /dev/null
> > +++ b/
> > @@ -0,0 +1 @@
> > +file3
> > \ No newline at end of file
> 
> This was generated by "diff to current" in the history view of a file,
> which was changed between symlink and normal file.

This patch fixes it.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4f34c29..d1f4aeb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3936,7 +3936,8 @@ sub git_blobdiff {
 
 		# open patch output
 		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-			'-p', $hash_parent_base, $hash_base,
+			'-p', ($format eq 'html' ? "--full-index" : ()),
+			$hash_parent_base, $hash_base,
 			"--", (defined $file_parent ? $file_parent : ()), $file_name
 			or die_error(undef, "Open git-diff-tree failed");
 	}
@@ -3971,7 +3972,8 @@ sub git_blobdiff {
 		}
 
 		# open patch output
-		open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,
+		open $fd, "-|", git_cmd(), "diff", @diff_opts,
+			'-p', ($format eq 'html' ? "--full-index" : ()),
 			$hash_parent, $hash, "--"
 			or die_error(undef, "Open git-diff failed");
 	} else  {
-- 
1.5.0.5

Re: [PATCH] gitweb: show no difference message

[Jakub Narebski]

On Sun, Mar 25, 2007, Martin Koegler wrote:

> Currently, gitweb shows only header and footer, if no differences are
> found. This patch adds a "No differences are found" message for the html
> output.

This is a good idea, as it reduces confusion for first-time gitweb user,
who might not know what it means to have an empty diff page.

Currently we have only one place (I think) where gitweb can generate
link to "blobdiff", namely "diff to parent" link in "history" view
for plain file, when e.g. some change was (explicitely or accidentally)
reverted.

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 5214050..fbadab4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2376,6 +2376,7 @@ sub git_patchset_body {
>  
>  	my $patch_idx = 0;
>  	my $patch_line;
> +	my $empty = 0;
>  	my $diffinfo;
>  	my (%from, %to);
>  
> @@ -2396,6 +2397,7 @@ sub git_patchset_body {
>  		# git diff header
>  		#assert($patch_line =~ m/^diff /) if DEBUG;
>  		#assert($patch_line !~ m!$/$!) if DEBUG; # is chomp-ed
> +		$empty++;
>  		push @diff_header, $patch_line;
>  
>  		# extended diff header
> @@ -2559,6 +2561,8 @@ sub git_patchset_body {
>  		print "</div>\n"; # class="patch"
>  	}
>  
> +	print "<div class=\"diff header\">No differences found</div>\n" if (!$empty);
> +
>  	print "</div>\n"; # class="patchset"
>  }
>  

A few nits.

First, programming style. You named the variable $empty, when it
evaluates to true when the patch is _not_ empty, i.e. when some
differences are found.

Second, HTML. I'm not so sure if this info belongs to _patchset_,
but if git-diff or git-diff-tree returns empty patch output,
then we do not generate 'patch' div. So I'm a bit for this
solution.

Third, CSS (style). I'm reluctant about using "diff header" for
styling "no differences found" div; it is used to style

	diff --git a/filename b/filename

header. I think that it would be better to add a separate CSS class,
e.g. "diff notfound", or "diff nochanges", or "diff nodifferences"
class for this message.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: show no difference message

[Jakub Narebski]

On Mon, Mar 26, 2007, Jakub Narebski wrote:
> On Sun, Mar 25, 2007, Martin Koegler wrote:

>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 5214050..fbadab4 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2376,6 +2376,7 @@ sub git_patchset_body {
>>  
>>  	my $patch_idx = 0;
>>  	my $patch_line;
>> +	my $empty = 0;
>>  	my $diffinfo;
>>  	my (%from, %to);
>>  
>> @@ -2396,6 +2397,7 @@ sub git_patchset_body {
>>  		# git diff header
>>  		#assert($patch_line =~ m/^diff /) if DEBUG;
>>  		#assert($patch_line !~ m!$/$!) if DEBUG; # is chomp-ed
>> +		$empty++;
>>  		push @diff_header, $patch_line;
>>  
>>  		# extended diff header
[...]
> First, programming style. You named the variable $empty, when it
> evaluates to true when the patch is _not_ empty, i.e. when some
> differences are found.

By the way, I _think_ that you can use $patch_idx instead of creating
new variable for that.

-- 
Jakub Narebski
Poland

[PATCH] gitweb: Support comparing blobs with different names

[Martin Koegler]

Currently, blobdiff can only compare blobs with different file
names, if no hb/hpb parameters are present.

This patch adds support for comparing two blobs specified by any
combination of hb/f/h and hpb/fp/hp.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
Updated gitweb blobdiff patch to take advantage of my git-diff patch series.

 gitweb/gitweb.perl |  148 +++++++++++++++++++---------------------------------
 1 files changed, 53 insertions(+), 95 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cbd8d03..4b42f62 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3902,109 +3902,66 @@ sub git_blobdiff {
 	my $fd;
 	my @difftree;
 	my %diffinfo;
-	my $expires;
-
-	# preparing $fd and %diffinfo for git_patchset_body
-	# new style URI
-	if (defined $hash_base && defined $hash_parent_base) {
-		if (defined $file_name) {
-			# read raw output
-			open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-				$hash_parent_base, $hash_base,
-				"--", (defined $file_parent ? $file_parent : ()), $file_name
-				or die_error(undef, "Open git-diff-tree failed");
-			@difftree = map { chomp; $_ } <$fd>;
-			close $fd
-				or die_error(undef, "Reading git-diff-tree failed");
-			@difftree
-				or die_error('404 Not Found', "Blob diff not found");
-
-		} elsif (defined $hash &&
-		         $hash =~ /[0-9a-fA-F]{40}/) {
-			# try to find filename from $hash
-
-			# read filtered raw output
-			open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-				$hash_parent_base, $hash_base, "--"
-				or die_error(undef, "Open git-diff-tree failed");
-			@difftree =
-				# ':100644 100644 03b21826... 3b93d5e7... M	ls-files.c'
-				# $hash == to_id
-				grep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }
-				map { chomp; $_ } <$fd>;
-			close $fd
-				or die_error(undef, "Reading git-diff-tree failed");
-			@difftree
-				or die_error('404 Not Found', "Blob diff not found");
-
-		} else {
-			die_error('404 Not Found', "Missing one of the blob diff parameters");
-		}
-
-		if (@difftree > 1) {
-			die_error('404 Not Found', "Ambiguous blob diff specification");
-		}
-
-		%diffinfo = parse_difftree_raw_line($difftree[0]);
-		$file_parent ||= $diffinfo{'from_file'} || $file_name || $diffinfo{'file'};
-		$file_name   ||= $diffinfo{'to_file'}   || $diffinfo{'file'};
+	my $expires = '+1d';
+	my $from, $to;
 
-		$hash_parent ||= $diffinfo{'from_id'};
-		$hash        ||= $diffinfo{'to_id'};
+	$file_parent ||= $file_name;
 
-		# non-textual hash id's can be cached
-		if ($hash_base =~ m/^[0-9a-fA-F]{40}$/ &&
-		    $hash_parent_base =~ m/^[0-9a-fA-F]{40}$/) {
-			$expires = '+1d';
-		}
+	# non-textual hash id's can be cached
+	if (defined $hash && $hash !~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = undef;
+	} elsif (defined $hash_parent && $hash_parent !~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = undef;
+	} elsif (defined $hash_base && $hash_base !~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = undef;
+	} elsif (defined $hash_parent_base && $hash_parent_base !~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = undef;
+	}
+
+	# if hash parameter is missing, read it from the commit.
+	if (defined $hash_base && defined $file_name && !defined $hash) {
+		$hash = git_get_hash_by_path($hash_base, $file_name);
+	}
 
-		# open patch output
-		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-			'-p', ($format eq 'html' ? "--full-index" : ()),
-			$hash_parent_base, $hash_base,
-			"--", (defined $file_parent ? $file_parent : ()), $file_name
-			or die_error(undef, "Open git-diff-tree failed");
+	if (defined $hash_parent_base && defined $file_parent && !defined $hash_parent) {
+	    $hash_parent = git_get_hash_by_path($hash_parent_base, $file_parent);
+	}
+	
+	if (!defined $hash || ! defined $hash_parent) {
+		die_error('404 Not Found', "Missing one of the blob diff parameters");
 	}
 
-	# old/legacy style URI
-	if (!%diffinfo && # if new style URI failed
-	    defined $hash && defined $hash_parent) {
-		# fake git-diff-tree raw output
-		$diffinfo{'from_mode'} = $diffinfo{'to_mode'} = "blob";
-		$diffinfo{'from_id'} = $hash_parent;
-		$diffinfo{'to_id'}   = $hash;
-		if (defined $file_name) {
-			if (defined $file_parent) {
-				$diffinfo{'status'} = '2';
-				$diffinfo{'from_file'} = $file_parent;
-				$diffinfo{'to_file'}   = $file_name;
-			} else { # assume not renamed
-				$diffinfo{'status'} = '1';
-				$diffinfo{'from_file'} = $file_name;
-				$diffinfo{'to_file'}   = $file_name;
-			}
-		} else { # no filename given
-			$diffinfo{'status'} = '2';
-			$diffinfo{'from_file'} = $hash_parent;
-			$diffinfo{'to_file'}   = $hash;
-		}
+	if (defined $hase_base && defined $file_name) {
+		$to = $hash_base . ':' . $file_name;
+	} else {
+		$to = $hash;
+	}
 
-		# non-textual hash id's can be cached
-		if ($hash =~ m/^[0-9a-fA-F]{40}$/ &&
-		    $hash_parent =~ m/^[0-9a-fA-F]{40}$/) {
-			$expires = '+1d';
-		}
+	if (defined $hase_parent_base && defined $file_parent) {
+		$from = $hash_parent_base . ':' . $file_parent;
+	} else {
+		$from = $hash_parent;
+	}
 
-		# open patch output
-		open $fd, "-|", git_cmd(), "diff", @diff_opts,
-			'-p', ($format eq 'html' ? "--full-index" : ()),
-			$hash_parent, $hash, "--"
-			or die_error(undef, "Open git-diff failed");
-	} else  {
-		die_error('404 Not Found', "Missing one of the blob diff parameters")
-			unless %diffinfo;
+	# fake git-diff-tree raw output
+	$diffinfo{'from_mode'} = $diffinfo{'to_mode'} = "blob";
+	$diffinfo{'from_id'} = $hash_parent;
+	$diffinfo{'to_id'}   = $hash;
+	if (defined $file_name) {
+		$diffinfo{'status'} = '2';
+		$diffinfo{'from_file'} = $file_parent;
+		$diffinfo{'to_file'}   = $file_name;
+	} else { # no filename given
+		$diffinfo{'status'} = '2';
+		$diffinfo{'from_file'} = $hash_parent;
+		$diffinfo{'to_file'}   = $hash;
 	}
 
+	# open patch output
+	open $fd, "-|", git_cmd(), "diff", @diff_opts, '--raw', '-p', '--full-index',
+	$from, $to, "--"
+		or die_error(undef, "Open git-diff failed");
+
 	# header
 	if ($format eq 'html') {
 		my $formats_nav =
@@ -4028,11 +3985,12 @@ sub git_blobdiff {
 		}
 
 	} elsif ($format eq 'plain') {
+		my $patch_file_name = $file_name || $hash;
 		print $cgi->header(
 			-type => 'text/plain',
 			-charset => 'utf-8',
 			-expires => $expires,
-			-content_disposition => 'inline; filename="' . "$file_name" . '.patch"');
+			-content_disposition => 'inline; filename="' . "$patch_file_name" . '.patch"');
 
 		print "X-Git-Url: " . $cgi->self_url() . "\n\n";
 
-- 
1.5.1.1.199.g1a18-dirty