174942 - Patch Viewer

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P3)

Description

[John Keiser (jkeiser)]

I have written a Patch Viewer that handles all sorts of patches, can add context
into the patch by getting files out of a cvs tree, can link to Bonsai or LXR if
the installation specifies, allows a collapsable view of the patch for
reviewers, and is pretty nice-looking (like Bonsai's patch view in fact).  This
bug is for its review, suggestions, and implementation since bug 115910 is
getting rather full :)

This also can potentially "fix" bug 119502 as the patch viewer is capable of
translating any type of diff into a unified diff suitable for patching with the
format=raw parameter.

Comment 1

[John Keiser (jkeiser)]

Forgot to CC the implementor of the other somewhat similar Patch Viewer :) 
Setting blocking that bug for now so that I don't lose track.

Comment 2

[John Keiser (jkeiser)]

Attachment: New Files (patchviewer.tgz)

This is the preliminary set of new files.  It seems to work against all sorts
of diffs--unified and non-unified against file and directory, cvs unified and
non-unified, cvs unified with new/removed files (haven't tested new/remove with
non-unified), and hugeass cvs unified.

Actual patch for Bugzilla upcoming.

Comment 3

[John Keiser (jkeiser)]

Attachment: Patch v0.6

Still TODO:
- use templates instead of printing out raw (this will fix the problem where
HTML isn't getting escaped)
- test non-unified diff with new and removed files
- make everything one hugeass table (maybe just each file a single table) to
reduce jaggedness when browser gets small
- reduce the number of notifications sent in PatchIterators (send section-level
notifications instead of line-by-line) to aid performance (seems pretty fast
now anyway)

This patch is against 2.16.1 because that is the source I happened to download
:)

Comment 4

[John Keiser (jkeiser)]

Also:
- ensure security of the cvs co (I don't believe we try to check out files that
are not under the cvsroot but I will have to check)

Comment 5

[John Keiser (jkeiser)]

Attachment: Patch v0.7

Comment 6

[John Keiser (jkeiser)]

Attachment: New Files v0.7

OK, this version makes each file its own table, templatizes the output and adds
the notification streamlining.	There are probably garbage files in the
DiffPatch/ and PatchIterator/ directories, ignore those and focus on the .pm's.


TODO:
- testing (retest everything plus the new/removed non-unified thing)
- hook raw patch output back up
- consider putting file summaries up (+/- lines, etc.); make LXR links
- put LXR/Bonsai configuration into app somewhere

Patch is getting close.

Comment 7

[Joel Peshkin]

A few notes...
The definitions for the repository location, etc... should not be in the program
code.  If there is no repository defined, the patch viewer should be hidden from
the UI.  Quite probably, the repository configuration information should
actually live in the DB as a per-component item.

Comment 8

[John Keiser (jkeiser)]

Thus the TODO item: - put LXR/Bonsai configuration into app somewhere

Per-module configuration is a good idea.

Comment 9

[John Keiser (jkeiser)]

Also, the patch viewer does not need to be hidden if there is no repository.  It
works fine without a repository.

Comment 10

[John Keiser (jkeiser)]

I would like to put a little interdiff-using utility in here (for installs that
have interdiff) so that we can see differences between two patches.

Comment 11

[John Keiser (jkeiser)]

Attachment: Patch v1.0

OK, this contains all enhancements up 'til now except the interdiff.  That
seems like it will be pretty simple so I may fix it in the next review round
with an action=interdiff&id=<attachid>&oldid=<attachid>.

Seeking r= and comments on my Bugzilla newbieness :)

Comment 12

[John Keiser (jkeiser)]

Attachment: New Files v1.0

These are the new files associated with it.  Since unfortunately this was
against 2.16 (I will do it against cvs soon enough), cvs add wants to fail.

Comment 13

[John Keiser (jkeiser)]

Attachment: Patch v1.5

This version includes the ability to make a diff between two versions with
interdiff.  This is the last feature version.

Comment 14

[John Keiser (jkeiser)]

Attachment: New Files v1.5

This is the new files (a few tweaks to templates were necessary though the bulk
of the work was in the patch--you could, of course, see this if Patch Viewer
were in Bugzilla :)

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 104122 [details] [diff] [review]
Patch v1.5

