Bugs in gitweb

Bugs in gitweb

[Luben Tuikov]

gitweb has two bugs in git_patchset_body (now you know who
you are without data-mining with git-blame).

The first bug is a missing "</div>" on commitdiff.
Saw this one a couple of days ago, but was swamped
and had no time to post.

The second bug is displaying a renamed file with the
same name as the one of the preceding "patch". Saw
this one today when I pulled.

To exhibit these, you need to "commitdiff" a commit
which has at least one rename and at least one regular
diff preceding the rename.

Can the guilty parties (you know who you are) please
fix this.

"And while at it" can you please actually *do* "refactor"
git_patchset_body *into smaller functions each one doing
a single particular task*.

It is sad to see git_patchset_body in such despicable state
all the while seeing words like "refactor" in the commit logs
of that function.

git_patchset_body is grossly overloaded for what it is
supposed to do to, and being one single huge blob, it is
hard to maintain.

Thanks,
    Luben

[PATCH 0/5] gitweb: git_patchset_body fixes

[Jakub Narebski]

Luben Tuikov wrote:

> gitweb has two bugs in git_patchset_body (now you know who
> you are without data-mining with git-blame).
> 
> The first bug is a missing "</div>" on commitdiff.
> Saw this one a couple of days ago, but was swamped
> and had no time to post.

Could you please provide either minimal example from
scratch, or parameters for git.git repository view, or
an URL? I could not reproduce this...

> The second bug is displaying a renamed file with the
> same name as the one of the preceding "patch". Saw
> this one today when I pulled.
> 
> To exhibit these, you need to "commitdiff" a commit
> which has at least one rename and at least one regular
> diff preceding the rename.

Haven't found this one, but I hope one of those patches
would fix this.

[...]
> "And while at it" can you please actually *do* "refactor"
> git_patchset_body *into smaller functions each one doing
> a single particular task*.
> 
> It is sad to see git_patchset_body in such despicable state
> all the while seeing words like "refactor" in the commit logs
> of that function.

(Perhaps I overuse word "refactor").

> git_patchset_body is grossly overloaded for what it is
> supposed to do to, and being one single huge blob, it is
> hard to maintain.

I'm not sure if splitting git_patchset_body into smaller functions
would be worth doing, as 1) such functions would be used only
by git_patchset_body, 2) quite I bit of info has to be passed.

-- >8 --

Table of contents
 [PATCH 1/5] gitweb: Fix error in git_patchest_body for file
             creation/deletion patch 
 [PATCH 2/5] gitweb: Fix error in "rename to"/"copy to" git diff
             header output 
 [PATCH 3/5] gitweb: Fix errors in git_patchset_body for empty patches
 [PATCH 4/5] Revert "gitweb: There can be empty patches
             (in git_patchset_body)"
 [PATCH 5/5] gitweb: Fix split patches output (e.g. file to symlink)

Diffstat:
 gitweb/gitweb.perl |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

-- 
Jakub Narebski
Poland

Re: [PATCH 0/5] gitweb: git_patchset_body fixes

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> Luben Tuikov wrote:
> 
> > gitweb has two bugs in git_patchset_body (now you know who
> > you are without data-mining with git-blame).
> > 
> > The first bug is a missing "</div>" on commitdiff.
> > Saw this one a couple of days ago, but was swamped
> > and had no time to post.
> 
> Could you please provide either minimal example from
> scratch, or parameters for git.git repository view, or
> an URL? I could not reproduce this...

I cannot post a URL to the repo for many reasons.
I don't have time to allocate to trying to define a test
case from scratch, nor am I certain that I can attain that,
since that would be a trivial bug to fix and am sure that
it would've been caught.  But as a heavy git user in a complex
(git) environment, the limits and corner cases are sometimes hit.

I cannot suggest anything other than using git professionally
in a complex environment, as opposed to being a pastime.

I also understand that it is a fine line of separating the two,
and it would appear that the former group not always has the time
and the latter not always has the complex environment to hit
the corner cases.

> > The second bug is displaying a renamed file with the
> > same name as the one of the preceding "patch". Saw
> > this one today when I pulled.
> > 
> > To exhibit these, you need to "commitdiff" a commit
> > which has at least one rename and at least one regular
> > diff preceding the rename.
> 
> Haven't found this one, but I hope one of those patches
> would fix this.

I hope so too.

> > "And while at it" can you please actually *do* "refactor"
> > git_patchset_body *into smaller functions each one doing
> > a single particular task*.
> > 
> > It is sad to see git_patchset_body in such despicable state
> > all the while seeing words like "refactor" in the commit logs
> > of that function.
> 
> (Perhaps I overuse word "refactor").

Perhaps.  Or maybe it is used incorrectly.

