"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Tracey Emery <tracey@traceyemery.net>
Subject:
Re: got remove recursion
To:
gameoftrees@openbsd.org
Date:
Thu, 12 Dec 2019 13:37:11 -0700

Download raw body.

Thread
On Thu, Dec 12, 2019 at 07:02:08PM +0100, Stefan Sperling wrote:
> 
> Maybe 'report_unchanged'?

That makes more sense.

> We should try to somehow improve the clarity of this code but I don't
> have a good suggestion right now.

Thanks for combing through the details. It makes much more sense now.
Below is a diff based on the add recursion diff. I think it's greatly
improved. Thoughts?

-- 

Tracey Emery

diff 6db9f7f6b17d77932489a53452856224c8ca1645 /home/basepr1me/Documents/got/got_rmrc
blob - 6b4ec9a6c5c0f9240144d60d19c72d8af64f0b0e
file + TODO
--- TODO
+++ TODO
@@ -11,7 +11,6 @@ lib:
 
 got:
 - 'histedit -c' prompts for log message even if there are no changes to commit
-- recursive removal: got rm -R
 
 tog:
 - implement horizonal scrolling in all views
blob - fa001c2db61fe9127ca82232eb555991984c9dcb
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -645,7 +645,7 @@ With -R, add files even if they match a
 .Cm got status
 ignore pattern.
 .El
-.It Cm remove Ar file-path ...
+.It Cm remove Oo Fl R Oc Ar file-path ...
 Remove versioned files from a work tree and schedule them for deletion
 from the repository in the next commit.
 .Pp
@@ -655,6 +655,13 @@ are as follows:
 .Bl -tag -width Ds
 .It Fl f
 Perform the operation even if a file contains uncommitted modifications.
+.It Fl R
+Permit recursion into directories.
+If this option is not specified,
+.Cm got remove
+will refuse to run if a specified
+.Ar path
+is a directory.
 .El
 .It Cm rm
 Short alias for
