Sparse index: delete ignored files outside sparse cone

[derrickstolee]

We launched an experimental release [1] of the sparse-index feature to our
internal users. We immediately discovered a problem due to the isolated
way in which we tested the sparse index: we were never building the
project and changing our sparse-checkout definition.

[1] https://github.com/microsoft/git/releases/tag/v2.32.0.vfs.0.102.exp

Users who ran a build in one project and then moved to another still had
build artifacts in their worktree that lived inside the old directories.
Since the files are marked by the .gitignore patterns, these files were
not removed by the 'git sparse-checkout set' command. However, they make
the sparse-index unusable because every 'git status' command needs to
expand the sparse-directory entries in order to see if the files are
tracked or not. This made the first experimental release actually slower
for all users because of this cost.

The solution we shipped to these customers was to change the way our fork
handles these ignored files. Specifically, instead of Git completely
ignoring the files, we changed Git to understand that with cone-mode
sparse-checkout patterns, the users is asking for entire directories to be
removed from the worktree. The link [1] included earlier has this change.

I believe that this is a reasonable expectation, though I recognize that
it might look like breaking the expectations of how .gitignore files work.

Since feedback demonstrated that this is a desired behavior, v2 includes
this behavior for all "cone mode" repositories.

I'm interested in the community's thoughts about this change, as it seems
like one that we should make carefully and intentionally.

While the rewrite of the t7519 test seems unrelated, it is required to
avoid a test failure with this change that deletes files outside of the cone.
By moving the test into repositories not at $TRASH_DIRECTORY, we
gain more control over the repository structure.

Updates in V5

• Updated the locality of a cache_entry pointer.

• Rephrased a comment.

• Removed the patch adding a config option.

• I tried, but failed, to create a scenario where the call to cache_tree_update() causes a test to fail. I still think this is valuable as defensive programming for the reasons mentioned in the patch, which is why I didn't remove them here.

Updates in V4

• Fixed an issue with the split index.

• The helper methods are used more consistently.

• The helper method path_in_cone_mode_sparse_checkout() is introduced.

• Commit messages are edited for clarity.

• A new config option is added to disable the behavior being added in this series.

• I split the commit that involves cache_tree_update(). I have not yet succeeded in creating tests to demonstrate why this is required outside of needing it in the Scalar functional tests, which includes a version of partial clone. I will continue to investigate recreating this scenario in the Git test suite, but I wanted to send this version out to get feedback on the things that have changed.

Update in V3

• As promised [2], the helper methods are fixed to work with
  non-cone-mode patterns. A later series will use them to their
  fullest potential (changing git add, git rm, and git mv when
  interacting with sparse entries).

[2] https://lore.kernel.org/git/bac76c72-955d-1ade-4ecf-778ffc45f297@gmail.com/

Updates in V2

• This version correctly leaves untracked files alone. If untracked files
  are found, then the directory is left as-is, in case those ignored files
  are important to the user's work resolving those untracked files.

• This behavior is now enabled by core.sparseCheckoutCone=true.

• To use a sparse index as an in-memory data structure even when
  index.sparse is disabled, a new patch is included to modify the
  prototype of convert_to_sparse() to include a flags parameter.

• A few cleanup patches that I was collecting based on feedback
  from the experimental release and intending for my
  next series were necessary for this implementation.

• Cleaned up the tests (no NEEDSWORK) and the remainders of a
  previous implementation that used run_subcommand().

Thanks,
-Stolee

cc: gitster@pobox.com
cc: newren@gmail.com
cc: matheus.bernardino@usp.br
cc: stolee@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Eric Sunshine sunshine@sunshineco.com
cc: René Scharfe l.s.r@web.de

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.1009.git.1627579637.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1009/derrickstolee/sparse-index/ignored-files-v1

