[BUG] De-activating 'Ignore All Whitespace Changes' will not always correctly refresh Diff!

[goran-w]

Issue 1:

There are currently cases where de-activating the 'Ignore All Whitespace Changes' (toggle-button) option doesn't refresh the Diff view correctly, but if we thereafter switch the selection away from the changed file and then back to it again it DOES refresh (with different contents).

In these cases, after de-activating the option, the Diff view still says "NO CHANGES OR ONLY EOL CHANGES" while the correct behavior would be to show an actual (whitespace-only) Diff. (Activating the option does NOT have any similar refresh-issues.)

This seems to happen when the number of lines in the Diff is the same, regardless of the ignore-whitespace option.

I can easily reproduce this issue when either (or both) of these types of changes are the only ones made in the file :

• Add/remove space characters on lines in the file.
• Change line-endings in the file between LF and CR+LF.

However, as soon as I add/remove a line-break, toggling the ignore-whitespace option starts behaving as expected.

Related issue 2:

Adding whitespace within a "word" (splitting the word) and removing whitespace between two "words" (joining the words) should NOT be considered a whitespace-change, since it changes the syntax / semantics of code / text!

In other words, only "whitespace-to-whitespace" changes should be considered "ignoreable"...

To correct this, the --ignore-all-space flag should be replaced by --ignore-space-change!

Related issue 3:

When 'Ignore All Whitespace Changes' is active, if there are both (any number of) whitespace/EOL-only changes and a single 'hunk' of non-whitespace/EOL changes, doing Discard on this hunk will Discard the whole file!

This, to me, feels unexpected - without the ignore-whitespace option this Diff would have displayed one or more other hunks (or a larger hunk) for the whitespace/EOL-changes, and I don't expect these changes to be discarded too (since I'm "presumably" discarding ONLY a specific "semantic" change and not all the other changes that are actually present in the file, even though they are not shown).

(Slightly) Related issue 4:

If the previous version of a file did not have an EOL on its last line, and we add an EOL there, this becomes indicated by a red circular stop-icon in the left-side "before" diff-pane (in side-by-side mode, and only visible when NOT ignoring whitespace).

Apparently, we can't Discard the resulting hunk at the very bottom of the file IF there are other hunks in the same file!

(NOTE: Stage / Unstage can be done on this problematic hunk without issues.)

However, if we first Discard ALL other hunks in the file, THEN we can also Discard this EOL-addition hunk at the very bottom of the file.

Interestingly, in an "inverse" situation where the previous version of a file DID have an EOL on its last line, and we remove that EOL (as indicated by a red circular stop-icon in the right-side "after" diff-pane), then the resulting hunk does NOT have any issue with Discard (even when it's the single hunk in the file).

[goran-w]

@love-linger Thanks, Issue 1 is now fixed.

It should be very easy to fix Issue 2, we just need to change the option-flag used. As an example of this issue, the following "semantically relevant" changes are currently being ignored as whitespace-only changes, which is clearly wrong :

Issue 3 probably needs to be investigated further, to understand why it happens and what is the "correct" behavior...

Finally, here's an example of a case where Issue 4 appears :

[love-linger]

Issue 2 & 4 has been fixed.

[love-linger]

Issue 3 is by design. When there's only one hunk, discarding it will simply restore this file instead of patch it revertly

[goran-w]

Issue 3 is by design. When there's only one hunk, discarding it will simply restore this file instead of patch it revertly

That´s a problem though, since restoring the whole file would also undo any unrelated whitespace-only changes elsewhere in the file (i.e not just the change in that hunk) !

[goran-w]

Issue 2 & 4 has been fixed.

Thanks, however I'm afraid the fix for issue 4 was undone in subsequent commit c76b770408b54d2b01d3c9bab275d7b982cd4f70?

To re-fix, the condition on line 31 should now read: if (Models.DiffOption.IgnoreCRAtEOL || ignoreWhitespace)

[love-linger]

It's not the same thing. Enable --ignore-cr-at-eol in diff option in Preferences controls the --ignore-cr-at-eol option in git diff command. Ignore Whitespace Changes controls --ignore-space-change option in git diff command.

If you have enabled both --ignore-cr-at-eol and --ignore-space-change, the change about the last empty line will not be able to be staged/unstaged/discard on Windows

[goran-w]

It's not the same thing. Enable --ignore-cr-at-eol in diff option in Preferences controls the --ignore-cr-at-eol option in git diff command. Ignore Whitespace Changes controls --ignore-space-change option in git diff command.

If you have enabled both --ignore-cr-at-eol and --ignore-space-change, the change about the last empty line will not be able to be staged/unstaged/discard on Windows

I thought applying --ignore-cr-at-eol was the fix for issue 4 in commit 4fcb12bbf7cff19653dc8748bb74a737e0ac7725 (along with applying --ignore-space-change to fix issue 2) ?

Also, I thought your subsequent commit c76b770408b54d2b01d3c9bab275d7b982cd4f70 was supposed to be pure refactoring but it actually reverts the --ignore-cr-at-eol part of the previous change?

So, does this mean issue 4 is again "un-fixed"? Or is there another fix for issue 4 that I missed?

[love-linger]

I thought applying --ignore-cr-at-eol was the fix for issue 4 in commit https://github.com/sourcegit-scm/sourcegit/commit/4fcb12bbf7cff19653dc8748bb74a737e0ac7725 (along with applying --ignore-space-change to fix issue 2) ?

Issue 4 was fixed by f88c424c10e74707570062b90c602509ee19b2f8 (via adding \\ No newline at end of file for the last line)

[goran-w]

Issue 4 was fixed by f88c424 (via adding \\ No newline at end of file for the last line)

Aha, I see. So adding the --ignore-cr-at-eol flag (for the ignoreWhitespace case) was a mistake, which was reverted in the subsequent commit.

What remains now is issue 3, then:

When there's only one hunk, discarding it will simply restore this file instead of patch it revertly

That is not always the right thing to do, since restoring the whole file could give a different result than patching the last hunk - specifically if there are whitespace-only changes elsewhere in the file and 'Ignore All Whitespace Changes' is active. (Discarding a hunk should not affect unrelated changes in other parts of the file!)

[love-linger]

Issue 3 also be fixed.