From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got remove recursion
To: Tracey Emery <tracey@traceyemery.net>, gameoftrees@openbsd.org
Date: Thu, 12 Dec 2019 19:38:37 +0100

On Thu, Dec 12, 2019 at 07:02:08PM +0100, Stefan Sperling wrote:
> Technically, get_file_status() should not even be called with a path
> that does not correspond to a file index entry (which by definition
> contains files only, not directories).
> 
> We should try to somehow improve the clarity of this code but I don't
> have a good suggestion right now.

A similar situation exists in your add -R patch and I failed  to spot it.
The addition code is also calling got_fileindex_entry_get() with one path
and get_file_status() with the obtained file index entry but with an
unrelated path.

Here is how I would solve that case. The status walk already takes care of
figuring out what is a file and what is a directory. Which means that
schedule_addition() will only be called on files in the first place.
So the 'ondisk_path' in struct schedule_addition_args is simply not needed :)
Perhaps this helps with figuring out how to improve your deletion patch?

OK for this diff?

diff --git a/lib/worktree.c b/lib/worktree.c
index a1011c7..96c1281 100644
--- a/lib/worktree.c
+++ b/lib/worktree.c
@@ -2776,7 +2776,6 @@ done:
 struct schedule_addition_args {
 	struct got_worktree *worktree;
 	struct got_fileindex *fileindex;
-	const char *ondisk_path;
 	got_worktree_checkout_cb progress_cb;
 	void *progress_arg;
 	struct got_repository *repo;
@@ -2791,37 +2790,32 @@ schedule_addition(void *arg, unsigned char status, unsigned char staged_status,
 	const struct got_error *err = NULL;
 	struct got_fileindex_entry *ie;
 	struct stat sb;
-	char *path = NULL;
+	char *ondisk_path;
+
+	if (asprintf(&ondisk_path, "%s/%s", a->worktree->root_path,
+	    relpath) == -1)
+		return got_error_from_errno("asprintf");
 
 	ie = got_fileindex_entry_get(a->fileindex, relpath, strlen(relpath));
 	if (ie) {
-		err = get_file_status(&status, &sb, ie, a->ondisk_path,
+		err = get_file_status(&status, &sb, ie, ondisk_path,
 		    a->repo);
 		if (err)
-			return err;
+			goto done;
 		/* Re-adding an existing entry is a no-op. */
 		if (status == GOT_STATUS_ADD)
-			return NULL;
-		return got_error_path(relpath, GOT_ERR_FILE_STATUS);
+			goto done;
+		err = got_error_path(relpath, GOT_ERR_FILE_STATUS);
+		if (err)
+			goto done;
 	}
 
-	if (status == GOT_STATUS_UNVERSIONED) {
-		if (lstat(a->ondisk_path, &sb) != 0) {
-			err = got_error_from_errno2("lstat", a->ondisk_path);
-			return err;
-		}
-		if (S_ISDIR(sb.st_mode)) {
-			if (asprintf(&path, "%s/%s",
-			    got_worktree_get_root_path(a->worktree),
-			    relpath) == -1) {
-				err = got_error_from_errno("asprintf");
-				return err;
-			}
-		} else
-			path = strdup(a->ondisk_path);
+	if (status != GOT_STATUS_UNVERSIONED) {
+		err = got_error_path(ondisk_path, GOT_ERR_FILE_STATUS);
+		goto done;
 	}
 
-	err = got_fileindex_entry_alloc(&ie, path, relpath, NULL, NULL);
+	err = got_fileindex_entry_alloc(&ie, ondisk_path, relpath, NULL, NULL);
 	if (err)
 		goto done;
 
@@ -2830,12 +2824,13 @@ schedule_addition(void *arg, unsigned char status, unsigned char staged_status,
 		got_fileindex_entry_free(ie);
 		goto done;
 	}
-
-	free(path);
-	return (*a->progress_cb)(a->progress_arg, GOT_STATUS_ADD, relpath);
 done:
-	free(path);
-	return err;
+	free(ondisk_path);
+	if (err)
+		return err;
+	if (status == GOT_STATUS_ADD)
+		return NULL;
+	return (*a->progress_cb)(a->progress_arg, GOT_STATUS_ADD, relpath);
 }
 
 const struct got_error *
@@ -2865,14 +2860,8 @@ got_worktree_schedule_add(struct got_worktree *worktree,
 	saa.repo = repo;
 
 	TAILQ_FOREACH(pe, paths, entry) {
-		char *ondisk_path;
-		if (asprintf(&ondisk_path, "%s/%s", worktree->root_path,
-		    pe->path) == -1)
-			return got_error_from_errno("asprintf");
-		saa.ondisk_path = ondisk_path;
 		err = worktree_status(worktree, pe->path, fileindex, repo,
 			schedule_addition, &saa, NULL, NULL, no_ignores);
-		free(ondisk_path);
 		if (err)
 			break;
 	}
diff --git a/regress/cmdline/add.sh b/regress/cmdline/add.sh
index 5964e3a..c77f4d7 100755
--- a/regress/cmdline/add.sh
+++ b/regress/cmdline/add.sh
@@ -52,7 +52,7 @@ function test_double_add {
 	echo "new file" > $testroot/wt/foo
 	(cd $testroot/wt && got add foo > /dev/null)
 
-	(cd $testroot/wt && got add foo)
+	(cd $testroot/wt && got add foo > $testroot/stdout)
 	ret="$?"
 	if [ "$ret" != "0" ]; then
 		echo "got add failed unexpectedly" >&2
@@ -60,6 +60,12 @@ function test_double_add {
 		return 1
 	fi
 
+	echo -n > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
 	test_done "$testroot" "$ret"
 }