To fetch this version to local tag pr-1009/derrickstolee/sparse-index/ignored-files-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1009/derrickstolee/sparse-index/ignored-files-v1

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Thu, Jul 29, 2021 at 11:27 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> My commits are technically based on ds/add-with-sparse-index, but could
> apply earlier, I believe.
>
> We launched an experimental release [1] of the sparse-index feature to our
> internal users. We immediately discovered a problem due to the isolated way
> in which we tested the sparse index: we were never building the project and
> changing our sparse-checkout definition.
>
> [1] https://github.com/microsoft/git/releases/tag/v2.32.0.vfs.0.102.exp
>
> Users who ran a build in one project and then moved to another still had
> build artifacts in their worktree that lived inside the old directories.
> Since the files are marked by the .gitignore patterns, these files were not
> removed by the 'git sparse-checkout set' command. However, they make the
> sparse-index unusable because every 'git status' command needs to expand the
> sparse-directory entries in order to see if the files are tracked or not.
> This made the first experimental release actually slower for all users
> because of this cost.

As highlighted at
https://lore.kernel.org/git/CABPp-BHwRHhYE3BPxCG+fRFWf2dCZ60rnO=ykztquKoOp_x_QQ@mail.gmail.com/
?

"""So, although 'git status'
shouldn't have to check the tracked files anymore, it is still going
to have to look at each of the *untracked* files and compare to
.gitignore files in order to correctly classify each file as ignored
or just plain untracked."""

As I noted there,

"""about a month ago my
colleagues made the tool just auto-invoke [--remove-old-ignores] from other
sparsify invocations.  To my knowledge, there have been no complaints
about that being automatically turned on; but there were
complaints/confusion before about the directories being left around.
(Of course, non-ignored files are still left around by that option.)"""

I think your changes here shouldn't be limited to a sparse-index but
should apply to cone-mode sparse checkouts generally.

> The solution we shipped to these customers was to change the way our fork
> handles these ignored files. Specifically, instead of Git completely
> ignoring the files, we changed Git to understand that with cone-mode
> sparse-checkout patterns, the users is asking for entire directories to be
> removed from the worktree. The link [1] included earlier has this change.

Could you clarify what you are removing?
  1) Tracked but now SKIP_WOKRTREE files
  2) .gitignore'd files
  3) other untracked files

(1) has always been removed by sparse-checkout.  I think you're adding
(2) but it's not clear if you are also removing (3).

> I believe that this is a reasonable expectation, though I recognize that it
> might look like breaking the expectations of how .gitignore files work.
>
> To limit the scope of this change, the focus is limited to behave this way
> only when the sparse-index is enabled, because otherwise the sparse index
> does not behave as intended. With that restriction in mind, the
> implementation can use the sparse directory entries to assist.
>
> I imagine that when the sparse index is mature enough, we will enable it by
> default when cone mode sparse-checkout is enabled, in which case this
> behavior would expand to all users with cone mode sparse-checkout.

I'm all in favor of just doing it for all cone mode users, at least
for .gitignore'd files.

> I'm interested in the community's thoughts about this change, as it seems
> like one that we should make carefully and intentionally.

Yeah, I approached it pretty cautiously, just adding a new option in
our tool (`sparsify --remove-old-ignores`) because I wasn't sure if it
should be done by default but wanted to provide an easy way for users
to access it.  We got numerous complaints and after some months, added
a config option to run it automatically.  We still got complaints and
eventually my colleagues just made it automatically run and provided
no way to turn it off.  No user complaints since.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Thu, Jul 29, 2021 at 11:27 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> 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.

Is this the issue I highlighted at
https://lore.kernel.org/git/CABPp-BHsiLTtv6AuycRrQ5K6q0=ZTJe0kd7uTUL+UT=nxj66zA@mail.gmail.com/,
the paragraph "...I thought the point of add_patterns()..." or is
there more or other things involved here?

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

:-)

However, note that our repository is probably ~25x smaller than yours
in terms of number of files, and 'git status' performance really isn't
that big of an issue for us.  Users complained to us about these
directories sticking around simply because they were confused by the
directories remaining.  (Did sparsify not work?  What's all that
leftover stuff?  Why do they have to deal with it?  etc.)

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

This is such a good explanation of why automatically removing these
directories was perfectly fine for us to do.