Quick glance (I haven't looked at the new files yet, or tested)

+  $::FORM{'format'} = $::FORM{'format'} || $_[0];

do:

$::FORM{'format'} ||= $_[0] instead

+  $::FORM{'context'} = defined($::FORM{'context'}) ? $::FORM{'context'} :
+						     "patch";

Becomes:

$::FORM{'context'} = "patch" unless defined($::FORM{'context'});

You should do |binmode INTERDIFF|, both for windows and for perl5.8+UTF8. Yes,
diffs will be text/plain, but there are often non-ascii names  in there
(hakon's name finds bugs in things every so often)

Instad of INTERDIFF, you can use $interdiff. We require perl5.6, which
autovivifies teh filehandles for us, so thats not an issue.

Rather than a cvspath, maybe you want a cvsbin parm, insteaad? Also, this
shouldn't be in localconfig; move it to params by editing defparams.pl

Don't use CGI to print the headers, just use |print|. Once Bugzilla::CGI goes
in, I'm going to do a global replacement anyway, using that. There's also the
issue that CGI.pm enforces a charset on text/*, which may not be what we want.

You're not diffing against cvs tip - DefParam is no more (theres a hash,
instead)

Drop the xml.cgi change :)

That should be enough to keep you going :)

Comment 16

[John Keiser (jkeiser)]

> Instad of INTERDIFF, you can use $interdiff. We require perl5.6, which
> autovivifies teh filehandles for us, so thats not an issue.

Is there anything wrong with INTERDIFF?  Not that I mind changing it, but I am
wondering if it has to be changed.

> Rather than a cvspath, maybe you want a cvsbin parm, insteaad? Also, this
> shouldn't be in localconfig; move it to params by editing defparams.pl

cvspath is in checksetup.pl because I was copying the mysql_binaries stuff and
this is the same deal; does that need to be moved too?

> Don't use CGI to print the headers, just use |print|. Once Bugzilla::CGI goes
> in, I'm going to do a global replacement anyway, using that. There's also the
> issue that CGI.pm enforces a charset on text/*, which may not be what we want.

I guess I can, but that means the cache header is a lot harder to print out, and
probably won't get printed at all ...

> That should be enough to keep you going :)

I won't be able to get back to Bugzilla on Monday (unless I have a landfill
account I can hack on) and these are relatively minor things, so perhaps review
can continue even though I haven't uploaded a new patch ...

Comment 17

[Bradley Baetz (:bbaetz)]

INTERDIFF is globla, and may cause mod_perl problems if the script dies w/o it
being closed. Using a lexical var will close teh file when it goes out of scope.
See
http://perl.apache.org/docs/1.0/guide/porting.html#Filehandlers_and_locks_leakages

The mysql bin stuff is in checksetup because checksetup uses it, possibly before
params have been used. Thats not an issue for the cvs stuff.

You can manually print an Expires. Anyway, at teh least you should use new
CGI(""), so that CGI.pm doesn't try to do the parsing which you're not going to
use. Actually, you can call CGI::header, since the CGI stuff has a functional
interface too.

Comment 18

[John Keiser (jkeiser)]

Attachment: Patch v1.6.3

This version of the patch merges to tip, handles bbaetz's review, and now
cleans up after itself better (it was not removing the temp directory at the
end).  I have a landfill directory now, http://landfill.bugzilla.org/bz176656
... play with it, report bugs, etc.

If it helps to review or play with, you can see this patch here:
http://landfill.bugzilla.org/bz176656/attachment.cgi?id=101&action=prettyview

To see interdiff in action, take this example (an interdiff between two
versions of this patch, which even has many new files):
http://landfill.bugzilla.org/bz176656/attachment.cgi?oldid=99&action=interdiff&newid=101&headers=1


My little testsuite of patch formats is here:
http://landfill.bugzilla.org/bz176656/show_bug.cgi?id=894

I did make interdiffpath => interdiffbin and cvspath => cvsbin, but I did not
move these variables into params for the moment because they seem out of place
there (also it is hard to autodetect the paths in params, yes?).

Comment 19

[John Keiser (jkeiser)]

BTW, for some damn fool reason I forgot to include white-space: pre in the css
file, which made all spacing go away.  That is now fixed on landfill; I will
upload a new patch at such time as a review is completed.

Comment 20

[John Keiser (jkeiser)]

I have further refined the CSS on landfill thanks to timeless and caillon ... it
makes the two-column behavior completely fixed to the size of the browser
window, even if there are long lines.  It also fixes a bug where IE doesn't
support white-space: pre!  Due to deficiencies in IE's CSS2 support that I have
been unable to solve, IE actually cuts off text when it reaches the end of a line :(

Oh, and also new and removed files show up as one col instead of two.

Comment 21

[Jesse Ruderman]

*** Bug 70436 has been marked as a duplicate of this bug. ***

Comment 22

[John Keiser (jkeiser)]

Attachment: Patch v1.6.4

This patch makes visual enhancements:
- makes table spacing and borders look as good as I can make them in both Moz /
IE
- makes empty lines take up space again
- makes pre do wrapping, at least on Mozilla (CSS3 supports this extension). 
IE truncates :(
- gives minus lines a different color from plus lines
- makes added and removed files take up both columns
- makes plus and minus lines be in the right columns :)
- removes a little cruft from PatchIterator::template

With Patch Viewer highlights:
http://landfill.bugzilla.org/bz176656/attachment.cgi?id=104&action=prettyview
Diff from v1.6.3:
http://landfill.bugzilla.org/bz176656/attachment.cgi?oldid=101&action=interdiff&newid=104&headers=1

Comment 23

[ArronM (:paper)]

ooh, pretty.  Any chance of getting a "cvs diff -uw" added to the drop down list? :)

Comment 24

[John Keiser (jkeiser)]

I have considered adding "drop whitespace changes" to it, but that requires
writing another filter (probably not a truly hard job, just needs to be done). 
I think it's best to leave that as a separate bug :)

Comment 25

[John Keiser (jkeiser)]

FYI, I've added <a name> in each section and a Link To This link so that people
can grab the section, so that reviewers have something to refer to when they
talk to people.  It's on landfill.  I'll upload a new patch with the first batch
of reviews or with more changes if I find them.  I use the system frequently to
review patches and it's working well for me.

I've also thought of a potential way to do whitespace changes--highlight them in
gray instead of green so that reviewers could ignore them if they wish.  It
wouldn't be as perfect as doing an actual diff -w but in many cases it will
serve the purpose better.  The algoritm to figure out what parts of a section
were whitespace changes will be interesting :)  I don't think we should do it
with this patch, but I might if the review waits a lot longer.

Comment 26

[John Keiser (jkeiser)]

Attachment: Patch v1.7.0

This patch is up to date with cvs, and has one new feature.  changelog:
- added a new "View Attachment As Diff" button in edit mode to make reviewing
that much easier (it shows up an an iframe where the patch view iframe and edit
as comment textarea currently show up)
- patch viewer now works with patch sections with > 1000 lines (while loops in
templates stop at 1000; now it supports up to 1,000,000)
- dashed lines bordering the columns of the patch for easier delineation of
removed / added lines

Comment 27

[John Keiser (jkeiser)]

Oh, and that patch also adds:
- "Link To This" so that you can link to any given section and refer to it
during review

You can see the View Attachment As Diff stuff in action at
http://landfill.bugzilla.org/bz176656/attachment.cgi?id=107&action=edit.

Comment 28

[John Keiser (jkeiser)]

Attachment: Patch v1.7.0

Oops, that last diff was apparently made before I fixed cvs conflicts.

Comment 29

[John Keiser (jkeiser)]

Attachment: Patch v1.7.1

This patch should be just about exactly the same as 1.7.0, updated to trunk
(there were no conflicts).  Uploading to make for absolute sure.

Comment 30

[John Keiser (jkeiser)]

Comment on attachment 115698 [details] [diff] [review]
Patch v1.7.1

Oh, the big difference was I didn't include DiffPrinter/html.pm in this one. 
That file is just not being used.

Comment 31

[Daniel Berlin]

Small bug in cvsroot parsing.
you have: "if ($cvsroot =~ /^:([^:]*):(.*)(\/.*)$/) {"
it should be: " if ($cvsroot =~ /^:([^:]*):(.*?)(\/.*)$/) {"

The reason being that the root starts at the first /, and without this, it'll start at the *last* / due to maximal munch.

IE in :pserver:anoncvs:@gcc.gnu.org:/cvs/gcc, your current version will claim the root ($3) is "/gcc", when in reality it's "/cvs/gcc".

Otherwise, it works great.

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 111309 [details] [diff] [review]
Patch v1.7.0

General Comments:

* if the context is Patch or File, that option should be highlighted and not 
clickable;
* if all files are collapsed or expanded, the option should be highlighted and 
not clickable;
* setting headers to zero didn't have an effect on my buttmonkey installation, 
although it's running an older version of the patch;
* copy-paste of code into another application inserts blank spaces between each 
line and copies collapsed code; not sure if there's anything you can do about 
that, but it would be better if it didn't do either;
* line length should be 80 characters except for good reason;
* nit: anchors with onclick handlers instead of URLs should put informative 
messages in the status bar onmouseover;
* nit: it would be very good if collapsed state were retained when settings 
(f.e. lines of context) are changed;
* nit: quote attribute values;
* nit: change "prettyview" to "diff" in file names and the value of the
"action" 
variable;
* validate (normalize?) "headers" in attachment.cgi instead of in the 
pretty-view header and footer templates;
* modules should be in the bugzilla/ directory and have license headers if they 
are to be checked in to our CVS repository;
* nowhere at the top does the page say the name of the attachment; it should;
* "target" should be renamed to something like "next_iterator".


File-specific Comments:

mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl 



mozilla/webtools/bugzilla/template/en/default/attachment/prettyview-header.html
.tmpl:

>    var elem = document.checkboxform.firstChild;

Any particular reason you are using DOM0 methods/properties?  Is this code 
functional on a DOM0 browser?

>  [% h1 = BLOCK %][% IF attachid %]Attachment #[% attachid %][% ELSE %]Diff 
Between Attachments #<a href="attachment.cgi?action=prettyview&id=[% oldid 
%]">[% oldid %]</a> and <a href="attachment.cgi?action=prettyview&id=[% newid 
%]">[% newid %]</a>[% END %] for <a href="show_bug.cgi?id=[% bugid %]">Bug #[% 
bugid %]</a>[% END %]

Nit: & -> &amp; within attribute values (i.e. hrefs)

>  [% PROCESS global/header.html.tmpl 
>    title = title
>    h1 = h1
>    h2 = h2
>    style = style
>    onload = onload
>    javascript = javascript
>  %]

Nit: None of these variable assignments need to happen unless the variables get 
changed in the processed template and those changes shouldn't make their way to 
the processing template (in which case you can use an INCLUDE and still not set 
the variables explicitly.

>[% ELSE %]
><html>
><head>
><style type="text/css">
>[% style %]
></style>
><script type="text/javascript" language="JavaScript">
><!--
>[% javascript %]
>-->
></script>
></head>
><body onload="[% onload FILTER html %]">
>[% END %]

Nit: indent for code readability, not output readability.

>    [% IF headers %]|[%END%] Differences between <form style="display: 
inline"><select name="oldid">[% FOREACH patch = other_patches %]

Why display this even if you aren't generally displaying headers?

>      <option value="[% patch.id %]">[% patch.desc %]</option>

patch.desc FILTER html

Why does the "Raw Unified" option always appear in interdiff but only appear in 
prettyview if headers is on?

>[%# Collapse / Expand %]
><a href="#" onclick="return collapse_all(false)">Collapse All</a>
>| <a href="#" onclick="return collapse_all(true)">Expand All</a>

Nit: "collapse_all" is an unfortunate name for a function that either collapses 
or expands all, although I can't come up with a better one.  Maybe something 
like "change_collapse_state", although it's ugly.

>  <form style="display: inline"><input type="hidden" name="action" 
value="prettyview"><input type="hidden" name="id" value="[% attachid %]"><input 
type="hidden" name="collapsed" value="[% collapsed FILTER html %]"><input 
type="hidden" name="headers" value="[% headers FILTER html %]"><input 
type="text" name="context" value="[% context FILTER html %]" size="3"></form>)

Strictly speaking headers doesn't need to be escaped, but it's probably better 
this way, and since it only happens once it won't have an impact on
performance.



mozilla/webtools/bugzilla/template/en/default/attachment/prettyview-footer.html
.tmpl

There's no closing </form> tag to complement the following code in 
prettyview-header.html.tmpl:

>[%# Restore Stuff %]
><form name="checkboxform">
><input type="checkbox" name="restore_indicator" style="display: none">



mozilla/webtools/bugzilla/template/en/default/attachment/prettyview-file.html.t
mpl

><table class="file_table"><thead><tr><td class="file_head" colspan="2"><a 
href="#" onclick="return twisty_click(this)">[% collapsed ? '(+)' : '(-)' 
%]</a><input type="checkbox" name="[% file.filename %]"[% collapsed ? '' : ' 
checked' %] style="display: none">

file.filename FILTER html

>        [% i = 0 %]
>        [% WHILE (i < group.plus.size || i < group.minus.size) %]
>          [% currentloop = 0 %]
>          [% WHILE currentloop < 500 && (i < group.plus.size || i < 
group.minus.size) %]
>            <tr class=changed><td><pre>[% group.minus.$i FILTER html 
%]</pre></td><td><pre>[% group.plus.$i FILTER html %]</pre></td></tr>
>            [% currentloop = currentloop + 1 %]
>            [% i = i + 1 %]
>          [% END %]
>        [% END %]

Why use currentloop here?  Couldn't you just check for i<500?



mozilla/webtools/bugzilla/defparams.pl

Nits: capitalize Bugzilla and CVS.



mozilla/webtools/bugzilla/checksetup.pl

>    $interdiff_executable =~ s/\r?\n$//;

Why not just chomp it like you did with the cvs executable?



mozilla/webtools/bugzilla/attachment.cgi

>sub validateID
>{
>    my $id = @_ ? $_[0] : 'id';

Nit: "param" would be a more immediately understandable variable name.

>sub interdiff
>{
>  # Get old patch data
>  my ($old_bugid, $old_filename) = get_unified_diff($::FORM{'oldid'});
>
>  # Get old patch data
>  my ($new_bugid, $new_filename) = get_unified_diff($::FORM{'newid'});

Err, the latter comment should be "# Get *new* patch data".

>  $ENV{'PATH'} = $::diffpath; # Intentionally done twice to suppress a warning

Not needed anymore since you use $::diffpath twice, but use "use vars" if you 
need to do this in the future.

>+    # Actually print out the patch
>+    print $cgi->header(-type => 'text/plain',
>+                            -expires => '+3M');

Nit: funky indentation.

>+  #
>+  # Delete temporary files
>+  #
>+  unlink($old_filename) or die "Could not unlink $old_filename: $!";
>+  unlink($new_filename) or die "Could not unlink $new_filename: $!";

I wonder if this would be better as a warn.

>    ThrowUserError("must_be_patch");

This should be a code error since there's no legitimate way for a user to 
request an interdiff on non-patches.

>+  # Reads in the patch, converting to unified diff in a temp file
>+  my $iter = new PatchIterator::RawPatchIterator;
>+  # fixes patch root (makes canonical if possible)
>+  $iter->target(new PatchIterator::FixPatchRootIterator(Param('cvsroot')));
>+  # Prints out to temporary file
>+  my ($fh, $filename) = File::Temp::tempfile();
>+  $iter->target->target(new DiffPrinter::raw($fh));

target->target?



mozilla/webtools/bugzilla/CVSClient.pm

>    # Format: :method:[user[:password]@]server[:port]/path

You need a colon before "/path".

> ^(([^\@:]*)(:([^\@]*))?\@)?([^:]*)(:(.*))?$

This regexp needs to take the possibility of a colon after the server or port 
into account.



mozilla/webtools/bugzilla/DiffPrinter/html.pm

Per previous conversation, to be removed.



mozilla/webtools/bugzilla/DiffPrinter/template.pm

>sub end_file {
>  my $this = shift;
>  my $file = $this->{ARGS}{file};
>  if ($file->{canonical} && $file->{old_revision} && $this->{ARGS}{bonsai_url}) 
{
>    $this->{ARGS}{bonsai_prefix} = 
"$this->{ARGS}{bonsai_url}/cvsblame.cgi?file=$file->{filename}&rev=$file->{old_
revision}";

& -> &amp;

>sub section {

Nit: other function names start with verbs.  This one would make more sense as
a 
verb as well.

>      if ($last_line_char ne ' ') {
>        push @{$section->{groups}}, {context => $context_lines,
>                                     plus => $plus_lines,
>                                     minus => $minus_lines};
>        $context_lines = [];
>        $plus_lines = [];
>        $minus_lines = [];
>        $last_line_char = '';
>      }
>      $last_line_char = ' ';

Why set $last_line_char to '' right before you set it to something else?



mozilla/webtools/bugzilla/DiffPrinter/raw.pm



mozilla/webtools/bugzilla/PatchIterator/BasePatchIterator.pm

Why repeat the words "Patch" and "Iterator" in the file name of this and other 
scripts?



mozilla/webtools/bugzilla/PatchIterator/FilterPatchIterator.pm



mozilla/webtools/bugzilla/PatchIterator/RawPatchIterator.pm

Nit: use 4-space indent

>sub rawline {

Nit: other function names start with verbs.  This one would make more sense as
a 
verb as well.

>sub startlines {
>sub endlines {

Nit: other functions separate verb and noun by underscore (_).

>  } elsif ($line =~ /^(\d+),?(\d*)([acd])(\d+),?(\d*)/) {
>    # Non-universal diff.  Calculate as though it were universal.
>
>    if ($3 eq 'a') {
>      # 'a' has the old number one off from diff -u ("insert after this line")
>      # vs. "insert at this line")
>
>    if ($3 eq 'd') {
>      # 'd' has the new number one off from diff -u ("delete after this line")
>      # vs. "delete at this line")

What about "c"?  Also note extra closing parentheses in these comments.

>  } elsif ($line =~ /^-($|[^-])/) {

Why not just /^-[^-]*$/ ?  Seems equivalent, but easier to parse.  Oh well, I 
guess it doesn't matter; it does the same thing.

>  open FILE, $filename or die "Could not open $this->{FILENAME}: $!";

$filename but $this->{FILENAME}?

>sub iterate_file {
>sub iterate_fh {
>sub iterate_string {

Ah, for strong typing and method overloading...



mozilla/webtools/bugzilla/PatchIterator/FixPatchRootIterator.pm

>           substr($file->{rcs_filename}, 0, length($this->{REPOSITORY_ROOT})) 
eq
>           $this->{REPOSITORY_ROOT}) {

>    $this->{DIFF_ROOT} = substr($this->{DIFF_ROOT}, 0,
>                                -length($file->{filename}));

Nit: These might be clearer as:

  $file->{rcs_filename} =~ /^\Q$this->{REPOSITORY_ROOT}\E/
  $this-{DIFF_ROOT} =~ /\Q$file->{filename}\E$/



mozilla/webtools/bugzilla/PatchIterator/AddCVSContextIterator.pm

>    CVSClient::cvs_co_rev($this->{CVSROOT}, $this->{REVISION}, 
$this->{FILENAME}) and return;

Nit: "and return" could use a comment.

>+  $this->{HAS_CVS_CONTEXT} = !$file->{is_add} && !$file->{is_remove};
...
>+  return if ! $this->{HAS_CVS_CONTEXT} || ! $this->{REVISION};

You only use HAS_CVS_CONTEXT once, so you should add the test for 
!$this->{REVISION}
to it or just eliminate HAS_CVS_CONTEXT entirely and do all three tests 
together.

>1

Nit: 1;

+sub determine_start {
+sub _start_section {
+sub flush_section {
+sub push_context_lines {
+sub trick_taint {

How do you determine what functions get an underscore prefix?

>      if (! exists($this->{SECTION})) {
>        $this->_start_section();

>sub start_file {
...
>  $this->{SECTION_END} = -1;

>sub _start_section {
...
>  $this->{SECTION_END} = $this->{SECTION}{old_start} - 1;

I don't see where SECTION_END gets used between the invocation of start_file
and 
that of _start_section.  Am I missing something?


>  die "File read too far!" if $this->{FILE_LINE} && $this->{FILE_LINE} > 
$start;

This line doesn't seem to do anything, since FILE_LINE isn't defined elsewhere. 
Did you mean NEXT_FILE_LINE?

In general, this module needs a lot more documentation.  F.e., the following 
code only runs for the first section in each file, but there's no indication of 
that anywhere, and it requires a lot of digging to figure it out.  It
shouldn't.



mozilla/webtools/bugzilla/PatchIterator/PatchInfoGrabber.pm



mozilla/webtools/bugzilla/PatchIterator/NarrowPatchIterator.pm



The parts of this that need second review are the templates (particularly for 
XSS vulnerabilities) and modifications to existing files (attachment.cgi, 
checksetup.pl, defparams.pl).  The rest of the patch (CVSClient.pm, 
DiffPrinter/*, and PatchIterator/*) also needs a second review if it goes into 
Bugzilla, but, since none of it is Bugzilla specific, it should probably become 
a series of CPAN modules instead, in which case a second review (and third, 
probably), for overall architecture at least, would be a good idea, but isn't
required for Bugzilla to use it.

Comment 33

[John Keiser (jkeiser)]

Attachment: Patch v1.8

This addresses myk's comments.

Comment 34

[John Keiser (jkeiser)]

Attachment: PatchIterator classes v1.8

This is the updated .tgz with the PatchIterator files.	Untarring it into the
bugzilla directory alongside the other scripts will create the PatchIterator/
directory and everything will work.

Comment 35

[John Keiser (jkeiser)]

Comment on attachment 122817 [details] [diff] [review]
Patch v1.8

Everything fixed, except:

> * if all files are collapsed or expanded, the option should be highlighted
> and not clickable;

This is not particularly easy (it involves dynamic HTML changes--possibly we
could use display: none) and is not a major problem IMO; filed in bug 195305,
let me know if this is a requirement.

> * copy-paste of code into another application inserts blank spaces between
> each line and copies collapsed code; not sure if there's anything you can do
> about that, but it would be better if it didn't do either;

There's not much I can do about it now, and given the way it outputs lines
(with table cells, next to each other, the output would suck) it would be
difficult at best to make the copy do much useful.  How does Bonsai work?

> * line length should be 80 characters except for good reason;

They generally are, except for a few places with long strings where the only
thing that makes a huge line wrap is one or two punctuation characters at the
end, and SendSQL statements, which prevalently have a long string and do not
fit the
80 character rule in the file.

I also did not wrap HTML template lines, which generally are not wrapped in
Bugzilla.  I plead module style :)

> * nit: it would be very good if collapsed state were retained when settings 
> (f.e. lines of context) are changed;

Another difficult one, filed in bug 195305.

> * nit: quote attribute values;

Done, I think; I got all the ones I could find.

> * modules should be in the bugzilla/ directory and have license headers if they 
> are to be checked in to our CVS repository;

Everything was diffed relative to bugzilla/ ... what do you need here?

> * "target" should be renamed to something like "next_iterator".

I remember us talking about this; next_iterator() doesn't convey flow while
target does, maybe sends_data_to() would be better?

File-specific Comments:

mozilla/webtools/bugzilla/template/en/default/attachment/prettyview-header.html
.tmpl:
> >    var elem = document.checkboxform.firstChild;
> Any particular reason you are using DOM0 methods/properties?  Is this code 
functional on a DOM0 browser?

It definitely won't work on any browser that doesn't support CSS, no.  Are you
referring to document.checkboxform?  This is very accepted programming
practice,
to the point of being the preferred way to access forms on the web.

> >    [% IF headers %]|[%END%] Differences between <form style="display: 
> >    inline"><select name="oldid">[% FOREACH patch = other_patches %]
> Why display this even if you aren't generally displaying headers?

It turned out to be something I wanted to be able to do from within the Edit
Attachment window.  I think I had a justification for it, but I can't recall
what so it must have been slim.  Basically headers=0 got overloaded to mean
"stuff that needs to be in attachment view".  Probably needs to be fixed,
probably not worth the effort right now.

> Why does the "Raw Unified" option always appear in interdiff but only appear in 
> prettyview if headers is on?

A bug.	Fixed.

> Nit: "collapse_all" is an unfortunate name for a function that either
> collapses or expands all, although I can't come up with a better one.  Maybe
> something like "change_collapse_state", although it's ugly.

Looking at it, I think the slight sharing of code is not worth the readability
headache.  Moved to two separate functions, collapse_all() and expand_all().


mozilla/webtools/bugzilla/template/en/default/attachment/prettyview-footer.html
.tmpl

> There's no closing </form> tag to complement the following code in 
> prettyview-header.html.tmpl:

The </form> is in prettyview-footer.html.tmpl.	The entire thing is one big
form so that the checkboxes can all be in it.


mozilla/webtools/bugzilla/template/en/default/attachment/prettyview-file.html.t
mpl
> >        [% i = 0 %]
> >        [% WHILE (i < group.plus.size || i < group.minus.size) %]
> >          [% currentloop = 0 %]
> >          [% WHILE currentloop < 500 && (i < group.plus.size || i < 
> group.minus.size) %]
> >            <tr class=changed><td><pre>[% group.minus.$i FILTER html 
> %]</pre></td><td><pre>[% group.plus.$i FILTER html %]</pre></td></tr>
> >            [% currentloop = currentloop + 1 %]
> >            [% i = i + 1 %]
> >          [% END %]
> >        [% END %]

> Why use currentloop here?  Couldn't you just check for i<500?

currentloop is reset every time you go through the loop.  The loop is designed
to break the bigger loop into smaller loops of 500 each, since the template
system places an upper limit of 999 iterations per loop and barfs if you go
over that.


mozilla/webtools/bugzilla/attachment.cgi
> >+  $iter->target->target(new DiffPrinter::raw($fh));
> target->target?

Yep.  In traditional Perl style, target is both a getter and a setter. 
->target->target->target->target(...) works too.


mozilla/webtools/bugzilla/CVSClient.pm
> > ^(([^\@:]*)(:([^\@]*))?\@)?([^:]*)(:(.*))?$
> This regexp needs to take the possibility of a colon after the server or port 
into account.

How could there be one?  We got rid of it in the first regexp.	This is
complicated stuff, maybe I'm missing something :)


mozilla/webtools/bugzilla/PatchIterator/RawPatchIterator.pm
> Nit: use 4-space indent

Yeah, that's planned but painful.  I may or may not do that upon the first
packaging but I'll do it.

>    if ($3 eq 'a') {
>      # 'a' has the old number one off from diff -u ("insert after this line")
>      # vs. "insert at this line")
>    if ($3 eq 'd') {
>      # 'd' has the new number one off from diff -u ("delete after this line")
>      # vs. "delete at this line")
> What about "c"?  Also note extra closing parentheses in these comments.

IIRC, 'c' was not messed up.

> >  } elsif ($line =~ /^-($|[^-])/) {
> Why not just /^-[^-]*$/ ?  Seems equivalent, but easier to parse.  Oh well, I 
> guess it doesn't matter; it does the same thing.

That wouldn't work because the line *can* contain dashes in it, just not the
first thing on the line.  It makes me wonder a bit whether this can actually
happen actually :)  I need to think about that.


mozilla/webtools/bugzilla/PatchIterator/AddCVSContextIterator.pm
> How do you determine what functions get an underscore prefix?

_start_section is sort of a mnemonic for start_section_internal--the external
and internal version of a function.

> >sub start_file {
> >  $this->{SECTION_END} = -1;
> I don't see where SECTION_END gets used between the invocation of start_file
> and that of _start_section.  Am I missing something?

I just like initializing my variables :)

Comment 36

[Daniel Berlin]

1.8 still has the cvsroot parsing bug i pointed out in an earlier comment.

Comment 37

[John Keiser (jkeiser)]

Attachment: Patch v1.8.1

Och, somehow I missed that.  It's fixed now, thanks :)

Comment 38

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 122880 [details] [diff] [review]
Patch v1.8.1

>This is not particularly easy (it involves dynamic HTML changes--possibly we
>could use display: none) and is not a major problem IMO; filed in bug 195305,
>let me know if this is a requirement.

Well, I guess this is fine as a nit. :-)


>I also did not wrap HTML template lines, which generally are not wrapped in
>Bugzilla. I plead module style :)

We're trying to change that. Please assist us. :-) Don't shorten lines where it
makes the text less readable or unnecessarily complicated, however.


>There's not much I can do about it now, and given the way it outputs lines
>(with table cells, next to each other, the output would suck) it would be
>difficult at best to make the copy do much useful. How does Bonsai work?

Bonsai is much simpler, of course, but one can copy lines from it just fine.
This is ok as a nit, but we should get a bug on file about it.


>> * modules should be in the bugzilla/ directory and have license headers if 
>> they are to be checked in to our CVS repository;
>
>Everything was diffed relative to bugzilla/ ... what do you need here?

Sorry, I wasn't clear. New modules are going into the "Bugzilla" subdirectory
of the Bugzilla root directory (which is generally called "bugzilla"). If
CVSClient.pm ends up as a Bugzilla-specific module, it should also go into that
subdirectory and be referenced in code as Bugzilla::CVSClient.
Speaking of CVSClient.pm, I noticed that you left it in the patch, while the
other modules all went into a tarball. It looks pretty generic to me, so I
would think it another candidate for CPAN and not Bugzilla, no?


>> * "target" should be renamed to something like "next_iterator".
>
>I remember us talking about this; next_iterator() doesn't convey flow while
>target does, maybe sends_data_to() would be better?

Yeah, that seems better.


>mozilla/webtools/bugzilla/CVSClient.pm
>> > ^(([^\@:]*)(:([^\@]*))?\@)?([^:]*)(:(.*))?$
>> This regexp needs to take the possibility of a colon after the server or port
>> into account.
>
>How could there be one? We got rid of it in the first regexp. This is
>complicated stuff, maybe I'm missing something :)

Sorry, my mistake, and I seemed to have confused you in the process. You
originally had this in CVSClient.pm:

# Format: :method:[user[:password]@]server[:port]/path

Now you have this:

# Format: :method:[user[:password]@]server[:port]:/path

But your original version is actually correct, pretty much, except that the
colon before the port can appear without the port, but not vice versa. I think
your regexp works right, however, so the only thing you need to modify is the
comment.


Other than that the code looks good. Here are my UI nits:

1. In diff mode you say f.e.: Patch v1.8.1 (Attachment #122880 [details] [diff]) for Bug #174942

While in interdiff mode you omit the word "attachment": Diff Between Patch
v1.7.1 (#115698) and Patch v1.8.1 (#122880) for Bug #174942

I suggest omitting the word "attachment" in both cases for consistency.

2. You link all of "Bug #nnn" but only the number of each attachment.  I
recommend linking the attachment name as well as its number, since the name is
more memorable and more clearly indicative of what the link provides.

3. "interdiff" mode has only three navigational links ("Raw Unified", "Collapse
All", and "Expand All"), but they still take up two lines at the top of the
screen.  The latter two links should be shifted up next to "Raw Unified" so
that they don't take up precious vertical screen real estate unnecessarily.

By the way, I love the dashed vertical border between the old/new version.  It
really helps separate the two sides from each other and makes the whole page
look much cleaner.

Unfortunately testing revealed a problem with interdiff.  After applying the
latest patch to a b.m.o copy and interdiffing it with the patch I reviewed (the
second 1.7.0), I noticed that sections of added/removed code were getting
displayed under the wrong file header.	I'll attach an example of this for your
perusal.

Also note that because of a recent check-in aimed at improving Bugzilla's
security story, you need to run tests on this patch and then deal with any
potentially insecure unescaped template variables the tests find (by either
escaping them with a filter or adding them to
template/en/default/filterexceptions.pl).  I did a run for you, and these are
the juicy bits (includes tabs warnings as well):

t/005no_tabs.......1..190
not ok 13 - defparams.pl contains tabs --WARNING
not ok 164 - template/en/default/attachment/list.html.tmpl contains tabs
--WARNING


t/008filter........1..117
not ok 94 - (en/default) template/en/default/attachment/diff-file.html.tmpl has
unfiltered directives:
#   22:collapsed ? '(+)' : '(-)'
#   22:collapsed ? '' : ' checked'
#   24:lxr_prefix
#   30:file.minus_lines
#   30:file.plus_lines
#   32:file.plus_lines
#   36:file.minus_lines
#   39:collapsed ? 'file_collapse' : 'file'
#   50:bonsai_prefix
#   56:bonsai_prefix
#   56:section.old_start
#   59:section.old_start
#   59:section.old_start + section.old_lines - 1
#   61:section.old_start
#   67:section_num
#   67:section_num
#   78:WHILE (i < group.plus.size || i < group.minus.size)
#   80:WHILE currentloop < 500 && (i < group.plus.size || i < group.minus.size)
# --ERROR
ok 95 - (en/default) attachment/diff-footer.html.tmpl is filter-safe
not ok 96 - (en/default) template/en/default/attachment/diff-header.html.tmpl
has unfiltered directives:
#   24:attachid
#   24:bugid
#   142:attachid
#   142:oldid
#   142:oldid
#   142:newid
#   142:newid
#   142:bugid
#   142:bugid
#   149:style
#   153:javascript
#   165:url()
#   166:url(action = 'edit')
#   168:url(format = 'raw')
#   172:patch.id
#   174:attachid
#   180:url(format = 'raw')
#   194:url(context = '')
#   199:url(context = 'file')
#   206:attachid
# --ERROR

Comment 39

[Myk Melez [:myk] [@mykmelez]]

Attachment: testcase showing interdiff problem

Take a look at f.e. globals.pl (the last file in this diff), although earlier
files also exhibit the faulty behavior.

Comment 40

[John Keiser (jkeiser)]

Attachment: Patch v1.9

Everything fixed, except:
> Bonsai is much simpler, of course, but one can copy lines from it just fine.
> This is ok as a nit, but we should get a bug on file about it.

Bug filed.

> Speaking of CVSClient.pm, I noticed that you left it in the patch, while the
> other modules all went into a tarball.

I placed it into PatchIterator for now.  It's not really big enough to warrant
its own module.

> >I remember us talking about this; next_iterator() doesn't convey flow while
> >target does, maybe sends_data_to() would be better?
> Yeah, that seems better.

Changed.

> Unfortunately testing revealed a problem with interdiff...

This is a problem with interdiff.  I have filed a potential solution as bug
210407.  As discussed, we'll probably just leave this off for the moment.

I *have*, however, added a warning into Bugzilla designed to detect conditions
where interdiff may not be happy.  It just spits out a warning in that case,
since it may show up OK too, depending on circumstances.  There is no way to
know without context.

> Also note that because of a recent check-in aimed at improving Bugzilla's
> security story, you need to run tests on this patch and then deal with any
> potentially insecure unescaped template variables the tests find (by either
> escaping them with a filter or adding them to
> template/en/default/filterexceptions.pl).

I have fixed these in this patch, except for the warnings that have to do with
misunderstanding url(), not understanding what WHILE is, not knowing ? :, and
not knowing that + and - will always produce a number, which is safe.  Not sure
what to do about these.

Comment 41

[John Keiser (jkeiser)]

Attachment: PatchIterator classes v1.9

This version of the PatchIterator classes includes PatchIterator::CVSClient and
changes target to sends_data_to().

Comment 42

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 126321 [details] [diff] [review]
Patch v1.9

+  # Reads in the patch, converting to unified diff in a temp file
+  my $iter = new PatchIterator::Raw;
+  # fixes patch root (makes canonical if possible)
+  $iter->sends_data_to(new PatchIterator::FixPatchRoot(Param('cvsroot')));
+  # Grabs the patch file info
+  my $patch_info_grabber = new PatchIterator::PatchInfoGrabber();
+  $iter->sends_data_to->sends_data_to($patch_info_grabber);
+  # Prints out to temporary file
+  my ($fh, $filename) = File::Temp::tempfile();
+  $patch_info_grabber->sends_data_to(new
PatchIterator::DiffPrinter::raw($fh));
+  # Iterate!
+  $iter->iterate_string($id, $thedata);

Nit: I know this works because you don't need a variable containing the new
FixPatchRoot object, but I still think it's hard to read and inconsistent with
what you do elsewhere.	I'd suggest:

my $fix_patch_root = new PatchIterator::FixPatchRoot(Param('cvsroot'))
$iter->sends_data_to($fix_patch_root);
# Grabs the patch file info
my $patch_info_grabber = new PatchIterator::PatchInfoGrabber();
$fix_patch_root->sends_data_to->($patch_info_grabber);


+sub warn_if_interdiff_might_fail {
+  my ($old_file_list, $new_file_list) = @_;
+  # Verify that the list of files diffed is the same
+  my @old_files = sort keys %{$old_file_list};
+  my @new_files = sort keys %{$new_file_list};
+  if (@old_files != @new_files ||
+      join(' ', @old_files) ne join(' ', @new_files)) {
+    return "This interdiff may show things in the wrong places due to there
being a different set of files in the two patches";
+  }
+
+  # Verify that the revisions in the files are the same
+  foreach my $file (keys %{$old_file_list}) {
+    if ($old_file_list->{$file}{old_revision} ne
+	 $new_file_list->{$file}{old_revision}) {
+      return "This interdiff could be incorrect due to the fact that the old
patch was made against a different revision than the new one."
+    }
+  }

For localizability, these strings should be codes that cause the templates to
spit out the strings ala global/user-error.html.tmpl.

Nit: the first word after a colon should not be capitalized, and since you
previously say "differences between [patch foo] and this patch" and don't
mention interdiff, the former terminology would be better here.
Nit: I don't think it's fair to blame the files when the limitation is in
interdiff.  I recommend this instead:

Warning: this difference between two patches may be inaccurate due to a
limitation in Bugzilla when comparing patches with different sets of files.
Warning: this difference between two patches may be inaccurate due to a
limitation in Bugzilla when comparing patches made against different revisions.

Nit: indent <td> two spaces from the indentation of its enclosing <tr>.

Nit: no space between "Raw Unified" and succeeding "|".

Nit: The "differences between" menu should default to the previous patch
chronologically.

Nit: all links are separated by "|" except "Collapse All" and "Expand All";
they should be consistent.

Fix the localizability issue (and optionally the nits) and you'll see r=myk,
with no second review required, but note that I'll be gone for 2.5 weeks and
not available to review for at least the first 1.5 of them.

Comment 43

[John Keiser (jkeiser)]

Attachment: Patch v1.9.1

All nits and issues fixed.

Comment 44

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 127278 [details] [diff] [review]
Patch v1.9.1

This looks good except for the misspelt occurrences of "Bugzill" in
defparams.pl and the fact that it fails to apply to the tip.  Fix that, and
r=myk.

Comment 45

[John Keiser (jkeiser)]

Attachment: Patch v1.9.2

Updated to trunk; fixed Bugzill -> Bugzilla

Comment 46

[John Keiser (jkeiser)]

Attachment: Patch v1.9.3

And this one passes the tests (I had conflict errors in test stuff I didn't
resolve properly).

Comment 47

[John Keiser (jkeiser)]

Attachment: Patch v1.9.4

This patch includes all the files.

Comment 48

[John Keiser (jkeiser)]

Attachment: Patch v1.9.5

Un-second-guess the changes in 008filter.t.  They were fine as before.

Comment 49

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 128875 [details] [diff] [review]
Patch v1.9.5

Looks good, works, tests are fine.  r=myk

Comment 50

[Myk Melez [:myk] [@mykmelez]]

Approved for check-in.

Comment 51

[John Keiser (jkeiser)]

Fix checked in.  File problems as new bugs please.

Comment 52

[John Keiser (jkeiser)]

Comment on attachment 126322 [details]
PatchIterator classes v1.9

PatchIterator is no longer the name for the class.  It is now found at
http://www.cpan.org/authors/id/J/JK/JKEISER/PatchReader-0.9.tar.gz