Fossil UI diff is "longer than necessary"
(1) By Florian Balmer (florian.balmer) on 2023-11-13 17:35:55 [source]
I've come across a Fossil UI diff that seems "longer than necessary" compared to
output of the diff
utility or the fossil xdiff
CLI command:
C:\...>.\fossil.exe version
This is fossil version 2.23 [47362306a7] 2023-11-01 18:56:47 UTC
C:\...>type v1 v2
v1
{
"profile": {
"add_person_enabled": false,
"browser_guest_enabled": false
}
}
v2
{
"profile": {
"add_person_enabled": false,
"browser_guest_enabled": false
},
"settings": {
"a11y": {
"overscroll_history_navigation": false
}
}
}
C:\...>diff -u5 v1 v2
--- v1 Mon Nov 13 18:07:23 2023
+++ v2 Mon Nov 13 18:07:23 2023
@@ -1,6 +1,11 @@
{
"profile": {
"add_person_enabled": false,
"browser_guest_enabled": false
+ },
+ "settings": {
+ "a11y": {
+ "overscroll_history_navigation": false
+ }
}
}
C:\...>fossil xdiff v1 v2
--- v1
+++ v2
@@ -1,6 +1,11 @@
{
"profile": {
"add_person_enabled": false,
"browser_guest_enabled": false
+ },
+ "settings": {
+ "a11y": {
+ "overscroll_history_navigation": false
+ }
}
}
C:\...>fossil xdiff --browser --side-by-side v1 v2
C:\...>fossil xdiff --browser --side-by-side -w v1 v2
C:\...>fossil xdiff --browser v1 v2
C:\...>fossil xdiff --browser -w v1 v2
Screenshots of the Fossil UI diffs, in the same order as typed above.
Shouldn't the UI diff be as compact as the CLI diff?
(2) By Preben Guldberg (preben) on 2023-11-13 21:41:24 in reply to 1 [link] [source]
Any chance v1 does not have a final newline, while v2 does?
At least I believe I see the results you do with that combination (though my macOS diff complains about a missing newline, which fossil does not).
(3) By Florian Balmer (florian.balmer) on 2023-11-14 07:06:44 in reply to 2 [link] [source]
I just re-checked, and both v1 and v2 have exactly one final LF. (I learned this a long time ago, when I tried to implement a wiki engine, I made sure every page text ends with a single line ending to get better diffs.)
The Fossil UI diffs I see are correct, I'm just surprised they are longer than the CLI diffs.
(4) By Preben Guldberg (preben) on 2023-11-14 13:26:55 in reply to 3 [link] [source]
Thanks for checking.
Looking closer, I think you uncovered a corner case involving the number of context lines and the number of unchanged lines at the end of the files.
For the regular fossil xdiff
the diff is generated by contextDiff(), which behaves
differently from formatDiff() used by the other formats.
For formatDiff(), each set of COPY/DELETE/INSERT triples to process is processed in order. Rough outline is:
- Determine the maximum number of triples to handle (
mxr
). This seems based on the number of edits, except for the trailing triples that only COPY lines. - Work through all the triples in a for loop (indexed by
r
). - When entering the loop, check how many triples to show In a block, taking the number of context lines into account.
- Generate the block, involving the
nr
triples computed in step 3..
The start of the loops is:
mxr = p->nEdit;
while( mxr>2 && R[mxr-1]==0 && R[mxr-2]==0 ){ mxr -= 3; }
for(r=0; r<mxr; r += 3*nr){
/* Figure out how many triples to show in a single block */
for(nr=1; R[r+nr*3]>0 && R[r+nr*3]<(int)nContext*2; nr++){}
Unfortunately, when computing nr
, we may consider more triples than mxr
.
In my tests, this affects the conditional guarding a call to smallGap() later, where
we merge blocks. (FWIW, if I ifdef that code block out, the problem does not appear.)
Mind testing if the following diff works for you as well?
Index: src/diff.c
==================================================================
--- src/diff.c
+++ src/diff.c
@@ -2223,11 +2223,11 @@
mxr = p->nEdit;
while( mxr>2 && R[mxr-1]==0 && R[mxr-2]==0 ){ mxr -= 3; }
for(r=0; r<mxr; r += 3*nr){
/* Figure out how many triples to show in a single block */
- for(nr=1; R[r+nr*3]>0 && R[r+nr*3]<(int)nContext*2; nr++){}
+ for(nr=1; 3*nr<mxr && R[r+nr*3]>0 && R[r+nr*3]<(int)nContext*2; nr++){}
/* If there is a regex, skip this block (generate no diff output)
** if the regex matches or does not match both insert and delete.
** Only display the block if one side matches but the other side does
** not.
(5) By Florian Balmer (florian.balmer) on 2023-11-14 17:57:34 in reply to 4 [link] [source]
Thanks for investigating! Your patch works well for me.
I think @drh will have to decide whether this should be applied (as well
as your changes to fossil diff
). Maybe if you commit this to a branch,
it might make it into a live field trial version on fossil-scm.org
and/or sqlite.org, now that the next release has just started.
(6) By Preben Guldberg (preben) on 2023-11-14 19:48:13 in reply to 5 [link] [source]
Glad to hear it worked for you too.
I've committed a long-diff-fix branch with the fix above, a similar issue in another function and a test case.
(7) By Florian Balmer (florian.balmer) on 2023-11-14 20:21:31 in reply to 6 [link] [source]
Nice! I've now updated my webserver and local versions of Fossil to inlcude your patches, so I'll report back any oddities I may see in the next time.
(8) By Florian Balmer (florian.balmer) on 2023-12-07 19:16:54 in reply to 7 [link] [source]
I've now been using the patches created here for about 3 weeks, and I'm happy about them, so I'd like to suggest them for trunk.
As already mentioned in another post a few moments ago, I'd love to see the many open branches land on trunk soon, so we can test and play around with the new features in daily use.