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

Commit Message

Derrick Stolee Aug. 17, 2021, 1:23 p.m. UTC

From: Derrick Stolee <[e-mail]>

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.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 Documentation/git-sparse-checkout.txt |  8 +++
 builtin/sparse-checkout.c             | 95 +++++++++++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh    | 59 +++++++++++++++++
 3 files changed, 162 insertions(+)

Comments

Johannes Schindelin Aug. 19, 2021, 8:48 a.m. UTC | #1

Hi Stolee,

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

> From: Derrick Stolee <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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
>

Derrick Stolee Aug. 20, 2021, 3:49 p.m. UTC | #2

On 8/19/2021 4:48 AM, Johannes Schindelin wrote:
> On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:
> 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.

Right. I should have modified the message from my earlier version when
the issues with untracked files came up.
 
> 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.

Keep in mind that 'git switch' does not change the sparse-checkout cone,
and the activity being described is something that would happen within
'git sparse-checkout set' or 'git sparse-checkout reapply'.

> 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.

Outside of "it's harder to write that feature", perhaps I could convince
you that it is better to do nothing in the presence of untracked files.

If a user has an untracked file within a directory that is leaving the
sparse cone, then that means they were doing something in that space and
perhaps has unfinished work. By leaving the files on-disk, they have an
opportunity to revert the change to the sparse-checkout cone and continue
their work interrupted. This includes keeping things like build artifacts
(that are ignored) so they can incrementally build from that position.

The general thought I have here is: having untracked, not-ignored files
in a directory that is leaving the sparse-checkout cone is an unexpected
behavior, so we should do as little as possible in that scenario.

It also makes it more clear to the user what happened: "You had untracked
files here, so we left them alone" is easier to understand than "You had
untracked files here, so we deleted the ones that were ignored and kept
the rest."

> 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.

Of course, you already have a retort to my claim that "simpler is better",
but I'll just focus on the point that "simpler for the user to understand"
is a different point than "simpler to implement".

> 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).

You are right that there are a lot of users out there, and they have
very different habits. Those habits are shaped by the way Git has worked
for a long time, so adding a way to go back to the previous behavior
would be good to have.

Thanks,
-Stolee

Elijah Newren Aug. 20, 2021, 3:56 p.m. UTC | #3

On Thu, Aug 19, 2021 at 1:48 AM Johannes Schindelin
<[e-mail]> wrote:
>
> Hi Stolee,
>
> On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:
>
> > From: Derrick Stolee <[e-mail]>
> >
> > 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.

I share this concern.

> 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.

The ignored files are already enumerated by the fill_directory call;
you just need to iterate over the dir->ignored_nr entries in
dir->ignored.

> 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).

I do the same thing, and I know other users that do as well...but I
don't put such files in directories that are irrelevant to me.  I
create cruft files near other files that I'm working on, or in a
special directory of its own, but not under some directory that is
irrelevant to the areas I'm working on.

For reference, we implemented something like this in our `sparsify`
wrapper we have internally, where 'git clean -fdX <all sparse
directories>` is executed whenever folks sparsify.  (We have our own
tool and don't have users use sparse-checkout directly, because our
tool computes dependencies to determine which directories are needed.)
 I was really hesitant to add that cleaning behavior by default, and
just made it an option.  My colleagues tired of all the bug reports
about left-around directories and made it the default, waiting to hear
complaints.  We never got one.  It's been over a year.

> 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).

That seems fine, but I suspect you're projecting the same concerns I
had, and it turns out much less useful than you'd expect (perhaps even
totally useless).

Elijah Newren Aug. 20, 2021, 4:15 p.m. UTC | #4

On Fri, Aug 20, 2021 at 8:50 AM Derrick Stolee <[e-mail]> wrote:
>
> On 8/19/2021 4:48 AM, Johannes Schindelin wrote:
> > On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:
> > 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.
>
> Right. I should have modified the message from my earlier version when
> the issues with untracked files came up.

:+1:

> > 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.
>
> Keep in mind that 'git switch' does not change the sparse-checkout cone,
> and the activity being described is something that would happen within
> 'git sparse-checkout set' or 'git sparse-checkout reapply'.

Good points.

> > 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.
>
> Outside of "it's harder to write that feature"

I don't think it is; see my other email.

> perhaps I could convince
> you that it is better to do nothing in the presence of untracked files.
>
> If a user has an untracked file within a directory that is leaving the
> sparse cone, then that means they were doing something in that space and
> perhaps has unfinished work. By leaving the files on-disk, they have an
> opportunity to revert the change to the sparse-checkout cone and continue
> their work interrupted. This includes keeping things like build artifacts
> (that are ignored) so they can incrementally build from that position.
>
> The general thought I have here is: having untracked, not-ignored files
> in a directory that is leaving the sparse-checkout cone is an unexpected
> behavior, so we should do as little as possible in that scenario.
>
> It also makes it more clear to the user what happened: "You had untracked
> files here, so we left them alone" is easier to understand than "You had
> untracked files here, so we deleted the ones that were ignored and kept
> the rest."

This explanation seems reasonable, but it certainly belongs in the
commit message.

However, doesn't it defeat the point of your removing the ignored
files?  Not only do you have directories full of files, but you won't
be able to have sparse directory entries due to the need to have
.gitignore files from underneath them.

Perhaps this is okay because we expect this to be an unusual case.
But, if so, do we want a more verbose warning (not with every
directory that fails to be deleted, but just at the end if there were
any), suggesting to the user that they clean up the relevant
directories and do a sparse-checkout reapply?  And, ultimately, once
the directory is gone, is that good enough to allow us to keep our
indexes sparse and fast, or are we looking at a huge amount of effort
spent on .gitignore files underneath sparse directories?

> > 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.
>
> Of course, you already have a retort to my claim that "simpler is better",
> but I'll just focus on the point that "simpler for the user to understand"
> is a different point than "simpler to implement".

I'm confused now.  Which behavior are you arguing is simpler for the
user to understand?

Johannes Schindelin Aug. 23, 2021, 8 p.m. UTC | #5

Hi Elijah,

On Fri, 20 Aug 2021, Elijah Newren wrote:

> On Thu, Aug 19, 2021 at 1:48 AM Johannes Schindelin
> <[e-mail]> wrote:
> >
> > 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).
>
> I do the same thing, and I know other users that do as well...but I
> don't put such files in directories that are irrelevant to me.  I create
> cruft files near other files that I'm working on, or in a special
> directory of its own, but not under some directory that is irrelevant to
> the areas I'm working on.
>
> For reference, we implemented something like this in our `sparsify`
> wrapper we have internally, where 'git clean -fdX <all sparse
> directories>` is executed whenever folks sparsify.  (We have our own
> tool and don't have users use sparse-checkout directly, because our tool
> computes dependencies to determine which directories are needed.) I was
> really hesitant to add that cleaning behavior by default, and just made
> it an option.  My colleagues tired of all the bug reports about
> left-around directories and made it the default, waiting to hear
> complaints.  We never got one.  It's been over a year.

It is really nice to hear that you have evidence from users using this in
practice. I find that very convincing, and it weighs much more than all of
my (very theoretical) considerations.

Thank you,
Dscho

Patch

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