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?

-----------------------------------------------
 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 *