From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got: rm * removes current directory
To: Omar Polo <op@omarpolo.com>
Cc: Mark Jamsek <mark@jamsek.com>, Mikhail <mp39590@gmail.com>, gameoftrees@openbsd.org
Date: Mon, 29 May 2023 16:56:25 +0200

On Mon, May 29, 2023 at 04:43:32PM +0200, Omar Polo wrote:
> On 2023/05/29 13:37:57 +0200, Stefan Sperling <stsp@stsp.name> 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?
> 
> shorter than what I'd expected! :)
> 
> ok op@, thank you a lot for this.

Great. I am glad we found a solution everyone is happy with.

> One minor style nit below, and I agree with jamsek re asprintf vs
> strlen fwiw.

Like this?

diff /home/stsp/src/got
commit - 83769d30329a2744571b359ad7c849db5249ca79
path + /home/stsp/src/got
blob - 9d45bcc89fba03349491b41c260b4fc0c9606531
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -4477,15 +4477,17 @@ got_worktree_schedule_delete(struct got_worktree *work
 
 	TAILQ_FOREACH(pe, paths, entry) {
 		char *ondisk_status_path;
+		size_t len;
 
-		if (asprintf(&ondisk_status_path, "%s%s%s",
+		len = asprintf(&ondisk_status_path, "%s%s%s",
 		    got_worktree_get_root_path(worktree),
-		    pe->path[0] == '\0' ? "" : "/", pe->path) == -1) {
+		    pe->path[0] == '\0' ? "" : "/", pe->path);
+		if (len == -1) {
 			err = got_error_from_errno("asprintf");
 			goto done;
 		}
 		sda.status_path = ondisk_status_path;
-		sda.status_path_len = strlen(ondisk_status_path);
+		sda.status_path_len = len;
 		err = worktree_status(worktree, pe->path, fileindex, repo,
 			schedule_for_deletion, &sda, NULL, NULL, 1, 1);
 		free(ondisk_status_path);

> > @@ -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) {
> 
> can you please fix the indentation here before committing?

Thanks, I did that.