Reimplement jumping from diff to file

Implements mostly accurate jumping from diff to file by actually
parsing the hunk and recounting its lines.

For lines which are present on the b side (the view we jump to) this
is accurate of course. For deleted lines ('-') we, mildly arbitrary,
have to choose a good enough line.

core/commands/diff.py

@@ -8,7 +8,6 @@
 from itertools import dropwhile, takewhile
 import os
 import re
-import bisect
 
 import sublime
 from sublime_plugin import WindowCommand, TextCommand, EventListener
@@ -20,7 +19,7 @@
 
 
 if False:
-    from typing import Callable, Iterator, List, Optional, Tuple, TypeVar
+    from typing import Callable, Iterator, List, Optional, Set, Tuple, TypeVar
     from mypy_extensions import TypedDict
 
     T = TypeVar('T')
@@ -589,6 +588,10 @@ def get_hunk_diff(self, pt):
         return header + diff
 
 
+HUNKS_LINES_RE = re.compile(r'@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? ')
+HEADER_TO_FILE_RE = re.compile(r'\+\+\+ b/(.+)$')
+
+
 class GsDiffOpenFileAtHunkCommand(TextCommand, GitCommand):
 
     """
@@ -597,54 +600,167 @@ class GsDiffOpenFileAtHunkCommand(TextCommand, GitCommand):
     """
 
     def run(self, edit):
+        # type: (sublime.Edit) -> None
         # Filter out any cursors that are larger than a single point.
         cursor_pts = tuple(cursor.a for cursor in self.view.sel() if cursor.a == cursor.b)
 
-        diff_starts = tuple(region.a for region in self.view.find_all("^diff"))
-        hunk_starts = tuple(region.a for region in self.view.find_all("^@@"))
-
-        for cursor_pt in cursor_pts:
-            diff_start = diff_starts[bisect.bisect(diff_starts, cursor_pt) - 1]
-            diff_start_line = self.view.substr(self.view.line(diff_start))
-
-            hunk_start = hunk_starts[bisect.bisect(hunk_starts, cursor_pt) - 1]
-            hunk_line_str = self.view.substr(self.view.line(hunk_start))
-            hunk_line, _ = self.view.rowcol(hunk_start)
-            cursor_line, _ = self.view.rowcol(cursor_pt)
-            additional_lines = cursor_line - hunk_line - 1
-
-            # Example: "diff --git a/src/js/main.spec.js b/src/js/main.spec.js" --> "src/js/main.spec.js"
-            use_prepix = re.search(r" b/(.+?)$", diff_start_line)
-            if use_prepix is None:
-                filename = diff_start_line.split(" ")[-1]
-            else:
-                filename = use_prepix.groups()[0]
-
-            # Example: "@@ -9,6 +9,7 @@" --> 9
-            lineno = int(re.search(r"^@@ \-\d+(,-?\d+)? \+(\d+)", hunk_line_str).groups()[1])
-            lineno = lineno + additional_lines
+        def first_per_file(items):
+            # type: (Iterator[Tuple[str, int, int]]) -> Iterator[Tuple[str, int, int]]
+            seen = set()  # type: Set[str]
+            for item in items:
+                filename, _, _ = item
+                if filename not in seen:
+                    seen.add(filename)
+                    yield item
 
-            self.load_file_at_line(filename, lineno)
+        diff = parse_diff_in_view(self.view)
+        jump_positions = filter_(self.jump_position_to_file(diff, pt) for pt in cursor_pts)
+        for jp in first_per_file(jump_positions):
+            self.load_file_at_line(*jp)
 
-    def load_file_at_line(self, filename, lineno):
+    def load_file_at_line(self, filename, row, col):
+        # type: (str, int, int) -> None
         """
         Show file at target commit if `git_savvy.diff_view.target_commit` is non-empty.
         Otherwise, open the file directly.
         """
         target_commit = self.view.settings().get("git_savvy.diff_view.target_commit")
         full_path = os.path.join(self.repo_path, filename)
