Re: [PATCH v3 00/11] Avoid removing the current working directory, even if it becomes empty

From: Derrick Stolee <stolee@gmail.com>
Date: Nov 29, 2021, 17:57 UTC
Message-ID: <39e2bc15-298c-0932-109b-e92da3979dc2@gmail.com>
In-Reply-To: <pull.1140.v3.git.git.1637966463.gitgitgadget@gmail.com>

On 11/26/2021 5:40 PM, Elijah Newren via GitGitGadget wrote:

Show 11 quoted lines

> 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>'.

Show 15 quoted lines

> 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

...

Show 5 quoted lines

>        			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