[PATCH 0/6] gitweb: 'commitdiff' view improvements

[PATCH 0/6] gitweb: 'commitdiff' view improvements

[Jakub Narebski]

The following series makes a few improvements to 'commitdiff' view.
The final goal of the series is to use '--cc' (compact combined)
format for merge commits in gitweb, as requested by Linus Torvalds
in
  Message-ID: <Pine.LNX.4.64.0610061202060.3952@g5.osdl.org>
with a few other improvements.

Table of contents:
------------------
 gitweb: Provide links to commitdiff to each parent in 'commitdiff' view
 gitweb: Improve "next" link in commitdiff view
 gitweb: Split git_patchset_body into separate subroutines
 gitweb: Create special from-file/to-file header for combined diff
 gitweb: Add links to blobdiffs in from-file/to-file header for merges
 gitweb: '--cc' for merges in 'commitdiff' view

The first two patches are totally independent of other patches, and
are not required by anu patches.

The "Split git_patchset_body" is required for all subsequent patches;
those patches shows why it was needed. The last patch is conceptually
indepenent on the "improve from-file/to-file header for merges"
patches.

Diffstat:
---------
 gitweb/gitweb.css  |    5 +
 gitweb/gitweb.perl |  481 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 362 insertions(+), 124 deletions(-)

-- 
Jakub Narebski
ShadeHawk on #git
Poland

[PATCH 1/6] gitweb: Provide links to commitdiff to each parent in 'commitdiff' view

[Jakub Narebski]

Since commit-fb1dde4a we show combined diff for merges in 'commitdiff'
view, and since commit-208ecb2e also in 'commit' view. Sometimes
though one would want to see diff to one of merge commit parents. It
is easy in 'commit' view: in the commit header part there are "diff"
links for each of parent header. This commit adds such links also for
'commitdiff' view.

Add to difftree / whatchanged table row with "1", "2", ... links to
'commitdiff' view for diff with n-th parent for merge commits, as a
table header.  This is visible only in 'comitdiff' view, and only for
a merge commit (comit with more than one parent).

To save space links are shown as "n", where "n" is number of a parent,
and not as for example shortened (to 7 characters) sha1 of a parent
commit.  To make it easier to discover what links is for, each link
has 'title' attribute explaining the link.

