Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sparse index: delete ignored files outside sparse cone #1009

10 changes: 10 additions & 0 deletions Documentation/git-sparse-checkout.txt
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,16 @@ case-insensitive check. This corrects for case mismatched filenames in the
'git sparse-checkout set' command to reflect the expected cone in the working
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

On Thu, Aug 19, 2021 at 1:48 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> 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.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

On Fri, Aug 20, 2021 at 8:50 AM Derrick Stolee <stolee@gmail.com> 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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Elijah,

On Fri, 20 Aug 2021, Elijah Newren wrote:

> On Thu, Aug 19, 2021 at 1:48 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> 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

directory.

When changing the sparse-checkout patterns in cone mode, Git will inspect each
tracked directory that is not within the sparse-checkout cone to see if it
contains any untracked files. If all of those files are ignored due to the
`.gitignore` patterns, then the directory will be deleted. If any of the
untracked files within that directory is not ignored, then no deletions will
occur within that directory and a warning message will appear. If these files
are important, then reset your sparse-checkout definition so they are included,
use `git add` and `git commit` to store them, then remove any remaining files
manually to ensure Git can behave optimally.


SUBMODULES
----------
Expand Down
15 changes: 15 additions & 0 deletions attr.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "utf8.h"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Stolee,

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

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  attr.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/attr.c b/attr.c
> index d029e681f28..a1009f78029 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -14,6 +14,7 @@
>  #include "utf8.h"
>  #include "quote.h"
>  #include "thread-utils.h"
> +#include "dir.h"
>
>  const char git_attr__true[] = "(builtin)true";
>  const char git_attr__false[] = "\0(builtin)false";
> @@ -744,6 +745,19 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,
>  	if (!istate)
>  		return NULL;
>
> +	/*
> +	 * In the case of cone-mode sparse-checkout, getting the
> +	 * .gitattributes file from a directory is meaningless: all
> +	 * contained paths will be sparse if the .gitattributes is also
> +	 * sparse. In the case of a sparse index, it is critical that we
> +	 * don't go looking for one as it will expand the index.
> +	 */
> +	init_sparse_checkout_patterns(istate);
At first I thought that `init_sparse_checkout_patterns()` is called by
`path_in_sparse_checkout()` below, and therefore would not be necessary.

But it is! Without it, we have no way to test whether `use_cone_patterns`
is set, because, well, it gets set by `init_sparse_checkout_patterns()`.

Would it therefore make sense to refactor the code to have a
`path_in_sparse_checkout_cone()` function? Or add a flag
`only_in_cone_mode` as function parameter to `path_in_sparse_checkout()`?

Ciao,
Dscho

> +	if (istate->sparse_checkout_patterns &&
> +	    istate->sparse_checkout_patterns->use_cone_patterns &&
> +	    path_in_sparse_checkout(path, istate) == NOT_MATCHED)

> +		return NULL;
> +
>  	buf = read_blob_data_from_index(istate, path, NULL);
>  	if (!buf)
>  		return NULL;
> --
> gitgitgadget
>
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

On 8/19/2021 4:11 AM, Johannes Schindelin wrote:
> On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:
>> +	/*
>> +	 * In the case of cone-mode sparse-checkout, getting the
>> +	 * .gitattributes file from a directory is meaningless: all
>> +	 * contained paths will be sparse if the .gitattributes is also
>> +	 * sparse. In the case of a sparse index, it is critical that we
>> +	 * don't go looking for one as it will expand the index.
>> +	 */
>> +	init_sparse_checkout_patterns(istate);
> At first I thought that `init_sparse_checkout_patterns()` is called by
> `path_in_sparse_checkout()` below, and therefore would not be necessary.
> 
> But it is! Without it, we have no way to test whether `use_cone_patterns`
> is set, because, well, it gets set by `init_sparse_checkout_patterns()`.
> 
> Would it therefore make sense to refactor the code to have a
> `path_in_sparse_checkout_cone()` function? Or add a flag
> `only_in_cone_mode` as function parameter to `path_in_sparse_checkout()`?

