Skip to content

Commit

Permalink
Big changes to how diffs are generated
Browse files Browse the repository at this point in the history 
The original issue was that looking at all changes (action=rc all=1) the
resulting diff didn't always make sense if you clicked on the diff link.
It showed the difference between that revision and the current revision.
The PrintHtmlDiff sub was changed significantly to make it easier to
understand and to help fix this issue.

The drawback is that it now requires a new key in page and keep files:
lastmajorsummary. It goes with lastmajor and diff-major and records the
summary for that particular edit. As new changes will start recording
this new key, the change will slowly propagate in existing wikis.
Whenever you look at minor diffs, however, the existing summary key is
chosen. Plus, whenever you want to look at differences between
particular revisions, this is equivalent to looking at minor diffs. So
the only situation that is problematic is an edit history like the
following:

A - major change
B - major change (major diff, major summary, last major revision)
C - minor change

When looking at this page with diff=2, we want to show major diff, major
summary, last major revision. If B happened before this commit was
installed, the summary will be missing.
  • Loading branch information
@kensanata
kensanata committed Sep 6, 2015
1 parent de6a3f1 commit e25a621
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 72 deletions.
4 changes: 2 additions & 2 deletions t/major.t
View file
Expand Up @@ -39,14 +39,14 @@ test_page(get_page('action=browse id=bla diff=1'), 'No diff available', 'one', '
test_page(get_page('action=browse id=bla diff=2'), 'No diff available', 'one', 'Last edit');
update_page('bla', 'two', '', 1); # lastmajor is 1
test_page(get_page('action=browse id=bla diff=1'), 'No diff available', 'two', 'Last major edit',
'diff=1;id=bla;diffrevision=1');
'diff=2;id=bla;diffrevision=1');
test_page(get_page('action=browse id=bla diff=2'), 'one', 'two', 'Last edit');
update_page('bla', 'three'); # lastmajor is 3
test_page(get_page('action=browse id=bla diff=1'), 'two', 'three', 'Last edit');
test_page(get_page('action=browse id=bla diff=2'), 'two', 'three', 'Last edit');
update_page('bla', 'four', '', 1); # lastmajor is 3
test_page(get_page('action=browse id=bla diff=1'), 'two', 'three', 'Last major edit',
'diff=1;id=bla;diffrevision=3');
'diff=2;id=bla;diffrevision=3');
test_page(get_page('action=browse id=bla diff=2'), 'three', 'four', 'Last edit');
update_page('bla', 'five'); # lastmajor is 5
test_page(get_page('action=browse id=bla diff=1'), 'four', 'five', 'Last edit');
Expand Down
47 changes: 26 additions & 21 deletions t/revisions.t
View file
@@ -1,24 +1,20 @@
# Copyright (C) 2006 Alex Schroeder <alex@emacswiki.org>
# Copyright (C) 2006–2015 Alex Schroeder <alex@gnu.org>
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2 of the License, or
# (at your option) any later version.
# This program is free software; you can redistribute it and/or modify it under
# the terms of the GNU General Public License as published by the Free Software
# Foundation; either version 3 of the License, or (at your option) any later
# version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
# This program is distributed in the hope that it will be useful, but WITHOUT
# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the
# Free Software Foundation, Inc.
# 59 Temple Place, Suite 330
# Boston, MA 02111-1307 USA
# You should have received a copy of the GNU General Public License along with
# this program. If not, see <http://www.gnu.org/licenses/>.

require 't/test.pl';
package OddMuse;
use Test::More tests => 17;
use Test::More tests => 23;

## Test revision and diff stuff

