Skip to content

fix: account for CWD when building git ls-tree pathspecs #75

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.

Sign up for GitHub

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

Merged
csoceanu merged 3 commits into from
Aug 17, 2026
Merged

fix: account for CWD when building git ls-tree pathspecs #75

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d1004b
fix: account for CWD when building git ls-tree pathspecs
csoceanu Aug 17, 2026
d93db96
fix: use ./ prefix in git cat-file and git show for CWD-relative paths
csoceanu Aug 17, 2026
d5bcc2a
fix: use git rev-parse --show-prefix instead of filesystem heuristic
csoceanu Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 33 additions & 12 deletions src/doc_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,27 @@ def _get_base_branch_ref():
return f"origin/{base_branch}"


def _get_effective_subfolder():

@fullsend-ai-review fullsend-ai-review Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] design-concern

The _get_effective_subfolder() function uses a CWD-dependent filesystem check to determine whether the subfolder prefix should be included in git pathspecs. The logic is intentional but implicitly depends on CWD state rather than explicitly detecting it.

Suggested fix: Consider adding a brief inline comment explaining the CWD-detection logic for future maintainers.

"""Get the DOCS_SUBFOLDER for git pathspecs, accounting for CWD.

When CWD is already inside the docs subfolder (after
setup_docs_environment), the subfolder prefix must be omitted from
git pathspecs because they are CWD-relative.

Uses ``git rev-parse --show-prefix`` to determine CWD's position
within the repo, avoiding filesystem heuristics.
"""
docs_subfolder = os.environ.get("DOCS_SUBFOLDER", "")
if not docs_subfolder:

@fullsend-ai-review fullsend-ai-review Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] edge-case

The _get_effective_subfolder() heuristic uses a filesystem existence check (Path(docs_subfolder).exists()) to determine whether CWD is inside the docs subfolder. This can produce a false result if the DOCS_SUBFOLDER directory contains a sub-directory with the same name (e.g., docs/docs/). The scenario is unlikely and the heuristic is consistent with get_docs_root().

return ""
result = run_command_safe(["git", "rev-parse", "--show-prefix"], check=False)
if result.returncode == 0:
prefix = result.stdout.strip().rstrip("/")
if prefix == docs_subfolder:

@fullsend-ai-review fullsend-ai-review Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] edge-case

_get_effective_subfolder() uses exact equality (prefix == docs_subfolder) to detect when CWD is inside the docs subfolder. If CWD were deeper than the subfolder root (e.g., docs/commands/ while DOCS_SUBFOLDER is docs), the prefix would not match and the function would return the full subfolder, producing a double-prefixed pathspec. In current code, setup_docs_environment() only does os.chdir(docs_subfolder) (never deeper), so this edge case is not triggered today.

Suggested fix: Consider adding a comment documenting the assumption that CWD is exactly the subfolder root, never deeper.

return ""
return docs_subfolder


def get_folder_doc_hashes_from_ref(folder, docs_root=None):
"""
Get hashes of docs in a folder from the base branch git ref.
Expand All @@ -249,12 +270,12 @@ def get_folder_doc_hashes_from_ref(folder, docs_root=None):
docs_root = get_docs_root()

ref = _get_base_branch_ref()
docs_subfolder = os.environ.get("DOCS_SUBFOLDER", "")
subfolder = _get_effective_subfolder()

if folder == ROOT_LEVEL_FOLDER:
search_path = docs_subfolder or "."
search_path = subfolder or "."
else:
search_path = f"{docs_subfolder}/{folder}" if docs_subfolder else folder
search_path = f"{subfolder}/{folder}" if subfolder else folder

result = run_command_safe(
["git", "ls-tree", "-r", "--name-only", ref, "--", search_path],
Expand All @@ -273,8 +294,8 @@ def get_folder_doc_hashes_from_ref(folder, docs_root=None):
continue

rel_to_docs = file_path
if docs_subfolder and file_path.startswith(docs_subfolder + "/"):
rel_to_docs = file_path[len(docs_subfolder) + 1 :]
if subfolder and file_path.startswith(subfolder + "/"):
rel_to_docs = file_path[len(subfolder) + 1 :]

parts = Path(rel_to_docs).parent.parts
if any(p.startswith(".") or p.startswith("_") for p in parts):
Expand All @@ -293,7 +314,7 @@ def get_folder_doc_hashes_from_ref(folder, docs_root=None):
# translation (\r\n → \n), breaking hash consistency for CRLF files.
try:
content_result = subprocess.run(
["git", "cat-file", "blob", f"{ref}:{file_path}"],
["git", "cat-file", "blob", f"{ref}:./{file_path}"],
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL,
)
Expand Down Expand Up @@ -428,12 +449,12 @@ def _get_docs_content_from_ref(folder):
unavailable, empty list if folder has no doc files on the ref.
"""
ref = _get_base_branch_ref()
docs_subfolder = os.environ.get("DOCS_SUBFOLDER", "")
subfolder = _get_effective_subfolder()

