On 11/26/2021 5:40 PM, 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. As I was reviewing v2, this version popped up in my inbox. Sorry about that. The only actionable comment from my review of v2 was the addition of a check that the worktree is in the expected state after commands are aborted due to trying to remove the current working directory. Your suggested git diff --exit-code HEAD should work for this. I might even add a "git rev-parse HEAD" to make sure we are on the right ref, but that's perhaps too specific to something like 'git reset --hard <branch>'. > Changes since v2: > > * the series is now only about the working tree. So if the original cwd is > outside the worktree (or we're in a bare repo), then the new code is a > no-op. > * fixed ugly early die() possibility (uses strbuf_getcwd() instead of > xgetcwd()) > * modified the initial tests to show both expected and desired behavior. > subsequent patches fix the tests. One new patch added at the end which > simplifies the tests to only check for desired behavior. > * NULLify startup_info->original_cwd when it matches the toplevel worktree; > that is already protected and we don't need secondary protection for it. > This simplified some other codepaths so we don't have to check for > startup_info->original_cwd == "". > * clarified some commit messages Looking at these changes I like all of them. > Range-diff vs v2: > > 1: 38a120f5c03 ! 1: 4b0044656b0 t2501: add various tests for removing the current working directory I like this new test structure. Using test_expect_success to document existing behavior is a good strategy. > 2: f6129a8ac9c ! 2: 200ddece05d setup: introduce startup_info->original_cwd> 3: e74975e83cc ! 3: 68ae90546fe unpack-trees: refuse to remove startup_info->original_cwd > 4: e06806e3a32 ! 4: 1bb8905900c unpack-trees: add special cwd handling > 5: 46728f74ea1 ! 5: 8a33d74e7cf symlinks: do not include startup_info->original_cwd in dir removal > 6: 01ce9444dae ! 6: 11e4ec881bb clean: do not attempt to remove startup_info->original_cwd These changes looked good. > -: ----------- > 7: 39b1f3a225e rebase: do not attempt to remove startup_info->original_cwd I had a small comment on this one, only because I think there is a condition in your 'if' statement that is either unhelpful or is hiding something. > 7: edec0894ca2 ! 8: 0110462a19c stash: do not attempt to remove startup_info->original_cwd ... > cp.git_cmd = 1; > + if (startup_info->original_cwd && > -+ *startup_info->original_cwd && > + !is_absolute_path(startup_info->original_cwd)) > + cp.dir = startup_info->original_cwd; And here is a similar use of is_absolute_path() that could be resolved the same way as whatever you choose for the v3 patch 7. > 8: 1815f18592b ! 9: 2c73a09a2e8 dir: avoid incidentally removing the original_cwd in remove_path() > 9: adaad7aeaac ! 10: d4e50b4053d dir: new flag to remove_dir_recurse() to spare the original_cwd > -: ----------- > 11: 7eb6281be4b t2501: simplify the tests since we can now assume desired behavior This last test cleanup is good to have. Thanks, -Stolee
