From: Mark Jamsek Subject: Re: got: rm * removes current directory To: Omar Polo , Mikhail , gameoftrees@openbsd.org Date: Mon, 29 May 2023 22:41:35 +1000 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 GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68