+        window = self.view.window()
+        if not window:
+            return
+
         if target_commit:
-            self.view.window().run_command("gs_show_file_at_commit", {
+            window.run_command("gs_show_file_at_commit", {
                 "commit_hash": target_commit,
                 "filepath": full_path,
-                "lineno": lineno
+                "lineno": row,
             })
         else:
-            self.view.window().open_file(
-                "{file}:{row}:{col}".format(file=full_path, row=lineno, col=0),
+            window.open_file(
+                "{file}:{row}:{col}".format(file=full_path, row=row, col=col),
                 sublime.ENCODED_POSITION
             )
 
+    def jump_position_to_file(self, diff, pt):
+        # type: (ParsedDiff, int) -> Optional[Tuple[str, int, int]]
+        head_and_hunk_offsets = head_and_hunk_for_pt(diff, pt)
+        if not head_and_hunk_offsets:
+            return None
+
+        header_region, hunk_region = head_and_hunk_offsets
+        header = self.view.substr(sublime.Region(*header_region))
+        hunk = self.view.substr(sublime.Region(*hunk_region))
+        hunk_start, _ = hunk_region
+
+        rowcol = real_rowcol_in_hunk(hunk, relative_rowcol_in_hunk(self.view, hunk_start, pt))
+        if not rowcol:
+            return None
+
+        row, col = rowcol
+
+        filename = extract_filename_from_header(header)
+        if not filename:
+            return None
+
+        return filename, row, col
+
+
+def relative_rowcol_in_hunk(view, hunk_start, pt):
+    # type: (sublime.View, int, int) -> Tuple[int, int]
+    """Return rowcol of given pt relative to hunk start"""
+    head_row, _ = view.rowcol(hunk_start)
+    row_on_pt, col = view.rowcol(pt)
+    # If `col=0` the user is on the meta char (e.g. '+- ') which is not
+    # present in the source. We pin `col` to 1 because the target API
+    # `open_file` expects 1-based row, col offsets.
+    return row_on_pt - head_row, max(col, 1)
+
+
+def real_rowcol_in_hunk(hunk, relative_rowcol):
+    # type: (str, Tuple[int, int]) -> Optional[Tuple[int, int]]
+    """Translate relative to absolute row, col pair"""
+    hunk_lines = split_hunk(hunk)
+    if not hunk_lines:
+        return None
+
+    row_in_hunk, col = relative_rowcol
+
+    # If the user is on the header line ('@@ ..') pretend to be on the
+    # first changed line instead.
+    if row_in_hunk == 0:
+        row_in_hunk = next(
+            index
+            for index, (first_char, _, _) in enumerate(hunk_lines, 1)
+            if first_char in ('+', '-')
+        )
+
+    first_char, line, b = hunk_lines[row_in_hunk - 1]
+
+    # Happy path since the user is on a present line
+    if first_char != '-':
+        return b, col
+
+    # The user is on a deleted line ('-') we cannot jump to. If possible,
+    # select the next guaranteed to be available line
+    for next_first_char, next_line, next_b in hunk_lines[row_in_hunk:]:
+        if next_first_char == '+':
+            return next_b, min(col, len(next_line) + 1)
+        elif next_first_char == ' ':
+            # If we only have a contextual line, choose this or the
+            # previous line, pretty arbitrary, depending on the
+            # indentation.
+            next_lines_indentation = line_indentation(next_line)
+            if next_lines_indentation == line_indentation(line):
+                return next_b, next_lines_indentation + 1
+            else:
+                return max(1, b - 1), 1
+    else:
+        return b, 1
+
+
+def split_hunk(hunk):
+    # type: (str) -> Optional[List[Tuple[str, str, int]]]
+    """Split a hunk into (first char, line content, row) tuples
+
+    Note that rows point to available rows on the b-side.
+    """
+
+    head, *tail = hunk.rstrip().split('\n')
+    match = HUNKS_LINES_RE.search(head)
+    if not match:
+        return None
+
+    b = int(match.group(2))
+    return list(_recount_lines(tail, b))
+
+
+def _recount_lines(lines, b):
+    # type: (List[str], int) -> Iterator[Tuple[str, str, int]]
+
+    # Be aware that we only consider the b-line numbers, and that we
+    # always yield a b value, even for deleted lines.
+    for line in lines:
+        first_char, tail = line[0], line[1:]
+        yield (first_char, tail, b)
+
+        if first_char != '-':
+            b += 1
+
+
+def line_indentation(line):
+    # type: (str) -> int
+    return len(line) - len(line.lstrip())
+
+
+def extract_filename_from_header(header):
+    # type: (str) -> Optional[str]
+    match = HEADER_TO_FILE_RE.search(header)
+    if not match:
+        return None
+
+    return match.group(1)
+
 
 class GsDiffNavigateCommand(GsNavigate):
 

tests/test_diff_view.py

@@ -231,6 +231,103 @@ def test_find_hunk_in_view(self, IN, expected):
         self.assertEqual(actual, expected)
 
 
+class TestDiffViewJumpingToFile(DeferrableTestCase):
+    @classmethod
+    def setUpClass(cls):
+        sublime.run_command("new_window")
+        cls.window = sublime.active_window()
+        s = sublime.load_settings("Preferences.sublime-settings")
+        s.set("close_windows_when_empty", False)
+
+    @classmethod
+    def tearDownClass(self):
+        self.window.run_command('close_window')
+
+    def tearDown(self):
+        unstub()
+
+    @p.expand([
+        (79, ('barz', 16, 1)),
+        (80, ('barz', 16, 1)),
+        (81, ('barz', 16, 2)),
+
+        (85, ('barz', 17, 1)),
+        (86, ('barz', 17, 2)),
+
+        # on a '-' try to select next '+' line
+        (111, ('barz', 20, 1)),  # jump to 'four'
+
+        (209, ('boox', 17, 1)),  # jump to 'thr'
+        (210, ('boox', 17, 2)),
+        (211, ('boox', 17, 3)),
+        (212, ('boox', 17, 4)),
+        (213, ('boox', 17, 1)),
+        (214, ('boox', 17, 1)),
+
+        (223, ('boox', 19, 1)),  # all jump to 'sev'
+        (228, ('boox', 19, 1)),
+        (233, ('boox', 19, 1)),
+
+        (272, ('boox', 25, 5)),
+        (280, ('boox', 25, 5)),
+
+        (319, ('boox', 30, 1)),  # but do not jump if indentation does not match
+
+        # cursor on the hunk info line selects first diff line
+        (58, ('barz', 17, 1)),
+        (89, ('barz', 20, 1)),
+    ])
+    def test_a(self, CURSOR, EXPECTED):
+        VIEW_CONTENT = """\
+prelude
+--
+diff --git a/fooz b/barz
+--- a/fooz
++++ b/barz
+@@ -16,1 +16,1 @@ Hi
+ one
++two
+@@ -20,1 +20,1 @@ Ho
+-three
+ context
++four
+diff --git a/foxx b/boxx
+--- a/foox
++++ b/boox
+@@ -16,1 +16,1 @@ Hello
+ one
+-two
++thr
+ fou
+-fiv
+-six
++sev
+ eig
+@@ -24 +24 @@ Hello
+     one
+-    two
+     thr
+@@ -30 +30 @@ Hello
+     one
+-    two
+ thr
+"""
+        view = self.window.new_file()
+        self.addCleanup(view.close)
+        view.run_command('append', {'characters': VIEW_CONTENT})
+        view.set_scratch(True)
+
+        cmd = module.GsDiffOpenFileAtHunkCommand(view)
+        when(cmd).load_file_at_line(...)
+
+        view.sel().clear()
+        view.sel().add(CURSOR)
+
+        cmd.run({'unused_edit'})
+
+        verify(cmd).load_file_at_line(*EXPECTED)
+
+
 class TestDiffViewHunking(DeferrableTestCase):
     @classmethod
     def setUpClass(cls):