New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite DiffParser #399

Merged
merged 2 commits into from Dec 25, 2014

Conversation

Projects
None yet
2 participants
@living180
Contributor

living180 commented Dec 22, 2014

This PR contains a full rewrite of the cola.diffparser module. The primary reason for this is to eliminate the ambiguity noted by @davvid in his comment on PR #329:

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.

Finding the text from the GUI selection in the diff is no longer done, removing any ambiguity. Instead, since the Stage/Unstage/Revert operations all operate on lines as a whole, rework the code to operate in terms of lines. The DiffEditor widget has a new method, selected_lines(), which returns the indexes of the first and last selected lines (or of the line containing the cursor, if there is no selection). The ApplyDiffSelection command class and the DiffParser class were rewritten to operate in terms of these line indexes. To ensure that the correct lines are always included in the generated patch, DiffParser no longer calls out to git to get the diff, but instead uses the diff text from the model (otherwise if the file had changed on disk since git-cola refreshed its model, we can't be sure that the line numbers from the DiffEditor widget would correspond to the line numbers from git diff).

In addition, the rewritten DiffParser class has been made purely functional: its state is set in the initializer, and its methods operate in a purely functional manner on that state. All the imperative functionality (creating temporary files and applying the diff) has been moved into the do() method of the ApplyDiffSelection command class.

living180 added some commits Jul 29, 2014

widgets.diff: simplify DiffEditor.has_selection()
When I added the DiffEditor.has_selection() method, I missed the fact
that QTextCursor has a hasSelection() method.  Simplify
DiffEditor.has_selection() by reimplmenting it in terms of
QTextCursor.hasSelection().

Signed-off-by: Daniel Harding <dharding@living180.net>
diffparse: rewrite with improved accuracy
Previously, the DiffParser.process_diff_selection() method took the
contents of the text selection and searched for it in the output of git
diff to determine which subset of the diff to process.  Most of the time
this worked, but if only a small portion of the diff was selected, such
that the selection appeared multiple times in the diff, the wrong part
of the diff might be processed.  This is especially bad when performing
a Revert operation, because the user can lose data if reverting the
wrong lines.

This commit provides a complete rewrite of the cola.diffparse module
such that the Stage/Unstage/Revert operations should (modulo any bugs)
always process exactly the line(s) selected by the user.

Following is a summary of the changes, starting from the DiffParser
class and working outward:

Simplify the purpose of the DiffParser class.  It no longer knows
anything about models or how to apply changes to the index or the
worktree.  Nor does it call out to git anymore to load any data about
the diff.  The initializer now takes just two parameters:  the filename
(or relative path) of the file being diffed, and the actual text of the
diff (without the diff header).  The initializer uses a new standalone
function _parse_diff() to convert the diff text to a list of _DiffHunk
objects, which are stored on the instance in the hunks attribute.
DiffParser now has just two methods, the first of which is
generate_patch().  It takes the indexes of the first and last lines of
the diff to include in the patch, along with an optional boolean flag
indicating if a reverse patch should be generated.  It returns a string
containing the patch (including the diff header, generated using the
filename passed to the initializer), or None if the resulting patch
would not change anything.  The second method is generate_hunk_patch(),
which is a simple wrapper around the generate_hunk() method.  It takes a
single line index along with a reverse flag, and generates patch using
only the hunk containing the specified line.

Next is the ApplyDiffSelection command class, which is the sole user of
the DiffParser class.  Update the initializer to replace the offset and
selection_text parameters (indicating the start and contents of the
selection, respectively), with three parameters indicating the indexes
of the first and last selected lines (or of the line containing the
cursor if there is no selection), along with a boolean indicating if
there is actually any text selected or not (to differentiate between a
selection on a single line versus no selection at all).  Merge the
functionality of the old DiffParser.process_diff_selection() method into
the do() method.  This method now first constructs a DiffParser
instance, passing it the filename and diff text from the model.  It then
uses the has_selection attribute to call either the generate_patch() or
the generate_hunk_patch() method of the DiffParser instance, depending
on whether or not there is a selection.  Finally, if the resulting patch
is not None, it writes it to a temporary file which is passed to either
the model's apply_diff_to_worktree() or apply_diff() methods, depending
on the operation being performed (Revert vs. Stage/Unstage).  It then
logs the status and updates the model as before.

Lastly, update the DiffEditor widget class.  Add a selected_lines()
method, which returns the indexes of the first and last selected lines
(or of the line containing the cursor if there is no selection).  Update
the process_diff_selection() method to use this new method along with
the has_selection() method to pass the appropriate line indexes and the
has_selection flag when constructing the ApplyDiffSelection command.
Also, to reflect the change to ApplyDiffSelection, replace the staged
parameter with a reverse parameter, and update the callers of the
process_diff_selection() method appropriately.

In addition, update the tests to reflect these changes, and remove the
diff-*-reverse.txt fixtures as they are no longer needed (the code now
converts the forward diff to a reverse diff when necessary).

Signed-off-by: Daniel Harding <dharding@living180.net>
@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Dec 25, 2014

Member

This is definitely one of the more gnarly parts of the code base. Thank you for improving it!

Member

davvid commented Dec 25, 2014

This is definitely one of the more gnarly parts of the code base. Thank you for improving it!

davvid added a commit that referenced this pull request Dec 25, 2014

Merge pull request #399 from living180/diffparser_work
diff: rewrite DiffParser

Signed-off-by: David Aguilar <davvid@gmail.com>

@davvid davvid merged commit 36be309 into git-cola:master Dec 25, 2014

@living180

This comment has been minimized.

Show comment
Hide comment
@living180

living180 Dec 25, 2014

Contributor

Glad to! I've been iterating over this for a number of weeks, so I'm happy I finally got to a point of having something I felt good about pushing up for merging.

Contributor

living180 commented Dec 25, 2014

Glad to! I've been iterating over this for a number of weeks, so I'm happy I finally got to a point of having something I felt good about pushing up for merging.

@living180 living180 deleted the living180:diffparser_work branch Dec 29, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment