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

PATH's different names in Windows and Linux causing git-branchless hook execution to fail #1256

Closed
c00t opened this issue Feb 29, 2024 · 7 comments · Fixed by #1267
Closed

Comments

@c00t
Copy link
Contributor

c00t commented Feb 29, 2024

I do a lot of game engine programming so i can't avoid using git under native windows. When using git-branchless, I also had similar problem as (#370 , #1144 ). Begin debugging...

In #370


Definitely seems like a git on Windows bug. Just sort of sucks because it makes git branchless sort of broken on Windows. I'm moved to exclusively using git in WSL/Ubuntu so it's no longer a problem for me on my home computer. I'm starting a new job next week which is going to be on Windows and I don't really know how I'll be interacting with source control and I don't know much about how the environment is set up. Depending on how that turns out, I may dig more into what it takes to make this work properly.

No, This is not a Git for Windows problem, after I debugged it, I realized that it is due to the naming of the Path environment variable, in the following code, git-branchless reads the environment variable named PATH, but in windows, the variable name should be Path.

let GitRunInfo {
// We're calling a Git hook, but not Git itself.
path_to_git: _,
// We always want to call the hook in the Git working copy,
// regardless of where the Git executable was invoked.
working_directory: _,
env,
} = self;
let path = {
let mut path_components: Vec<PathBuf> =
vec![std::fs::canonicalize(&hook_dir).wrap_err("Canonicalizing hook dir")?];
if let Some(path) = env.get(OsStr::new("PATH")) {
path_components.extend(std::env::split_paths(path));
}
std::env::join_paths(path_components).wrap_err("Joining path components")?
};

I believe this is due to the fact that the windows shell is insensitive to the case of environment variable names (both pwsh and cmd), which many developers don't pay attention to on windows, you can read the environment variables correctly in the shell with Path PATH or even PaTh.

quick fix:

        let GitRunInfo {
            // We're calling a Git hook, but not Git itself.
            path_to_git: _,
            // We always want to call the hook in the Git working copy,
            // regardless of where the Git executable was invoked.
            working_directory: _,
            env,
        } = self;
        let path = {
            let mut path_components: Vec<PathBuf> =
                vec![std::fs::canonicalize(&hook_dir).wrap_err("Canonicalizing hook dir")?];
            if let Some(path) = env.get(OsStr::new("PATH")) {
                path_components.extend(std::env::split_paths(path));
            }
            #[cfg(target_os = "windows")]
            if let Some(path) = env.get(OsStr::new("Path")) {
                path_components.extend(std::env::split_paths(path));
            }
            std::env::join_paths(path_components).wrap_err("Joining path components")?
        };

yep, windows target's std crate also:

fn main() {
    println!("{:?}", std::env::var_os("Path")); // valid
    println!("{:?}", std::env::var_os("PATH")); // valid
    println!("{:?}", std::env::var_os("PaTh")); // valid
    // but 
    let x = std::env::vars_os().collect();
    // put "Path" inside x
}

Originally posted by @c00t in #370 (comment)

@c00t
Copy link
Contributor Author

c00t commented Feb 29, 2024

Since #370 is marked as no-planned-fix, pull request still needed? Or just a quick fix in your planned commit?

@AaronLieberman
Copy link

After restoring my hooks to their original state (without the full path workaround I had been using), I deleted my local Path variable and added PATH and re-ran one of my failing test cases. The hook git branchless was able to find git correctly and everything seemed to work. Nice find, C00t! Is someone who has git-branchless set up for development able to make a PR for this? I guess I could... but I'd have to figure out how to get Rust set up for development again.

@c00t
Copy link
Contributor Author

c00t commented Mar 1, 2024

@AaronLieberman I've done a quick fix in my fork, and if you use cargo you can install it using below command. But I'm not sure if this fix is correct (i.e., only modifying the implementation of the run_hook function) and if such a small modification would need to be merged as a PR.

cargo install --git https://github.com/c00t/git-branchless -f

@oltolm
Copy link

oltolm commented Mar 3, 2024

A workaround for this bug is to create and execute a file fix-path.cmd with the following contents

@echo off
set OLDPATH=%PATH%
set PATH=
set PATH=%OLDPATH%
set OLDPATH=

@AaronLieberman
Copy link

@arxanas @c00t's fix works for me. I tested it locally based on that branch and it's working for me on Windows as well on Ubuntu on WSL2. It looks like quite a safe change as from the code it looks like it wouldn't affect non-Windows platforms. Having this in the mainline would be a big improvement so that Windows would work reliably.

Would you be able to merge it into mainline? I tried to open up a PR for it but I'm not in the contributors group so it wouldn't let me. This change fixes issues #1256, #370, and #1144.

@arxanas
Copy link
Owner

arxanas commented Mar 4, 2024

@c00t can you open a PR, or a otherwise affirmatively state that your code is fine to merge under the git-branchless license?

@c00t
Copy link
Contributor Author

c00t commented Mar 5, 2024

@arxanas done #1267

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 a pull request may close this issue.

4 participants