One way to clean this up as-is would be to replace
 
>> +	if (istate->sparse_checkout_patterns &&
>> +	    istate->sparse_checkout_patterns->use_cone_patterns &&
>> +	    path_in_sparse_checkout(path, istate) == NOT_MATCHED)

with

	if (!path_in_sparse_checkout(path, istate) &&
	    istate->sparse_checkout_patterns->use_cone_patterns)

to get a similar effect. However, it creates wasted pattern-match
checks when not in cone-mode, so you are probably right that a
path_in_cone_mode_sparse_checkout() would be best to short-circuit
in the non-cone-mode case.

This special casing should be rare, so I don't think it is worth
adding a flag to path_in_sparse_checkout() and making those call
sites more complicated.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

On Tue, Aug 17, 2021 at 6:23 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  attr.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/attr.c b/attr.c
> index d029e681f28..a1009f78029 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -14,6 +14,7 @@
>  #include "utf8.h"
>  #include "quote.h"
>  #include "thread-utils.h"
> +#include "dir.h"
>
>  const char git_attr__true[] = "(builtin)true";
>  const char git_attr__false[] = "\0(builtin)false";
> @@ -744,6 +745,19 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,
>         if (!istate)
>                 return NULL;
>
> +       /*
> +        * In the case of cone-mode sparse-checkout, getting the
> +        * .gitattributes file from a directory is meaningless: all
> +        * contained paths will be sparse if the .gitattributes is also
> +        * sparse. In the case of a sparse index, it is critical that we
> +        * don't go looking for one as it will expand the index.
> +        */

"all contained paths will be sparse if the .gitattributes is also sparse"?

Do you mean something more like "the .gitattributes only applies for
files under the given directory, and if the directory is sparse, then
neither the .gitattributes file or any other file under that directory
will be present" ?

Also, out of curiosity, I was suggesting in the past we do something
like this for .gitignore files, for the same reason.  Do we have such
logic in place, or is that another area of the code that hasn't been
handled yet?

> +       init_sparse_checkout_patterns(istate);
> +       if (istate->sparse_checkout_patterns &&
> +           istate->sparse_checkout_patterns->use_cone_patterns &&
> +           path_in_sparse_checkout(path, istate) == NOT_MATCHED)
> +               return NULL;
> +
>         buf = read_blob_data_from_index(istate, path, NULL);
>         if (!buf)
>                 return NULL;
> --
> gitgitgadget

Though the code appears correct, I too am curious about the questions
Dscho asked about the code in this patch.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

On 8/19/2021 4:53 PM, Elijah Newren wrote:
> On Tue, Aug 17, 2021 at 6:23 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> +       /*
>> +        * In the case of cone-mode sparse-checkout, getting the
>> +        * .gitattributes file from a directory is meaningless: all
>> +        * contained paths will be sparse if the .gitattributes is also
>> +        * sparse. In the case of a sparse index, it is critical that we
>> +        * don't go looking for one as it will expand the index.
>> +        */
> 
> "all contained paths will be sparse if the .gitattributes is also sparse"?
> 
> Do you mean something more like "the .gitattributes only applies for
> files under the given directory, and if the directory is sparse, then
> neither the .gitattributes file or any other file under that directory
> will be present" ?

Yes, you understand correctly and explain it better. Thanks.
 
> Also, out of curiosity, I was suggesting in the past we do something
> like this for .gitignore files, for the same reason.  Do we have such
> logic in place, or is that another area of the code that hasn't been
> handled yet?

I don't believe this has been handled. It definitely is less obvious
what to do there, because the point of .gitignore is to skip files that
exist in the working tree even if Git didn't put them there. Meanwhile,
.gitattributes is about how Git writes tracked files, but Git doesn't
write sparse tracked files.

> Though the code appears correct, I too am curious about the questions
> Dscho asked about the code in this patch.
 
Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