> If users depend on ignored files within the sparse directories, then
> they have created a bad shape in their repository. Regardless, such
> shapes would create risk that changing the behavior for all cone mode
> users might be too risky to take on at the moment. Since this data shape
> makes it impossible to get performance benefits using the sparse index,
> we limit the change to only be enabled when the sparse index is enabled.
> Users can opt out of this behavior by disabline the sparse index.

s/disabline/disabling/, otherwise, I fully agree with this paragraph.

> Depending on user feedback or real-world use, we might want to consider
> expanding the behavior change to all of cone mode. Since we are
> currently restricting to the sparse index case, we can use the existence
> of sparse directory entries in the index as indicators of which
> directories should be removed.

Let's just expand it to all of cone mode.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-sparse-checkout.txt |  6 +++
>  builtin/sparse-checkout.c             | 73 +++++++++++++++++++++++++++
>  t/t1091-sparse-checkout-builtin.sh    | 42 +++++++++++++++
>  3 files changed, 121 insertions(+)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index fdcf43f87cb..c443189f418 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -210,6 +210,12 @@ 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.
>
> +When the sparse index is enabled through the `index.sparse` config option,
> +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.
> +
>
>  SUBMODULES
>  ----------
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index a4bdd7c4940..5c03636b6ec 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -15,6 +15,7 @@
>  #include "wt-status.h"
>  #include "quote.h"
>  #include "sparse-index.h"
> +#include "run-command.h"
>
>  static const char *empty_base = "";
>
> @@ -100,6 +101,71 @@ static int sparse_checkout_list(int argc, const char **argv)
>         return 0;
>  }
>
> +static void clean_tracked_sparse_directories(struct repository *r)
> +{
> +       int i;
> +       struct strbuf path = STRBUF_INIT;
> +       size_t pathlen;
> +
> +       /*
> +        * If we are not using cone mode patterns, then we cannot
> +        * delete directories outside of the sparse cone.
> +        */
> +       if (!r || !r->index || !r->index->sparse_checkout_patterns ||
> +           !r->index->sparse_checkout_patterns->use_cone_patterns)
> +               return;
> +       /*
> +        * NEEDSWORK: For now, only use this behavior when index.sparse
> +        * is enabled. We may want this behavior enabled whenever using
> +        * cone mode patterns.
> +        */
> +       prepare_repo_settings(r);
> +       if (!r->worktree || !r->settings.sparse_index)
> +               return;

s/may/do/  :-)

> +
> +       /*
> +        * Since we now depend on the sparse index to enable this
> +        * behavior, use it to our advantage. This process is more
> +        * complicated without it.
> +        */

Ah, the real reason why you limited this change to sparse-index comes out.  :-)

> +       convert_to_sparse(r->index);
> +
> +       strbuf_addstr(&path, r->worktree);
> +       strbuf_complete(&path, '/');
> +       pathlen = path.len;
> +
> +       for (i = 0; i < r->index->cache_nr; i++) {
> +               struct cache_entry *ce = r->index->cache[i];
> +
> +               /*
> +                * Is this a sparse directory? If so, then definitely
> +                * include it. All contained content is outside of the
> +                * patterns.

"include"?  I would have used the word "remove", but it gets confusing
because the question is if we want to include it in our list of things
to remove or remove it from the working directory.

> +                */
> +               if (S_ISSPARSEDIR(ce->ce_mode) &&
> +                   repo_file_exists(r, ce->name)) {
> +                       strbuf_setlen(&path, pathlen);
> +                       strbuf_addstr(&path, ce->name);
> +
> +                       /*
> +                        * Removal is "best effort". If something blocks
> +                        * the deletion, then continue with a warning.
> +                        */
> +                       if (remove_dir_recursively(&path, 0))
> +                               warning(_("failed to remove directory '%s'"), path.buf);

Um, doesn't this delete untracked files that are not ignored as well
as the ignored files?  If so, was that intentional?  I'm fully on
board with removing the gitignore'd files, but I'm worried removing
other untracked files is dangerous.

My implementation of this concept (in an external tool) was more along
the lines of

  * Get $LIST_OF_NON_SPARSE_DIRECTORIES by walking `git ls-files -t`
output and finding common fully-sparse directories
  * git clean -fX $LIST_OF_NON_SPARSE_DIRECTORIES


> +               }
> +       }
> +
> +       strbuf_release(&path);
> +
> +       /*
> +        * This is temporary: the sparse-checkout builtin is not
> +        * integrated with the sparse-index yet, so we need to keep
> +        * it full during the process.
> +        */
> +       ensure_full_index(r->index);
> +}
> +
>  static int update_working_directory(struct pattern_list *pl)
>  {
>         enum update_sparsity_result result;
> @@ -141,6 +207,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;
>  }

