Skip to content
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.

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

Fix CredentialHelper::add_command #1137

Merged
merged 2 commits into from
Mar 17, 2025

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Mar 5, 2025

Previously the add_helper function would check whether the command contains a \ or / character to determine whether it is absolute. It should have used starts_with instead of contains. In addition an absolute on Windows a path my start with a drive letter. The git cli implements this by checking if the second character is a ':'.

See also: https://github.com/git/git/blob/6a64ac7b014fa2cfa7a69af3c253bcd53a94b428/credential.c#L492-L493

Fixes #1136

@rustbot rustbot added the S-waiting-on-review Status: Waiting on review label Mar 5, 2025
@aibaars aibaars force-pushed the fix-credential-helper branch from 16bd0dc to ef3bca9 Compare March 10, 2025 10:41
@aibaars aibaars force-pushed the fix-credential-helper branch from ef3bca9 to c776e8d Compare March 10, 2025 10:42
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

fn is_absolute_path(path: &str) -> bool {
path.starts_with('/')
|| path.starts_with('\\')
|| cfg!(windows) && path.chars().nth(1).is_some_and(|x| x == ':')
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor note, this is slightly different from https://github.com/git-for-windows/git/blob/cca1f38702730b35f52c29efd62864b85e85ddcc/compat/win32/path-utils.c#L9-L31, but probably not enough to matter here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, git2-rs uses the str type while the git C uses a char* and if I understand the git C code correctly, it just tries to handle the case where the drive letter is a UTF8 character. I'd expect the str::nth function to take care of that already. However, there might indeed be some behaviour differences in corner cases at I'm not aware of.

@ehuss ehuss added this pull request to the merge queue Mar 17, 2025
Merged via the queue into rust-lang:master with commit 1e1687e Mar 17, 2025
7 checks passed
@ehuss
Copy link
Contributor

ehuss commented Mar 17, 2025

Note that this was relaxed in #429 which previously looked for starts_with, but I agree it is probably better to match behavior with git itself, and the : check should accommodate the original issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting on review
Projects
None yet
3 participants