> > git_patchset_body is grossly overloaded for what it is
> > supposed to do to, and being one single huge blob, it is
> > hard to maintain.
> 
> I'm not sure if splitting git_patchset_body into smaller functions
> would be worth doing, as 1) such functions would be used only
> by git_patchset_body, 2) quite I bit of info has to be passed.

When things get too complex, they should be done out of
principle, not out of "how I feel about it", since principles
can be proved and what they define can be easily tracked.

Yes, it is worth doing.  It doesn't matter that those smaller
functions would be used only by git_patchset_body, when
(by principle) the workings, logic and justification would
be clearly exposed by those smaller units of function.  Thus
it would be possible to prove that the function is correct
or not, and bugs would be able to be isolated easily.

Smaller units of function should do one thing and do it well.
If you cannot isolate them "because quite a bit of info has
to be passed" then the logic of git_patchset_body is faulty,
by _definition_, and needs to be scrapped and re-engineered
(from scratch).

   Luben

Re: [PATCH 0/5] gitweb: git_patchset_body fixes

[Jakub Narebski]

Dnia poniedziałek 8. stycznia 2007 04:50, Luben Tuikov napisał:
> --- Jakub Narebski <jnareb@gmail.com> wrote:
>> Luben Tuikov wrote:

>>> "And while at it" can you please actually *do* "refactor"
>>> git_patchset_body *into smaller functions each one doing
>>> a single particular task*.
>>> 
>>> It is sad to see git_patchset_body in such despicable state
>>> all the while seeing words like "refactor" in the commit logs
>>> of that function.
>> 
>> (Perhaps I overuse word "refactor").
> 
> Perhaps.  Or maybe it is used incorrectly.

That is also possible.

That said git_patchset_body was rewritten from AWK-like event-driven
single loop, with lots of state variables, to few loops following
the structure of patchset, and of git diff.

>>> git_patchset_body is grossly overloaded for what it is
>>> supposed to do to, and being one single huge blob, it is
>>> hard to maintain.
>> 
>> I'm not sure if splitting git_patchset_body into smaller functions
>> would be worth doing, as 1) such functions would be used only
>> by git_patchset_body, 2) quite a bit of info has to be passed.
> 
> When things get too complex, they should be done out of
> principle, not out of "how I feel about it", since principles
> can be proved and what they define can be easily tracked.
> 
> Yes, it is worth doing.  It doesn't matter that those smaller
> functions would be used only by git_patchset_body, when
> (by principle) the workings, logic and justification would
> be clearly exposed by those smaller units of function.  Thus
> it would be possible to prove that the function is correct
> or not, and bugs would be able to be isolated easily.
> 
> Smaller units of function should do one thing and do it well.
> If you cannot isolate them "because quite a bit of info has
> to be passed" then the logic of git_patchset_body is faulty,
> by _definition_, and needs to be scrapped and re-engineered
> (from scratch).

On first glance it would be easy to separate (refactor) individual
patch parsing and output into separate subroutine; perhaps also
separate parsing and output of diff header.

The problems with this approach are two: empty patches (e.g. pure
rename patch) and split patches (e.g. file to symlink patch).
And we are dealing with two sources of information: "raw" difftree
part, and the patch(set) part.

So it is not that easy. But I can certainly try...

-- 
Jakub Narebski
Poland

Re: [PATCH 0/5] gitweb: git_patchset_body fixes

[Luben Tuikov]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

Ok, I see that Junio has committed the fixes to "next" -- thanks!
That saved me time having to manually apply them.

Now, the results are that I see the same bug.

gitweb and gitcore as reported by the xml header
is 1.4.4.4.g7c5d, which is "next" as of a few minutes
ago, merged over twice into my branchA and branchB
(names changed).

The commit that fails looks is the attachment
with names and contents changed to protect the
innocent.

The complaint Firefox 2.0.0.1 says is the err.txt
file attached.

    Luben