(Adding a comment here just to separate the two blocks below.)

> @@ -540,8 +608,11 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
>  {
>         int result;
>         int changed_config = 0;
> +       struct pattern_list *old_pl = xcalloc(1, sizeof(*old_pl));
>         struct pattern_list *pl = xcalloc(1, sizeof(*pl));
>
> +       get_sparse_checkout_patterns(old_pl);
> +
>         switch (m) {
>         case ADD:
>                 if (core_sparse_checkout_cone)
> @@ -567,7 +638,9 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
>                 set_config(MODE_NO_PATTERNS);
>
>         clear_pattern_list(pl);
> +       clear_pattern_list(old_pl);
>         free(pl);
> +       free(old_pl);
>         return result;
>  }

You create an old_pl, populate it with get_sparse_checkout_patterns(),
do nothing with it, then clear and free it?  I'm confused by the above
two blocks.

>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 38fc8340f5c..43eb314c94a 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -642,4 +642,46 @@ 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 &&
> +
> +       # NEEDSWORK: --sparse-index is required for now
> +       git -C repo sparse-checkout init --cone --sparse-index &&
> +       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 &&

What's the expectation for repo/obj?

> +
> +       git -C repo status --porcelain=v2 >out &&
> +       test_must_be_empty out
> +'
> +
>  test_done
> --

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 7/30/2021 9:52 AM, Elijah Newren wrote:
> On Thu, Jul 29, 2021 at 11:27 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
...
>> 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.
> 
> Is this the issue I highlighted at
> https://lore.kernel.org/git/CABPp-BHsiLTtv6AuycRrQ5K6q0=ZTJe0kd7uTUL+UT=nxj66zA@mail.gmail.com/,
> the paragraph "...I thought the point of add_patterns()..." or is
> there more or other things involved here?

This is exactly that point. I'm not sure why I didn't pick up on your
earlier concerns as something to do _immediately_, but for some reason
I thought I could delay it until later.

>> If users depend on ignored files within the sparse directories, then
>> they have created a bad shape in their repository. Regardless, such
>> shapes would create risk that changing the behavior for all cone mode
>> users might be too risky to take on at the moment. Since this data shape
>> makes it impossible to get performance benefits using the sparse index,
>> we limit the change to only be enabled when the sparse index is enabled.
>> Users can opt out of this behavior by disabline the sparse index.
> 
> s/disabline/disabling/, otherwise, I fully agree with this paragraph.

Will fix. Thanks.
 
>> Depending on user feedback or real-world use, we might want to consider
>> expanding the behavior change to all of cone mode. Since we are
>> currently restricting to the sparse index case, we can use the existence
>> of sparse directory entries in the index as indicators of which
>> directories should be removed.
> 
> Let's just expand it to all of cone mode.

I'm open to that. I'll leave this version open for feedback a bit longer
before doing so.

>> +       /*
>> +        * NEEDSWORK: For now, only use this behavior when index.sparse
>> +        * is enabled. We may want this behavior enabled whenever using
>> +        * cone mode patterns.
>> +        */
>> +       prepare_repo_settings(r);
>> +       if (!r->worktree || !r->settings.sparse_index)
>> +               return;
> 
> s/may/do/  :-)

Maybe!
 
