Honor GIT_WORK_TREE - #4269
Merged
Merged
Honor GIT_WORK_TREE#4269
Conversation
bk2204
marked this pull request as draft
October 6, 2020 20:59
chrisd8088
approved these changes
Oct 7, 2020
chrisd8088
left a comment
Member
There was a problem hiding this comment.
This is great, thank you! I had a couple of minor comments, but otherwise LGTM!
bk2204
force-pushed
the
fixed-env-vars
branch
from
October 13, 2020 20:31
2671d06 to
0c1a30a
Compare
We have several different places in our code that need to canonicalize paths. In our case, that usually involves resolving a Cygwin path to a native Windows path, turning the path absolute, and calling filepath.EvalSymlinks. Let's add a function that does exactly that and call it from the places in the git package where we do this already. We pass an additional argument to indicate whether it's acceptable if the path is missing, and if so, we return an absolute but uncanonicalized path in that case. This is useful for canonicalizing Git environment variables which may or may not point to a valid location; we want Git, not us, to make the decision about whether a missing path is a problem in such a case.
Currently, the subprocess package reads from the environment as it's created at startup when the init function is called. However, we'll soon want to modify the environment in this case before it gets processed, so let's change the code to use a mutex to initialize the environment once before using it and simply call that before using the environment we've set up. We'll want to reset the environment as well in a future commit, so let's be sure to add a function for that. We reuse the same internal function and just ignore the return value to make our code paths simpler.
When we use certain Git environment variables to find the Git directory and the working tree, we want those values to work regardless of what directory we're in. Currently we don't change the working directory, but we soon will in order to mimic Git's behavior. In order to make relative values of these environment variables work correctly once we change directories, let's turn them into absolute canonical paths when we set up the repository. We save the old environment variables into a variable and pass this variable to the helper for the git lfs env command, which wants to print the original values, not our modified ones. Note that we can't make the canonicalization conditional on the subcommand being run because the configuration structure has been initialized by the time we get to argument parsing and the configuration code invokes Git upon initialization. Because the Cygwin path canonicalization invokes a subprocess on Windows and the subprocess code caches the environment, we must also reset the cache after adjusting these environment variables so that future subprocess invocations, like those for Git, work as expected with the updated environment.
bk2204
force-pushed
the
fixed-env-vars
branch
from
October 14, 2020 16:49
0c1a30a to
0f8368e
Compare
bk2204
marked this pull request as ready for review
October 14, 2020 16:49
In the normal case, Git commands perform repository autodiscovery based on the current working directory. However, in some cases, it's possible to specify a Git working tree unrelated to the current working directory by using GIT_WORK_TREE. In such a case, we want to make sure that we change into the working tree such that our working directory is always within the working tree, if one exists. This is what Git does, and it means that when we write files into the repository, such as a .gitattributes file, we write them into the proper place. Note also that we adjust the code to require that the working directory be non-empty when we require a working copy instead of that the repository be non-bare. That's because we don't want people to be working inside of the Git directory in such situations, where the repository would be non-bare but would not have a working tree. We add tests for this case for track and untrack, which require a working tree, and for checkout, which requires only a repository. This means that we can verify the behavior of the functions we've added without needing to add tests for this case to each of the subcommands.
bk2204
force-pushed
the
fixed-env-vars
branch
from
October 14, 2020 20:58
0f8368e to
75fb8f3
Compare
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 7, 2024
In commit 32244d9 of PR git-lfs#1825 the APPVEYOR_REPO_COMMIT_MESSAGE variable was added to our CI test suite environment in order to demonstrate that a problem exposed by the prior commit 0951c69 in the same PR had been addresssed. Specifically, the earlier commit had the string "GIT_SSH" in its message, and the AppVeyor CI service automatically provided the message from the most recent commit in a Git branch under test in the APPVEYOR_REPO_COMMIT_MESSAGE environment variable. At the time, the Environ() function in our "lfs" package would populate map it returns with the names and values of any environment variables whose names or values contained the string "GIT_". The result was that the APPVEYOR_REPO_COMMIT_MESSAGE variable was included in the returned environment, which would cause the tests in what is now our t/t-env.sh test script to fail. The problem was resolved by adjusting the Environ() function to only include environment variables in its map when their name contained the string "GIT_". To validate this fix, our test script environment was updated to write a fixed value into the APPVEYOR_REPO_COMMIT_MESSAGE environment variable (overriding whatever the AppVeyor service set) with an example string containing the string "GIT_". The example string used was the commit message from the commit which exposed the problem. Subsequently, we further revised the Environ() function in commit f4f8fae of PR git-lfs#4269 so it only includes environment variables whose names start with "GIT_", excluding those whose names just include that string. As we no longer use the AppVeyor service since we migrated our CI jobs to GitHub Actions in PR git-lfs#3808, and because the intent of the APPVEYOR_REPO_COMMIT_MESSAGE variable is now somewhat obscure, we replace it with a dedicated TEST_GIT_EXAMPLE environment variable in our test scripts where the output of "git lfs env" is checked. We ensure the "GIT_" string is included in both the name and value of this variable, and we preface it with some comments to clarify its purpose. If a regression were ever to be introduced into the Environ() function such that it did not exclude environment variables with "GIT_" in their names or values, the tests in the t/t-env.sh and t/t-worktree.sh scripts should still catch the problem.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 30, 2024
In commit 32244d9 of PR git-lfs#1825 the APPVEYOR_REPO_COMMIT_MESSAGE variable was added to our CI test suite environment in order to demonstrate that a problem exposed by the prior commit 0951c69 in the same PR had been addressed. Specifically, the earlier commit had the string "GIT_SSH" in its message, and the AppVeyor CI service automatically provided the message from the most recent commit in a Git branch under test in the APPVEYOR_REPO_COMMIT_MESSAGE environment variable. At the time, the Environ() function in our "lfs" package would populate the map it returns with the names and values of any environment variables whose names or values contained the string "GIT_". The result was that the APPVEYOR_REPO_COMMIT_MESSAGE variable was included in the returned environment, which would cause the tests in what is now our t/t-env.sh test script to fail. The problem was resolved by adjusting the Environ() function to only include environment variables in its map when their name contained the string "GIT_". To validate this fix, our test script environment was updated to write a fixed value into the APPVEYOR_REPO_COMMIT_MESSAGE environment variable (overriding whatever the AppVeyor service set) with an example string containing the string "GIT_". The example string used was the commit message from the commit which exposed the problem. Subsequently, we further revised the Environ() function in commit f4f8fae of PR git-lfs#4269 so it only includes environment variables whose names start with "GIT_", excluding those whose names just include that string. As we no longer use the AppVeyor service since we migrated our CI jobs to GitHub Actions in PR git-lfs#3808, and because the intent of the APPVEYOR_REPO_COMMIT_MESSAGE variable is now somewhat obscure, we replace it with a dedicated TEST_GIT_EXAMPLE environment variable in our test scripts where the output of "git lfs env" is checked. We ensure the "GIT_" string is included in both the name and value of this variable, and we preface it with some comments to clarify its purpose. If a regression were ever to be introduced into the Environ() function such that it did not exclude environment variables with "GIT_" in their names or values, the tests in the t/t-env.sh and t/t-worktree.sh scripts should still catch the problem.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Nov 5, 2024
In commit 32244d9 of PR git-lfs#1825 the APPVEYOR_REPO_COMMIT_MESSAGE variable was added to our CI test suite environment in order to demonstrate that a problem exposed by the prior commit 0951c69 in the same PR had been addressed. Specifically, the earlier commit had the string "GIT_SSH" in its message, and the AppVeyor CI service automatically provided the message from the most recent commit in a Git branch under test in the APPVEYOR_REPO_COMMIT_MESSAGE environment variable. At the time, the Environ() function in our "lfs" package would populate the map it returns with the names and values of any environment variables whose names or values contained the string "GIT_". The result was that the APPVEYOR_REPO_COMMIT_MESSAGE variable was included in the returned environment data, which would cause the tests in what is now our t/t-env.sh test script to fail. This problem was resolved by adjusting the Environ() function to only include environment variables in its map when their name contained the string "GIT_". To validate this fix, our test script environment was updated to write a fixed value into the APPVEYOR_REPO_COMMIT_MESSAGE environment variable (overriding whatever the AppVeyor service set) with an example string containing the string "GIT_". The example string used was the commit message from the commit which exposed the problem. Subsequently, we further revised the Environ() function in commit f4f8fae of PR git-lfs#4269 so it only includes environment variables whose names start with "GIT_", excluding those whose names merely contain that string. As we no longer use the AppVeyor service since we migrated our CI jobs to GitHub Actions in PR git-lfs#3808, and because the intent of the APPVEYOR_REPO_COMMIT_MESSAGE variable is now somewhat obscure, we replace it with a dedicated TEST_GIT_EXAMPLE environment variable that contains the "GIT_" string in both its name and value. We define this TEST_GIT_EXAMPLE variable only in the test scripts where the output of "git lfs env" is checked, and we preface its definition with some comments to clarify its purpose. If a regression were ever to be introduced into the Environ() function such that it did not exclude environment variables with "GIT_" in their names or values, the tests in the t/t-env.sh and t/t-worktree.sh scripts should detect the problem due to the presence of the TEST_GIT_EXAMPLE variable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, we don't honor GIT_WORK_TREE like Git does. Git implicitly changes into the working tree of the repository if this variable is set, but we currently don't do that.
Let's fix that by mimicking Git's behavior so this works correctly, which necessitates transforming the various environment variables we pass to Git so that they are valid when we change directories.
These commits are logical, bisectable, and independent and have sane commit messages.
Fixes #4213
/cc @Mr-Tao as reporter