Skip to content

fix(trash): strip only trailing trash suffix in to_origin_dir_path - #2341

Open
Siraryansingh wants to merge 3 commits into
oscal-compass:developfrom
Siraryansingh:fix/trash-to-origin-dir-path-bk
Open

fix(trash): strip only trailing trash suffix in to_origin_dir_path#2341
Siraryansingh wants to merge 3 commits into
oscal-compass:developfrom
Siraryansingh:fix/trash-to-origin-dir-path-bk

Conversation

@Siraryansingh

Copy link
Copy Markdown

What this PR does / why we need it:

Fixes to_origin_dir_path() and to_origin_file_path() in trestle/common/trash.py so they strip only the trailing __bk and .bk suffix instead of splitting on all occurrences of __bk or .bk inside directory and file names.

Previously, directory names containing __bk (e.g. alpha__bkbeta) were incorrectly truncated to alpha during recovery.

Which issue(s) this PR fixes:

Fixes #2210

How was this tested?:

  • Added test_to_origin_dir_path_with_bk_in_name unit test in tests/trestle/utils/trash_test.py
  • Ran all 17 trash unit tests via pytest (17 passed in 0.04s)
  • Linted with ruff (all checks passed)

Fix to_origin_dir_path and to_origin_file_path so they strip only the trailing __bk and .bk suffix rather than splitting on all occurrences of __bk or .bk inside directory and file names.

Fixes oscal-compass#2210

Signed-off-by: Aryan Singh <aryansingh.as1012@gmail.com>
@Siraryansingh
Siraryansingh requested a review from a team as a code owner August 29, 2026 12:39
@degenaro

Copy link
Copy Markdown
Collaborator

@Siraryansingh Thx for this PR. Are you on our slack channel #oscal-compass-trestle-agileauthoring-c2p? To join you need a LF ID, you can get one here https://identity.linuxfoundation.org/.

Below is Bob's review. Please fix the DCO and missing test issues, thx!

=====

PR #2341 Review — fix(trash): strip only trailing trash suffix in to_origin_dir_path

Author: Aryan Singh (@Siraryansingh)
Base: develop
Files changed: trestle/common/trash.py (+7 / -4), tests/trestle/utils/trash_test.py (+10 / 0)
Issue fixed: #2210


🚫 Blocker — DCO not correctly signed

The commit 6e1a56a contains:

Signed-off-by: Aryan Singh <aryansingh.as1012@gmail.com>

However, the commit was authored under the GitHub identity siraryansingh2005@gmail.com. The DCO requires that the email in the Signed-off-by trailer exactly matches the email associated with the commit author identity. Because these two addresses differ, the DCO check fails.

Resolution: The author must amend the commit so that the Signed-off-by email matches the author email on record, then force-push:

git commit --amend --signoff
# verify the Signed-off-by email matches your git user.email, then:
git push --force-with-lease

Summary

This PR fixes a real bug in trestle/common/trash.py: both to_origin_dir_path() and to_origin_file_path() used str.split() to remove the __bk / .bk trash suffixes. That approach silently truncates any path component or filename that contains those strings in the middle — for example, a directory named alpha__bkbeta would be recovered as alpha.


Fix Quality ✅ Correct

The change is minimal and precise.

Before (same pattern in both functions):

parts = item.split(TRESTLE_TRASH_DIR_EXT)
origin_path_parts.append(parts[0])

str.split() splits on every occurrence, so alpha__bkbeta__bk['alpha', 'beta', '']'alpha'.

After:

if item.endswith(TRESTLE_TRASH_DIR_EXT):
    item = item[:-len(TRESTLE_TRASH_DIR_EXT)]
origin_path_parts.append(item)

This correctly strips only the trailing suffix. alpha__bkbeta__bkalpha__bkbeta. ✅

The fix is applied symmetrically to both to_origin_dir_path() (directory parts) and to_origin_file_path() (filename).


Test Coverage ✅ Adequate — one minor gap

  • test_to_origin_dir_path_with_bk_in_name correctly exercises to_origin_dir_path() with a directory whose name contains __bk in the middle.
  • All 17 trash unit tests pass locally.

Minor gap (non-blocking): There is no analogous regression test for to_origin_file_path() with a filename that contains .bk in the middle (e.g. report.bk2.md). A suggested addition:

def test_to_origin_file_path_with_bk_in_name(tmp_path: pathlib.Path) -> None:
    """Test to_origin_file_path handles file names containing .bk."""
    test_utils.ensure_trestle_config_dir(tmp_path)
    (tmp_path / trash.TRESTLE_TRASH_DIR).mkdir(exist_ok=True, parents=True)
    bk_file = tmp_path / 'report.bk2.md'
    trash_file_path = trash.to_trash_file_path(bk_file)
    origin_file_path = trash.to_origin_file_path(trash_file_path)
    assert bk_file.resolve() == origin_file_path.resolve()

Round-trip Correctness ✅

to_trash_dir_path() appends the suffix to the full relative path string exactly once. The fixed to_origin_dir_path() now strips it from the end exactly once. The forward/reverse round-trip is mathematically consistent.


Code Style ✅

item[:-len(TRESTLE_TRASH_DIR_EXT)] is idiomatic Python. An equally readable alternative is item.removesuffix(TRESTLE_TRASH_DIR_EXT) (Python 3.9+), but the current form is acceptable.


Overall Verdict

Concern Status
DCO signature mismatch 🚫 Blocker
Logic correctness ✅ Correct
Test for directory case ✅ Present
Test for file case with .bk in name ⚠️ Missing (non-blocking suggestion)
Code style ✅ Fine

The fix itself is correct and ready to merge once the DCO issue is resolved.

@degenaro

Copy link
Copy Markdown
Collaborator

Also, some tips. Run locally:

make develop
make code-format
make code-lint
make mdformat
make test

@degenaro

Copy link
Copy Markdown
Collaborator

Some additional recommendations:

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

to_origin_dir_path truncates directory names containing __bk

2 participants