[-- Attachment #2: 3446761547-err.txt --]
[-- Type: text/plain, Size: 242 bytes --]

XML Parsing Error: mismatched tag. Expected: </div>.
Location: http://lion.vitesse.com/git/?p=vhal.git;a=commitdiff;h=b4e81df5f58bc24bb690b5b3687fd36180e071b8;hp=bd5f8325508ee5151d84d65f50b72fd7f3ecc77a
Line Number 95, Column 3:

</body>
--^

[-- Attachment #3: 28319668-diff.txt --]
[-- Type: text/plain, Size: 1083 bytes --]

$ git-diff-tree -r --no-commit-id --patch-with-raw --full-index b4e81df5f58bc24bb690b5b3687fd36180e071b8 > /tmp/diff.txt

:100644 100644 a4bf8891fc1153946b2d1531fdfe09f53a532c65 28929eb48708d0bc33d4c80c4349e753332ea4e1 M	fileA
:000000 100644 0000000000000000000000000000000000000000 36dec28bfc5ec04cff264fd1b7c4b47ab7bad3e3 A	fileB
:100644 000000 36dec28bfc5ec04cff264fd1b7c4b47ab7bad3e3 0000000000000000000000000000000000000000 D	fileC

diff --git a/fileA b/fileA
index a4bf8891fc1153946b2d1531fdfe09f53a532c65..28929eb48708d0bc33d4c80c4349e753332ea4e1 100644
--- a/fileA
+++ b/fileA
@@ -1,6 +1,6 @@

... one line change ...

diff --git a/fileB b/fileB
new file mode 100644
index 0000000000000000000000000000000000000000..36dec28bfc5ec04cff264fd1b7c4b47ab7bad3e3
--- /dev/null
+++ b/fileB
@@ -0,0 +1,89 @@

... it is obvious what happened here ...

diff --git a/fileC b/fileC
deleted file mode 100644
index 36dec28bfc5ec04cff264fd1b7c4b47ab7bad3e3..0000000000000000000000000000000000000000
--- a/fileC
+++ /dev/null
@@ -1,89 +0,0 @@

... again it is obvious what happened here ...

Re: [PATCH 0/5] gitweb: git_patchset_body fixes

[Jakub Narebski]

Luben Tuikov wrote:

> Ok, I see that Junio has committed the fixes to "next" -- thanks!
> That saved me time having to manually apply them.
> 
> Now, the results are that I see the same bug.
 
(The same bug = no closing div).

I'll try to reproduce this.


What about the other error, 

>>> The second bug is displaying a renamed file with the
>>> same name as the one of the preceding "patch". Saw
>>> this one today when I pulled.

Is it corrected by those patches?
-- 
Jakub Narebski
Poland

Re: [PATCH 0/5] gitweb: git_patchset_body fixes

[Jakub Narebski]

Jakub Narebski wrote:
> Luben Tuikov wrote:
> 
>> Ok, I see that Junio has committed the fixes to "next" -- thanks!
>> That saved me time having to manually apply them.
>> 
>> Now, the results are that I see the same bug.
>  
> (The same bug = no closing div).
> 
> I'll try to reproduce this.

I failed to reproduce this error.

Could you apply the following patch, and report the result of
  grep "</div>\|<div" | grep -v "<div.*</div>
(find all opening and closing <div> elements, but omitting
those which are opened and closed in the same line)?

-- >8 --
gitweb: Mark closing </div>'s in code and output
    
Add Perl comments and HTML comments (<!-- ... -->) to </div> output,
to mark which 'div' element the closing </div> corresponds to.
    
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---

 gitweb/gitweb.perl |   81 ++++++++++++++++++++++++++-------------------------
 1 files changed, 41 insertions(+), 40 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 25e5079..7b6cb4c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1800,14 +1800,14 @@ EOF
 		      $cgi->hidden(-name => "a") . "\n" .
 		      $cgi->hidden(-name => "h") . "\n" .
 		      $cgi->popup_menu(-name => 'st', -default => 'commit',
-				       -values => ['commit', 'author', 'committer', 'pickaxe']) .
+		                       -values => ['commit', 'author', 'committer', 'pickaxe']) .
 		      $cgi->sup($cgi->a({-href => href(action=>"search_help")}, "?")) .
 		      " search:\n",
 		      $cgi->textfield(-name => "s", -value => $searchtext) . "\n" .
-		      "</div>" .
+		      "</div><!-- search -->\n" . # class="search"
 		      $cgi->end_form() . "\n";
 	}
