On Tue, Dec 7, 2021 at 8:09 AM Derrick Stolee <stolee@xxxxxxxxx> 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) Interesting testcase... > 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 && However, this line is broken even if the directory weren't removed. Not because of the "git status --porcelain" part, but because of two other reasons: 1) test_sparse_match presumes it is run from the directory above the repos while you still have it in $repo/deep/deeper1 2) The point of test_sparse_match is to compare the results in two different repositories, but it'll do this twice and the first time without the changes having been made to the sparse-index repo. Perhaps this belonged outside the surrounding for loop? I think I'd either drop the "test_sparse_match" or else just drop the whole line; the real comparison is the expect/actual files. Dropping this line makes it a good test. > 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. Yeah, sounds good. Since you piqued my interest, though, the problem is that we're passing an absolute path to remove_dir_recursively() inside clean_tracked_sparse_directories() when we should be passing a relative path. (We always chdir(r->worktree) in setup.c, so there's no need to prepend the path with r->worktree+'/'.) Still, the current series is long enough and unless there are issues others have spotted with it, I'd rather just let it proceed as-is and then send this fix and a correction of your testcase in separately.
