Skip to content

Conversation

@slarse
Copy link
Contributor

@slarse slarse commented Nov 26, 2025

🧢 Changes

Continuation of #11363 with a suggestion to generalize the testsupport file mode normalization.

@vercel
Copy link

vercel bot commented Nov 26, 2025

@slarse is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the rust Pull requests that update Rust code label Nov 26, 2025
Comment on lines +313 to +314
directory @ 0o40000 => directory | normalize_permission_bits(mode),
symlink @ 0o120000 => symlink | normalize_permission_bits(mode),
Copy link
Contributor Author

@slarse slarse Nov 26, 2025

Choose a reason for hiding this comment

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

From a Git perspective it's a bit weird to propagate permission bits on directories and symlinks, as Git does not store those. But the current snapshots do so.

Do we want to "properly" normalize this to a more Git-aligned view (i.e. zero out the permission bits for these types in existing snapshots), or is this fine?

Copy link
Contributor Author

@slarse slarse Nov 26, 2025

Choose a reason for hiding this comment

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

I realize that if we don't care about "proper" Git normalization for the snapshots, this entire thing could be reduced to just:

    fn normalize_mode(mode: u32) -> u32 {
        let normalize_permission_bits = |mode: u32| if mode & 0o100 == 0 { 0o644 } else { 0o755 };
        (mode & 0o170000) | normalize_permission_bits(mode)
    }

Because every match arm is just doing the same thing right now, except for the panic, which I don't feel strongly about keeping.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, why not go with the minimal version that also can't panic. Let's merge this as well and you can follow it up.

Copy link
Collaborator

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks so much for picking this up!

Despite merging the PR now, I'd also be looking forward to the two-line version that won't panic :).

Comment on lines +313 to +314
directory @ 0o40000 => directory | normalize_permission_bits(mode),
symlink @ 0o120000 => symlink | normalize_permission_bits(mode),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, why not go with the minimal version that also can't panic. Let's merge this as well and you can follow it up.

@Byron Byron merged commit 7608fa2 into gitbutlerapp:master Nov 27, 2025
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants