Re: [PATCH v3 8/8] sparse-checkout: clear tracked sparse dirs

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Stolee,

On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> When changing the scope of a sparse-checkout using cone mode, we might
> have some tracked directories go out of scope. The current logic removes
> the tracked files from within those directories, but leaves the ignored
> files within those directories. This is a bit unexpected to users who
> have given input to Git saying they don't need those directories
> anymore.
>
> This is something that is new to the cone mode pattern type: the user
> has explicitly said "I want these directories and _not_ those
> directories." The typical sparse-checkout patterns more generally apply
> to "I want files with with these patterns" so it is natural to leave
> ignored files as they are. This focus on directories in cone mode
> provides us an opportunity to change the behavior.
>
> Leaving these ignored files in the sparse directories makes it
> impossible to gain performance benefits in the sparse index. When we
> track into these directories, we need to know if the files are ignored
> or not, which might depend on the _tracked_ .gitignore file(s) within
> the sparse directory. This depends on the indexed version of the file,
> so the sparse directory must be expanded.
>
> By deleting the sparse directories when changing scope (or running 'git
> sparse-checkout reapply') we regain these performance benefits as if the
> repository was in a clean state.
>
> Since these ignored files are frequently build output or helper files
> from IDEs, the users should not need the files now that the tracked
> files are removed. If the tracked files reappear, then they will have
> newer timestamps than the build artifacts, so the artifacts will need to
> be regenerated anyway.
>
> Use the sparse-index as a data structure in order to find the sparse
> directories that can be safely deleted. Re-expand the index to a full
> one if it was full before.

This description makes sense, and is easy to explain.

It does not cover the case where untracked files are found and the
directory is _not_ removed as a consequence, though. I would like to ask
to add this to the commit message, because it is kind of important.

The implementation of this behavior looks fine to me.

About this behavior itself: in my experience, the more tricky a feature is
to explain, the more likely the design should have been adjusted in the
first place. And I find myself struggling a little bit to explain what
files `git switch` touches in cone mode in addition to tracked files.

So I wonder whether an easier-to-explain behavior would be the following:
ignored files in directories that fell out of the sparse-checkout cone are
deleted. (Even if there are untracked, unignored files in the same
directory tree.)

This is different than what this patch implements: we would now have to
delete the ignored and out-of-cone files _also_ when there are untracked
files in the same directory, i.e. we could no longer use the sweet
`remove_dir_recursively()` call. Therefore, the implementation of what I
suggested would be much more complicated: you would have to enumerate the
ignored files and remove them individually.

Having said that, even after mulling over this behavior and sleeping over
it, I am unsure what the best way forward would be. Just because it is
easy to explain does not make it right.

It is tricky to decide mostly because "ignored" files are definitely not
always build output. Apart from VIM's temporary files, users like me
frequently write other files and/or directories that we simply do not want
to see tracked in Git. For example, I often test things in an `a1.c` file
that -- for convenience -- lives in the current worktree. Obviously I
don't want Git to track it, but I also don't want it to be deleted, so I
often add corresponding lines to `.git/info/exclude`. Likewise, I
sometimes download additional information related to what I am
implementing, and that also lives in the current worktree (but then, I
usually am too lazy to add an entry to `.git/info/exclude` in those
cases).

Now, I don't want to over-index on my own habits. There are so many users
out there, and most of them have different preferences from mine.

Which leaves me to wonder whether we need at least a flag to turn this
behavior on and off? Something like
`core.ignoredFilesInSparseConesArePrecious = true` (obviously with a
better, shorter name).

Ciao,
Dscho

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-sparse-checkout.txt |  8 +++
>  builtin/sparse-checkout.c             | 95 +++++++++++++++++++++++++++
>  t/t1091-sparse-checkout-builtin.sh    | 59 +++++++++++++++++
>  3 files changed, 162 insertions(+)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index fdcf43f87cb..f9022b9d555 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -210,6 +210,14 @@ case-insensitive check. This corrects for case mismatched filenames in the
>  'git sparse-checkout set' command to reflect the expected cone in the working
>  directory.
>
> +The cone mode sparse-checkout patterns will also remove ignored files that
> +are not within the sparse-checkout definition. This is important behavior
> +to preserve the performance of the sparse index, but also matches that
> +cone mode patterns care about directories, not files. If there exist files
> +that are untracked and not ignored, then Git will not delete files within
> +that directory other than the tracked files that are now out of scope.
> +These files should be removed manually to ensure Git can behave optimally.
> +
>
>  SUBMODULES
>  ----------
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 8ba9f13787b..b06c8f885ac 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -100,6 +100,99 @@ static int sparse_checkout_list(int argc, const char **argv)
>  	return 0;
>  }
>
> +static void clean_tracked_sparse_directories(struct repository *r)
> +{
> +	int i, was_full = 0;
> +	struct strbuf path = STRBUF_INIT;
> +	size_t pathlen;
> +	struct string_list_item *item;
> +	struct string_list sparse_dirs = STRING_LIST_INIT_DUP;
> +
> +	/*
> +	 * If we are not using cone mode patterns, then we cannot
> +	 * delete directories outside of the sparse cone.
> +	 */
> +	if (!r || !r->index || !r->worktree)
> +		return;
> +	init_sparse_checkout_patterns(r->index);
> +	if (!r->index->sparse_checkout_patterns ||
> +	    !r->index->sparse_checkout_patterns->use_cone_patterns)
> +		return;
> +
> +	/*
> +	 * Use the sparse index as a data structure to assist finding
> +	 * directories that are safe to delete. This conversion to a
> +	 * sparse index will not delete directories that contain
> +	 * conflicted entries or submodules.
> +	 */
> +	if (!r->index->sparse_index) {
> +		/*
> +		 * If something, such as a merge conflict or other concern,
> +		 * prevents us from converting to a sparse index, then do
> +		 * not try deleting files.
> +		 */
> +		if (convert_to_sparse(r->index, SPARSE_INDEX_IGNORE_CONFIG))
> +			return;
> +		was_full = 1;
> +	}
> +
> +	strbuf_addstr(&path, r->worktree);
> +	strbuf_complete(&path, '/');
> +	pathlen = path.len;
> +
> +	/*
> +	 * Collect directories that have gone out of scope but also
> +	 * exist on disk, so there is some work to be done. We need to
> +	 * store the entries in a list before exploring, since that might
> +	 * expand the sparse-index again.
> +	 */
> +	for (i = 0; i < r->index->cache_nr; i++) {
> +		struct cache_entry *ce = r->index->cache[i];
> +
> +		if (S_ISSPARSEDIR(ce->ce_mode) &&
> +		    repo_file_exists(r, ce->name))
> +			string_list_append(&sparse_dirs, ce->name);
> +	}
> +
> +	for_each_string_list_item(item, &sparse_dirs) {
> +		struct dir_struct dir = DIR_INIT;
> +		struct pathspec p = { 0 };
> +		struct strvec s = STRVEC_INIT;
> +
> +		strbuf_setlen(&path, pathlen);
> +		strbuf_addstr(&path, item->string);
> +
> +		dir.flags |= DIR_SHOW_IGNORED_TOO;
> +
> +		setup_standard_excludes(&dir);
> +		strvec_push(&s, path.buf);
> +
> +		parse_pathspec(&p, PATHSPEC_GLOB, 0, NULL, s.v);
> +		fill_directory(&dir, r->index, &p);
> +
> +		if (dir.nr) {
> +			warning(_("directory '%s' contains untracked files,"
> +				  " but is not in the sparse-checkout cone"),
> +				item->string);
> +		} else if (remove_dir_recursively(&path, 0)) {
> +			/*
> +			 * Removal is "best effort". If something blocks
> +			 * the deletion, then continue with a warning.
> +			 */
> +			warning(_("failed to remove directory '%s'"),
> +				item->string);
> +		}
> +
> +		dir_clear(&dir);
> +	}
> +
> +	string_list_clear(&sparse_dirs, 0);
> +	strbuf_release(&path);
> +
> +	if (was_full)
> +		ensure_full_index(r->index);
> +}
> +
>  static int update_working_directory(struct pattern_list *pl)
>  {
>  	enum update_sparsity_result result;
> @@ -141,6 +234,8 @@ static int update_working_directory(struct pattern_list *pl)
>  	else
>  		rollback_lock_file(&lock_file);
>
> +	clean_tracked_sparse_directories(r);
> +
>  	r->index->sparse_checkout_patterns = NULL;
>  	return result;
>  }
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 38fc8340f5c..71236981e64 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -642,4 +642,63 @@ test_expect_success MINGW 'cone mode replaces backslashes with slashes' '
>  	check_files repo/deep a deeper1
>  '
>
> +test_expect_success 'cone mode clears ignored subdirectories' '
> +	rm repo/.git/info/sparse-checkout &&
> +
> +	git -C repo sparse-checkout init --cone &&
> +	git -C repo sparse-checkout set deep/deeper1 &&
> +
> +	cat >repo/.gitignore <<-\EOF &&
> +	obj/
> +	*.o
> +	EOF
> +
> +	git -C repo add .gitignore &&
> +	git -C repo commit -m ".gitignore" &&
> +
> +	mkdir -p repo/obj repo/folder1/obj repo/deep/deeper2/obj &&
> +	for file in folder1/obj/a obj/a folder1/file.o folder1.o \
> +		    deep/deeper2/obj/a deep/deeper2/file.o file.o
> +	do
> +		echo ignored >repo/$file || return 1
> +	done &&
> +
> +	git -C repo status --porcelain=v2 >out &&
> +	test_must_be_empty out &&
> +
> +	git -C repo sparse-checkout reapply &&
> +	test_path_is_missing repo/folder1 &&
> +	test_path_is_missing repo/deep/deeper2 &&
> +	test_path_is_dir repo/obj &&
> +	test_path_is_file repo/file.o &&
> +
> +	git -C repo status --porcelain=v2 >out &&
> +	test_must_be_empty out &&
> +
> +	git -C repo sparse-checkout set deep/deeper2 &&
> +	test_path_is_missing repo/deep/deeper1 &&
> +	test_path_is_dir repo/deep/deeper2 &&
> +	test_path_is_dir repo/obj &&
> +	test_path_is_file repo/file.o &&
> +
> +	>repo/deep/deeper2/ignored.o &&
> +	>repo/deep/deeper2/untracked &&
> +
> +	# When an untracked file is in the way, all untracked files
> +	# (even ignored files) are preserved.
> +	git -C repo sparse-checkout set folder1 2>err &&
> +	grep "contains untracked files" err &&
> +	test_path_is_file repo/deep/deeper2/ignored.o &&
> +	test_path_is_file repo/deep/deeper2/untracked &&
> +
> +	# The rest of the cone matches expectation
> +	test_path_is_missing repo/deep/deeper1 &&
> +	test_path_is_dir repo/obj &&
> +	test_path_is_file repo/file.o &&
> +
> +	git -C repo status --porcelain=v2 >out &&
> +	echo "? deep/deeper2/untracked" >expect &&
> +	test_cmp expect out
> +'
> +
>  test_done
> --
> gitgitgadget
>