On Fri, Aug 20, 2021 at 8:39 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 8/19/2021 4:53 PM, Elijah Newren wrote:
> > On Tue, Aug 17, 2021 at 6:23 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +       /*
> >> +        * In the case of cone-mode sparse-checkout, getting the
> >> +        * .gitattributes file from a directory is meaningless: all
> >> +        * contained paths will be sparse if the .gitattributes is also
> >> +        * sparse. In the case of a sparse index, it is critical that we
> >> +        * don't go looking for one as it will expand the index.
> >> +        */
> >
> > "all contained paths will be sparse if the .gitattributes is also sparse"?
> >
> > Do you mean something more like "the .gitattributes only applies for
> > files under the given directory, and if the directory is sparse, then
> > neither the .gitattributes file or any other file under that directory
> > will be present" ?
>
> Yes, you understand correctly and explain it better. Thanks.
>
> > Also, out of curiosity, I was suggesting in the past we do something
> > like this for .gitignore files, for the same reason.  Do we have such
> > logic in place, or is that another area of the code that hasn't been
> > handled yet?
>
> I don't believe this has been handled. It definitely is less obvious
> what to do there, because the point of .gitignore is to skip files that
> exist in the working tree even if Git didn't put them there. Meanwhile,
> .gitattributes is about how Git writes tracked files, but Git doesn't
> write sparse tracked files.

Well, one advantage of deleting ignored files in sparse directories
when we sparsify, is that we know if any files are left, they are all
untracked and not ignored.  So we don't need to load the .gitignore
file for those sparse directories.

Sure, there's a small edge case of users adding new untracked files
that would have matched the .gitignore file, but it's also clear that
they removed the .gitignore file when they sparsified, so I don't see
a problem in reporting it as untracked while that directory was
as-sparsified-away-as-possible.

#include "quote.h"
#include "thread-utils.h"
#include "dir.h"

const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
Expand Down Expand Up @@ -744,6 +745,20 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,
if (!istate)
return NULL;

/*
* The .gitattributes file only applies to files within its
* parent directory. In the case of cone-mode sparse-checkout,
* the .gitattributes file is sparse if and only if all paths
* within that directory are also sparse. Thus, don't load the
* .gitattributes file since it will not matter.
*
* In the case of a sparse index, it is critical that we don't go
* looking for a .gitattributes file, as doing so would cause the
* index to expand.
*/
if (!path_in_cone_mode_sparse_checkout(path, istate))
return NULL;

buf = read_blob_data_from_index(istate, path, NULL);
if (!buf)
return NULL;
Expand Down
7 changes: 1 addition & 6 deletions builtin/add.c
Original file line number Diff line number Diff line change
Expand Up @@ -190,21 +190,16 @@ static int refresh(int verbose, const struct pathspec *pathspec)
struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Stolee,

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

> From: Derrick Stolee <dstolee@microsoft.com>
>
> As we integrate the sparse index into more builtins, we occasionally
> need to check the sparse-checkout patterns to see if a path is within
> the sparse-checkout cone. Create some helper methods that help
> initialize the patterns and check for pattern matching to make this
> easier.
>
> The existing callers of commands like get_sparse_checkout_patterns() use
> a custom 'struct pattern_list' that is not necessarily the one in the
> 'struct index_state', so there are not many previous uses that could
> adopt these helpers. There are just two in builtin/add.c and
> sparse-index.c that can use path_in_sparse_checkout().

