On Tue, Dec 07 2021, Derrick Stolee wrote: > On 12/1/2021 1:40 AM, Elijah Newren via GitGitGadget wrote: >> Traditionally, if folks run git commands such as checkout or rebase from a >> subdirectory, that git command could remove their current working directory >> and result in subsequent git and non-git commands either getting confused or >> printing messages that confuse the user (e.g. "fatal: Unable to read current >> working directory: No such file or directory"). Many commands either >> silently avoid removing directories that are not empty (i.e. those that have >> untracked or modified files in them)[1], or show an error and abort, >> depending on which is more appropriate for the command in question. With >> this series, we augment the reasons to avoid removing directories to include >> not just has-untracked-or-modified-files, but also to avoid removing the >> original_cwd as well. > > I did not clearly voice my approval of the core idea here, but I do like it. > > I think this fits squarely into a category of "help the user not get stuck" > which Git has enough of those situations that we don't need this one. Even > expert users won't know for sure if a 'git checkout' will cause their current > directory to be removed, however unlikely. > > In the Git project, we spend a lot of time in the root of our workdir, but > this is not the typical case for large projects. I remember spending most of > my time in a previous role working four levels deep in the directory hierarchy. > > > I read the previous two range-diffs and took another pass at this v5 and > didn't see anything worth commenting on. This version is good to go. > > There is _also_ more work to do, as follow-ups. In particular, the thing > that I thought about was sparse-checkout and created this test which still > fails at the end of your series (as an addition to t1092) > > test_expect_success 'remove cwd' ' > init_repos && > > test_sparse_match git sparse-checkout set deep/deeper1/deepest && > for repo in sparse-checkout sparse-index > do > ( > cd $repo/deep/deeper1 && > test-tool getcwd >"$TRASH_DIRECTORY/expect" && > git sparse-checkout set && > > test-tool getcwd >"$TRASH_DIRECTORY/actual" && > test_sparse_match git status --porcelain && > cd "$TRASH_DIRECTORY" && > test_cmp expect actual > ) > done > ' > > Please do not let this test delay the advancement of this series. As we > find these kinds of issues, we can fix them one-by-one as needed. Not to pile on about "the core idea", just a question while this is fresh in your mind: I think that those cases would per [1] be ones where a more isolated change of reading the $PWD from the environment would make all those commands work as expected. Or would the "$TRASH_DIRECTORY" also otherwise go away in this examples? Anyway, just per [1] and the potential future follow-ups is this (I don't think so, but maybe I'm wrong) or other examples you have things that specifically need the "retain the getcwd()" part of this series? Or just (as I think would be the case with that "git status") to not have setup.c die quite as eagerly as it does now when getcwd() fails, but it can find its way back to the .git via the environment's $PWD? Also note that writing any tests to check the current behavior is a pain due to the bin-wrappers as I discussed in some previous threads, but that's a test-only oddity, and won't impact how git would behave once installed (although now it doesn't behave well at all). So probing the current behavior via tests is hard, unless you use GIT_TEST_INSTALLED to get around the bin-wrappers. There *are* definitely cases where not-just-that-setup.c change but also the "don't remove the CWD" is an inherently better & more complete solution. But I think that's mainly to do with 3rd party shellscripts & other programs outside of our control. I'm assuming that you were working with this on Windows, where presumably there's fewer/none such shellscripts you rely on, but that's now two presumes in a row, so... :) 1. https://lore.kernel.org/git/211207.86ee6opy0f.gmgdl@xxxxxxxxxxxxxxxxxxx/
