On Fri, Dec 13, 2019 at 08:06:19AM -0700, Tracey Emery wrote:
> Hello,
> 
> Does adding a new argument of '-k Keep removed files from work tree on
> disk.' to 'got remove' make sense, or is it just adding useless bloat?

I am not opposed to this idea.

> I don't think git has this option and don't know about other versioning
> systems that might set a precedent for its addition.

'svn rm' has this option as well and calls it --keep-local.
It is described in 'svn help rm' like this:

  1. Each item specified by a PATH is scheduled for deletion upon
    the next commit.  Files, and directories that have not been
    committed, are immediately removed from the working copy
    unless the --keep-local option is given.
    PATHs that are, or contain, unversioned or modified items will
    not be removed unless the --force or --keep-local option is given.

(The careful wording about directories there is due to the fact that SVN
supports versioning of directories, which Git's model cannot represent.)

So SVN's --force is like our -f, and -k would be like --keep-local.
Seems fine to me.

> Thoughts?

Comments inline.
Please take note of my comment about the test case in particular.

> diff f2a9dc41d851ff2d575b08c2766583ff11cdd7af /home/basepr1me/Documents/got/got/got_rmkp
> blob - 42403d111ef601d3428f3750208becb5665fcfc0
> 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 Oo Fl R Oc Ar file-path ...
> +.It Cm remove Oo Fl k Oc Oo Fl R Oc Ar file-path ...

This line neglects to mention -f. Could we fix that while here?

>  Remove versioned files from a work tree and schedule them for deletion
>  from the repository in the next commit.
>  .Pp
> @@ -655,6 +655,8 @@ are as follows:
>  .Bl -tag -width Ds
>  .It Fl f
>  Perform the operation even if a file contains uncommitted modifications.
> +.It Fl k
> +Keep removed files from work tree on disk.

I would suggest to describe the effect of -k more concisely like this:

  "Keep affected files on disk."

Perhaps also explain which status codes 'got status' will show for
affected files, before and after they are committed?

>  .It Fl R
>  Permit recursion into directories.
>  If this option is not specified,
> blob - 5b73c98b079e00ed10051699743ef328c40800d9
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -4276,7 +4276,7 @@ done:
>  __dead static void
>  usage_remove(void)
>  {
> -	fprintf(stderr, "usage: %s remove [-f] [-R] file-path ...\n",
> +	fprintf(stderr, "usage: %s remove [-f]  [-k] [-R] file-path ...\n",
>  	    getprogname());
>  	exit(1);
>  }
> @@ -4304,15 +4304,18 @@ 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, can_recurse = 0;
> +	int ch, delete_local_mods = 0, can_recurse = 0, no_unlink = 0;

Perhaps rename 'no_unlink' to 'keep_on_disk' for consistency?

>  	TAILQ_INIT(&paths);
>  
> -	while ((ch = getopt(argc, argv, "fR")) != -1) {
> +	while ((ch = getopt(argc, argv, "fkR")) != -1) {
>  		switch (ch) {
>  		case 'f':
>  			delete_local_mods = 1;
>  			break;
> +		case 'k':
> +			no_unlink = 1;
> +			break;
>  		case 'R':
>  			can_recurse = 1;
>  			break;
> @@ -4386,7 +4389,7 @@ cmd_remove(int argc, char *argv[])
>  	}
>  
>  	error = got_worktree_schedule_delete(worktree, &paths,
> -	    delete_local_mods, print_remove_status, NULL, repo);
> +	    delete_local_mods, print_remove_status, NULL, repo, no_unlink);
>  	if (error)
>  		goto done;
>  done:
> blob - 3aaddd7755f9fd3f50ff866dbf1c0774053f89d3
> file + include/got_worktree.h
> --- include/got_worktree.h
> +++ include/got_worktree.h
> @@ -173,7 +173,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_delete_cb, void *,
> -    struct got_repository *);
> +    struct got_repository *, int);
>  
>  /* A callback function which is used to select or reject a patch. */
>  typedef const struct got_error *(*got_worktree_patch_cb)(int *, void *,
> blob - fa31d1e75f6e61a0d42966b946ed7caccc110980
> file + lib/worktree.c
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -2889,6 +2889,7 @@ struct schedule_deletion_args {
>  	void *progress_arg;
>  	struct got_repository *repo;
>  	int delete_local_mods;
> +	int no_unlink;
>  };
>  
>  static const struct got_error *
> @@ -2936,11 +2937,14 @@ schedule_for_deletion(void *arg, unsigned char status,
>  		}
>  	}
>  
> +	if (a->no_unlink)
> +		goto mark;
> +
>  	if (status != GOT_STATUS_MISSING && unlink(ondisk_path) != 0) {
>  		err = got_error_from_errno2("unlink", ondisk_path);
>  		goto done;
>  	}

Instead of adding 'goto mark' here, this new check could simply become
part of the existing if-statement:

 	if (!keep_on_disk && status != GOT_STATUS_MISSING &&
	    unlink(ondisk_path) != 0) {

> -
> +mark:
>  	got_fileindex_entry_mark_deleted_from_disk(ie);
>  done:
>  	free(ondisk_path);
> @@ -2956,7 +2960,7 @@ const struct got_error *
>  got_worktree_schedule_delete(struct got_worktree *worktree,
>      struct got_pathlist_head *paths, int delete_local_mods,
>      got_worktree_delete_cb progress_cb, void *progress_arg,
> -    struct got_repository *repo)
> +    struct got_repository *repo, int no_unlink)
>  {
>  	struct got_fileindex *fileindex = NULL;
>  	char *fileindex_path = NULL;
> @@ -2978,6 +2982,7 @@ got_worktree_schedule_delete(struct got_worktree *work
>  	sda.progress_arg = progress_arg;
>  	sda.repo = repo;
>  	sda.delete_local_mods = delete_local_mods;
> +	sda.no_unlink = no_unlink;
>  
>  	TAILQ_FOREACH(pe, paths, entry) {
>  		err = worktree_status(worktree, pe->path, fileindex, repo,
> blob - 551feb1fb11346676d2f161b062a605bde93e6d9
> file + regress/cmdline/rm.sh
> --- regress/cmdline/rm.sh
> +++ regress/cmdline/rm.sh
> @@ -242,8 +242,74 @@ function test_rm_directory {
>  	test_done "$testroot" "$ret"
>  }
>  
> +function test_rm_directory_keep_files {
> +	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 -k -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

What does 'got status' show at this point, before deletions are committed?
That would be a rather important thing to check and verify.

> +
> +	(cd $testroot/wt && got commit -m "keep" > /dev/null)
> +	(cd $testroot/wt && got st . > $testroot/stdout)
> +
> +	echo '?  alpha' > $testroot/stdout.expected
> +	echo '?  beta' >> $testroot/stdout.expected
> +	echo '?  epsilon/zeta' >> $testroot/stdout.expected
> +	echo '?  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
> +run_test test_rm_directory_keep_files
> 
>