Note that one would need to remember that difftree table in 'commit'
view has one less column (it doesn't have "patch" link column), if one
would want to add such table header also in 'commit' view.

Example output:
                          1       2       3 
  Makefile      patch | diff1 | diff2 | diff3 | blob | history
  cache.h       patch | diff1 | diff2 | diff3 | blob | history

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.css  |    5 +++++
 gitweb/gitweb.perl |   21 +++++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 9f0822f..7908fe3 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -181,10 +181,15 @@ table.diff_tree {
 	font-family: monospace;
 }
 
+table.combined.diff_tree th {
+	text-align: center;
+}
+
 table.combined.diff_tree td {
 	padding-right: 24px;
 }
 
+table.combined.diff_tree th.link,
 table.combined.diff_tree td.link {
 	padding: 0px 2px;
 }
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e92596c..e2d5222 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2401,6 +2401,26 @@ sub git_difftree_body {
 	print "<table class=\"" .
 	      (@parents > 1 ? "combined " : "") .
 	      "diff_tree\">\n";
+
+	# header only for combined diff in 'commitdiff' view
+	my $has_header = @parents > 1 && $action eq 'commitdiff';
+	if ($has_header) {
+		# table header
+		print "<thead><tr>\n" .
+		       "<th></th><th></th>\n"; # filename, patchN link
+		for (my $i = 0; $i < @parents; $i++) {
+			my $par = $parents[$i];
+			print "<th>" .
+			      $cgi->a({-href => href(action=>"commitdiff",
+			                             hash=>$hash, hash_parent=>$par),
+			               -title => 'commitdiff to parent number ' .
+			                          ($i+1) . ': ' . substr($par,0,7)},
+			              $i+1) .
+			      "&nbsp;</th>\n";
+		}
+		print "</tr></thead>\n<tbody>\n";
+	}
+
 	my $alternate = 1;
 	my $patchno = 0;
 	foreach my $line (@{$difftree}) {
@@ -2673,6 +2693,7 @@ sub git_difftree_body {
 		} # we should not encounter Unmerged (U) or Unknown (X) status
 		print "</tr>\n";
 	}
+	print "</tbody>" if $has_header;
 	print "</table>\n";
 }
 

[PATCH 2/6] gitweb: Improve "next" link in commitdiff view

[Jakub Narebski]

Check if 'hp' (hash_parent) parameter to 'commitdiff' view is one of
'h' (hash) commit parents, i.e. if commitdiff is of the form
"<commit>^<n> <commit>", and mark it as such in the bottom part of
navigation bar. The "next" link in commitdiff view was introduced
in commit 151602df00b8e5c5b4a8193f59a94b85f9b5aebc

If 'hb' is n-th parent of 'h', show the following at the bottom
of navigation bar:
  (from parent n: _commit_)

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is sort of companion to previous commit, meaning thatI'd like to
see if I came from link to diff to one of parents in a merge commit.
Purely decoration, but I think it is nice.

 gitweb/gitweb.perl |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e2d5222..4561d4e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4479,7 +4479,14 @@ sub git_commitdiff {
 				$hash_parent_short = substr($hash_parent, 0, 7);
 			}
 			$formats_nav .=
-				' (from: ' .
+				' (from';
+			for (my $i = 0; $i < @{$co{'parents'}}; $i++) {
+				if ($co{'parents'}[$i] eq $hash_parent) {
+					$formats_nav .= ' parent ' . ($i+1);
+					last;
+				}
+			}
+			$formats_nav .= ': ' .
 				$cgi->a({-href => href(action=>"commitdiff",
 				                       hash=>$hash_parent)},
 				        esc_html($hash_parent_short)) .

[PATCH 3/6] gitweb: Split git_patchset_body into separate subroutines

[Jakub Narebski]

This commit makes git_patchset_body easier to read, and reduces level of
nesting and indent level. It adds more lines that it removes because of
extra parameter passing in subroutines, and subroutine calls in
git_patchset_body. Also because there are few added comments.


Below there are descriptions of all split-off subroutines:

Separate formatting "git diff" header into format_git_diff_header_line.
While at it fix it so it always escapes pathname. It would be even more
useful if we decide to use `--cc' for merges, and need to generate by
hand empty patches for anchors.

Separate formatting extended (git) diff header lines into
format_extended_diff_header_line. This one is copied without changes.

Separate formatting two-lines from-file/to-file diff header into
format_diff_from_to_header subroutine. While at it fix it so it always
escapes pathname. Beware calling convention: it takes _two_ lines.

Separate generating %from and %to hashes (with info used among others to
generate hyperlinks) into parse_from_to_diffinfo subroutine. This one is
copied without changes.

Separate checking if file was deleted (and among others therefore does
not have link to the result file) into is_deleted subroutine. This would
allow us to easily change the algotithm to find if file is_deleted in
the result.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Perhaps only first paragraph (and signoff line) should be in the
commit message, although I think those comments are useful. 

 gitweb/gitweb.perl |  313 ++++++++++++++++++++++++++++++++++------------------
 1 files changed, 202 insertions(+), 111 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4561d4e..aee4f23 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -954,7 +954,149 @@ sub format_subject_html {
 	}
 }
 
-# format patch (diff) line (rather not to be used for diff headers)
+# format git diff header line, i.e. "diff --(git|combined|cc) ..."
+sub format_git_diff_header_line {
+	my $line = shift;
+	my $diffinfo = shift;
+	my ($from, $to) = @_;
+
+	if ($diffinfo->{'nparents'}) {
+		# combined diff
+		$line =~ s!^(diff (.*?) )"?.*$!$1!;
+		if ($to->{'href'}) {
+			$line .= $cgi->a({-href => $to->{'href'}, -class => "path"},
+			                 esc_path($to->{'file'}));
+		} else { # file was deleted (no href)
+			$line .= esc_path($to->{'file'});
+		}
+	} else {
+		# "ordinary" diff
+		$line =~ s!^(diff (.*?) )"?a/.*$!$1!;
+		if ($from->{'href'}) {
+			$line .= $cgi->a({-href => $from->{'href'}, -class => "path"},
+			                 'a/' . esc_path($from->{'file'}));
+		} else { # file was added (no href)
+			$line .= 'a/' . esc_path($from->{'file'});
+		}
+		$line .= ' ';
+		if ($to->{'href'}) {
+			$line .= $cgi->a({-href => $to->{'href'}, -class => "path"},
+			                 'b/' . esc_path($to->{'file'}));
+		} else { # file was deleted
+			$line .= 'b/' . esc_path($to->{'file'});
+		}
+	}
+
+	return "<div class=\"diff header\">$line</div>\n";
+}
+
+# format extended diff header line, before patch itself
+sub format_extended_diff_header_line {
+	my $line = shift;
+	my $diffinfo = shift;
+	my ($from, $to) = @_;
+
+	# match <path>
+	if ($line =~ s!^((copy|rename) from ).*$!$1! && $from->{'href'}) {
+		$line .= $cgi->a({-href=>$from->{'href'}, -class=>"path"},
+		                       esc_path($from->{'file'}));
+	}
+	if ($line =~ s!^((copy|rename) to ).*$!$1! && $to->{'href'}) {
+		$line .= $cgi->a({-href=>$to->{'href'}, -class=>"path"},
+		                 esc_path($to->{'file'}));
+	}
+	# match single <mode>
+	if ($line =~ m/\s(\d{6})$/) {
+		$line .= '<span class="info"> (' .
+		         file_type_long($1) .
+		         ')</span>';
+	}
+	# match <hash>
+	if ($line =~ m/^index [0-9a-fA-F]{40},[0-9a-fA-F]{40}/) {
+		# can match only for combined diff
+		$line = 'index ';
+		for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
+			if ($from->{'href'}[$i]) {
+				$line .= $cgi->a({-href=>$from->{'href'}[$i],
+				                  -class=>"hash"},
+				                 substr($diffinfo->{'from_id'}[$i],0,7));
+			} else {
+				$line .= '0' x 7;
+			}
+			# separator
+			$line .= ',' if ($i < $diffinfo->{'nparents'} - 1);
+		}
+		$line .= '..';
+		if ($to->{'href'}) {
+			$line .= $cgi->a({-href=>$to->{'href'}, -class=>"hash"},
+			                 substr($diffinfo->{'to_id'},0,7));
+		} else {
+			$line .= '0' x 7;
+		}
+
+	} elsif ($line =~ m/^index [0-9a-fA-F]{40}..[0-9a-fA-F]{40}/) {
+		# can match only for ordinary diff
+		my ($from_link, $to_link);
+		if ($from->{'href'}) {
+			$from_link = $cgi->a({-href=>$from->{'href'}, -class=>"hash"},
+			                     substr($diffinfo->{'from_id'},0,7));
+		} else {
+			$from_link = '0' x 7;
+		}
+		if ($to->{'href'}) {
+			$to_link = $cgi->a({-href=>$to->{'href'}, -class=>"hash"},
+			                   substr($diffinfo->{'to_id'},0,7));
+		} else {
+			$to_link = '0' x 7;
+		}
+		my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
+		$line =~ s!$from_id\.\.$to_id!$from_link..$to_link!;
+	}
+
+	return $line . "<br/>\n";
+}
+
+# format from-file/to-file diff header
+sub format_diff_from_to_header {
+	my ($from_line, $to_line, $diffinfo, $from, $to) = @_;
+	my $line;
+	my $result = '';
+
+	$line = $from_line;
+	#assert($line =~ m/^---/) if DEBUG;
+	# no extra formatting "^--- /dev/null"
+	if ($line =~ m!^--- "?a/!) {
+		if (!$diffinfo->{'nparents'} && # multiple 'from'
+		    $from->{'href'}) {
+			$line = '--- a/' .
+			        $cgi->a({-href=>$from->{'href'}, -class=>"path"},
+			                esc_path($from->{'file'}));
+		} else {
+			$line = '--- a/' .
+			        esc_path($from->{'file'});
+		}
+	}
+	$result .= qq!<div class="diff from_file">$line</div>\n!;
+
+	$line = $to_line;
+	#assert($line =~ m/^\+\+\+/) if DEBUG;
+	# no extra formatting for "^+++ /dev/null"
+	if ($line =~ m!^\+\+\+ "?b/!) {
+		if ($to->{'href'}) {
+			$line = '+++ b/' .
+			        $cgi->a({-href=>$to->{'href'}, -class=>"path"},
+			                esc_path($to->{'file'}));
+		} else {
+			$line = '+++ b/' .
+			        esc_path($to->{'file'});
+		}
+	}
+	$result .= qq!<div class="diff to_file">$line</div>\n!;
+
+	return $result;
+}
+
+# format patch (diff) line (not to be used for diff headers)
 sub format_diff_line {
 	my $line = shift;
 	my ($from, $to) = @_;
@@ -1680,6 +1822,48 @@ sub parse_ls_tree_line ($;%) {
 	return wantarray ? %res : \%res;
 }
 
+# generates _two_ hashes, references to which are passed as 2 and 3 argument
+sub parse_from_to_diffinfo {
+	my ($diffinfo, $from, $to, @parents) = @_;
+
+	if ($diffinfo->{'nparents'}) {
+		# combined diff
+		$from->{'file'} = [];
+		$from->{'href'} = [];
+		fill_from_file_info($diffinfo, @parents)
+			unless exists $diffinfo->{'from_file'};
+		for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
+			$from->{'file'}[$i] = $diffinfo->{'from_file'}[$i] || $diffinfo->{'to_file'};
+			if ($diffinfo->{'status'}[$i] ne "A") { # not new (added) file
+				$from->{'href'}[$i] = href(action=>"blob",
+				                           hash_base=>$parents[$i],
+				                           hash=>$diffinfo->{'from_id'}[$i],
+				                           file_name=>$from->{'file'}[$i]);
+			} else {
+				$from->{'href'}[$i] = undef;
+			}
+		}
+	} else {
+		$from->{'file'} = $diffinfo->{'from_file'} || $diffinfo->{'file'};
+		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'});
+		} else {
+			delete $from->{'href'};
+		}
+	}
+
+	$to->{'file'} = $diffinfo->{'to_file'} || $diffinfo->{'file'};
+	if (!is_deleted($diffinfo)) { # file exists in result
+		$to->{'href'} = href(action=>"blob", hash_base=>$hash,
+		                     hash=>$diffinfo->{'to_id'},
+		                     file_name=>$to->{'file'});
+	} else {
+		delete $to->{'href'};
+	}
+}
+
 ## ......................................................................
 ## parse to array of hashes functions
 
@@ -2387,6 +2571,11 @@ sub from_ids_eq {
 	}
 }
 
+sub is_deleted {
+	my $diffinfo = shift;
+
+	return $diffinfo->{'to_id'} eq ('0' x 40);
+}
 
 sub git_difftree_body {
 	my ($difftree, $hash, @parents) = @_;
@@ -2444,7 +2633,7 @@ sub git_difftree_body {
 			fill_from_file_info($diff, @parents)
 				unless exists $diff->{'from_file'};
 
-			if ($diff->{'to_id'} ne ('0' x 40)) {
+			if (!is_deleted($diff)) {
 				# file exists in the result (child) commit
 				print "<td>" .
 				      $cgi->a({-href => href(action=>"blob", hash=>$diff->{'to_id'},
@@ -2765,6 +2954,8 @@ sub git_patchset_body {
 			} else {
 				$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
 			}
+			# modifies %from, %to hashes
+			parse_from_to_diffinfo($diffinfo, \%from, \%to, @hash_parents);
 			if ($diffinfo->{'nparents'}) {
 				# combined diff
 				$from{'file'} = [];
@@ -2794,7 +2985,7 @@ sub git_patchset_body {
 			}
 
 			$to{'file'} = $diffinfo->{'to_file'} || $diffinfo->{'file'};
-			if ($diffinfo->{'to_id'} ne ('0' x 40)) { # file exists in result
+			if (!is_deleted($diffinfo)) { # file exists in result
 				$to{'href'} = href(action=>"blob", hash_base=>$hash,
 				                   hash=>$diffinfo->{'to_id'},
 				                   file_name=>$to{'file'});
@@ -2808,105 +2999,15 @@ sub git_patchset_body {
 
 		# print "git diff" header
 		$patch_line = shift @diff_header;
-		if ($diffinfo->{'nparents'}) {
-
-			# combined diff
-			$patch_line =~ s!^(diff (.*?) )"?.*$!$1!;
-			if ($to{'href'}) {
-				$patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"},
-				                       esc_path($to{'file'}));
-			} else { # file was deleted
-				$patch_line .= esc_path($to{'file'});
-			}
-
-		} else {
-
-			$patch_line =~ s!^(diff (.*?) )"?a/.*$!$1!;
-			if ($from{'href'}) {
-				$patch_line .= $cgi->a({-href => $from{'href'}, -class => "path"},
-				                       'a/' . esc_path($from{'file'}));
-			} else { # file was added
-				$patch_line .= 'a/' . esc_path($from{'file'});
-			}
-			$patch_line .= ' ';
-			if ($to{'href'}) {
-				$patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"},
-				                       'b/' . esc_path($to{'file'}));
-			} else { # file was deleted
-				$patch_line .= 'b/' . esc_path($to{'file'});
-			}
-
-		}
-		print "<div class=\"diff header\">$patch_line</div>\n";
+		print format_git_diff_header_line($patch_line, $diffinfo,
+		                                  \%from, \%to);
 
 		# print extended diff header
 		print "<div class=\"diff extended_header\">\n" if (@diff_header > 0);
 	EXTENDED_HEADER:
 		foreach $patch_line (@diff_header) {
-			# match <path>
-			if ($patch_line =~ s!^((copy|rename) from ).*$!$1! && $from{'href'}) {
-				$patch_line .= $cgi->a({-href=>$from{'href'}, -class=>"path"},
-				                       esc_path($from{'file'}));
-			}
-			if ($patch_line =~ s!^((copy|rename) to ).*$!$1! && $to{'href'}) {
-				$patch_line .= $cgi->a({-href=>$to{'href'}, -class=>"path"},
-				                       esc_path($to{'file'}));
-			}
-			# match single <mode>
-			if ($patch_line =~ m/\s(\d{6})$/) {
-				$patch_line .= '<span class="info"> (' .
-				               file_type_long($1) .
-				               ')</span>';
-			}
-			# match <hash>
-			if ($patch_line =~ m/^index [0-9a-fA-F]{40},[0-9a-fA-F]{40}/) {
-				# can match only for combined diff
-				$patch_line = 'index ';
-				for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
-					if ($from{'href'}[$i]) {
-						$patch_line .= $cgi->a({-href=>$from{'href'}[$i],
-						                        -class=>"hash"},
-						                       substr($diffinfo->{'from_id'}[$i],0,7));
-					} else {
-						$patch_line .= '0' x 7;
-					}
-					# separator
-					$patch_line .= ',' if ($i < $diffinfo->{'nparents'} - 1);
-				}
-				$patch_line .= '..';
-				if ($to{'href'}) {
-					$patch_line .= $cgi->a({-href=>$to{'href'}, -class=>"hash"},
-					                       substr($diffinfo->{'to_id'},0,7));
-				} else {
-					$patch_line .= '0' x 7;
-				}
-
-			} elsif ($patch_line =~ m/^index [0-9a-fA-F]{40}..[0-9a-fA-F]{40}/) {
-				# can match only for ordinary diff
-				my ($from_link, $to_link);
-				if ($from{'href'}) {
-					$from_link = $cgi->a({-href=>$from{'href'}, -class=>"hash"},
-					                     substr($diffinfo->{'from_id'},0,7));
-				} else {
-					$from_link = '0' x 7;
-				}
-				if ($to{'href'}) {
-					$to_link = $cgi->a({-href=>$to{'href'}, -class=>"hash"},
-					                   substr($diffinfo->{'to_id'},0,7));
-				} else {
-					$to_link = '0' x 7;
-				}
-				#affirm {
-				#	my ($from_hash, $to_hash) =
-				#		($patch_line =~ m/^index ([0-9a-fA-F]{40})..([0-9a-fA-F]{40})/);
-				#	my ($from_id, $to_id) =
-				#		($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
-				#	($from_hash eq $from_id) && ($to_hash eq $to_id);
-				#} if DEBUG;
-				my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
-				$patch_line =~ s!$from_id\.\.$to_id!$from_link..$to_link!;
-			}
-			print $patch_line . "<br/>\n";
+			print format_extended_diff_header_line($patch_line, $diffinfo,
+			                                       \%from, \%to);
 		}
 		print "</div>\n"  if (@diff_header > 0); # class="diff extended_header"
 
@@ -2918,24 +3019,14 @@ sub git_patchset_body {
 		}
 		next PATCH if ($patch_line =~ m/^diff /);
 		#assert($patch_line =~ m/^---/) if DEBUG;
-		if (!$diffinfo->{'nparents'} && # not from-file line for combined diff
-		    $from{'href'} && $patch_line =~ m!^--- "?a/!) {
-			$patch_line = '--- a/' .
-			              $cgi->a({-href=>$from{'href'}, -class=>"path"},
-			                      esc_path($from{'file'}));
-		}
-		print "<div class=\"diff from_file\">$patch_line</div>\n";
+		#assert($patch_line eq $last_patch_line) if DEBUG;
 
 		$patch_line = <$fd>;
 		chomp $patch_line;
+		#assert($patch_line =~ m/^\+\+\+/) if DEBUG;
 
-		#assert($patch_line =~ m/^+++/) if DEBUG;
-		if ($to{'href'} && $patch_line =~ m!^\+\+\+ "?b/!) {
-			$patch_line = '+++ b/' .
-			              $cgi->a({-href=>$to{'href'}, -class=>"path"},
-			                      esc_path($to{'file'}));
-		}
-		print "<div class=\"diff to_file\">$patch_line</div>\n";
+		print format_diff_from_to_header($last_patch_line, $patch_line,
+		                                 $diffinfo, \%from, \%to);
 
 		# the patch itself
 	LINE:

[PATCH 4/6] gitweb: Create special from-file/to-file header for combined diff

[Jakub Narebski]

Instead of using default, diff(1) like from-file/to-file header for
combined diff (for a merge commit), which looks like:

  --- a/git-gui/git-gui.sh
  +++ b/_git-gui/git-gui.sh_

(where _link_ denotes [hidden] hyperlink), create from-file(n)/to-file
header, using "--- <n>/_<filename>_" for each of parents, e.g.:

  --- 1/_git-gui/git-gui.sh_
  --- 2/_git-gui.sh_
  +++ b/_git-gui/git-gui.sh_

Test it on one of merge commits involving rename, e.g.
  95f97567c1887d77f3a46b42d8622c76414d964d (rename at top)
  5bac4a671907604b5fb4e24ff682d5b0e8431931 (file from one branch)

This is mainly meant to easier see renames in a merge commit.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Junio and Pasky likes this, and were source of inspiration for
the next patch.

 gitweb/gitweb.perl |   38 +++++++++++++++++++++++++++-----------
 1 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index aee4f23..13114bc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1064,19 +1064,35 @@ sub format_diff_from_to_header {
 
 	$line = $from_line;
 	#assert($line =~ m/^---/) if DEBUG;
-	# no extra formatting "^--- /dev/null"
-	if ($line =~ m!^--- "?a/!) {
-		if (!$diffinfo->{'nparents'} && # multiple 'from'
-		    $from->{'href'}) {
-			$line = '--- a/' .
-			        $cgi->a({-href=>$from->{'href'}, -class=>"path"},
-			                esc_path($from->{'file'}));
-		} else {
-			$line = '--- a/' .
-			        esc_path($from->{'file'});
+	# no extra formatting for "^--- /dev/null"
+	if (! $diffinfo->{'nparents'}) {
+		# ordinary (single parent) diff
+		if ($line =~ m!^--- "?a/!) {
+			if ($from->{'href'}) {
+				$line = '--- a/' .
+				        $cgi->a({-href=>$from->{'href'}, -class=>"path"},
+				                esc_path($from->{'file'}));
+			} else {
+				$line = '--- a/' .
+				        esc_path($from->{'file'});
+			}
+		}
+		$result .= qq!<div class="diff from_file">$line</div>\n!;
+
+	} else {
+		# combined diff (merge commit)
+		for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
+			if ($from->{'href'}[$i]) {
+				$line = '--- ' .
+				        ($i+1) . "/" .
+				        $cgi->a({-href=>$from->{'href'}[$i], -class=>"path"},
+				                esc_path($from->{'file'}[$i]));
+			} else {
+				$line = '--- /dev/null';
+			}
+			$result .= qq!<div class="diff from_file">$line</div>\n!;
 		}
 	}
-	$result .= qq!<div class="diff from_file">$line</div>\n!;
 
 	$line = $to_line;
 	#assert($line =~ m/^\+\+\+/) if DEBUG;

[PATCH 5/6] gitweb: Add links to blobdiffs in from-file/to-file header for merges

[Jakub Narebski]

Add links to diff to file ('blobdiff' view) for each of individual
versions of the file in a merge commit to the from-file/to-file header
in the patch part of combined 'commitdiff' view for merges.

The from-file/to-file header for combined diff now looks like:

  --- _1_/_git-gui/git-gui.sh_
  --- _2_/_git-gui.sh_
  +++ b/_git-gui/git-gui.sh_

where _<filename>_ link is link to appropriate version of a file
('blob' view), and _<n>_ is link to respective diff to mentioned
version of a file ('blobdiff' view). There is even hint provided in
the form of title attribute.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
As requested by Junio C Hamano and Petr "Pasky" Baudis in:
  Message-ID: <7v1wh1en8k.fsf@assigned-by-dhcp.cox.net>
  Message-ID: <20070528132959.GT4489@pasky.or.cz>

I don't know if the link placement is best possible; I tried for
combined diff output be similar to ordinary diff output, but also
provide reqeusted additional functionality (which does not make
sense for ordinary diff). You have to know ehere to search for
the link.

 gitweb/gitweb.perl |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 13114bc..c7acfad 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1058,7 +1058,7 @@ sub format_extended_diff_header_line {
 
 # format from-file/to-file diff header
 sub format_diff_from_to_header {
-	my ($from_line, $to_line, $diffinfo, $from, $to) = @_;
+	my ($from_line, $to_line, $diffinfo, $from, $to, @parents) = @_;
 	my $line;
 	my $result = '';
 
@@ -1084,7 +1084,17 @@ sub format_diff_from_to_header {
 		for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
 			if ($from->{'href'}[$i]) {
 				$line = '--- ' .
-				        ($i+1) . "/" .
+				        $cgi->a({-href=>href(action=>"blobdiff",
+				                             hash_parent=>$diffinfo->{'from_id'}[$i],
+				                             hash_parent_base=>$parents[$i],
+				                             file_parent=>$from->{'file'}[$i],
+				                             hash=>$diffinfo->{'to_id'},
+				                             hash_base=>$hash,
+				                             file_name=>$to->{'file'}),
+				                 -class=>"path",
+				                 -title=>"diff" . ($i+1)},
+				                $i+1) .
+				        '/' .
 				        $cgi->a({-href=>$from->{'href'}[$i], -class=>"path"},
 				                esc_path($from->{'file'}[$i]));
 			} else {
@@ -3042,7 +3052,8 @@ sub git_patchset_body {
 		#assert($patch_line =~ m/^\+\+\+/) if DEBUG;
 
 		print format_diff_from_to_header($last_patch_line, $patch_line,
-		                                 $diffinfo, \%from, \%to);
+		                                 $diffinfo, \%from, \%to,
+		                                 @hash_parents);
 
 		# the patch itself
 	LINE:

[PATCH 6/6] gitweb: '--cc' for merges in 'commitdiff' view

[Jakub Narebski]

Allow choosing between '-c' (combined diff) and '--cc' (compact
combined) diff format in 'commitdiff' view for merge (multiparent)
commits.  Default is now '--cc'.

In the bottom part of navigation bar there is link allowing to change
diff format: "combined" for '-c' (when using '--cc') and "compact" for
'--cc' (when using '-c'), just on the right of "raw" link to
'commitdiff_plain" view. 

About patchset part of diff --cc output: the difftree (whatchanged
table) has "patch" links to anchors to individual patches (on the same
page). The --cc option further compresses the patch output by
omitting some hunks; when this optimization makes all hunks disappear,
the patch is not shown (like in any other "empty diff" case). But the
fact that patch has been simplified out is not reflected in the raw
(difftree) part of diff output; the raw part is the same for '-c' and
'--cc' options. As correcting difftree is rather out of the question,
as it would require scanning patchset part before writing out
difftree, we add "Simple merge" empty diffs as a place to have anchor
to in place of those simplified out and removed patches.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Linus wanted '--cc' for merges as it is more compact. Here it is.

 gitweb/gitweb.perl |  111 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 99 insertions(+), 12 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c7acfad..a6383dc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1122,6 +1122,31 @@ sub format_diff_from_to_header {
 	return $result;
 }
 
+# create note for patch simplified by combined diff
+sub format_diff_cc_simplified {
+	my ($diffinfo, @parents) = @_;
+	my $result = '';
+
+	$result .= "<div class=\"diff header\">" .
+	           "diff --cc ";
+	if (!is_deleted($diffinfo)) {
+		$result .= $cgi->a({-href => href(action=>"blob",
+		                                  hash_base=>$hash,
+		                                  hash=>$diffinfo->{'to_id'},
+		                                  file_name=>$diffinfo->{'to_file'}),
+		                    -class => "path"},
+		                   esc_path($diffinfo->{'to_file'}));
+	} else {
+		$result .= esc_path($diffinfo->{'to_file'});
+	}
+	$result .= "</div>\n" . # class="diff header"
+	           "<div class=\"diff nodifferences\">" .
+	           "Simple merge" .
+	           "</div>\n"; # class="diff nodifferences"
+
+	return $result;
+}
+
 # format patch (diff) line (not to be used for diff headers)
 sub format_diff_line {
 	my $line = shift;
@@ -2973,13 +2998,33 @@ sub git_patchset_body {
 			# advance raw git-diff output if needed
 			$patch_idx++ if defined $diffinfo;
 
-			# read and prepare patch information
-			if (ref($difftree->[$patch_idx]) eq "HASH") {
-				# pre-parsed (or generated by hand)
-				$diffinfo = $difftree->[$patch_idx];
-			} else {
-				$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
+			# compact combined diff output can have some patches skipped
+			# find which patch (using pathname of result) we are at now
+			my $to_name;
+			if ($diff_header[0] =~ m!^diff --cc "?(.*)"?$!) {
+				$to_name = $1;
 			}
+
+			do {
+				# read and prepare patch information
+				if (ref($difftree->[$patch_idx]) eq "HASH") {
+					# pre-parsed (or generated by hand)
+					$diffinfo = $difftree->[$patch_idx];
+				} else {
+					$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
+				}
+
+				# check if current raw line has no patch (it got simplified)
+				if (defined $to_name && $to_name ne $diffinfo->{'to_file'}) {
+					print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n" .
+					      format_diff_cc_simplified($diffinfo, @hash_parents) .
+					      "</div>\n";  # class="patch"
+
+					$patch_idx++;
+					$patch_number++;
+				}
+			} until (!defined $to_name || $to_name eq $diffinfo->{'to_file'} ||
+			         $patch_idx > $#$difftree);
 			# modifies %from, %to hashes
 			parse_from_to_diffinfo($diffinfo, \%from, \%to, @hash_parents);
 			if ($diffinfo->{'nparents'}) {
@@ -3069,6 +3114,27 @@ sub git_patchset_body {
 		print "</div>\n"; # class="patch"
 	}
 
+	# for compact combined (--cc) format, with chunk and patch simpliciaction
+	# patchset might be empty, but there might be unprocessed raw lines
+	for ($patch_idx++ if $patch_number > 0;
+	     $patch_idx < @$difftree;
+	     $patch_idx++) {
+		# read and prepare patch information
+		if (ref($difftree->[$patch_idx]) eq "HASH") {
+			# pre-parsed (or generated by hand)
+			$diffinfo = $difftree->[$patch_idx];
+		} else {
+			$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
+		}
+
+		# generate anchor for "patch" links in difftree / whatchanged part
+		print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n" .
+		      format_diff_cc_simplified($diffinfo, @hash_parents) .
+		      "</div>\n";  # class="patch"
+
+		$patch_number++;
+	}
+
 	if ($patch_number == 0) {
 		if (@hash_parents > 1) {
 			print "<div class=\"diff nodifferences\">Trivial merge</div>\n";
@@ -4582,7 +4648,11 @@ sub git_commitdiff {
 		die_error(undef, "Unknown commit object");
 	}
 
-	# we need to prepare $formats_nav before any parameter munging
+	# choose format for commitdiff for merge
+	if (! defined $hash_parent && @{$co{'parents'}} > 1) {
+		$hash_parent = '--cc';
+	}
+	# we need to prepare $formats_nav before almost any parameter munging
 	my $formats_nav;
 	if ($format eq 'html') {
 		$formats_nav =
@@ -4590,7 +4660,8 @@ sub git_commitdiff {
 			                       hash=>$hash, hash_parent=>$hash_parent)},
 			        "raw");
 
-		if (defined $hash_parent) {
+		if (defined $hash_parent &&
+		    $hash_parent ne '-c' && $hash_parent ne '--cc') {
 			# commitdiff with two commits given
 			my $hash_parent_short = $hash_parent;
 			if ($hash_parent =~ m/^[0-9a-fA-F]{40}$/) {
@@ -4622,6 +4693,17 @@ sub git_commitdiff {
 				')';
 		} else {
 			# merge commit
+			if ($hash_parent eq '--cc') {
+				$formats_nav .= ' | ' .
+					$cgi->a({-href => href(action=>"commitdiff",
+					                       hash=>$hash, hash_parent=>'-c')},
+					        'combined');
+			} else { # $hash_parent eq '-c'
+				$formats_nav .= ' | ' .
+					$cgi->a({-href => href(action=>"commitdiff",
+					                       hash=>$hash, hash_parent=>'--cc')},
+					        'compact');
+			}
 			$formats_nav .=
 				' (merge: ' .
 				join(' ', map {
@@ -4634,9 +4716,10 @@ sub git_commitdiff {
 	}
 
 	my $hash_parent_param = $hash_parent;
-	if (!defined $hash_parent) {
+	if (!defined $hash_parent_param) {
+		# --cc for multiple parents, --root for parentless
 		$hash_parent_param =
-			@{$co{'parents'}} > 1 ? '-c' : $co{'parent'} || '--root';
+			@{$co{'parents'}} > 1 ? '--cc' : $co{'parent'} || '--root';
 	}
 
 	# read commitdiff
@@ -4713,10 +4796,14 @@ TEXT
 
 	# write patch
 	if ($format eq 'html') {
-		git_difftree_body(\@difftree, $hash, $hash_parent || @{$co{'parents'}});
+		my $use_parents = !defined $hash_parent ||
+			$hash_parent eq '-c' || $hash_parent eq '--cc';
+		git_difftree_body(\@difftree, $hash,
+		                  $use_parents ? @{$co{'parents'}} : $hash_parent);
 		print "<br/>\n";
 
-		git_patchset_body($fd, \@difftree, $hash, $hash_parent || @{$co{'parents'}});
+		git_patchset_body($fd, \@difftree, $hash,
+		                  $use_parents ? @{$co{'parents'}} : $hash_parent);
 		close $fd;
 		print "</div>\n"; # class="page_body"
 		git_footer_html();

Re: [PATCH 0/6] gitweb: 'commitdiff' view improvements

[Junio C Hamano]

Overall it makes the view of merge commit much more pleasant.
Good job.

[1/6]
Jakub Narebski <jnareb@gmail.com> writes:

> To save space links are shown as "n", where "n" is number of a parent,
> and not as for example shortened (to 7 characters) sha1 of a parent
> commit.  To make it easier to discover what links is for, each link
> has 'title' attribute explaining the link.

Nice; this is a good way to give "diff $it^$n $it".  The 'title'
does not feel that useful but it does not add much to the weight
of the payload so it probably is Ok.

> Example output:
>                           1       2       3 
>   Makefile      patch | diff1 | diff2 | diff3 | blob | history
>   cache.h       patch | diff1 | diff2 | diff3 | blob | history

It is now tempting to change diff[1234...] for each path to
"diff", isn't it?

[3/6]
Jakub Narebski <jnareb@gmail.com> writes:

> This commit makes git_patchset_body easier to read, and reduces level of
> nesting and indent level. It adds more lines that it removes because of
> extra parameter passing in subroutines, and subroutine calls in
> git_patchset_body. Also because there are few added comments.

Very nice.  That gigantic loop in patchset_body has always been
an eyesore.  The main loop is much easier to follow now.