On 23-05-29 01:37PM, Stefan Sperling wrote:
> On Mon, May 29, 2023 at 08:06:12PM +1000, Mark Jamsek wrote:
> > On 23-05-29 11:46AM, Omar Polo wrote:
> > > just to be clear, assuming a repository and a worktree with the
> > > following entries:
> > > 
> > > 	README
> > > 	foo/bar.c
> > > 	foo/x.y
> > > 
> > > what do you expect after doing "got rm foo/bar.c foo/x.y" ?
> > > 
> > > With the proposed diff "foo" will be removed as it only affect what
> > > "cd foo && got rm bar.c x.y" does, leaving foo around as a special
> > > case to avoid removing the current working directory.  This is the
> > > quirk I don't like, but it's personal preference.
> > > 
> > > If everything, I'd prefer if we settle on "got rm foo/bar.c foo/x.y"
> > > leaving "foo" around (which is what I assume jamsek intended), it
> > > would be coherent with rm and don't have any special case.  It could
> > > be tricky though, as with "got rm -R foo" I'd expect foo to be deleted
> > > as well.
> > 
> > Yes, you are right. I would expect what you describe: 'foo' remains on
> > disk.
> > 
> > So irrespective of whether `got rm`ing all the files of a subdir from
> > within that directory or from above that directory, the directory will
> > remain.
> > 
> > In that case, I agree with your aversion to the quirk.
> 
> How about this diff, then?

Yes, this is good!

It works as expected for the cases you describe, and zaps the directory
when expected. Regress is happy too.

I think this is a definitive improvement over the current behaviour.
Thank you :)

Perhaps it is unnecessary, and for the sake of consistency probably not
worth it because it is a common pattern we use, but we could keep the
asprintf(&ondisk_status_path, ...) return value and drop the
sda.status_path_len = strlen(ondisk_status_path).

Either way, though, this is ok for me!

>  -----------------------------------------------
>  only delete empty directories appearing in arguments to 'got rm'
>  
>  Make 'got rm' keep empty directories which are not explicitly listed for
>  deletion. Deleting such directories is problematic in several use cases.
>  
>  Avoids deleting the current working directory when the user runs "got rm *"
>  (pointed out by Mikhail), and avoids deletion of an empty directory "foo/"
>  after 'got rm foo/a foo/b' (pointed out by op@).
>  
> diff 6c685612338950f89dc47cd0ef36bcd65fe6404f 606bc3f6e6b57a4f5af6adc58598b113c10f0e7d
> commit - 6c685612338950f89dc47cd0ef36bcd65fe6404f
> commit + 606bc3f6e6b57a4f5af6adc58598b113c10f0e7d
> blob - 622c0ce1ca669ed14ca0d26259483e9beea95863
> blob + 4d8da43fee75c79d47d340a1513c7d62b093fc68
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -4312,6 +4312,8 @@ struct schedule_deletion_args {
>  	int delete_local_mods;
>  	int keep_on_disk;
>  	int ignore_missing_paths;
> +	const char *status_path;
> +	size_t status_path_len;
>  	const char *status_codes;
>  };
>  
> @@ -4410,11 +4412,17 @@ schedule_for_deletion(void *arg, unsigned char status,
>  		root_len = strlen(a->worktree->root_path);
>  		do {
>  			char *parent;
> +
>  			err = got_path_dirname(&parent, ondisk_path);
>  			if (err)
>  				goto done;
>  			free(ondisk_path);
>  			ondisk_path = parent;
> +			if (got_path_cmp(ondisk_path, a->status_path,
> +			    strlen(ondisk_path), a->status_path_len) != 0 &&
> +			    !got_path_is_child(ondisk_path, a->status_path,
> +			    a->status_path_len))
> +				break;
>  			if (rmdir(ondisk_path) == -1) {
>  				if (errno != ENOTEMPTY)
>  					err = got_error_from_errno2("rmdir",
> @@ -4468,8 +4476,19 @@ got_worktree_schedule_delete(struct got_worktree *work
>  	sda.status_codes = status_codes;
>  
>  	TAILQ_FOREACH(pe, paths, entry) {
> +		char *ondisk_status_path;
> +
> +		if (asprintf(&ondisk_status_path, "%s%s%s",
> +		    got_worktree_get_root_path(worktree),
> +			pe->path[0] == '\0' ? "" : "/", pe->path) == -1) {
> +			err = got_error_from_errno("asprintf");
> +			goto done;
> +		}
> +		sda.status_path = ondisk_status_path;
> +		sda.status_path_len = strlen(ondisk_status_path);
>  		err = worktree_status(worktree, pe->path, fileindex, repo,
>  			schedule_for_deletion, &sda, NULL, NULL, 1, 1);
> +		free(ondisk_status_path);
>  		if (err)
>  			break;
>  	}
> @@ -9838,7 +9857,9 @@ got_worktree_patch_schedule_rm(const char *path, struc
>      struct got_worktree *worktree, struct got_fileindex *fileindex,
>      got_worktree_delete_cb progress_cb, void *progress_arg)
>  {
> +	const struct got_error *err;
>  	struct schedule_deletion_args sda;
> +	char *ondisk_status_path;
>  
>  	memset(&sda, 0, sizeof(sda));
>  	sda.worktree = worktree;
> @@ -9850,9 +9871,16 @@ got_worktree_patch_schedule_rm(const char *path, struc
>  	sda.keep_on_disk = 0;
>  	sda.ignore_missing_paths = 0;
>  	sda.status_codes = NULL;
> +	if (asprintf(&ondisk_status_path, "%s/%s",
> +	    got_worktree_get_root_path(worktree), path) == -1)
> +		return got_error_from_errno("asprintf");
> +	sda.status_path = ondisk_status_path;
> +	sda.status_path_len = strlen(ondisk_status_path);
>  
> -	return worktree_status(worktree, path, fileindex, repo,
> +	err = worktree_status(worktree, path, fileindex, repo,
>  	    schedule_for_deletion, &sda, NULL, NULL, 1, 1);
> +	free(ondisk_status_path);
> +	return err;
>  }
>  
>  const struct got_error *
> 

-- 
Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68