Very good!

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/add.c  |  8 ++------
>  dir.c          | 33 +++++++++++++++++++++++++++++++++
>  dir.h          |  6 ++++++
>  sparse-index.c | 12 +++---------
>  4 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 17528e8f922..f675bdeae4a 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -190,8 +190,6 @@ static int refresh(int verbose, const struct pathspec *pathspec)
>  	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
>  	int flags = REFRESH_IGNORE_SKIP_WORKTREE |
>  		    (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
> -	struct pattern_list pl = { 0 };
> -	int sparse_checkout_enabled = !get_sparse_checkout_patterns(&pl);
>
>  	seen = xcalloc(pathspec->nr, 1);
>  	refresh_index(&the_index, flags, pathspec, seen,
> @@ -199,12 +197,10 @@ static int refresh(int verbose, const struct pathspec *pathspec)
>  	for (i = 0; i < pathspec->nr; i++) {
>  		if (!seen[i]) {
>  			const char *path = pathspec->items[i].original;
> -			int dtype = DT_REG;
>
>  			if (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||
> -			    (sparse_checkout_enabled &&
> -			     !path_matches_pattern_list(path, strlen(path), NULL,
> -							&dtype, &pl, &the_index))) {
> +			    (core_apply_sparse_checkout &&

Do we need to test for `core_apply_sparse_checkout` here? Or does the `if
(!istate->sparse_checkout_patterns) return MATCHED;` early return in
`path_in_sparse_checkout()` suffice to catch this?

The remainder of the patch looks good to me.

Thank you,
Dscho

> +			     path_in_sparse_checkout(path, &the_index) == NOT_MATCHED)) {
>  				string_list_append(&only_match_skip_worktree,
>  						   pathspec->items[i].original);
>  			} else {
> diff --git a/dir.c b/dir.c
> index 03c4d212672..6fd4f2e0f27 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1439,6 +1439,39 @@ done:
>  	return result;
>  }
>
> +int init_sparse_checkout_patterns(struct index_state *istate)
> +{
> +	if (!core_apply_sparse_checkout ||
> +	    istate->sparse_checkout_patterns)
> +		return 0;
> +
> +	CALLOC_ARRAY(istate->sparse_checkout_patterns, 1);
> +
> +	if (get_sparse_checkout_patterns(istate->sparse_checkout_patterns) < 0) {
> +		FREE_AND_NULL(istate->sparse_checkout_patterns);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +int path_in_sparse_checkout(const char *path,
> +			    struct index_state *istate)
> +{
> +	const char *base;
> +	int dtype = DT_REG;
> +	init_sparse_checkout_patterns(istate);
> +
> +	if (!istate->sparse_checkout_patterns)
> +		return MATCHED;
> +
> +	base = strrchr(path, '/');
> +	return path_matches_pattern_list(path, strlen(path), base ? base + 1 : path,
> +					 &dtype,
> +					 istate->sparse_checkout_patterns,
> +					 istate) > 0;
> +}
> +
>  static struct path_pattern *last_matching_pattern_from_lists(
>  		struct dir_struct *dir, struct index_state *istate,
>  		const char *pathname, int pathlen,
> diff --git a/dir.h b/dir.h
> index b3e1a54a971..b899ee43d81 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -394,6 +394,12 @@ enum pattern_match_result path_matches_pattern_list(const char *pathname,
>  				const char *basename, int *dtype,
>  				struct pattern_list *pl,
>  				struct index_state *istate);
> +
> +int init_sparse_checkout_patterns(struct index_state *state);
> +
> +int path_in_sparse_checkout(const char *path,
> +			    struct index_state *istate);
> +
>  struct dir_entry *dir_add_ignored(struct dir_struct *dir,
>  				  struct index_state *istate,
>  				  const char *pathname, int len);
> diff --git a/sparse-index.c b/sparse-index.c
> index b6e90417556..2efc9fd4910 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -34,17 +34,14 @@ static int convert_to_sparse_rec(struct index_state *istate,
>  	int i, can_convert = 1;
>  	int start_converted = num_converted;
>  	enum pattern_match_result match;
> -	int dtype = DT_UNKNOWN;
>  	struct strbuf child_path = STRBUF_INIT;
> -	struct pattern_list *pl = istate->sparse_checkout_patterns;
>
>  	/*
>  	 * 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;
>
> @@ -153,11 +150,8 @@ int convert_to_sparse(struct index_state *istate)
>  	if (!istate->repo->settings.sparse_index)
>  		return 0;
>
> -	if (!istate->sparse_checkout_patterns) {
> -		istate->sparse_checkout_patterns = xcalloc(1, sizeof(struct pattern_list));
> -		if (get_sparse_checkout_patterns(istate->sparse_checkout_patterns) < 0)
> -			return 0;
> -	}
> +	if (init_sparse_checkout_patterns(istate) < 0)
> +		return 0;
>
>  	/*
>  	 * We need cone-mode patterns to use sparse-index. If a user edits
> --
> gitgitgadget
>
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

On 8/19/2021 4:07 AM, Johannes Schindelin wrote:
...
>>  			if (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||
>> -			    (sparse_checkout_enabled &&
>> -			     !path_matches_pattern_list(path, strlen(path), NULL,
>> -							&dtype, &pl, &the_index))) {
>> +			    (core_apply_sparse_checkout &&
> 
> Do we need to test for `core_apply_sparse_checkout` here? Or does the `if
> (!istate->sparse_checkout_patterns) return MATCHED;` early return in
> `path_in_sparse_checkout()` suffice to catch this?
> 
> The remainder of the patch looks good to me.
> 
> Thank you,
> Dscho
> 
>> +			     path_in_sparse_checkout(path, &the_index) == NOT_MATCHED)) {

Thank you for pointing out this. This is actually a stale change from an earlier
version where path_in_sparse_checkout returned an 'enum pattern_match_result'
but now casts down to an 'int', meaning '0' is not in the sparse-checkout and '1'
is that it _is_ in the sparse-checkout.

>> +int path_in_sparse_checkout(const char *path,
>> +			    struct index_state *istate)
>> +{
>> +	const char *base;
>> +	int dtype = DT_REG;
>> +	init_sparse_checkout_patterns(istate);
>> +
>> +	if (!istate->sparse_checkout_patterns)
>> +		return MATCHED;

So here, if we do not have sparse-checkout patterns (for example, if
core_apply_sparse_checkout is false), then this returns MATCHED (== 1).

To be extra clear, this should just be 'return 1;'.

>> +	base = strrchr(path, '/');
>> +	return path_matches_pattern_list(path, strlen(path), base ? base + 1 : path,
>> +					 &dtype,
>> +					 istate->sparse_checkout_patterns,
>> +					 istate) > 0;

Here, we are selecting the portion of 'enum pattern_match_result' that
we care about (MATCHED and MATCHED_RECURSIVE). The UNMATCHED (0) and
UNDECIDED (-1) are the other possibilities, but file paths will not
return UNDECIDED, that is instead for directories in non-cone mode
patterns.

Thanks,
-Stolee

int flags = REFRESH_IGNORE_SKIP_WORKTREE |
(verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
struct pattern_list pl = { 0 };
int sparse_checkout_enabled = !get_sparse_checkout_patterns(&pl);

seen = xcalloc(pathspec->nr, 1);
refresh_index(&the_index, flags, pathspec, seen,
_("Unstaged changes after refreshing the index:"));
for (i = 0; i < pathspec->nr; i++) {
if (!seen[i]) {
const char *path = pathspec->items[i].original;
int dtype = DT_REG;

if (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||
(sparse_checkout_enabled &&
!path_matches_pattern_list(path, strlen(path), NULL,
&dtype, &pl, &the_index))) {
!path_in_sparse_checkout(path, &the_index)) {
string_list_append(&only_match_skip_worktree,
pathspec->items[i].original);
} else {
Expand Down
94 changes: 94 additions & 0 deletions builtin/sparse-checkout.c
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,98 @@ 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;
if (init_sparse_checkout_patterns(r->index) ||
!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_MEMORY_ONLY))
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;
Expand Down Expand Up @@ -141,6 +233,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;
}
Expand Down
52 changes: 52 additions & 0 deletions dir.c
Original file line number Diff line number Diff line change
Expand Up @@ -1439,6 +1439,58 @@ enum pattern_match_result path_matches_pattern_list(
return result;
}

int init_sparse_checkout_patterns(struct index_state *istate)
{
if (!core_apply_sparse_checkout)
return 1;
if (istate->sparse_checkout_patterns)
return 0;

CALLOC_ARRAY(istate->sparse_checkout_patterns, 1);

if (get_sparse_checkout_patterns(istate->sparse_checkout_patterns) < 0) {
FREE_AND_NULL(istate->sparse_checkout_patterns);
return -1;
}

return 0;
}

static int path_in_sparse_checkout_1(const char *path,
struct index_state *istate,
int require_cone_mode)
{
const char *base;
int dtype = DT_REG;

/*
* We default to accepting a path if there are no patterns or
* they are of the wrong type.
*/
if (init_sparse_checkout_patterns(istate) ||
(require_cone_mode &&
!istate->sparse_checkout_patterns->use_cone_patterns))
return 1;

base = strrchr(path, '/');
return path_matches_pattern_list(path, strlen(path), base ? base + 1 : path,
&dtype,
istate->sparse_checkout_patterns,
istate) > 0;
}

int path_in_sparse_checkout(const char *path,
struct index_state *istate)
{
return path_in_sparse_checkout_1(path, istate, 0);
}

int path_in_cone_mode_sparse_checkout(const char *path,
struct index_state *istate)
{
return path_in_sparse_checkout_1(path, istate, 1);
}

static struct path_pattern *last_matching_pattern_from_lists(
struct dir_struct *dir, struct index_state *istate,
const char *pathname, int pathlen,
Expand Down
8 changes: 8 additions & 0 deletions dir.h
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,14 @@ enum pattern_match_result path_matches_pattern_list(const char *pathname,
const char *basename, int *dtype,
struct pattern_list *pl,
struct index_state *istate);

int init_sparse_checkout_patterns(struct index_state *state);

int path_in_sparse_checkout(const char *path,
struct index_state *istate);
int path_in_cone_mode_sparse_checkout(const char *path,
struct index_state *istate);

struct dir_entry *dir_add_ignored(struct dir_struct *dir,
struct index_state *istate,
const char *pathname, int len);
Expand Down
4 changes: 2 additions & 2 deletions read-cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -3069,7 +3069,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
int ret;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

On 8/17/2021 9:23 AM, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> The convert_to_sparse() method checks for the GIT_TEST_SPARSE_INDEX
> environment variable or the "index.sparse" config setting before
> converting the index to a sparse one. This is for ease of use since all
> current consumers are preparing to compress the index before writing it
> to disk. If these settings are not enabled, then convert_to_sparse()
> silently returns without doing anything.
> 
> We will add a consumer in the next change that wants to use the sparse
> index as an in-memory data structure, regardless of whether the on-disk
> format should be sparse.
> 
> To that end, create the SPARSE_INDEX_IGNORE_CONFIG flag that will skip
> these config checks when enabled. All current consumers are modified to
> pass '0' in the new 'flags' parameter.

...

> -int convert_to_sparse(struct index_state *istate)
> +int convert_to_sparse(struct index_state *istate, int flags)
>  {
>  	int test_env;
>  
> @@ -135,20 +135,22 @@ int convert_to_sparse(struct index_state *istate)
>  	if (!istate->repo)
>  		istate->repo = the_repository;
>  

Above this hunk, we fail automatically if the index has a split index.

The purpose of this flag is instead to say "convert to sparse for the
purpose of in-memory computations, not for writing to disk". For such
a case, we could move the split index check to be within the hunk
below. It would be appropriate to rename the flag to something like
SPARSE_INDEX_IN_MEMORY or SPARSE_INDEX_NO_DISK_WRITE to make the
intention more clear.

Thanks to SZEDER for pointing out this failure. I will fix it in the
next version.

> -	/*
> -	 * The GIT_TEST_SPARSE_INDEX environment variable triggers the
> -	 * index.sparse config variable to be on.
> -	 */
> -	test_env = git_env_bool("GIT_TEST_SPARSE_INDEX", -1);
> -	if (test_env >= 0)
> -		set_sparse_index_config(istate->repo, test_env);
> +	if (!(flags & SPARSE_INDEX_IGNORE_CONFIG)) {
> +		/*
> +		 * The GIT_TEST_SPARSE_INDEX environment variable triggers the
> +		 * index.sparse config variable to be on.
> +		 */
> +		test_env = git_env_bool("GIT_TEST_SPARSE_INDEX", -1);
> +		if (test_env >= 0)
> +			set_sparse_index_config(istate->repo, test_env);
>  
> -	/*
> -	 * Only convert to sparse if index.sparse is set.
> -	 */
> -	prepare_repo_settings(istate->repo);
> -	if (!istate->repo->settings.sparse_index)
> -		return 0;
> +		/*
> +		 * Only convert to sparse if index.sparse is set.
> +		 */
> +		prepare_repo_settings(istate->repo);
> +		if (!istate->repo->settings.sparse_index)
> +			return 0;
> +	}

If you want to try this, here is a diff that can help:

--- >8 ---

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index b06c8f885ac..c6a512a2107 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -131,7 +131,7 @@ static void clean_tracked_sparse_directories(struct repository *r)
 		 * prevents us from converting to a sparse index, then do
 		 * not try deleting files.
 		 */
-		if (convert_to_sparse(r->index, SPARSE_INDEX_IGNORE_CONFIG))
+		if (convert_to_sparse(r->index, SPARSE_INDEX_MEMORY_ONLY))
 			return;
 		was_full = 1;
 	}
diff --git a/sparse-index.c b/sparse-index.c
index e0a854f9fc3..267503b21fa 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -128,14 +128,20 @@ int convert_to_sparse(struct index_state *istate, int flags)
 {
 	int test_env;
 
-	if (istate->split_index || istate->sparse_index || !istate->cache_nr ||
+	if (istate->sparse_index || !istate->cache_nr ||
 	    !core_apply_sparse_checkout || !core_sparse_checkout_cone)
 		return 0;
 
 	if (!istate->repo)
 		istate->repo = the_repository;
 
-	if (!(flags & SPARSE_INDEX_IGNORE_CONFIG)) {
+	if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
+		/*
+		 * The sparse index is not integrated with a split index.
+		 */
+		if (istate->sparse_index)
+			return 0;
+
 		/*
 		 * The GIT_TEST_SPARSE_INDEX environment variable triggers the
 		 * index.sparse config variable to be on.
diff --git a/sparse-index.h b/sparse-index.h
index 475f4f0f8da..9f3d7bc7faf 100644
--- a/sparse-index.h
+++ b/sparse-index.h
@@ -2,7 +2,7 @@
 #define SPARSE_INDEX_H__
 
 struct index_state;
-#define SPARSE_INDEX_IGNORE_CONFIG (1 << 0)
+#define SPARSE_INDEX_MEMORY_ONLY (1 << 0)
 int convert_to_sparse(struct index_state *istate, int flags);
 
 /*


--- >8 ---

Thanks,
-Stolee

int was_full = !istate->sparse_index;

ret = convert_to_sparse(istate);
ret = convert_to_sparse(istate, 0);

if (ret) {
warning(_("failed to convert to a sparse-index"));
Expand Down Expand Up @@ -3182,7 +3182,7 @@ static int write_shared_index(struct index_state *istate,
int ret, was_full = !istate->sparse_index;

move_cache_to_base_index(istate);
convert_to_sparse(istate);
convert_to_sparse(istate, 0);

trace2_region_enter_printf("index", "shared/do_write_index",
the_repository, "%s", get_tempfile_path(*temp));
Expand Down
76 changes: 43 additions & 33 deletions sparse-index.c
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,14 @@ static int convert_to_sparse_rec(struct index_state *istate,
{
int i, can_convert = 1;
int start_converted = num_converted;
enum pattern_match_result match;
int dtype = DT_UNKNOWN;
struct strbuf child_path = STRBUF_INIT;
struct pattern_list *pl = istate->sparse_checkout_patterns;

/*
* 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);
if (match != NOT_MATCHED)
if (path_in_sparse_checkout(ct_path, istate))
can_convert = 0;

for (i = start; can_convert && i < end; i++) {
Expand Down Expand Up @@ -127,41 +122,51 @@ static int index_has_unmerged_entries(struct index_state *istate)
return 0;
}

int convert_to_sparse(struct index_state *istate)
int convert_to_sparse(struct index_state *istate, int flags)
{
int test_env;
if (istate->split_index || istate->sparse_index ||
if (istate->sparse_index || !istate->cache_nr ||
!core_apply_sparse_checkout || !core_sparse_checkout_cone)
return 0;

if (!istate->repo)
istate->repo = the_repository;

/*
* The GIT_TEST_SPARSE_INDEX environment variable triggers the
* index.sparse config variable to be on.
*/
test_env = git_env_bool("GIT_TEST_SPARSE_INDEX", -1);
if (test_env >= 0)
set_sparse_index_config(istate->repo, test_env);

/*
* Only convert to sparse if index.sparse is set.
*/
prepare_repo_settings(istate->repo);
if (!istate->repo->settings.sparse_index)
return 0;
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
/*
* The sparse index is not (yet) integrated with a split index.
*/
if (istate->split_index)
return 0;
/*
* The GIT_TEST_SPARSE_INDEX environment variable triggers the
* index.sparse config variable to be on.
*/
test_env = git_env_bool("GIT_TEST_SPARSE_INDEX", -1);
if (test_env >= 0)
set_sparse_index_config(istate->repo, test_env);

if (!istate->sparse_checkout_patterns) {
istate->sparse_checkout_patterns = xcalloc(1, sizeof(struct pattern_list));
if (get_sparse_checkout_patterns(istate->sparse_checkout_patterns) < 0)
/*
* Only convert to sparse if index.sparse is set.
*/
prepare_repo_settings(istate->repo);
if (!istate->repo->settings.sparse_index)
return 0;
}

if (!istate->sparse_checkout_patterns->use_cone_patterns) {
warning(_("attempting to use sparse-index without cone mode"));
return -1;
}
if (init_sparse_checkout_patterns(istate))
return 0;

/*
* We need cone-mode patterns to use sparse-index. If a user edits
* their sparse-checkout file manually, then we can detect during
* parsing that they are not actually using cone-mode patterns and
* hence we need to abort this conversion _without error_. Warnings
* already exist in the pattern parsing to inform the user of their
* bad patterns.
*/
if (!istate->sparse_checkout_patterns->use_cone_patterns)
return 0;

/*
* NEEDSWORK: If we have unmerged entries, then stay full.
Expand All @@ -172,10 +177,15 @@ int convert_to_sparse(struct index_state *istate)

/* Clear and recompute the cache-tree */
cache_tree_free(&istate->cache_tree);
if (cache_tree_update(istate, 0)) {
warning(_("unable to update cache-tree, staying full"));
return -1;
}
/*
* Silently return if there is a problem with the cache tree update,
* which might just be due to a conflict state in some entry.
*
* This might create new tree objects, so be sure to use
* WRITE_TREE_MISSING_OK.
*/
if (cache_tree_update(istate, WRITE_TREE_MISSING_OK))
return 0;

remove_fsmonitor(istate);

Expand Down
3 changes: 2 additions & 1 deletion sparse-index.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
#define SPARSE_INDEX_H__

struct index_state;
int convert_to_sparse(struct index_state *istate);
#define SPARSE_INDEX_MEMORY_ONLY (1 << 0)
int convert_to_sparse(struct index_state *istate, int flags);

/*
* Some places in the codebase expect to search for a specific path.
Expand Down
Loading