blob - 67836e9dac99a1e361945334537edee3a628ccf6
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -4276,16 +4276,17 @@ done:
 __dead static void
 usage_remove(void)
 {
-	fprintf(stderr, "usage: %s remove [-f] file-path ...\n", getprogname());
+	fprintf(stderr, "usage: %s remove [-f] [-R] file-path ...\n",
+	    getprogname());
 	exit(1);
 }
 
 static const struct got_error *
 print_remove_status(void *arg, unsigned char status,
-    unsigned char staged_status, const char *path,
-    struct got_object_id *blob_id, struct got_object_id *staged_blob_id,
-    struct got_object_id *commit_id)
+    unsigned char staged_status, const char *path)
 {
+	while (path[0] == '/')
+		path++;
 	if (status == GOT_STATUS_NONEXISTENT)
 		return NULL;
 	if (status == staged_status && (status == GOT_STATUS_DELETE))
@@ -4303,17 +4304,20 @@ cmd_remove(int argc, char *argv[])
 	char *cwd = NULL;
 	struct got_pathlist_head paths;
 	struct got_pathlist_entry *pe;
-	int ch, delete_local_mods = 0;
+	int ch, delete_local_mods = 0, can_recurse = 0;
 
 	TAILQ_INIT(&paths);
 
-	while ((ch = getopt(argc, argv, "f")) != -1) {
+	while ((ch = getopt(argc, argv, "fR")) != -1) {
 		switch (ch) {
 		case 'f':
 			delete_local_mods = 1;
 			break;
+		case 'R':
+			can_recurse = 1;
+			break;
 		default:
-			usage_add();
+			usage_remove();
 			/* NOTREACHED */
 		}
 	}
@@ -4351,6 +4355,35 @@ cmd_remove(int argc, char *argv[])
 	error = get_worktree_paths_from_argv(&paths, argc, argv, worktree);
 	if (error)
 		goto done;
+
+	if (!can_recurse) {
+		char *ondisk_path;
+		struct stat sb;
+		TAILQ_FOREACH(pe, &paths, entry) {
+			if (asprintf(&ondisk_path, "%s/%s",
+			    got_worktree_get_root_path(worktree),
+			       pe->path) == -1) {
+				error = got_error_from_errno("asprintf");
+				goto done;
+			}
+			if (lstat(ondisk_path, &sb) == -1) {
+				if (errno == ENOENT) {
+					free(ondisk_path);
+					continue;
+				}
+				error = got_error_from_errno2("lstat",
+				    ondisk_path);
+				free(ondisk_path);
+				goto done;
+			}
+			free(ondisk_path);
+			if (S_ISDIR(sb.st_mode)) {
+				error = got_error_msg(GOT_ERR_BAD_PATH,
+				    "removing directories requires -R option");
+				goto done;
+			}
+		}
+	}
 
 	error = got_worktree_schedule_delete(worktree, &paths,
 	    delete_local_mods, print_remove_status, NULL, repo);
blob - 33c5e5f8529ba9bf1fcf626666817f9317db7d02
file + include/got_worktree.h
--- include/got_worktree.h
+++ include/got_worktree.h
@@ -104,6 +104,10 @@ const struct got_error *got_worktree_set_base_commit_i
 typedef const struct got_error *(*got_worktree_checkout_cb)(void *,
     unsigned char, const char *);
 
+/* A callback function which is invoked when a path is removed. */
+typedef const struct got_error *(*got_worktree_delete_cb)(void *,
+    unsigned char, unsigned char, const char *);
+
 /*
  * Attempt to check out files into a work tree from its associated repository
  * and path prefix, and update the work tree's file index accordingly.
@@ -168,7 +172,7 @@ const struct got_error *got_worktree_schedule_add(stru
  */
 const struct got_error *
 got_worktree_schedule_delete(struct got_worktree *,
-    struct got_pathlist_head *, int, got_worktree_status_cb, void *,
+    struct got_pathlist_head *, int, got_worktree_delete_cb, void *,
     struct got_repository *);
 
 /* A callback function which is used to select or reject a patch. */
blob - a1011c791c8036dbbd7c2f29f5cd7ca8ecafb60c
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -2308,12 +2308,13 @@ struct diff_dir_cb_arg {
     void *cancel_arg;
     /* A pathlist containing per-directory pathlists of ignore patterns. */
     struct got_pathlist_head ignores;
+    int report_unchanged;
 };
 
 static const struct got_error *
 report_file_status(struct got_fileindex_entry *ie, const char *abspath,
     got_worktree_status_cb status_cb, void *status_arg,
-    struct got_repository *repo)
+    struct got_repository *repo, int report_unchanged)
 {
 	const struct got_error *err = NULL;
 	unsigned char status = GOT_STATUS_NO_CHANGE;
@@ -2328,7 +2329,7 @@ report_file_status(struct got_fileindex_entry *ie, con
 		return err;
 
 	if (status == GOT_STATUS_NO_CHANGE &&
-	    staged_status == GOT_STATUS_NO_CHANGE)
+	    staged_status == GOT_STATUS_NO_CHANGE && !report_unchanged)
 		return NULL;
 
 	if (got_fileindex_entry_has_blob(ie)) {
@@ -2377,7 +2378,7 @@ status_old_new(void *arg, struct got_fileindex_entry *
 	}
 
 	err = report_file_status(ie, abspath, a->status_cb, a->status_arg,
-	    a->repo);
+	    a->repo, a->report_unchanged);
 	free(abspath);
 	return err;
 }
@@ -2608,7 +2609,7 @@ status_new(void *arg, struct dirent *de, const char *p
 static const struct got_error *
 report_single_file_status(const char *path, const char *ondisk_path,
 struct got_fileindex *fileindex, got_worktree_status_cb status_cb,
-void *status_arg, struct got_repository *repo)
+void *status_arg, struct got_repository *repo, int report_unchanged)
 {
 	struct got_fileindex_entry *ie;
 	struct stat sb;
@@ -2616,7 +2617,7 @@ void *status_arg, struct got_repository *repo)
 	ie = got_fileindex_entry_get(fileindex, path, strlen(path));
 	if (ie)
 		return report_file_status(ie, ondisk_path, status_cb,
-		    status_arg, repo);
+		    status_arg, repo, report_unchanged);
 
 	if (lstat(ondisk_path, &sb) == -1) {
 		if (errno != ENOENT)
@@ -2637,7 +2638,8 @@ static const struct got_error *
 worktree_status(struct got_worktree *worktree, const char *path,
     struct got_fileindex *fileindex, struct got_repository *repo,
     got_worktree_status_cb status_cb, void *status_arg,
-    got_cancel_cb cancel_cb, void *cancel_arg, int no_ignores)
+    got_cancel_cb cancel_cb, void *cancel_arg, int no_ignores,
+    int report_unchanged)
 {
 	const struct got_error *err = NULL;
 	DIR *workdir = NULL;
@@ -2655,7 +2657,8 @@ worktree_status(struct got_worktree *worktree, const c
 			err = got_error_from_errno2("opendir", ondisk_path);
 		else
 			err = report_single_file_status(path, ondisk_path,
-			    fileindex, status_cb, status_arg, repo);
+			    fileindex, status_cb, status_arg, repo,
+			    report_unchanged);
 	} else {
 		fdiff_cb.diff_old_new = status_old_new;
 		fdiff_cb.diff_old = status_old;
@@ -2669,6 +2672,7 @@ worktree_status(struct got_worktree *worktree, const c
 		arg.status_arg = status_arg;
 		arg.cancel_cb = cancel_cb;
 		arg.cancel_arg = cancel_arg;
+		arg.report_unchanged = report_unchanged;
 		TAILQ_INIT(&arg.ignores);
 		if (!no_ignores) {
 			err = add_ignores(&arg.ignores, worktree->root_path,
@@ -2706,7 +2710,7 @@ got_worktree_status(struct got_worktree *worktree,
 
 	TAILQ_FOREACH(pe, paths, entry) {
 		err = worktree_status(worktree, pe->path, fileindex, repo,
-			status_cb, status_arg, cancel_cb, cancel_arg, 0);
+			status_cb, status_arg, cancel_cb, cancel_arg, 0, 0);
 		if (err)
 			break;
 	}
@@ -2871,7 +2875,7 @@ got_worktree_schedule_add(struct got_worktree *worktre
 			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);
+			schedule_addition, &saa, NULL, NULL, no_ignores, 0);
 		free(ondisk_path);
 		if (err)
 			break;
@@ -2889,59 +2893,90 @@ done:
 	return err;
 }
 
+struct schedule_deletion_args {
+	struct got_worktree *worktree;
+	struct got_fileindex *fileindex;
+	got_worktree_delete_cb progress_cb;
+	void *progress_arg;
+	struct got_repository *repo;
+	int delete_local_mods;
+};
+
 static const struct got_error *
-schedule_for_deletion(const char *ondisk_path, struct got_fileindex *fileindex,
-    const char *relpath, int delete_local_mods,
-    got_worktree_status_cb status_cb, void *status_arg,
-    struct got_repository *repo)
+schedule_for_deletion(void *arg, unsigned char status,
+    unsigned char staged_status, const char *relpath,
+    struct got_object_id *blob_id, struct got_object_id *staged_blob_id,
+    struct got_object_id *commit_id)
 {
+	struct schedule_deletion_args *a = arg;
 	const struct got_error *err = NULL;
 	struct got_fileindex_entry *ie = NULL;
-	unsigned char status, staged_status;
 	struct stat sb;
+	char *ondisk_path;
 
-	ie = got_fileindex_entry_get(fileindex, relpath, strlen(relpath));
-	if (ie == NULL)
-		return got_error(GOT_ERR_BAD_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 == NULL) {
+		err = got_error(GOT_ERR_BAD_PATH);
+		goto done;
+	}
+
 	staged_status = get_staged_status(ie);
 	if (staged_status != GOT_STATUS_NO_CHANGE) {
 		if (staged_status == GOT_STATUS_DELETE)
-			return NULL;
-		return got_error_path(relpath, GOT_ERR_FILE_STAGED);
+			goto done;
+		err = got_error_path(relpath, GOT_ERR_FILE_STAGED);
+		goto done;
 	}
 
-	err = get_file_status(&status, &sb, ie, ondisk_path, repo);
+	err = get_file_status(&status, &sb, ie, ondisk_path, a->repo);
 	if (err)
-		return err;
+		goto done;
 
 	if (status != GOT_STATUS_NO_CHANGE) {
 		if (status == GOT_STATUS_DELETE)
-			return NULL;
-		if (status == GOT_STATUS_MODIFY && !delete_local_mods)
-			return got_error_path(relpath, GOT_ERR_FILE_MODIFIED);
+			goto done;
+		if (status == GOT_STATUS_MODIFY && !a->delete_local_mods) {
+			err = got_error_path(relpath, GOT_ERR_FILE_MODIFIED);
+			goto done;
+		}
 		if (status != GOT_STATUS_MODIFY &&
-		    status != GOT_STATUS_MISSING)
-			return got_error_path(relpath, GOT_ERR_FILE_STATUS);
+		    status != GOT_STATUS_MISSING) {
+			err = got_error_path(relpath, GOT_ERR_FILE_STATUS);
+			goto done;
+		}
 	}
 
-	if (status != GOT_STATUS_MISSING && unlink(ondisk_path) != 0)
-		return got_error_from_errno2("unlink", ondisk_path);
+	if (status != GOT_STATUS_MISSING && unlink(ondisk_path) != 0) {
+		err = got_error_from_errno2("unlink", ondisk_path);
+		goto done;
+	}
 
 	got_fileindex_entry_mark_deleted_from_disk(ie);
-	return report_file_status(ie, ondisk_path, status_cb, status_arg, repo);
+done:
+	free(ondisk_path);
+	if (err)
+		return err;
+	if (status == GOT_STATUS_DELETE)
+		return NULL;
+	return (*a->progress_cb)(a->progress_arg, GOT_STATUS_DELETE,
+	    staged_status, relpath);
 }
 
 const struct got_error *
 got_worktree_schedule_delete(struct got_worktree *worktree,
     struct got_pathlist_head *paths, int delete_local_mods,
-    got_worktree_status_cb status_cb, void *status_arg,
+    got_worktree_delete_cb progress_cb, void *progress_arg,
     struct got_repository *repo)
 {
 	struct got_fileindex *fileindex = NULL;
 	char *fileindex_path = NULL;
 	const struct got_error *err = NULL, *sync_err, *unlockerr;
 	struct got_pathlist_entry *pe;
+	struct schedule_deletion_args sda;
 
 	err = lock_worktree(worktree, LOCK_EX);
 	if (err)
@@ -2951,14 +2986,16 @@ got_worktree_schedule_delete(struct got_worktree *work
 	if (err)
 		goto done;
 
+	sda.worktree = worktree;
+	sda.fileindex = fileindex;
+	sda.progress_cb = progress_cb;
+	sda.progress_arg = progress_arg;
+	sda.repo = repo;
+	sda.delete_local_mods = delete_local_mods;
+
 	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");
-		err = schedule_for_deletion(ondisk_path, fileindex, pe->path,
-		    delete_local_mods, status_cb, status_arg, repo);
-		free(ondisk_path);
+		err = worktree_status(worktree, pe->path, fileindex, repo,
+			schedule_for_deletion, &sda, NULL, NULL, 0, 1);
 		if (err)
 			break;
 	}
@@ -3519,7 +3556,7 @@ got_worktree_revert(struct got_worktree *worktree,
 	rfa.repo = repo;
 	TAILQ_FOREACH(pe, paths, entry) {
 		err = worktree_status(worktree, pe->path, fileindex, repo,
-		    revert_file, &rfa, NULL, NULL, 0);
+		    revert_file, &rfa, NULL, NULL, 0, 0);
 		if (err)
 			break;
 	}
@@ -4432,7 +4469,7 @@ got_worktree_commit(struct got_object_id **new_commit_
 	cc_arg.have_staged_files = have_staged_files;
 	TAILQ_FOREACH(pe, paths, entry) {
 		err = worktree_status(worktree, pe->path, fileindex, repo,
-		    collect_commitables, &cc_arg, NULL, NULL, 0);
+		    collect_commitables, &cc_arg, NULL, NULL, 0, 0);
 		if (err)
 			goto done;
 	}
@@ -4990,13 +5027,14 @@ rebase_commit(struct got_object_id **new_commit_id,
 		}
 		TAILQ_FOREACH(pe, merged_paths, entry) {
 			err = worktree_status(worktree, pe->path, fileindex,
-			    repo, collect_commitables, &cc_arg, NULL, NULL, 0);
+			    repo, collect_commitables, &cc_arg, NULL, NULL, 0,
+			    0);
 			if (err)
 				goto done;
 		}
 	} else {
 		err = worktree_status(worktree, "", fileindex, repo,
-		    collect_commitables, &cc_arg, NULL, NULL, 0);
+		    collect_commitables, &cc_arg, NULL, NULL, 0, 0);
 		if (err)
 			goto done;
 	}
