From: Omar Polo <op@omarpolo.com>
Subject: Re: got: rm * removes current directory
To: Stefan Sperling <stsp@stsp.name>
Cc: Mikhail <mp39590@gmail.com>, gameoftrees@openbsd.org
Date: Sun, 28 May 2023 17:11:07 +0200

On 2023/05/28 12:59:39 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Sat, May 27, 2023 at 11:19:56AM +0300, Mikhail wrote:
> > On Sat, May 27, 2023 at 10:15:53AM +0200, Omar Polo wrote:
> > > On 2023/05/27 10:58:41 +0300, Mikhail <mp39590@gmail.com> wrote:
> > > > core:~/work/got$ cd got
> > > > core:~/work/got/got$ ls
> > > > Makefile          git-repository.5  got-worktree.5    got.1
> > > > got.c             got.conf.5
> > > > core:~/work/got/got$ got rm *
> > > > D  got/Makefile
> > > > D  got/git-repository.5
> > > > D  got/got-worktree.5
> > > > D  got/got.1
> > > > D  got/got.c
> > > > D  got/got.conf.5
> > > > core:~/work/got/got$ ls
> > > > colorls: .: No such file or directory
> > > > core:~/work/got/got$ cd ..
> > > > core:~/work/got$ ls got
> > > > colorls: got: No such file or directory
> > > > 
> > > > git rm * removes only files, not current directory
> > > > 
> > > > in case this is a bug here is a test case:
> > > 
> > > Since git doesn't track directories, having 'got rm' removing empty
> > > directories seems a useful feature to me, even though having $PWD
> > > disappearing from under your feets is not nice.
> > > 
> > > However, what would be the value in keeping empty directories on-disk
> > > around?
> > 
> > I faced this when I was working on port - I needed to "got rm *" inside
> > patches/ and then I wanted to add my own patch files there.
> 
> I've been thinking about this for a while and did consider whether we
> should allow user-control over $PWD removal via options such as -f.
> But since the existing options already control other aspects of behaviour
> we'd need to add a new option for this, and I don't think that is worth it.
> 
> It is easy enough to remove an empty directory manually if needed so
> I suppose we should just stop deleting $PWD. This patch makes your new
> test pass and doesn't make any of the existing tests fail.

Don't know, I'm not sure we should be using $PWD in lib/ to change the
behaviour, although removing $PWD is not nice.

Admittedly I haven't noticed the issue so far and won't consider it a
bad thing anyway, as it did what it is supposed to do.

personal preference aside, no objections on the diff, it reads fine.

> diff /home/stsp/src/got
> commit - 77fc0a255189c6c18e1ea0d6ea82ae1d0ddb4ea7
> path + /home/stsp/src/got
> blob - 622c0ce1ca669ed14ca0d26259483e9beea95863
> file + lib/worktree.c
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -2147,7 +2147,11 @@ remove_ondisk_file(const char *root_path, const char *
>  {
>  	const struct got_error *err = NULL;
>  	char *ondisk_path = NULL, *parent = NULL;
> +	char cwd[PATH_MAX];
>  
> +	if (getcwd(cwd, sizeof(cwd)) == NULL)
> +		return got_error_from_errno("getcwd");
> +
>  	if (asprintf(&ondisk_path, "%s/%s", root_path, path) == -1)
>  		return got_error_from_errno("asprintf");
>  
> @@ -2160,7 +2164,9 @@ remove_ondisk_file(const char *root_path, const char *
>  		if (err)
>  			goto done;
>  		while (got_path_cmp(parent, root_path,
> -		    strlen(parent), root_len) != 0) {
> +		    strlen(parent), root_len) != 0 &&
> +		    got_path_cmp(parent, cwd,
> +		    strlen(parent), strlen(cwd)) != 0) {
>  			free(ondisk_path);
>  			ondisk_path = parent;
>  			parent = NULL;
> @@ -4394,7 +4400,8 @@ schedule_for_deletion(void *arg, unsigned char status,
>  	}
>  
>  	if (!a->keep_on_disk && status != GOT_STATUS_MISSING) {
> -		size_t root_len;
> +		char cwd[PATH_MAX];
> +		size_t cwd_len, root_len;
>  
>  		if (dirfd != -1) {
>  			if (unlinkat(dirfd, de_name, 0) == -1) {
> @@ -4407,12 +4414,21 @@ schedule_for_deletion(void *arg, unsigned char status,
>  			goto done;
>  		}
>  
> +		if (getcwd(cwd, sizeof(cwd)) == NULL) {
> +			err = got_error_from_errno("getcwd");
> +			goto done;
> +		}
> +		cwd_len = strlen(cwd);
> +
>  		root_len = strlen(a->worktree->root_path);
>  		do {
>  			char *parent;
>  			err = got_path_dirname(&parent, ondisk_path);
>  			if (err)
>  				goto done;
> +			if (got_path_cmp(parent, cwd, strlen(parent),
> +			    cwd_len) == 0)
> +				break;
>  			free(ondisk_path);
>  			ondisk_path = parent;
>  			if (rmdir(ondisk_path) == -1) {