Expand Down Expand Up @@ -56,11 +52,20 @@ my ($ts2) = get_page('action=browse revision=2 id=KeptRevisions') =~ /edited (.*
my ($ts3) = get_page('action=browse revision=3 id=KeptRevisions') =~ /edited (.*?) diff/ix;
ok($ts2 ne $ts3, 'Revision timestamp or author is different');

# Disable cache and request the correct last major diff
test_page(get_page('action=browse diff=1 id=KeptRevisions cache=0'),
'Difference between revision 2 and revision 3',
'second',
'third');
# Request the correct last major diff
xpath_test(get_page('action=browse diff=1 id=KeptRevisions'),
'//div[@class="diff"]/p/b[contains(text(), "Last major edit")]',
'//div[@class="diff"]/p/b/a[contains(text(), "later minor edits")]',
'//div[@class="diff"]/p/b/a[@href="http://localhost/wiki.pl?action=browse;diff=2;id=KeptRevisions;diffrevision=3"]',
'//div[@class="diff"]/div[@class="old"]/p/strong[contains(text(), "second")]',
'//div[@class="diff"]/div[@class="new"]/p/strong[contains(text(), "third")]',
'//div[@class="content browse"]/p[contains(text(), "fifth")]');

# Look at the remaining differences
test_page(get_page('action=browse diff=2 id=KeptRevisions diffrevision=3'),
'Difference between revision 3 and current revision',
'third',
'fifth');

# Show a diff from the history page comparing two specific revisions
test_page(get_page('action=browse diff=1 revision=4 diffrevision=2 id=KeptRevisions'),
Expand All @@ -71,5 +76,5 @@ test_page(get_page('action=browse diff=1 revision=4 diffrevision=2 id=KeptRevisi
# Show no difference
update_page('KeptRevisions', 'second');
test_page(get_page('action=browse diff=1 revision=6 diffrevision=2 id=KeptRevisions'),
'Difference between revision 2 and revision 6',
'Difference between revision 2 and current revision',
'The two revisions are the same');
22 changes: 11 additions & 11 deletions t/rollback.t
View file
@@ -1,4 +1,4 @@
# Copyright (C) 2006–2014 Alex Schroeder <alex@gnu.org>
# Copyright (C) 2006–2015 Alex Schroeder <alex@gnu.org>
#
# This program is free software; you can redistribute it and/or modify it under
# the terms of the GNU General Public License as published by the Free Software
Expand Down Expand Up @@ -105,28 +105,28 @@ my $rc = get_page('action=rc all=1 showedit=1 pwd=foo from=1');
# check all revisions of NicePage in recent changes
xpath_test($rc,
'//li/span[@class="time"]/following-sibling::span[@class="new"][text()="new"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage;revision=1"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="good guy one"]',
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=NicePage;diffrevision=2"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage;revision=2"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="good guy two"]',
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=NicePage;diffrevision=3"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage;revision=3"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="vandal one"]',
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=NicePage;diffrevision=4"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage;revision=4"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="vandal two"]',
# The first link to NicePage has no diffrevision (because
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=NicePage;revision=2"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage;revision=2"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="good guy two"]',
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=NicePage;revision=3"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage;revision=3"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="vandal one"]',
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=NicePage;revision=4"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage;revision=4"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="vandal two"]',
# The first link to NicePage has no revision (because
# it is the latest version) and no rollback link (because
# the timestamp is equal to $LastUpdate)
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=NicePage"][text()="diff"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[contains(text(),"Rollback to")]',
# The second link to NicePage has a diffrevision (because
# The second link to NicePage has a revision (because
# it is from an older version) and a rollback link (because
# the timestamp is smaller than $LastUpdate)
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=NicePage;diffrevision=4"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage;revision=4"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="vandal two"]',
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=NicePage;revision=4"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=NicePage;revision=4"][text()="NicePage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="vandal two"]',
# check that the minor spam is reverted with a minor rollback
'//li/span[@class="time"]/following-sibling::span[@class="new"][text()="new"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=MinorPage;revision=1"][text()="MinorPage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="tester"]',
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=MinorPage;diffrevision=2"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=MinorPage;revision=2"][text()="MinorPage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="testerror"]/following-sibling::em[text()="(minor)"]',
# The first link has no diffrevision (because it is the
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=MinorPage;revision=2"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=MinorPage;revision=2"][text()="MinorPage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="testerror"]/following-sibling::em[text()="(minor)"]',
# The first link has no revision (because it is the
# latest version) and no rollback link (because the
# timestamp is equal to $LastUpdate)
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=MinorPage"][text()="diff"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=MinorPage"][text()="MinorPage"]/following-sibling::span[@class="dash"]/following-sibling::strong[contains(text(),"Rollback to")]/following-sibling::em[text()="(minor)"]',
# The second link has a diffrevision (because it is from an
# The second link has a revision (because it is from an
# older version) and a rollback link (because the timestamp
# is smaller than $LastUpdate)
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=MinorPage;diffrevision=2"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=MinorPage;revision=2"][text()="MinorPage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="testerror"]/following-sibling::em[text()="(minor)"]',
'//li/span[@class="time"]/following-sibling::a[@class="diff"][@href="http://localhost/wiki.pl?action=browse;diff=2;id=MinorPage;revision=2"][text()="diff"]/following-sibling::input[@type="submit"][@value="rollback"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=MinorPage;revision=2"][text()="MinorPage"]/following-sibling::span[@class="dash"]/following-sibling::strong[text()="testerror"]/following-sibling::em[text()="(minor)"]',
# The first page has no rollback link
'//li/span[@class="time"]/following-sibling::span[@class="new"][text()="new"]/following-sibling::a[@class="revision"][@href="http://localhost/wiki.pl?action=browse;id=FirstPage"][text()="FirstPage"]',
# The second page has a rollback link
Expand Down
73 changes: 35 additions & 38 deletions wiki.pl
View file
Expand Up @@ -1415,7 +1415,7 @@ sub BrowsePage {
print GetHeader($id, NormalToFree($id), $oldId, undef, $status);
my $showDiff = GetParam('diff', 0);
if ($UseDiff and $showDiff) {
PrintHtmlDiff($showDiff, GetParam('diffrevision', $revision), $revision, $text, $revisionPage->{summary});
PrintHtmlDiff($showDiff, GetParam('diffrevision'), $revisionPage, $Page{revision});
print $q->hr();
}
PrintPageContent($text, $revision, $comment);
Expand Down Expand Up @@ -1774,9 +1774,9 @@ sub RcHtml {
if ($revision == 1) {
$diff .= '(' . $q->span({-class=>'new'}, T('new')) . ')';
} elsif ($all) {
$diff .= '(' . ScriptLinkDiff(2, $id, T('diff'), '', $all_revision) .')';
$diff .= '(' . ScriptLinkDiff(2, $id, T('diff'), $all_revision) .')';
} else {
$diff .= '(' . ScriptLinkDiff($minor ? 2 : 1, $id, T('diff'), '') . ')';
$diff .= '(' . ScriptLinkDiff($minor ? 2 : 1, $id, T('diff')) . ')';
}
}
$html .= $q->li($q->span({-class=>'time'}, CalcTime($ts)), $diff, $history,
Expand Down Expand Up @@ -2532,55 +2532,51 @@ sub GetGotoBar { # ignore $id parameter
}

sub PrintHtmlDiff {
my ($type, $old, $new, $text, $summary) = @_;
my ($type, $old, $page, $current) = @_;
$page //= \%Page;
$current //= $page->{revision};
$type = 2 if $old or $page->{revision} != $current; # explicit revisions means minor diffs!
my $summary = $page->{$type == 1 ? 'lastmajorsummary' : 'summary'};
my $intro = T('Last edit');
my $diff = GetCacheDiff($type == 1 ? 'major' : 'minor');
# compute old revision if cache is disabled or no cached diff is available
if (not $old and (not $diff or GetParam('cache', $UseCache) < 1)) {
if ($type == 1) {
$old = $Page{lastmajor} - 1;
if (not $new and $Page{lastmajor} != $Page{revision}) {
my $revisionPage;
($revisionPage, $new) = GetTextRevision($Page{lastmajor}, 1);
$text = $revisionPage->{text};
$summary = $revisionPage->{summary};
}
} elsif ($new) {
$old = $new - 1;
} else {
$old = $Page{revision} - 1;
}
}
if ($old > 0) { # generate diff if the computed old revision makes sense
$diff = GetKeptDiff($text, $old);
$intro = Tss('Difference between revision %1 and %2', $old,
$new ? Ts('revision %s', $new) : T('current revision'));
} elsif ($type == 1 and $Page{lastmajor} and $Page{lastmajor} != $Page{revision}) {
$summary = GetKeptRevision($Page{lastmajor})->{summary};
$intro = Ts('Last major edit (%s)', ScriptLinkDiff(1, $OpenPageName, T('later minor edits'),
undef, $Page{lastmajor} || 1));
my $diff;
# use the cached diff if possible
if (not $old or $old == $page->{$type == 1 ? 'lastmajor' : 'revision'} - 1) {
$diff = GetCacheDiff($type == 1 ? 'major' : 'minor', $page);
$old = $page->{$type == 1 ? 'lastmajor' : 'revision'} - 1 if not $old;
}
# if there was no cached diff: compute it, and new intro
if (not $diff and $old > 0) {
($diff, my $keptPage) = GetKeptDiff($page->{text}, $old);
my $to = $page->{revision} != $current ? Ts('revision %s', $page->{revision}) : T('current revision');
$intro = Tss('Difference between revision %1 and %2', $old, $to);
}
# if this is the last major diff and there are minor diffs to look at, and we
# didn't request a particular old revision
if ($type == 1 and $page->{lastmajor} and $page->{lastmajor} != $current) {
$intro = Ts('Last major edit (%s)', ScriptLinkDiff(2, $OpenPageName, T('later minor edits'),
undef, $page->{lastmajor} || 1));
}
$diff =~ s!<p><strong>(.*?)</strong></p>!'<p><strong>' . T($1) . '</strong></p>'!eg;
$diff ||= T('No diff available.');
$summary = $Page{summary} if not $summary and not $new;
$summary = $q->p({-class=>'summary'}, T('Summary:') . ' ' . QuoteHtml($summary)) if $summary;
print $q->div({-class=>'diff'}, $q->p($q->b($intro)), $summary, $diff);
print $q->div({-class=>'diff'}, $q->p($q->b($intro)),
$summary ? $q->p({-class=>'summary'}, T('Summary:') . ' ' . QuoteHtml($summary)) : '',
$diff);
}

sub GetCacheDiff {
my $type = shift;
my $diff = $Page{"diff-$type"};
$diff = $Page{"diff-minor"} if $diff eq '1'; # if major eq minor diff
my ($type, $page) = @_;
my $diff = $page->{"diff-$type"};
$diff = $page->{"diff-minor"} if $diff eq '1'; # if major eq minor diff
return $diff;
}

sub GetKeptDiff {
my ($new, $revision) = @_;
$revision ||= 1;
my ($revisionPage, $rev) = GetTextRevision($revision, 1);
return '' unless $rev;
return T("The two revisions are the same.") if $revisionPage->{text} eq $new;
return GetDiff($revisionPage->{text}, $new, $rev);
return '', $revisionPage unless $rev;
return T("The two revisions are the same."), $revisionPage if $revisionPage->{text} eq $new;
return GetDiff($revisionPage->{text}, $new, $rev), $revisionPage;
}

sub DoDiff { # Actualy call the diff program
Expand Down Expand Up @@ -3736,6 +3732,7 @@ sub Save { # call within lock, with opened page
SaveKeepFile(); # deletes blocks, flags, diff-major, and diff-minor, and sets keep-ts
ExpireKeepFiles();
$Page{lastmajor} = $revision unless $minor;
$Page{lastmajorsummary} = $summary unless $minor;
@Page{qw(ts revision summary username host minor text)} =
($Now, $revision, $summary, $user, $host, $minor, $new);
if ($UseDiff and $UseCache > 1 and $revision > 1 and not $upload and not TextIsFile($old)) {
Expand Down

0 comments on commit e25a621

Please sign in to comment.