Skip to content

Improve revert lines #329

Merged
merged 2 commits into from Jul 26, 2014

2 participants

@living180

The process used for generating reverse diffs for operations such as "Revert Selected Lines..." sometimes resulted in the wrong lines being reverted. The process worked as follows:

  • call out to git to generate the full forward and reverse diffs for the file
  • find the text from the GUI selection in the full forward diff
  • use the information about the location of the GUI selection in the forward diff to determine which lines to use from the reverse diff
  • generate the partial reverse diff from those lines

The problem with this is that if there are adjacent adds and removes, the order of lines in the forward diff will likely not be the same as in the reverse diff. As a result, the wrong lines can be selected from the reverse diff.

To address this issue, change the code to no longer use git to get the full reverse diff. Instead, transform the full forward diff to a reverse diff algorithmically.

living180 added some commits Jul 14, 2014
@living180 living180 widgets.diff: remove unused parameter
The reverse parameter for DiffEditor.process_diff_selection() was
unused, so remove it.

Signed-off-by: Daniel Harding <dharding@living180.net>
69c3e83
@living180 living180 diffparse: improve generation of reverse diffs
The process used for generating reverse diffs for operations such as
"Revert Selected Lines..." sometimes resulted in the wrong lines being
reverted.  The process worked as follows:

* call out to git to generate the full forward and reverse diffs for the
  file
* find the text from the GUI selection in the full forward diff
* use the information about the location of the GUI selection in the
  forward diff to determine which lines to use from the reverse diff
* generate the partial reverse diff from those lines

The problem with this is that if there are adjacent adds and removes,
the order of lines in the forward diff will likely not be the same as in
the reverse diff.  As a result, the wrong lines can be selected from the
reverse diff.

To address this issue, change the code to no longer use git to get the
full reverse diff.  Instead, transform the full forward diff to a
reverse diff algorithmically.

Signed-off-by: Daniel Harding <dharding@living180.net>
2473f60
@davvid
git-cola member
davvid commented Jul 23, 2014

This is very cool! I have always been a little nervous about how the revert stuff worked and this seems like a more sensible approach. I'm going to release v2.0.5 VerySoonNow ™️ and then I'll merge this in as soon as it's out there.

I remember needing the separate cached and reverse flags to support the three different modes -- staging, unstaging, and reverting. I like that this was able to simplify that (assuming you didn't forget to test all three).

I'll test it a bit more myself as well to make sure we didn't miss anything, but it looks good to merge post-v2.0.5.

The one last little problem is this part (and it's okay that it's still a problem since this MR is definitely an improvement):

  • find the text from the GUI selection in the full forward diff
  • use the information about the location of the GUI selection in the forward diff to determine which lines to use from the reverse diff

Finding the text from the GUI selection in the full forward diff is actually ambiguous. In practice it's not a problem because usually the selection is unique, but in theory it can get confused since the selection might find an earlier (incorrect) match. I don't know of a way to go from the Qt selection to the lines in the text, which is why it's done that way, but if you happen to know a way then that would eliminate the one last ambiguous/dangerous part.

I'm definitely going to merge this as-is, just mentioning it in case you happen to know how to do it.

@living180

I remember needing the separate cached and reverse flags to support the three different modes -- staging, unstaging, and reverting. I like that this was able to simplify that (assuming you didn't forget to test all three).

As far as I can tell, the cached and reverse flags are still used in the same way. The cached flag is still passed to the diff_source.get() call. I forced reverse to True if cached is True to avoid having to use cached or reverse in multiple places, but as far as I know, this didn't affect the underlying logic. I've been using these changes pretty heavily in my own development, and all three modes definitely still work.

The one last little problem is this part (and it's okay that it's still a problem since this MR is definitely an improvement):

find the text from the GUI selection in the full forward diff
use the information about the location of the GUI selection in the forward diff to determine which lines to use from the reverse diff

Finding the text from the GUI selection in the full forward diff is actually ambiguous. In practice it's not a problem because usually the selection is unique, but in theory it can get confused since the selection might find an earlier (incorrect) match. I don't know of a way to go from the Qt selection to the lines in the text, which is why it's done that way, but if you happen to know a way then that would eliminate the one last ambiguous/dangerous part.

Yes, this is something I've been looking at as well. I've started prototyping some changes, but paused on those until #330 got hashed out. Anyway, the approach I'm looking at is this:

  • don't call out to git anymore to get the diff, but get it from the model instead
  • get the text from the DiffEditor, split it into lines, and use the selection offsets (or cursor position if no selection) to determine the selected line(s)
  • make the DiffParser.process_diff_selection() take a start and end line number, plus a flag indicating hunk mode or line mode, instead of an offset and selection
  • from there, make DiffParser split its diff (from the model) into lines, and build the patch file using the line number information passed to process_diff_selection()

If you see any obvious problems with this approach, let me know. Otherwise I'll keep going down this route and see how things turn out. It may take me a bit to get something fully ready, depending on how busy my day job keeps me, but I'm happy to keep working on it as I have time.

@davvid
git-cola member
davvid commented Jul 26, 2014

v2.0.5 was just released, merging this now :-)

@davvid davvid merged commit 692556d into git-cola:master Jul 26, 2014
@davvid
git-cola member
davvid commented Jul 27, 2014

I was talking with a friend about the "finding the selection within the text" problem, and we came up with what may be a better heuristic/algorithm then we are currently using.

Right now we only search from the start of the text for the matching position within the diff. The proposal is to do it as follows:

  • (I believe) Qt lets us query for the cursor position, which is an absolute position within the full text
  • Use this cursor position as a pivot point.
  • Take the length of the selection and grab text of that length before the cursor position. Check whether it matches, if it does, we're done. This is the common case where someone makes a selection by dragging downwards.
  • Take the length of the selection and grab text of that length after the cursor position. If it matches, we're done. This handles the case where someone drags upwards.
  • If we've still not found a match, then do what we currently do and search from the beginning as a fallback. I don't think this code will ever get activated, but it's a good fallback.

That seems like a stronger and more robust way to do it. When checking for matches we'll probably need to do the same CRLF translation that we currently do. See cola/diffparse.py around line 283 for how we currently deal with that.

@living180 living180 deleted the living180:improve_revert_lines branch Jul 28, 2014
@living180 living180 referenced this pull request Dec 22, 2014
Merged

Rewrite DiffParser #399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.