@@ -5304,7 +5342,7 @@ got_worktree_rebase_abort(struct got_worktree *worktre
 	rfa.patch_arg = NULL;
 	rfa.repo = repo;
 	err = worktree_status(worktree, "", fileindex, repo,
-	    revert_file, &rfa, NULL, NULL, 0);
+	    revert_file, &rfa, NULL, NULL, 0, 0);
 	if (err)
 		goto sync;
 
@@ -5657,7 +5695,7 @@ got_worktree_histedit_abort(struct got_worktree *workt
 	rfa.patch_arg = NULL;
 	rfa.repo = repo;
 	err = worktree_status(worktree, "", fileindex, repo,
-	    revert_file, &rfa, NULL, NULL, 0);
+	    revert_file, &rfa, NULL, NULL, 0, 0);
 	if (err)
 		goto sync;
 
@@ -6113,7 +6151,7 @@ got_worktree_stage(struct got_worktree *worktree,
 	oka.have_changes = 0;
 	TAILQ_FOREACH(pe, paths, entry) {
 		err = worktree_status(worktree, pe->path, fileindex, repo,
-		    check_stage_ok, &oka, NULL, NULL, 0);
+		    check_stage_ok, &oka, NULL, NULL, 0, 0);
 		if (err)
 			goto done;
 	}
