Skip to content

fix(ls): preserve permission info as octal when -l/-la is passed#1675

Merged
KuSh merged 2 commits into
rtk-ai:developfrom
ryana-0154:fix/ls-la-octal-perms
May 25, 2026
Merged

fix(ls): preserve permission info as octal when -l/-la is passed#1675
KuSh merged 2 commits into
rtk-ai:developfrom
ryana-0154:fix/ls-la-octal-perms

Conversation

@ryana-0154
Copy link
Copy Markdown
Contributor

Summary

Example

Before:
.DS_Store 6.0K
init.py 984B

After (rtk ls -la):
644 .DS_Store 6.0K
644 init.py 984B

Test plan

  • cargo fmt --all -- --check
  • cargo clippy --all-targets — 0 errors (pre-existing warnings unchanged)
  • cargo test --all — 1692 passed, 6 ignored
  • New unit tests for perms_to_octal (common perms, setuid/setgid/sticky, garbage input) and for long vs. short output formatting

@pszymkowiak pszymkowiak added bug Something isn't working effort-small Quelques heures, 1 fichier filter-quality Filter produces incorrect/truncated signal labels May 2, 2026
@pszymkowiak
Copy link
Copy Markdown
Collaborator

[w] wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type bug-fix
🟢 Risk low

Summary

Fixes the rtk ls -la command which was producing output identical to rtk ls by preserving permission information as octal notation when -l is passed. Adds a perms_to_octal function to convert ls-style permission strings to octal format, handling special bits (setuid/setgid/sticky) with a 4-digit form. Existing rtk ls behavior without -l is unchanged.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

Linked issues: #1672


Analyzed automatically by wshm · This is an automated analysis, not a human review.

Copy link
Copy Markdown
Collaborator

@KuSh KuSh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, that's a great addition but it needs some minor changes first!

Comment thread src/cmds/system/ls.rs Outdated
Comment thread src/cmds/system/ls.rs Outdated
Comment thread src/cmds/system/ls.rs Outdated
Comment thread src/cmds/system/ls.rs Outdated
Copy link
Copy Markdown
Collaborator

@KuSh KuSh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, that's a great addition but it needs some minor changes first!

Comment thread src/cmds/system/ls.rs Outdated
ryana-0154 added a commit to ryana-0154/rtk that referenced this pull request May 16, 2026
Addresses PR rtk-ai#1675 review: instead of ignoring the perms field in
existing parse_ls_line tests, assert its value so regressions in
permission parsing are caught.
@ryana-0154 ryana-0154 requested a review from KuSh May 16, 2026 10:13
Copy link
Copy Markdown
Collaborator

@KuSh KuSh left a comment

Choose a reason for hiding this comment

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

Hi @ryana-0154, you'll need to resolve conflicts before we can merge that one, otherwise LGTM!

@ryana-0154
Copy link
Copy Markdown
Contributor Author

Hi @ryana-0154, you'll need to resolve conflicts before we can merge that one, otherwise LGTM!

Hi - there doesn't seem to be any merge conflicts to develop - can you confirm?

@KuSh
Copy link
Copy Markdown
Collaborator

KuSh commented May 17, 2026

Hi @ryana-0154, you'll need to resolve conflicts before we can merge that one, otherwise LGTM!

Hi - there doesn't seem to be any merge conflicts to develop - can you confirm?

Let me recheck, I did have one when I tried earlier

@KuSh
Copy link
Copy Markdown
Collaborator

KuSh commented May 22, 2026

@ryana-0154 there's a conflict, please rebase:

❯ git rebase upstream/develop
Auto-merging src/cmds/system/ls.rs
CONFLICT (content): Merge conflict in src/cmds/system/ls.rs
error: could not apply 39dee40... fix(ls): preserve permission info as octal when -l/-la is passed
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Could not apply 39dee40... # fix(ls): preserve permission info as octal when -l/-la is passed

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 23, 2026

CLA assistant check
All committers have signed the CLA.

@ryana-0154 ryana-0154 requested a review from KuSh May 23, 2026 17:49
@KuSh
Copy link
Copy Markdown
Collaborator

KuSh commented May 25, 2026

You seem to have rebased on the wrong branch (certainly master) you have to fix that. 57 files changed all over the place is too much for the subject 😉

Closes rtk-ai#1672

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ryana-0154 ryana-0154 force-pushed the fix/ls-la-octal-perms branch from e0cb485 to 6c4dfc7 Compare May 25, 2026 11:46
@ryana-0154
Copy link
Copy Markdown
Contributor Author

You seem to have rebased on the wrong branch (certainly master) you have to fix that. 57 files changed all over the place is too much for the subject 😉

duh... what an idiot :D Fixed!

@KuSh KuSh merged commit 6e76f91 into rtk-ai:develop May 25, 2026
11 checks passed
@aeppling aeppling mentioned this pull request May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working effort-small Quelques heures, 1 fichier filter-quality Filter produces incorrect/truncated signal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The result of rtk ls -la is equivalent to rtk ls, which leads to the loss of some info.

4 participants