if folder == ROOT_LEVEL_FOLDER:
search_path = docs_subfolder or "."
search_path = subfolder or "."
else:
search_path = f"{docs_subfolder}/{folder}" if docs_subfolder else folder
search_path = f"{subfolder}/{folder}" if subfolder else folder

result = run_command_safe(
["git", "ls-tree", "-r", "--name-only", ref, "--", search_path],
Expand All @@ -452,8 +473,8 @@ def _get_docs_content_from_ref(folder):
continue

rel_to_docs = file_path
if docs_subfolder and file_path.startswith(docs_subfolder + "/"):
rel_to_docs = file_path[len(docs_subfolder) + 1 :]
if subfolder and file_path.startswith(subfolder + "/"):
rel_to_docs = file_path[len(subfolder) + 1 :]

parts = Path(rel_to_docs).parent.parts
if any(p.startswith(".") or p.startswith("_") for p in parts):
Expand All @@ -467,7 +488,7 @@ def _get_docs_content_from_ref(folder):
continue

content_result = run_command_safe(
["git", "show", f"{ref}:{file_path}"],
["git", "show", f"{ref}:./{file_path}"],
check=False,
)
if content_result.returncode == 0 and content_result.stdout:
Expand Down
26 changes: 24 additions & 2 deletions tests/test_doc_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,25 @@ def test_handles_docs_subfolder(self, monkeypatch):
file_content = b"export docs"

with (
patch("doc_index._get_effective_subfolder", return_value="docs"),
patch("doc_index.run_command_safe") as mock_run,
patch("doc_index.subprocess.run") as mock_subprocess,
):
mock_run.return_value = MagicMock(returncode=0, stdout=ls_output)
mock_subprocess.return_value = MagicMock(returncode=0, stdout=file_content)
result = get_folder_doc_hashes_from_ref("commands")

assert result is not None

@fullsend-ai-review fullsend-ai-review Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] test-coverage-gap

The new test_handles_docs_subfolder_from_inside test verifies the output but does not assert the actual pathspec passed to git ls-tree. If _get_effective_subfolder returned the wrong value but the mock happened to return the right output anyway, the test would still pass.

Suggested fix: Add an assertion on the constructed pathspec argument to verify the CWD-aware path construction.

assert "commands/export.md" in result

def test_handles_docs_subfolder_from_inside(self, monkeypatch):
"""When CWD is inside the docs subfolder, pathspecs omit the prefix."""
monkeypatch.setenv("DOCS_SUBFOLDER", "docs")
ls_output = "commands/export.md\n"
file_content = b"export docs"

with (
patch("doc_index._get_effective_subfolder", return_value=""),
patch("doc_index.run_command_safe") as mock_run,
patch("doc_index.subprocess.run") as mock_subprocess,
):
Expand All @@ -357,7 +376,7 @@ def test_uses_custom_base_branch(self, monkeypatch):
ls_call = mock_run.call_args_list[0].args[0]
assert "origin/develop" in ls_call
cat_call = mock_subprocess.call_args_list[0].args[0]
assert "origin/develop:guides/setup.md" in cat_call
assert "origin/develop:./guides/setup.md" in cat_call

def test_root_level_folder_excludes_subdirectory_files(self, monkeypatch):
from doc_index import ROOT_LEVEL_FOLDER
Expand Down Expand Up @@ -452,7 +471,10 @@ def mock_run(cmd, **kwargs):
return MagicMock(returncode=0, stdout=ls_output)
return MagicMock(returncode=0, stdout="export content")

with patch("doc_index.run_command_safe", side_effect=mock_run):
with (
patch("doc_index._get_effective_subfolder", return_value="docs"),
patch("doc_index.run_command_safe", side_effect=mock_run),
):
result = _get_docs_content_from_ref("commands")

assert len(result) == 1
Expand Down
Loading