@@ -6132,7 +6170,7 @@ got_worktree_stage(struct got_worktree *worktree,
 	spa.staged_something = 0;
 	TAILQ_FOREACH(pe, paths, entry) {
 		err = worktree_status(worktree, pe->path, fileindex, repo,
-		    stage_path, &spa, NULL, NULL, 0);
+		    stage_path, &spa, NULL, NULL, 0, 0);
 		if (err)
 			goto done;
 	}
@@ -6483,7 +6521,7 @@ got_worktree_unstage(struct got_worktree *worktree,
 	upa.patch_arg = patch_arg;
 	TAILQ_FOREACH(pe, paths, entry) {
 		err = worktree_status(worktree, pe->path, fileindex, repo,
-		    unstage_path, &upa, NULL, NULL, 0);
+		    unstage_path, &upa, NULL, NULL, 0, 0);
 		if (err)
 			goto done;
 	}
blob - 03c8d8fc147c4298c5425973fe32318e931e8992
file + regress/cmdline/rm.sh
--- regress/cmdline/rm.sh
+++ regress/cmdline/rm.sh
@@ -193,7 +193,57 @@ function test_rm_and_add_elsewhere {
 	test_done "$testroot" "$ret"
 }
 
+function test_rm_directory {
+	local testroot=`test_init rm_directory`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got rm . > $testroot/stdout 2> $testroot/stderr)
+	ret="$?"
+	echo "got: removing directories requires -R option" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		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
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got rm -R . > $testroot/stdout)
+
+	echo 'D  alpha' > $testroot/stdout.expected
+	echo 'D  beta' >> $testroot/stdout.expected
+	echo 'D  epsilon/zeta' >> $testroot/stdout.expected
+	echo 'D  gamma/delta' >> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	test_done "$testroot" "$ret"
+}
+
 run_test test_rm_basic
 run_test test_rm_with_local_mods
 run_test test_double_rm
 run_test test_rm_and_add_elsewhere
+run_test test_rm_directory