-	print "</div>\n";
+	print "</div><!-- page_header -->\n"; # class="page_header"
 }
 
 sub git_footer_html {
@@ -1827,7 +1827,7 @@ sub git_footer_html {
 		print $cgi->a({-href => href(project=>undef, action=>"project_index"),
 		              -class => "rss_logo"}, "TXT") . "\n";
 	}
-	print "</div>\n" ;
+	print "</div><!-- page_footer -->\n" ; # class="page_footer"
 
 	if (-f $site_footer) {
 		open (my $fd, $site_footer);
@@ -1849,7 +1849,7 @@ sub die_error {
 <br /><br />
 $status - $error
 <br />
-</div>
+</div><!-- page_body -->
 EOF
 	git_footer_html();
 	exit;
@@ -1887,7 +1887,7 @@ sub git_print_page_nav {
 		       $_ : $cgi->a({-href => href(%{$arg{$_}})}, "$_")
 		 } @navs);
 	print "<br/>\n$extra<br/>\n" .
-	      "</div>\n";
+	      "</div><!-- page_nav -->\n"; # class="page_nav"
 }
 
 sub format_paging_nav {
@@ -1934,7 +1934,7 @@ sub git_print_header_div {
 	print "<div class=\"header\">\n" .
 	      $cgi->a({-href => href(%args), -class => "title"},
 	      $title ? $title : $action) .
-	      "\n</div>\n";
+	      "\n</div><!-- header -->\n"; # class="header"
 }
 
 #sub git_print_authorship (\%) {
@@ -1952,7 +1952,7 @@ sub git_print_authorship {
 		printf(" (%02d:%02d %s)",
 		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
 	}
-	print "]</div>\n";
+	print "]</div>\n"; # class="author_date"
 }
 
 sub git_print_page_path {
@@ -1990,7 +1990,7 @@ sub git_print_page_path {
 			print esc_path($basename);
 		}
 	}
-	print "<br/></div>\n";
+	print "<br/></div><!-- page_path -->\n"; # class="page_path"
 }
 
 # sub git_print_log (\@;%) {
@@ -2188,7 +2188,7 @@ sub git_difftree_body {
 	if ($#{$difftree} > 10) {
 		print(($#{$difftree} + 1) . " files changed:\n");
 	}
-	print "</div>\n";
+	print "</div><!-- list_head -->\n"; # class="list_head"
 
 	print "<table class=\"diff_tree\">\n";
 	my $alternate = 1;
@@ -2518,7 +2518,8 @@ sub git_patchset_body {
 			}
 			print $patch_line . "<br/>\n";
 		}
-		print "</div>\n"  if (@diff_header > 0); # class="diff extended_header"
+		print "</div><!-- diff extended_header -->\n"
+			if (@diff_header > 0); # class="diff extended_header"
 
 		# from-file/to-file diff header
 		$patch_line = $last_patch_line;
@@ -2555,10 +2556,10 @@ sub git_patchset_body {
 		}
 
 	} continue {
-		print "</div>\n"; # class="patch"
+		print "</div><!-- patch $patch_idx -->\n"; # class="patch"
 	}
 
-	print "</div>\n"; # class="patchset"
+	print "</div><!-- patchset -->\n"; # class="patchset"
 }
 
 # . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
@@ -2971,7 +2972,7 @@ sub git_project_list {
 		open (my $fd, $home_text);
 		print <$fd>;
 		close $fd;
-		print "</div>\n";
+		print "</div><!-- index_include -->\n"; # class="index_include"
 	}
 	git_project_list_body(\@list, $order);
 	git_footer_html();
@@ -3123,14 +3124,14 @@ sub git_tag {
 			"</td></tr>\n";
 	}
 	print "</table>\n\n" .
-	      "</div>\n";
+	      "</div><!-- title_text -->\n"; # class="title_text"
 	print "<div class=\"page_body\">";
 	my $comment = $tag{'comment'};
 	foreach my $line (@$comment) {
 		chomp $line;
 		print esc_html($line, -nbsp=>1) . "<br/>\n";
 	}
-	print "</div>\n";
+	print "</div><!-- page_body -->\n"; # class="page_body"
 	git_footer_html();
 }
 
@@ -3201,7 +3202,7 @@ HTML
 		my $rev = substr($full_rev, 0, 8);
 		my $author = $meta->{'author'};
 		my %date = parse_date($meta->{'author-time'},
-				      $meta->{'author-tz'});
+		                      $meta->{'author-tz'});
 		my $date = $date{'iso-tz'};
 		if ($group_size) {
 			$current_color = ++$current_color % $num_colors;
@@ -3213,9 +3214,9 @@ HTML
 			print " rowspan=\"$group_size\"" if ($group_size > 1);
 			print ">";
 			print $cgi->a({-href => href(action=>"commit",
-						     hash=>$full_rev,
-						     file_name=>$file_name)},
-				      esc_html($rev));
+			                             hash=>$full_rev,
+			                             file_name=>$file_name)},
+			              esc_html($rev));
 			print "</td>\n";
 		}
 		open (my $dd, "-|", git_cmd(), "rev-parse", "$full_rev^")
@@ -3224,19 +3225,19 @@ HTML
 		close $dd;
 		chomp($parent_commit);
 		my $blamed = href(action => 'blame',
-				  file_name => $meta->{'filename'},
-				  hash_base => $parent_commit);
+		                  file_name => $meta->{'filename'},
+		                  hash_base => $parent_commit);
 		print "<td class=\"linenr\">";
 		print $cgi->a({ -href => "$blamed#l$orig_lineno",
-				-id => "l$lineno",
-				-class => "linenr" },
-			      esc_html($lineno));
+		                -id => "l$lineno",
+		                -class => "linenr" },
+		              esc_html($lineno));
 		print "</td>";
 		print "<td class=\"pre\">" . esc_html($data) . "</td>\n";
 		print "</tr>\n";
 	}
 	print "</table>\n";
