[PATCH b4] review: separate revisions from paths when diffing a commit

[PATCH b4] review: separate revisions from paths when diffing a commit

[Konstantin Khorenko]

Reviewing a series fails with "Could not generate diff" whenever the
work tree contains a file or directory named after the commit being
diffed:

  $ mkdir 1b7771486ae3d9dec8959dbb4da51e535364fb8e
  $ git diff 1b7771486ae3d9dec8959dbb4da51e535364fb8e~1 \
            1b7771486ae3d9dec8959dbb4da51e535364fb8e
  fatal: ambiguous argument '1b7771486ae3d9dec8959dbb4da51e535364fb8e':
  both revision and filename

Any tooling that names its output after a bare SHA (review notes,
logs, build artifacts) can shadow the revision this way. Both
arguments here are always revisions, so terminate them with '--' at
every site that diffs a commit against its parent.

Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
Assisted-by: claude-sonnet-5
---
 src/b4/review/_review.py         | 14 ++++++++++----
 src/b4/review_tui/_review_app.py |  8 +++++---
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/b4/review/_review.py b/src/b4/review/_review.py
index a375332..07a0d51 100644
--- a/src/b4/review/_review.py
+++ b/src/b4/review/_review.py
@@ -1305,7 +1305,9 @@ def reanchor_patch_comments(
             if not comments or not any(c.get('content') for c in comments):
                 continue
             if real_diff is None:
-                ecode, real_diff = b4.git_run_command(topdir, ['diff', f'{sha}~1', sha])
+                ecode, real_diff = b4.git_run_command(
+                    topdir, ['diff', f'{sha}~1', sha, '--']
+                )
                 if ecode != 0:
                     break
             _resolve_comment_positions(real_diff, comments)
@@ -1461,7 +1463,9 @@ def _integrate_agent_reviews(
         # Resolve comment positions against the real diff
         if comments:
             sha = commit_shas[idx]
-            ecode, real_diff = b4.git_run_command(topdir, ['diff', f'{sha}~1', sha])
+            ecode, real_diff = b4.git_run_command(
+                topdir, ['diff', f'{sha}~1', sha, '--']
+            )
             if ecode == 0:
                 _resolve_comment_positions(real_diff, comments)
 
@@ -1855,7 +1859,7 @@ def _integrate_sashiko_reviews(
         # Resolve comment positions against the real diff, then fall back
         # to structured finding locations for any still-unpositioned comments.
         sha = commit_shas[idx]
-        ecode, real_diff = b4.git_run_command(topdir, ['diff', f'{sha}~1', sha])
+        ecode, real_diff = b4.git_run_command(topdir, ['diff', f'{sha}~1', sha, '--'])
         if ecode == 0:
             _resolve_comment_positions(real_diff, comments)
             if locations_by_file:
@@ -1950,7 +1954,9 @@ def _integrate_followup_inline_comments(
 
             # Resolve positions against the real diff
             sha = commit_shas[idx]
-            ecode, real_diff = b4.git_run_command(topdir, ['diff', f'{sha}~1', sha])
+            ecode, real_diff = b4.git_run_command(
+                topdir, ['diff', f'{sha}~1', sha, '--']
+            )
             if ecode == 0:
                 _resolve_comment_positions(real_diff, comments)
 
diff --git a/src/b4/review_tui/_review_app.py b/src/b4/review_tui/_review_app.py
index da60455..26ce90a 100644
--- a/src/b4/review_tui/_review_app.py
+++ b/src/b4/review_tui/_review_app.py
@@ -699,7 +699,9 @@ from b4.review_tui._modals import (
                     viewer.write(Rule(style='dim'))
                     viewer.write(Text(''))
 
-        ecode, diff_out = b4.git_run_command(self._topdir, ['diff', f'{sha}~1', sha])
+        ecode, diff_out = b4.git_run_command(
+            self._topdir, ['diff', f'{sha}~1', sha, '--']
+        )
         if ecode > 0:
             viewer.write(Text('Could not generate diff', style=ts['error']))
             return
@@ -1336,7 +1338,7 @@ from b4.review_tui._modals import (
                 return
             sha = self._commit_shas[patch_idx]
             ecode, real_diff = b4.git_run_command(
-                self._topdir, ['diff', f'{sha}~1', sha]
+                self._topdir, ['diff', f'{sha}~1', sha, '--']
             )
             if ecode > 0:
                 self.notify('Could not get diff', severity='error')
@@ -2054,7 +2056,7 @@ from b4.review_tui._modals import (
 
                 sha = self._commit_shas[idx]
                 ecode, real_diff = b4.git_run_command(
-                    self._topdir, ['diff', f'{sha}~1', sha]
+                    self._topdir, ['diff', f'{sha}~1', sha, '--']
                 )
                 if ecode == 0:
                     b4.review._resolve_comment_positions(real_diff, comments)
-- 
2.43.0

Re: [PATCH b4] review: separate revisions from paths when diffing a commit

[Konstantin Ryabitsev]

> Reviewing a series fails with "Could not generate diff" whenever the
> work tree contains a file or directory named after the commit being
> diffed:
> 
>   $ mkdir 1b7771486ae3d9dec8959dbb4da51e535364fb8e
>   $ git diff 1b7771486ae3d9dec8959dbb4da51e535364fb8e~1 \
>             1b7771486ae3d9dec8959dbb4da51e535364fb8e
>   fatal: ambiguous argument '1b7771486ae3d9dec8959dbb4da51e535364fb8e':
>   both revision and filename
> 
> Any tooling that names its output after a bare SHA (review notes,
> logs, build artifacts) can shadow the revision this way. Both
> arguments here are always revisions, so terminate them with '--' at
> every site that diffs a commit against its parent.

Good catch, but I think it's not quite complete -- the same problem
would occur every place we do "git show" between two commit-id SHAs. I
can either take this as-is and add these myself, or you can send a v2
that also fixes this for "git show" calls. What do you prefer?

-- 
Konstantin Ryabitsev <konstantin@linuxfoundation.org>