>> +
>> +       /*
>> +        * Since we now depend on the sparse index to enable this
>> +        * behavior, use it to our advantage. This process is more
>> +        * complicated without it.
>> +        */
> 
> Ah, the real reason why you limited this change to sparse-index comes out.  :-)
> 
>> +       convert_to_sparse(r->index);
>> +
>> +       strbuf_addstr(&path, r->worktree);
>> +       strbuf_complete(&path, '/');
>> +       pathlen = path.len;
>> +
>> +       for (i = 0; i < r->index->cache_nr; i++) {
>> +               struct cache_entry *ce = r->index->cache[i];
>> +
>> +               /*
>> +                * Is this a sparse directory? If so, then definitely
>> +                * include it. All contained content is outside of the
>> +                * patterns.
> 
> "include"?  I would have used the word "remove", but it gets confusing
> because the question is if we want to include it in our list of things
> to remove or remove it from the working directory.

This comment is a bit stale, sorry. Earlier I was populating a list of
the directories, but in this version I'm just deleting them immediately.

>> +                */
>> +               if (S_ISSPARSEDIR(ce->ce_mode) &&
>> +                   repo_file_exists(r, ce->name)) {
>> +                       strbuf_setlen(&path, pathlen);
>> +                       strbuf_addstr(&path, ce->name);
>> +
>> +                       /*
>> +                        * Removal is "best effort". If something blocks
>> +                        * the deletion, then continue with a warning.
>> +                        */
>> +                       if (remove_dir_recursively(&path, 0))
>> +                               warning(_("failed to remove directory '%s'"), path.buf);
> 
> Um, doesn't this delete untracked files that are not ignored as well
> as the ignored files?  If so, was that intentional?  I'm fully on
> board with removing the gitignore'd files, but I'm worried removing
> other untracked files is dangerous.

I believe that 'git sparse-checkout (set|add|reapply)' will fail before
reaching this method if there are untracked files that could potentially
be removed. I will double-check to ensure this is the case. It is
definitely my intention to protect any untracked, non-ignored files in
these directories by failing the sparse-checkout modification.

> My implementation of this concept (in an external tool) was more along
> the lines of
> 
>   * Get $LIST_OF_NON_SPARSE_DIRECTORIES by walking `git ls-files -t`
> output and finding common fully-sparse directories
>   * git clean -fX $LIST_OF_NON_SPARSE_DIRECTORIES

I initially was running 'git clean -dfx -- <dir> ...' but that also
requires parsing and expanding the index (or being very careful with
the sparse index).

> 
>> +               }
>> +       }
>> +
>> +       strbuf_release(&path);
>> +
>> +       /*
>> +        * This is temporary: the sparse-checkout builtin is not
>> +        * integrated with the sparse-index yet, so we need to keep
>> +        * it full during the process.
>> +        */
>> +       ensure_full_index(r->index);
>> +}
>> +
>>  static int update_working_directory(struct pattern_list *pl)
>>  {
>>         enum update_sparsity_result result;
>> @@ -141,6 +207,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;
>>  }
> 
> (Adding a comment here just to separate the two blocks below.)
> 
>> @@ -540,8 +608,11 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
>>  {
>>         int result;
>>         int changed_config = 0;
>> +       struct pattern_list *old_pl = xcalloc(1, sizeof(*old_pl));
>>         struct pattern_list *pl = xcalloc(1, sizeof(*pl));
>>
>> +       get_sparse_checkout_patterns(old_pl);
>> +
>>         switch (m) {
>>         case ADD:
>>                 if (core_sparse_checkout_cone)
>> @@ -567,7 +638,9 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
>>                 set_config(MODE_NO_PATTERNS);
>>
>>         clear_pattern_list(pl);
>> +       clear_pattern_list(old_pl);
>>         free(pl);
>> +       free(old_pl);
>>         return result;
>>  }
> 
> You create an old_pl, populate it with get_sparse_checkout_patterns(),
> do nothing with it, then clear and free it?  I'm confused by the above
> two blocks.

Sorry that this is also stale. I should read my own patches more carefully.

I was originally going to focus only on the directories that were "leaving"
the sparse-checkout definition, but that did not catch the 'reapply' case
or cases where the user created the directories by other means.

Will delete these bits.
>> +       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 &&
> 
> What's the expectation for repo/obj?

I will add

	test_path_is_dir repo/obj

because this ignored directory should not be removed. This is
simulating the build artifacts that might appear in ignored
directories whose parents are in the sparse-checkout definition.

Thanks,
-Stolee

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Mon, Aug 2, 2021 at 8:34 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 7/30/2021 9:52 AM, Elijah Newren wrote:
> > On Thu, Jul 29, 2021 at 11:27 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
...
> >> +                */
> >> +               if (S_ISSPARSEDIR(ce->ce_mode) &&
> >> +                   repo_file_exists(r, ce->name)) {
> >> +                       strbuf_setlen(&path, pathlen);
> >> +                       strbuf_addstr(&path, ce->name);
> >> +
> >> +                       /*
> >> +                        * Removal is "best effort". If something blocks
> >> +                        * the deletion, then continue with a warning.
> >> +                        */
> >> +                       if (remove_dir_recursively(&path, 0))
> >> +                               warning(_("failed to remove directory '%s'"), path.buf);
> >
> > Um, doesn't this delete untracked files that are not ignored as well
> > as the ignored files?  If so, was that intentional?  I'm fully on
> > board with removing the gitignore'd files, but I'm worried removing
> > other untracked files is dangerous.
>
> I believe that 'git sparse-checkout (set|add|reapply)' will fail before
> reaching this method if there are untracked files that could potentially
> be removed. I will double-check to ensure this is the case. It is
> definitely my intention to protect any untracked, non-ignored files in
> these directories by failing the sparse-checkout modification.
>
> > My implementation of this concept (in an external tool) was more along
> > the lines of
> >
> >   * Get $LIST_OF_NON_SPARSE_DIRECTORIES by walking `git ls-files -t`
> > output and finding common fully-sparse directories
> >   * git clean -fX $LIST_OF_NON_SPARSE_DIRECTORIES
>
> I initially was running 'git clean -dfx -- <dir> ...' but that also
> requires parsing and expanding the index (or being very careful with
> the sparse index).

`git clean -dfx -- <dir> ...` could also be very dangerous because
it'd delete untracked non-ignored files.  You want -X rather than -x.
One of those cases where capitalization is critical.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 8/2/2021 12:17 PM, Elijah Newren wrote:
> On Mon, Aug 2, 2021 at 8:34 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 7/30/2021 9:52 AM, Elijah Newren wrote:
>>> On Thu, Jul 29, 2021 at 11:27 AM Derrick Stolee via GitGitGadget
>>> <gitgitgadget@gmail.com> wrote:
> ...
>>>> +                */
>>>> +               if (S_ISSPARSEDIR(ce->ce_mode) &&
>>>> +                   repo_file_exists(r, ce->name)) {
>>>> +                       strbuf_setlen(&path, pathlen);
>>>> +                       strbuf_addstr(&path, ce->name);
>>>> +
>>>> +                       /*
>>>> +                        * Removal is "best effort". If something blocks
>>>> +                        * the deletion, then continue with a warning.
>>>> +                        */
>>>> +                       if (remove_dir_recursively(&path, 0))
>>>> +                               warning(_("failed to remove directory '%s'"), path.buf);
>>>
>>> Um, doesn't this delete untracked files that are not ignored as well
>>> as the ignored files?  If so, was that intentional?  I'm fully on
>>> board with removing the gitignore'd files, but I'm worried removing
>>> other untracked files is dangerous.
>>
>> I believe that 'git sparse-checkout (set|add|reapply)' will fail before
>> reaching this method if there are untracked files that could potentially
>> be removed. I will double-check to ensure this is the case. It is
>> definitely my intention to protect any untracked, non-ignored files in
>> these directories by failing the sparse-checkout modification.

This is _not_ true, and I can document it with a test.

Having untracked files outside of the sparse cone is just as bad as
ignored files, so I want to ensure that these get cleaned up, too.

The correct thing would be to prevent the 'git sparse-checkout
(set|add|reapply)' command from making any changes to the sparse-checkout
cone or the worktree if there are untracked files that would be deleted.
(Right? Or is there another solution that I'm missing here?)

>>> My implementation of this concept (in an external tool) was more along
>>> the lines of
>>>
>>>   * Get $LIST_OF_NON_SPARSE_DIRECTORIES by walking `git ls-files -t`
>>> output and finding common fully-sparse directories
>>>   * git clean -fX $LIST_OF_NON_SPARSE_DIRECTORIES
>>
>> I initially was running 'git clean -dfx -- <dir> ...' but that also
>> requires parsing and expanding the index (or being very careful with
>> the sparse index).
> 
> `git clean -dfx -- <dir> ...` could also be very dangerous because
> it'd delete untracked non-ignored files.  You want -X rather than -x.
> One of those cases where capitalization is critical.

Good point. I'd like to avoid using `git clean` as a subcommand, if
possible, that way we have one fewer thing to do before integrating
the `git sparse-checkout` builtin with the sparse index.

Thanks,
-Stolee

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Aug 4, 2021 at 7:55 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 8/2/2021 12:17 PM, Elijah Newren wrote:
> > On Mon, Aug 2, 2021 at 8:34 AM Derrick Stolee <stolee@gmail.com> wrote:
> >>
> >> On 7/30/2021 9:52 AM, Elijah Newren wrote:
> >>> On Thu, Jul 29, 2021 at 11:27 AM Derrick Stolee via GitGitGadget
> >>> <gitgitgadget@gmail.com> wrote:
> > ...
> >>>> +                */
> >>>> +               if (S_ISSPARSEDIR(ce->ce_mode) &&
> >>>> +                   repo_file_exists(r, ce->name)) {
> >>>> +                       strbuf_setlen(&path, pathlen);
> >>>> +                       strbuf_addstr(&path, ce->name);
> >>>> +
> >>>> +                       /*
> >>>> +                        * Removal is "best effort". If something blocks
> >>>> +                        * the deletion, then continue with a warning.
> >>>> +                        */
> >>>> +                       if (remove_dir_recursively(&path, 0))
> >>>> +                               warning(_("failed to remove directory '%s'"), path.buf);
> >>>
> >>> Um, doesn't this delete untracked files that are not ignored as well
> >>> as the ignored files?  If so, was that intentional?  I'm fully on
> >>> board with removing the gitignore'd files, but I'm worried removing
> >>> other untracked files is dangerous.
> >>
> >> I believe that 'git sparse-checkout (set|add|reapply)' will fail before
> >> reaching this method if there are untracked files that could potentially
> >> be removed. I will double-check to ensure this is the case. It is
> >> definitely my intention to protect any untracked, non-ignored files in
> >> these directories by failing the sparse-checkout modification.
>
> This is _not_ true, and I can document it with a test.
>
> Having untracked files outside of the sparse cone is just as bad as
> ignored files, so I want to ensure that these get cleaned up, too.
>
> The correct thing would be to prevent the 'git sparse-checkout
> (set|add|reapply)' command from making any changes to the sparse-checkout
> cone or the worktree if there are untracked files that would be deleted.
> (Right? Or is there another solution that I'm missing here?)

We could sparsify as much as possible and print warnings, much like we
do with tracked files that are modified but not staged.  In fact, it
might feel inconsistent if we sparsify as much as possible for one
type of file, and abort if we cannot completely sparsify for a
different type of file.  We could consider changing how we treat
tracked files that are modified but not staged and have them abort the
sparse-checkout commands as well, but I worry that might cause
problems during conflict resolution in the middle of
merges/rebases/cherry-picks/reverts.  I don't want users caught where
they need to update their sparsity paths to gain new files/directories
that will help them resolve some conflicts, but be unable to update
their sparsity paths because they have conflicts.

That said, the basic idea of aborting sparse-checkout in cone mode
when there are untracked unignored files in the way of removing
directories sounds reasonable, if there's some clever way to avoid or
ameliorate the inconsistency issues mentioned above.  Implementing it
might require walking all untracked (and tracked?) files twice,
though, because if there are untracked unignored files in the way, we
probably don't want to abort after first deleting lots of ignored
files.  (And there's a small race window in the double walk...)
However, I don't expect people to run sparse-checkout commands all
that often, so the double walk is probably a perfectly reasonable
performance cost.  I just wanted to note it.

> >>> My implementation of this concept (in an external tool) was more along
> >>> the lines of
> >>>
> >>>   * Get $LIST_OF_NON_SPARSE_DIRECTORIES by walking `git ls-files -t`
> >>> output and finding common fully-sparse directories
> >>>   * git clean -fX $LIST_OF_NON_SPARSE_DIRECTORIES
> >>
> >> I initially was running 'git clean -dfx -- <dir> ...' but that also
> >> requires parsing and expanding the index (or being very careful with
> >> the sparse index).
> >
> > `git clean -dfx -- <dir> ...` could also be very dangerous because
> > it'd delete untracked non-ignored files.  You want -X rather than -x.
> > One of those cases where capitalization is critical.
>
> Good point. I'd like to avoid using `git clean` as a subcommand, if
> possible, that way we have one fewer thing to do before integrating
> the `git sparse-checkout` builtin with the sparse index.

Oh, I didn't want to invoke a subcommand, I was just pointing out
where similar code might be found in case we wanted to call the same
functions from elsewhere (or maybe even turn some of it into library
functions we could call).  But that might be a moot point if we end up
making sparse-checkout fail if there are untracked unignored files
hanging around in the relevant directories.

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.1009.v2.git.1628625013.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1009/derrickstolee/sparse-index/ignored-files-v2

To fetch this version to local tag pr-1009/derrickstolee/sparse-index/ignored-files-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1009/derrickstolee/sparse-index/ignored-files-v2

[gitgitgadget]

This branch is now known as ds/sparse-index-ignored-files.

[gitgitgadget]

This patch series was integrated into seen via git@60a8c24.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch ds/sparse-index-ignored-files on the Git mailing list:

In cone mode, the sparse-index codepath learned to remove ignored
files (like build artifacts) outside the sparse cone, allowing the
entire directory outside the sparse cone to be removed, which is
especially useful when the sparse patterns change.

[gitgitgadget]

This patch series was integrated into seen via git@d3f52dc.

[gitgitgadget]

This patch series was integrated into seen via git@62a61cf.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 8/10/2021 3:50 PM, Derrick Stolee via GitGitGadget wrote:
...
> +enum pattern_match_result path_in_sparse_checkout(const char *path,
> +						  struct index_state *istate)
> +{
> +	int dtype = DT_REG;
> +	init_sparse_checkout_patterns(istate);
> +
> +	if (!istate->sparse_checkout_patterns)
> +		return MATCHED;
> +
> +	return path_matches_pattern_list(path, strlen(path), NULL, &dtype,
> +					 istate->sparse_checkout_patterns,
> +					 istate);

While expanding on this work to fix behavior in 'git (add|rm|mv)' around
sparse entries, I noticed a problem with this method, specifically with
non-cone-mode patterns:

1. The NULL here should be the "basename" of the path, not NULL. This doesn't
   matter for cone mode, but _does_ matter for more general patterns.

2. The return type here can be UNDECIDED with general patterns, which really
   means "not matched" but is distinct from NOT_MATCHED because of the recursive
   assumptions when a directory is returned with NOT_MATCHED. Since the usage
   pattern for path_in_sparse_checkout() is to get a boolean result, the
   return type should switch to 'int' and we should return
   "path_matches_pattern_list(...) > 0".

I'm still doing some more testing to ensure I've got the necessary tweaks in
place to work with the other changes I'm going for. Plan on me sending a v3
with the appropriate changes here.

>  	/*
>  	 * Is the current path outside of the sparse cone?
>  	 * Then check if the region can be replaced by a sparse
>  	 * directory entry (everything is sparse and merged).
>  	 */
> -	match = path_matches_pattern_list(ct_path, ct_pathlen,
> -					  NULL, &dtype, pl, istate);
> +	match = path_in_sparse_checkout(ct_path, istate);
>  	if (match != NOT_MATCHED)
>  		can_convert = 0;

We could remove the use of "match" here and use the boolean result of
path_in_sparse_checkout() instead.

Thanks,
-Stolee

[gitgitgadget]

This patch series was integrated into seen via git@7608a65.

[gitgitgadget]

This patch series was integrated into seen via git@cada03e.