-	print "</div>";
+	print "</div><!-- page_body -->\n"; # class="page_body"
 	close $fd
 		or print "Reading blob failed\n";
 	git_footer_html();
@@ -3335,7 +3336,7 @@ HTML
 	print "</table>\n\n";
 	close $fd
 		or print "Reading blob failed.\n";
-	print "</div>";
+	print "</div><!-- page_body -->"; # class="page_body"
 	git_footer_html();
 }
 
@@ -3465,7 +3466,7 @@ sub git_blob {
 		git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
 	} else {
 		print "<div class=\"page_nav\">\n" .
-		      "<br/><br/></div>\n" .
+		      "<br/><br/></div>\n" . # class="page_nav"
 		      "<div class=\"title\">$hash</div>\n";
 	}
 	git_print_page_path($file_name, "blob", $hash_base);
@@ -3491,7 +3492,7 @@ sub git_blob {
 	}
 	close $fd
 		or print "Reading blob failed.\n";
-	print "</div>";
+	print "</div><!-- page_body -->"; # class="page_body"
 	git_footer_html();
 }
 
@@ -3542,7 +3543,7 @@ sub git_tree {
 	} else {
 		undef $hash_base;
 		print "<div class=\"page_nav\">\n";
-		print "<br/><br/></div>\n";
+		print "<br/><br/></div>\n"; # class="page_nav"
 		print "<div class=\"title\">$hash</div>\n";
 	}
 	if (defined $file_name) {
@@ -3594,7 +3595,7 @@ sub git_tree {
 		print "</tr>\n";
 	}
 	print "</table>\n" .
-	      "</div>";
+	      "</div><!-- page_body -->"; # class="page_body";
 	git_footer_html();
 }
 
@@ -3671,19 +3672,19 @@ sub git_log {
 		      " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") .
 		      "<br/>\n" .
-		      "</div>\n" .
+		      "</div><!-- log_link -->\n" . # class="log_link"
 		      "<i>" . esc_html($co{'author_name'}) .  " [$ad{'rfc2822'}]</i><br/>\n" .
-		      "</div>\n";
+		      "</div><!-- title_text -->\n";  # class="title_text"
 
 		print "<div class=\"log_body\">\n";
 		git_print_log($co{'comment'}, -final_empty_line=> 1);
-		print "</div>\n";
+		print "</div><!-- log_body -->\n"; # class="log_body"
 	}
 	if ($#commitlist >= 100) {
 		print "<div class=\"page_nav\">\n";
 		print $cgi->a({-href => href(action=>"log", hash=>$hash, page=>$page+1),
 			       -accesskey => "n", -title => "Alt-n"}, "next");
-		print "</div>\n";
+		print "</div><!-- page_nav -->\n"; # class="page_nav"
 	}
 	git_footer_html();
 }
@@ -3808,11 +3809,11 @@ sub git_commit {
 		      "</tr>\n";
 	}
 	print "</table>".
-	      "</div>\n";
+	      "</div><!-- title_text -->\n"; # class="title_text"
 
 	print "<div class=\"page_body\">\n";
 	git_print_log($co{'comment'});
