Regression: erroneous change indicator in margin
Note: I am confident in this upstream filing, as the issue was observed on Ubuntu 22 and MacOS Sonoma, which represent both completely different renderers (x.org vs Aqua) and completely different package managers (Ubuntu APT repository vs homebrew).
The tl,dr: the margin indicator, between the line number column and the first column of text, does not reliably indicate the changed status of the line in question; this is a change from some previous versions where this indication was reliable, and I have ascertained this was not intended.
How can it be reproduced?
While this is also an issue with 3-way comparison, this can be reproduced as simply as performing a 2-way comparison (either with the GUI or with kdiff3 ~/Bibliothek/originalfassung ~/Bibliothek/Dietrich) of the attached files (originalfassung and Dietrich).
What behavior was expected?
It was expected to see the respective change indication colors for most lines in the margin (between the line number column and the first column of text), given the differences in the text, except maybe for the empty lines or the last line in each verse.
What behavior was observed, then?
The observed behavior is way fewer lines than expected having the respective change indication color in that margin, as seen in the earlier attached image. This does not appear to affect navigation between deltas: the software does acknowledge the lines in question as having changed. This does not affect change-related highlighting of the text proper: changed words or subwords are emphasized as such. This does not affect the file-wide summary next to the vertical scroller. This is independent of the display technology: similar behavior has been observed un Ubuntu 22 and MacOS Sonoma, so this is extremely unlikely to be either a bug in the platform-specific rendering code, or even a platform-dependent glitch.
Did it ever work as expected?
Yes: while I am unable to tell exactly when the cutoff occurred, I do remember kdiff3, including when obtained through the same channels, behaving properly at some point in the past.
How impactful is it?
We're talking about an issue that can make the user miss changed lines in comparisons; I wouldn't minimize it. For instance, overlong lines could cause actual changes to fail to appear in the viewport, meaning that without the margin indication either they do not appear at all without horizontal scrolling. As I'm going to explain, this is particularly impactful for semicolon-based languages, but other programming laguages are affected as well, even if to a lesser extent.
What did you find?
After noticing the issue, and failing to find any indication (either in the release notes or the issue tracker) that this was intended, I went to the code and (long story short) found out that as currently as current master (revision e852211a), in writeLine() in difftextwindow.cpp, the variable penColor is used both in a for loop and after it, such that these uses reflect the situation as of the last iteration of the loop (assuming there was at least one). I deduced this to be almost certainly in error, since this results in the margin reflecting the situation as of the last drawn character in the line, and went to figure out how this started.
After using a variety of investigation tools, I ended up stumbling on revision 5d55f0be which introduced the issue: the variable, back then called c, used to be shadowed by a new instance of c prior to this patch, such that it was unaffected by per-iteration code, resulting in the expected behavior. The patch introduced the issue by mistakenly merging the two variables as part of resolving -wshadow warnings; but despite the identical names, it was never the intent for these variables to be merged.
At the time of that revision, version 1.7.90 was the most recent in ChangeLog; no further 1.7.x version appears to have been released, and as a result, this regression first affected 1.8.0, followed by any later version, since it is still in master (revision e852211a) as of this writing.
To confirm the analysis, I have created the attached patch (which I am hereby releasing to the public domain), which I have tested as fixing the issue, as illustrated by the later attached image. It fixes the issue by restoring a QColor variable having loop iteration scope, while avoiding the variable shadowing issue by renaming the function-scoped variable (change which is more fail-safe than renaming the loop-iteration-scoped one). Note that while the patch was made over branch 1.10 (revision a82b8615), this is only because this was the latest branch which I could build on Ubuntu 22: this issue probably deserves an even earlier backport.