-	print "</div>\n";
+	print "</div><!-- page_body -->\n"; # class="page_body"
 
 	if (@$parents <= 1) {
 		# do not output difftree/whatchanged for merges
@@ -4018,7 +4019,7 @@ sub git_blobdiff {
 		git_patchset_body($fd, [ \%diffinfo ], $hash_base, $hash_parent_base);
 		close $fd;
 
-		print "</div>\n"; # class="page_body"
+		print "</div><!-- page_body -->\n"; # class="page_body"
 		git_footer_html();
 
 	} else {
@@ -4140,7 +4141,7 @@ sub git_commitdiff {
 		if (@{$co{'comment'}} > 1) {
 			print "<div class=\"log\">\n";
 			git_print_log($co{'comment'}, -final_empty_line=> 1, -remove_title => 1);
-			print "</div>\n"; # class="log"
+			print "</div><!-- log -->\n"; # class="log"
 		}
 
 	} elsif ($format eq 'plain') {
@@ -4175,7 +4176,7 @@ TEXT
 
 		git_patchset_body($fd, \@difftree, $hash, $hash_parent);
 		close $fd;
-		print "</div>\n"; # class="page_body"
+		print "</div><!-- page_body -->\n"; # class="page_body"
 		git_footer_html();
 
 	} elsif ($format eq 'plain') {


-- 
Jakub Narebski
Poland

Re: [PATCH 0/5] gitweb: git_patchset_body fixes

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> Jakub Narebski wrote:
> > Luben Tuikov wrote:
> > 
> >> Ok, I see that Junio has committed the fixes to "next" -- thanks!
> >> That saved me time having to manually apply them.
> >> 
> >> Now, the results are that I see the same bug.
> >  
> > (The same bug = no closing div).
> > 
> > I'll try to reproduce this.
> 
> I failed to reproduce this error.
> 
> Could you apply the following patch, and report the result of
>   grep "</div>\|<div" | grep -v "<div.*</div>
> (find all opening and closing <div> elements, but omitting
> those which are opened and closed in the same line)?

<div class="page_header">
<div class="search">
</div><!-- search -->
</div><!-- page_header -->
<div class="page_nav">
</div><!-- page_nav -->
<div class="header">
</div><!-- header -->
<div class="page_body">
<div class="log">
</div><!-- log -->
<div class="list_head">
</div><!-- list_head -->
<div class="patchset">
<div class="patch" id="patch1">
<div class="diff extended_header">
</div><!-- diff extended_header -->
</div><!-- patch 0 -->
<div class="patch" id="patch2">
<div class="diff extended_header">
</div><!-- diff extended_header -->
</div><!-- patchset -->
</div><!-- page_body -->
<div class="page_footer">
</div><!-- page_footer -->

The bug is clear.

     Luben

[PATCH] gitweb: Fix git_patchset_body not closing <div class="patch">

[Jakub Narebski]

Fix case when git_patchset_body didn't close <div class="patch">,
for patchsets with last patch empty.

perlsyn(1):
  The "last" command immediately exits the loop in question.
  The "continue" block, if any, is not executed.

Remove some commented out code in git_patchset_body.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---

Luben Tuikov wrote:
> --- Jakub Narebski <jnareb@gmail.com> wrote:
>> Jakub Narebski wrote:
>>> Luben Tuikov wrote:
>>> 
>>>> Ok, I see that Junio has committed the fixes to "next" -- thanks!
>>>> That saved me time having to manually apply them.
>>>> 
>>>> Now, the results are that I see the same bug.
>>>  
>>> (The same bug = no closing div).
>>> 
>>> I'll try to reproduce this.
>> 
>> I failed to reproduce this error.
>> 
>> Could you apply the following patch, and report the result of
>>   grep "</div>\|<div" | grep -v "<div.*</div>
>> (find all opening and closing <div> elements, but omitting
>> those which are opened and closed in the same line)?

[...] 
> <div class="patchset">
> <div class="patch" id="patch1">
> <div class="diff extended_header">
> </div><!-- diff extended_header -->
> </div><!-- patch 0 -->
> <div class="patch" id="patch2">
> <div class="diff extended_header">
> </div><!-- diff extended_header -->
> </div><!-- patchset -->
[...] 
> The bug is clear.

Oops. I'm bit new to Perl.

This should fix it.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 25e5079..88af2e6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2412,7 +2412,6 @@ sub git_patchset_body {
 
 			push @diff_header, $patch_line;
 		}
-		#last PATCH unless $patch_line;
 		my $last_patch_line = $patch_line;
 
 		# check if current patch belong to current raw line
@@ -2522,7 +2521,10 @@ sub git_patchset_body {
 
 		# from-file/to-file diff header
 		$patch_line = $last_patch_line;
-		last PATCH unless $patch_line;
+		if (! $patch_line) {
+			print "</div>\n"; # class="patch"
+			last PATCH;
+		}
 		next PATCH if ($patch_line =~ m/^diff /);
 		#assert($patch_line =~ m/^---/) if DEBUG;
 		if ($from{'href'} && $patch_line =~ m!^--- "?a/!) {
@@ -2533,7 +2535,6 @@ sub git_patchset_body {
 		print "<div class=\"diff from_file\">$patch_line</div>\n";
 
 		$patch_line = <$fd>;
-		#last PATCH unless $patch_line;
 		chomp $patch_line;
 
 		#assert($patch_line =~ m/^+++/) if DEBUG;
-- 
1.4.4.3

Re: [PATCH] gitweb: Fix git_patchset_body not closing <div class="patch">

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:

> Fix case when git_patchset_body didn't close <div class="patch">,
> for patchsets with last patch empty.
> 
> perlsyn(1):
>   The "last" command immediately exits the loop in question.
>   The "continue" block, if any, is not executed.
> 
> Remove some commented out code in git_patchset_body.
> 
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
Acked-by: Luben Tuikov <ltuikov@yahoo.com>

This patch fixes it.

    Luben


> ---
> 
> Luben Tuikov wrote:
> > --- Jakub Narebski <jnareb@gmail.com> wrote:
> >> Jakub Narebski wrote:
> >>> Luben Tuikov wrote:
> >>> 
> >>>> Ok, I see that Junio has committed the fixes to "next" -- thanks!
> >>>> That saved me time having to manually apply them.
> >>>> 
> >>>> Now, the results are that I see the same bug.
> >>>  
> >>> (The same bug = no closing div).
> >>> 
> >>> I'll try to reproduce this.
> >> 
> >> I failed to reproduce this error.
> >> 
> >> Could you apply the following patch, and report the result of
> >>   grep "</div>\|<div" | grep -v "<div.*</div>
> >> (find all opening and closing <div> elements, but omitting
> >> those which are opened and closed in the same line)?
> 
> [...] 
> > <div class="patchset">
> > <div class="patch" id="patch1">
> > <div class="diff extended_header">
> > </div><!-- diff extended_header -->
> > </div><!-- patch 0 -->
> > <div class="patch" id="patch2">
> > <div class="diff extended_header">
> > </div><!-- diff extended_header -->
> > </div><!-- patchset -->
> [...] 
> > The bug is clear.
> 
> Oops. I'm bit new to Perl.
> 
> This should fix it.
> 
>  gitweb/gitweb.perl |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 25e5079..88af2e6 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2412,7 +2412,6 @@ sub git_patchset_body {
>  
>  			push @diff_header, $patch_line;
>  		}
> -		#last PATCH unless $patch_line;
>  		my $last_patch_line = $patch_line;
>  
>  		# check if current patch belong to current raw line
> @@ -2522,7 +2521,10 @@ sub git_patchset_body {
>  
>  		# from-file/to-file diff header
>  		$patch_line = $last_patch_line;
> -		last PATCH unless $patch_line;
> +		if (! $patch_line) {
> +			print "</div>\n"; # class="patch"
> +			last PATCH;
> +		}
>  		next PATCH if ($patch_line =~ m/^diff /);
>  		#assert($patch_line =~ m/^---/) if DEBUG;
>  		if ($from{'href'} && $patch_line =~ m!^--- "?a/!) {
> @@ -2533,7 +2535,6 @@ sub git_patchset_body {
>  		print "<div class=\"diff from_file\">$patch_line</div>\n";
>  
>  		$patch_line = <$fd>;
> -		#last PATCH unless $patch_line;
>  		chomp $patch_line;
>  
>  		#assert($patch_line =~ m/^+++/) if DEBUG;
> -- 
> 1.4.4.3
> 
> 

Re: [PATCH 0/5] gitweb: git_patchset_body fixes

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> Luben Tuikov wrote:
> 
> > Ok, I see that Junio has committed the fixes to "next" -- thanks!
> > That saved me time having to manually apply them.
> > 
> > Now, the results are that I see the same bug.
>  
> (The same bug = no closing div).
> 
> I'll try to reproduce this.
> 
> 
> What about the other error, 
> 
> >>> The second bug is displaying a renamed file with the
> >>> same name as the one of the preceding "patch". Saw
> >>> this one today when I pulled.
> 
> Is it corrected by those patches?

Yes, the second bug is corrected by your patches.
The first one still stands.

    Luben

[PATCH 1/5] gitweb: Fix error in git_patchest_body for file creation/deletion patch

[Jakub Narebski]

$from_id, $to_id variables should be local per PATCH.

Fix error in git_patchset_body for file creation (deletion) patches,
where instead of /dev/null as from-file (to-file) diff header line, it
had link to previous file with current file name.  This error occured
only if there was another patch before file creation (deletion) patch.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Luben Tuikov wrote:

> The second bug is displaying a renamed file with the
> same name as the one of the preceding "patch". Saw
> this one today when I pulled.
> 
> To exhibit these, you need to "commitdiff" a commit
> which has at least one rename and at least one regular
> diff preceding the rename.

I think this should also fix this bug.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7906280..04c8015 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2378,7 +2378,6 @@ sub git_patchset_body {
 	my $patch_line;
 	my $diffinfo;
 	my (%from, %to);
-	my ($from_id, $to_id);
 
 	print "<div class=\"patchset\">\n";
 
@@ -2392,6 +2391,7 @@ sub git_patchset_body {
  PATCH:
 	while ($patch_line) {
 		my @diff_header;
+		my ($from_id, $to_id);
 
 		# git diff header
 		#assert($patch_line =~ m/^diff /) if DEBUG;
@@ -2439,11 +2439,15 @@ sub git_patchset_body {
 				$from{'href'} = href(action=>"blob", hash_base=>$hash_parent,
 				                     hash=>$diffinfo->{'from_id'},
 				                     file_name=>$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'});
+			} else {
+				delete $to{'href'};
 			}
 			# this is first patch for raw difftree line with $patch_idx index
 			# we index @$difftree array from 0, but number patches from 1
-- 
1.4.4.3

Re: [PATCH 1/5] gitweb: Fix error in git_patchest_body for file creation/deletion patch

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:

> $from_id, $to_id variables should be local per PATCH.

There. You've already refactored the logic, now pull it into
a separate function.

BTW, I'll try the patches (unless Junio commits them to "next"
first) Monday sometime.

    Luben

[PATCH 2/5] gitweb: Fix error in "rename to"/"copy to" git diff header output

[Jakub Narebski]

Fix error in git_patchset_body subroutine, which caused "rename to"/"copy
to" line in extended diff header to be displayed incorrectly.

While at it, fix align.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Stupid typu: was '=' instead of '.='.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 04c8015..1b4a4c0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2479,11 +2479,11 @@ sub git_patchset_body {
 			# 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'}));
+				                       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'}));
+				$patch_line .= $cgi->a({-href=>$to{'href'}, -class=>"path"},
+				                       esc_path($to{'file'}));
 			}
 			# match <mode>
 			if ($patch_line =~ m/\s(\d{6})$/) {
-- 
1.4.4.3

[PATCH 3/5] gitweb: Fix errors in git_patchset_body for empty patches

[Jakub Narebski]

We now do not skip over empty patches in git_patchset_body (where
empty means that they consist only of git diff header, and of extended
diff header, for example "pure rename" patch).  This means that after
extended diff header there can be next patch (i.e. /^diff /) or end of
patchset, and not necessary patch body (i.e. /^--- /).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1b4a4c0..7ebdfbb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2403,7 +2403,7 @@ sub git_patchset_body {
 		while ($patch_line = <$fd>) {
 			chomp $patch_line;
 
-			last EXTENDED_HEADER if ($patch_line =~ m/^--- /);
+			last EXTENDED_HEADER if ($patch_line =~ m/^--- |^diff /);
 
 			if ($patch_line =~ m/^index ([0-9a-fA-F]{40})..([0-9a-fA-F]{40})/) {
 				$from_id = $1;
@@ -2522,6 +2522,8 @@ sub git_patchset_body {
 
 		# from-file/to-file diff header
 		$patch_line = $last_patch_line;
+		last PATCH unless $patch_line;
+		next PATCH if ($patch_line =~ m/^diff /);
 		#assert($patch_line =~ m/^---/) if DEBUG;
 		if ($from{'href'}) {
 			$patch_line = '--- a/' .
-- 
1.4.4.3

[PATCH 4/5] Revert "gitweb: There can be empty patches (in git_patchset_body)"

[Jakub Narebski]

This reverts commit 1ebb948f656c03a5bdaab4de1a113b9ffcb98bea,
as that patch quieted warning but was not proper solution.
The previous commit was.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7ebdfbb..d60d7c6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2533,7 +2533,7 @@ sub git_patchset_body {
 		print "<div class=\"diff from_file\">$patch_line</div>\n";
 
 		$patch_line = <$fd>;
-		last PATCH unless $patch_line;
+		#last PATCH unless $patch_line;
 		chomp $patch_line;
 
 		#assert($patch_line =~ m/^+++/) if DEBUG;
-- 
1.4.4.3

[PATCH 5/5] gitweb: Fix split patches output (e.g. file to symlink)

[Jakub Narebski]

Do not replace /dev/null in two-line from-file/to-file diff header for
split patches ("split" patch mean more than one patch per one
diff-tree raw line) by a/file or b/file link.

Split patches differ from pair of deletion/creation patch in git diff
header: both a/file and b/file are hyperlinks, in all patches in a
split.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d60d7c6..f46a422 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2525,7 +2525,7 @@ sub git_patchset_body {
 		last PATCH unless $patch_line;
 		next PATCH if ($patch_line =~ m/^diff /);
 		#assert($patch_line =~ m/^---/) if DEBUG;
-		if ($from{'href'}) {
+		if ($from{'href'} && $patch_line =~ m!^--- "?a/!) {
 			$patch_line = '--- a/' .
 			              $cgi->a({-href=>$from{'href'}, -class=>"path"},
 			                      esc_path($from{'file'}));
@@ -2537,7 +2537,7 @@ sub git_patchset_body {
 		chomp $patch_line;
 
 		#assert($patch_line =~ m/^+++/) if DEBUG;
-		if ($to{'href'}) {
+		if ($to{'href'} && $patch_line =~ m!^\+\+\+ "?b/!) {
 			$patch_line = '+++ b/' .
 			              $cgi->a({-href=>$to{'href'}, -class=>"path"},
 			                      esc_path($to{'file'}));
-- 
1.4.4.3