[PATCH] sparse-checkout: avoid staging deletions of all files

[PATCH] sparse-checkout: avoid staging deletions of all files

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

sparse-checkout's purpose is to update the working tree to have it
reflect a subset of the tracked files.  As such, it shouldn't be
switching branches, making commits, downloading or uploading data, or
staging or unstaging changes.  Other than updating the worktree, the
only thing sparse-checkout should touch is the SKIP_WORKTREE bit of the
index.  In particular, this sets up a nice invariant: running
sparse-checkout will never change the status of any file in `git status`
(reflecting the fact that we only set the SKIP_WORKTREE bit if the file
is safe to delete, i.e. if the file is unmodified).

Traditionally, we did a _really_ bad job with this goal.  The
predecessor to sparse-checkout involved manual editing of
.git/info/sparse-checkout and running `git read-tree -mu HEAD`.  That
command would stage and unstage changes and overwrite dirty changes in
the working tree.

The initial implementation of the sparse-checkout command was no better;
it simply invoked `git read-tree -mu HEAD` as a subprocess and had the
same caveats, though this issue came up repeatedly in review comments
and workarounds for the problems were put in place before the feature
was merged[1, 2, 3, 4, 5, 6; especially see 4 & 6].

[1] https://lore.kernel.org/git/CABPp-BFT9A5n=_bx5LsjCvbogqwSjiwgr5amcjgbU1iAk4KLJg@mail.gmail.com/
[2] https://lore.kernel.org/git/CABPp-BEmwSwg4tgJg6nVG8a3Hpn_g-=ZjApZF4EiJO+qVgu4uw@mail.gmail.com/
[3] https://lore.kernel.org/git/CABPp-BFV7TA0qwZCQpHCqx9N+JifyRyuBQ-pZ_oGfe-NOgyh7A@mail.gmail.com/
[4] https://lore.kernel.org/git/CABPp-BHYCCD+Vx5fq35jH82eHc1-P53Lz_aGNpHJNcx9kg2K-A@mail.gmail.com/
[5] https://lore.kernel.org/git/CABPp-BF+JWYZfDqp2Tn4AEKVp4b0YMA=Mbz4Nz62D-gGgiduYQ@mail.gmail.com/
[6] https://lore.kernel.org/git/20191121163706.GV23183@szeder.dev/

However, these workarounds, in addition to disabling the feature in a
number of important cases, also missed one special case.  I'll get back
to it later.

In the 2.27.0 cycle, the disabling of the feature was lifted by finally
replacing the internal equivalent of `git read-tree -mu HEAD` with
something that did what we wanted: the new update_sparsity() function in
unpack-trees.c that only ever updates SKIP_WORKTREE bits in the index
and updates the working tree to match.  This new function handles all
the cases that were problematic for the old implementation, except that
it breaks the same special case that avoided the workarounds of the old
implementation, but broke it in a different way.

So...that brings us to the special case: a git clone performed with
--no-checkout.  As per the meaning of the flag, --no-checkout does not
check out any branch, with the implication that you aren't on one and
need to switch to one after the clone.  Implementationally, HEAD is
still set (so in some sense you are partially on a branch), but
  * the index is "unborn" (non-existent)
  * there are no files in the working tree (other than .git/)
  * the next time git switch (or git checkout) is run it will run
    unpack_trees with `initial_checkout` flag set to true.
It is not until you run, e.g. `git switch <somebranch>` that the index
will be written and files in the working tree populated.

With this special --no-checkout case, the traditional `read-tree -mu
HEAD` behavior would have done the equivalent of acting like checkout --
switch to the default branch (HEAD), write out an index that matches
HEAD, and update the working tree to match.  This special case slipped
through the avoid-making-changes checks in the original sparse-checkout
command and thus continued there.

After update_sparsity() was introduced and used (see commit f56f31af03
("sparse-checkout: use new update_sparsity() function", 2020-03-27)),
the behavior for the --no-checkout case changed:  Due to git's
auto-vivification of an empty in-memory index (see do_read_index() and
note that `must_exist` is false), and due to sparse-checkout's
update_working_directory() code to always write out the index after it
was done, we got a new bug.  That made it so that sparse-checkout would
switch the repository from a clone with an "unborn" index (i.e. still
needing an initial_checkout), to one that had a recorded index with no
entries.  Thus, instead of all the files appearing deleted in `git
status` being known to git as a special artifact of not yet being on a
branch, our recording of an empty index made it suddenly look to git as
though it was definitely on a branch with ALL files staged for deletion!
A subsequent checkout or switch then had to contend with the fact that
it wasn't on an initial_checkout but had a bunch of staged deletions.

Make sure that sparse-checkout changes nothing in the index other than
the SKIP_WORKTREE bit; in particular, when the index is unborn we do not
have any branch checked out so there is no sparsification or
de-sparsification work to do.  Simply return from
update_working_directory() early.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    sparse-checkout: avoid staging deletions of all files

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-801%2Fnewren%2Fsparse-checkout-and-unborn-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-801/newren/sparse-checkout-and-unborn-index-v1
Pull-Request: https://github.com/git/git/pull/801

 builtin/sparse-checkout.c          |  4 ++++
 t/t1091-sparse-checkout-builtin.sh | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 95d08824172..595463be68e 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -99,6 +99,10 @@ static int update_working_directory(struct pattern_list *pl)
 	struct lock_file lock_file = LOCK_INIT;
 	struct repository *r = the_repository;
 
+	/* If no branch has been checked out, there are no updates to make. */
+	if (is_index_unborn(r->index))
+		return UPDATE_SPARSITY_SUCCESS;
+
 	memset(&o, 0, sizeof(o));
 	o.verbose_update = isatty(2);
 	o.update = 1;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 88cdde255cd..bc287e5c1fa 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -100,6 +100,25 @@ test_expect_success 'clone --sparse' '
 	check_files clone a
 '
 
+test_expect_success 'interaction with clone --no-checkout (unborn index)' '
+	git clone --no-checkout "file://$(pwd)/repo" clone_no_checkout &&
+	git -C clone_no_checkout sparse-checkout init --cone &&
+	git -C clone_no_checkout sparse-checkout set folder1 &&
+	git -C clone_no_checkout sparse-checkout list >actual &&
+	cat >expect <<-\EOF &&
+	folder1
+	EOF
+	test_cmp expect actual &&
+	ls clone_no_checkout >actual &&
+	test_must_be_empty actual &&
+	test_path_is_missing clone_no_checkout/.git/index &&
+
+	# No branch is checked out until we manually switch to one
+	git -C clone_no_checkout switch master &&
+	test_path_is_file clone_no_checkout/.git/index &&
+	check_files clone_no_checkout a folder1
+'
+
 test_expect_success 'set enables config' '
 	git init empty-config &&
 	(

base-commit: 20514004ddf1a3528de8933bc32f284e175e1012
-- 
gitgitgadget

Re: [PATCH] sparse-checkout: avoid staging deletions of all files

[Derrick Stolee]

On 6/4/2020 4:17 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> sparse-checkout's purpose is to update the working tree to have it
> reflect a subset of the tracked files.  As such, it shouldn't be
> switching branches, making commits, downloading or uploading data, or
> staging or unstaging changes.  Other than updating the worktree, the
> only thing sparse-checkout should touch is the SKIP_WORKTREE bit of the
> index.  In particular, this sets up a nice invariant: running
> sparse-checkout will never change the status of any file in `git status`
> (reflecting the fact that we only set the SKIP_WORKTREE bit if the file
> is safe to delete, i.e. if the file is unmodified).
> 
> Traditionally, we did a _really_ bad job with this goal.  The
> predecessor to sparse-checkout involved manual editing of
> .git/info/sparse-checkout and running `git read-tree -mu HEAD`.  That
> command would stage and unstage changes and overwrite dirty changes in
> the working tree.
> 
> The initial implementation of the sparse-checkout command was no better;
> it simply invoked `git read-tree -mu HEAD` as a subprocess and had the
> same caveats, though this issue came up repeatedly in review comments
> and workarounds for the problems were put in place before the feature
> was merged[1, 2, 3, 4, 5, 6; especially see 4 & 6].
> 
> [1] https://lore.kernel.org/git/CABPp-BFT9A5n=_bx5LsjCvbogqwSjiwgr5amcjgbU1iAk4KLJg@mail.gmail.com/
> [2] https://lore.kernel.org/git/CABPp-BEmwSwg4tgJg6nVG8a3Hpn_g-=ZjApZF4EiJO+qVgu4uw@mail.gmail.com/
> [3] https://lore.kernel.org/git/CABPp-BFV7TA0qwZCQpHCqx9N+JifyRyuBQ-pZ_oGfe-NOgyh7A@mail.gmail.com/
> [4] https://lore.kernel.org/git/CABPp-BHYCCD+Vx5fq35jH82eHc1-P53Lz_aGNpHJNcx9kg2K-A@mail.gmail.com/
> [5] https://lore.kernel.org/git/CABPp-BF+JWYZfDqp2Tn4AEKVp4b0YMA=Mbz4Nz62D-gGgiduYQ@mail.gmail.com/
> [6] https://lore.kernel.org/git/20191121163706.GV23183@szeder.dev/
> 
> However, these workarounds, in addition to disabling the feature in a
> number of important cases, also missed one special case.  I'll get back
> to it later.
> 
> In the 2.27.0 cycle, the disabling of the feature was lifted by finally
> replacing the internal equivalent of `git read-tree -mu HEAD` with
> something that did what we wanted: the new update_sparsity() function in
> unpack-trees.c that only ever updates SKIP_WORKTREE bits in the index
> and updates the working tree to match.  This new function handles all
> the cases that were problematic for the old implementation, except that
> it breaks the same special case that avoided the workarounds of the old
> implementation, but broke it in a different way.
> 
> So...that brings us to the special case: a git clone performed with
> --no-checkout.  As per the meaning of the flag, --no-checkout does not
> check out any branch, with the implication that you aren't on one and
> need to switch to one after the clone.  Implementationally, HEAD is
> still set (so in some sense you are partially on a branch), but
>   * the index is "unborn" (non-existent)
>   * there are no files in the working tree (other than .git/)
>   * the next time git switch (or git checkout) is run it will run
>     unpack_trees with `initial_checkout` flag set to true.
> It is not until you run, e.g. `git switch <somebranch>` that the index
> will be written and files in the working tree populated.
> 
> With this special --no-checkout case, the traditional `read-tree -mu
> HEAD` behavior would have done the equivalent of acting like checkout --
> switch to the default branch (HEAD), write out an index that matches
> HEAD, and update the working tree to match.  This special case slipped
> through the avoid-making-changes checks in the original sparse-checkout
> command and thus continued there.
> 
> After update_sparsity() was introduced and used (see commit f56f31af03
> ("sparse-checkout: use new update_sparsity() function", 2020-03-27)),
> the behavior for the --no-checkout case changed:  Due to git's
> auto-vivification of an empty in-memory index (see do_read_index() and
> note that `must_exist` is false), and due to sparse-checkout's
> update_working_directory() code to always write out the index after it
> was done, we got a new bug.  That made it so that sparse-checkout would
> switch the repository from a clone with an "unborn" index (i.e. still
> needing an initial_checkout), to one that had a recorded index with no
> entries.  Thus, instead of all the files appearing deleted in `git
> status` being known to git as a special artifact of not yet being on a
> branch, our recording of an empty index made it suddenly look to git as
> though it was definitely on a branch with ALL files staged for deletion!
> A subsequent checkout or switch then had to contend with the fact that
> it wasn't on an initial_checkout but had a bunch of staged deletions.
 
Thank you for the detailed summary of how we got to this point and
what is going on. When I asked you to look into this, I did ask for
you to spend "less than an hour" only because I hoped the fix would
be easy. Clearly, there are a lot of things going on here, but you
seem to have a firm grasp on everything.

> Make sure that sparse-checkout changes nothing in the index other than
> the SKIP_WORKTREE bit; in particular, when the index is unborn we do not
> have any branch checked out so there is no sparsification or
> de-sparsification work to do.  Simply return from
> update_working_directory() early.

This makes sense to me. The sparse-checkout file will still be
modified, so when the user performs a checkout then Git will
respect the set patterns. Your test confirms this, too!

I think this behavior is a reasonable compromise from the
previous behavior and being as correct as possible now. Before,
the "git read-tree -mu HEAD" portion of "git sparse-chekcout set"
would populate the working directory, and now it will not.
This more closely matches how a user interacts with --no-checkout
in the non-sparse case.

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>     sparse-checkout: avoid staging deletions of all files
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-801%2Fnewren%2Fsparse-checkout-and-unborn-index-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-801/newren/sparse-checkout-and-unborn-index-v1
> Pull-Request: https://github.com/git/git/pull/801
> 
>  builtin/sparse-checkout.c          |  4 ++++
>  t/t1091-sparse-checkout-builtin.sh | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 95d08824172..595463be68e 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -99,6 +99,10 @@ static int update_working_directory(struct pattern_list *pl)
>  	struct lock_file lock_file = LOCK_INIT;
>  	struct repository *r = the_repository;
>  
> +	/* If no branch has been checked out, there are no updates to make. */
> +	if (is_index_unborn(r->index))
> +		return UPDATE_SPARSITY_SUCCESS;
> +
>  	memset(&o, 0, sizeof(o));
>  	o.verbose_update = isatty(2);
>  	o.update = 1;
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 88cdde255cd..bc287e5c1fa 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -100,6 +100,25 @@ test_expect_success 'clone --sparse' '
>  	check_files clone a
>  '
>  
> +test_expect_success 'interaction with clone --no-checkout (unborn index)' '
> +	git clone --no-checkout "file://$(pwd)/repo" clone_no_checkout &&
> +	git -C clone_no_checkout sparse-checkout init --cone &&
> +	git -C clone_no_checkout sparse-checkout set folder1 &&
> +	git -C clone_no_checkout sparse-checkout list >actual &&
> +	cat >expect <<-\EOF &&
> +	folder1
> +	EOF
> +	test_cmp expect actual &&
> +	ls clone_no_checkout >actual &&
> +	test_must_be_empty actual &&

My only comment on the test case is to see if you could use
the "check_files" macro instead of "ls". See 761e3d26
(sparse-checkout: improve OS ls compatibility, 2019-12-20)
for details.

> +	test_path_is_missing clone_no_checkout/.git/index &&
> +
> +	# No branch is checked out until we manually switch to one
> +	git -C clone_no_checkout switch master &&
> +	test_path_is_file clone_no_checkout/.git/index &&
> +	check_files clone_no_checkout a folder1
> +'

Thanks!
-Stolee

Re: [PATCH] sparse-checkout: avoid staging deletions of all files

[Elijah Newren]

On Thu, Jun 4, 2020 at 7:48 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 6/4/2020 4:17 AM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > sparse-checkout's purpose is to update the working tree to have it
> > reflect a subset of the tracked files.  As such, it shouldn't be
> > switching branches, making commits, downloading or uploading data, or
> > staging or unstaging changes.  Other than updating the worktree, the
> > only thing sparse-checkout should touch is the SKIP_WORKTREE bit of the
> > index.  In particular, this sets up a nice invariant: running
> > sparse-checkout will never change the status of any file in `git status`
> > (reflecting the fact that we only set the SKIP_WORKTREE bit if the file
> > is safe to delete, i.e. if the file is unmodified).
> >
> > Traditionally, we did a _really_ bad job with this goal.  The
> > predecessor to sparse-checkout involved manual editing of
> > .git/info/sparse-checkout and running `git read-tree -mu HEAD`.  That
> > command would stage and unstage changes and overwrite dirty changes in
> > the working tree.
> >
> > The initial implementation of the sparse-checkout command was no better;
> > it simply invoked `git read-tree -mu HEAD` as a subprocess and had the
> > same caveats, though this issue came up repeatedly in review comments
> > and workarounds for the problems were put in place before the feature
> > was merged[1, 2, 3, 4, 5, 6; especially see 4 & 6].
> >
> > [1] https://lore.kernel.org/git/CABPp-BFT9A5n=_bx5LsjCvbogqwSjiwgr5amcjgbU1iAk4KLJg@mail.gmail.com/
> > [2] https://lore.kernel.org/git/CABPp-BEmwSwg4tgJg6nVG8a3Hpn_g-=ZjApZF4EiJO+qVgu4uw@mail.gmail.com/
> > [3] https://lore.kernel.org/git/CABPp-BFV7TA0qwZCQpHCqx9N+JifyRyuBQ-pZ_oGfe-NOgyh7A@mail.gmail.com/
> > [4] https://lore.kernel.org/git/CABPp-BHYCCD+Vx5fq35jH82eHc1-P53Lz_aGNpHJNcx9kg2K-A@mail.gmail.com/
> > [5] https://lore.kernel.org/git/CABPp-BF+JWYZfDqp2Tn4AEKVp4b0YMA=Mbz4Nz62D-gGgiduYQ@mail.gmail.com/
> > [6] https://lore.kernel.org/git/20191121163706.GV23183@szeder.dev/
> >
> > However, these workarounds, in addition to disabling the feature in a
> > number of important cases, also missed one special case.  I'll get back
> > to it later.
> >
> > In the 2.27.0 cycle, the disabling of the feature was lifted by finally
> > replacing the internal equivalent of `git read-tree -mu HEAD` with
> > something that did what we wanted: the new update_sparsity() function in
> > unpack-trees.c that only ever updates SKIP_WORKTREE bits in the index
> > and updates the working tree to match.  This new function handles all
> > the cases that were problematic for the old implementation, except that
> > it breaks the same special case that avoided the workarounds of the old
> > implementation, but broke it in a different way.
> >
> > So...that brings us to the special case: a git clone performed with
> > --no-checkout.  As per the meaning of the flag, --no-checkout does not
> > check out any branch, with the implication that you aren't on one and
> > need to switch to one after the clone.  Implementationally, HEAD is
> > still set (so in some sense you are partially on a branch), but
> >   * the index is "unborn" (non-existent)
> >   * there are no files in the working tree (other than .git/)
> >   * the next time git switch (or git checkout) is run it will run
> >     unpack_trees with `initial_checkout` flag set to true.
> > It is not until you run, e.g. `git switch <somebranch>` that the index
> > will be written and files in the working tree populated.
> >
> > With this special --no-checkout case, the traditional `read-tree -mu
> > HEAD` behavior would have done the equivalent of acting like checkout --
> > switch to the default branch (HEAD), write out an index that matches
> > HEAD, and update the working tree to match.  This special case slipped
> > through the avoid-making-changes checks in the original sparse-checkout
> > command and thus continued there.
> >
> > After update_sparsity() was introduced and used (see commit f56f31af03
> > ("sparse-checkout: use new update_sparsity() function", 2020-03-27)),
> > the behavior for the --no-checkout case changed:  Due to git's
> > auto-vivification of an empty in-memory index (see do_read_index() and
> > note that `must_exist` is false), and due to sparse-checkout's
> > update_working_directory() code to always write out the index after it
> > was done, we got a new bug.  That made it so that sparse-checkout would
> > switch the repository from a clone with an "unborn" index (i.e. still
> > needing an initial_checkout), to one that had a recorded index with no
> > entries.  Thus, instead of all the files appearing deleted in `git
> > status` being known to git as a special artifact of not yet being on a
> > branch, our recording of an empty index made it suddenly look to git as
> > though it was definitely on a branch with ALL files staged for deletion!
> > A subsequent checkout or switch then had to contend with the fact that
> > it wasn't on an initial_checkout but had a bunch of staged deletions.
>
> Thank you for the detailed summary of how we got to this point and
> what is going on. When I asked you to look into this, I did ask for
> you to spend "less than an hour" only because I hoped the fix would
> be easy. Clearly, there are a lot of things going on here, but you
> seem to have a firm grasp on everything.
>
> > Make sure that sparse-checkout changes nothing in the index other than
> > the SKIP_WORKTREE bit; in particular, when the index is unborn we do not
> > have any branch checked out so there is no sparsification or
> > de-sparsification work to do.  Simply return from
> > update_working_directory() early.
>
> This makes sense to me. The sparse-checkout file will still be
> modified, so when the user performs a checkout then Git will
> respect the set patterns. Your test confirms this, too!
>
> I think this behavior is a reasonable compromise from the
> previous behavior and being as correct as possible now. Before,
> the "git read-tree -mu HEAD" portion of "git sparse-chekcout set"
> would populate the working directory, and now it will not.
> This more closely matches how a user interacts with --no-checkout
> in the non-sparse case.
>
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >     sparse-checkout: avoid staging deletions of all files
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-801%2Fnewren%2Fsparse-checkout-and-unborn-index-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-801/newren/sparse-checkout-and-unborn-index-v1
> > Pull-Request: https://github.com/git/git/pull/801
> >
> >  builtin/sparse-checkout.c          |  4 ++++
> >  t/t1091-sparse-checkout-builtin.sh | 19 +++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> > index 95d08824172..595463be68e 100644
> > --- a/builtin/sparse-checkout.c
> > +++ b/builtin/sparse-checkout.c
> > @@ -99,6 +99,10 @@ static int update_working_directory(struct pattern_list *pl)
> >       struct lock_file lock_file = LOCK_INIT;
> >       struct repository *r = the_repository;
> >
> > +     /* If no branch has been checked out, there are no updates to make. */
> > +     if (is_index_unborn(r->index))
> > +             return UPDATE_SPARSITY_SUCCESS;
> > +
> >       memset(&o, 0, sizeof(o));
> >       o.verbose_update = isatty(2);
> >       o.update = 1;
> > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> > index 88cdde255cd..bc287e5c1fa 100755
> > --- a/t/t1091-sparse-checkout-builtin.sh
> > +++ b/t/t1091-sparse-checkout-builtin.sh
> > @@ -100,6 +100,25 @@ test_expect_success 'clone --sparse' '
> >       check_files clone a
> >  '
> >
> > +test_expect_success 'interaction with clone --no-checkout (unborn index)' '
> > +     git clone --no-checkout "file://$(pwd)/repo" clone_no_checkout &&
> > +     git -C clone_no_checkout sparse-checkout init --cone &&
> > +     git -C clone_no_checkout sparse-checkout set folder1 &&
> > +     git -C clone_no_checkout sparse-checkout list >actual &&
> > +     cat >expect <<-\EOF &&
> > +     folder1
> > +     EOF
> > +     test_cmp expect actual &&
> > +     ls clone_no_checkout >actual &&
> > +     test_must_be_empty actual &&
>
> My only comment on the test case is to see if you could use
> the "check_files" macro instead of "ls". See 761e3d26
> (sparse-checkout: improve OS ls compatibility, 2019-12-20)
> for details.

I attempted to do so initially, but that function fails badly when
there are no files (other than the "hidden" files '.git', '.', and
'..') in the directory.  The reason for this comes from the "printf
'%s\n' *" -- the glob won't match anything and so it prints a literal
asterisk, which is not helpful.

I thought about writing an asterisk out to the expected file for
comparison, but that just made the testcase look confusing.  It was a
lot cleaner to just use ls with no glob coupled with
test_must_be_empty.

> > +     test_path_is_missing clone_no_checkout/.git/index &&
> > +
> > +     # No branch is checked out until we manually switch to one
> > +     git -C clone_no_checkout switch master &&
> > +     test_path_is_file clone_no_checkout/.git/index &&
> > +     check_files clone_no_checkout a folder1

However, when I did have files in the directory, then I used your
check_files function as seen here.  :-)


Does that make sense, or is there a better alternative?

Re: [PATCH] sparse-checkout: avoid staging deletions of all files

[Derrick Stolee]

On 6/4/2020 11:05 AM, Elijah Newren wrote:
> On Thu, Jun 4, 2020 at 7:48 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 6/4/2020 4:17 AM, Elijah Newren via GitGitGadget wrote:
>>> +test_expect_success 'interaction with clone --no-checkout (unborn index)' '
>>> +     git clone --no-checkout "file://$(pwd)/repo" clone_no_checkout &&
>>> +     git -C clone_no_checkout sparse-checkout init --cone &&
>>> +     git -C clone_no_checkout sparse-checkout set folder1 &&
>>> +     git -C clone_no_checkout sparse-checkout list >actual &&
>>> +     cat >expect <<-\EOF &&
>>> +     folder1
>>> +     EOF
>>> +     test_cmp expect actual &&
>>> +     ls clone_no_checkout >actual &&
>>> +     test_must_be_empty actual &&
>>
>> My only comment on the test case is to see if you could use
>> the "check_files" macro instead of "ls". See 761e3d26
>> (sparse-checkout: improve OS ls compatibility, 2019-12-20)
>> for details.
> 
> I attempted to do so initially, but that function fails badly when
> there are no files (other than the "hidden" files '.git', '.', and
> '..') in the directory.  The reason for this comes from the "printf
> '%s\n' *" -- the glob won't match anything and so it prints a literal
> asterisk, which is not helpful.
> 
> I thought about writing an asterisk out to the expected file for
> comparison, but that just made the testcase look confusing.  It was a
> lot cleaner to just use ls with no glob coupled with
> test_must_be_empty.
> 
>>> +     test_path_is_missing clone_no_checkout/.git/index &&
>>> +
>>> +     # No branch is checked out until we manually switch to one
>>> +     git -C clone_no_checkout switch master &&
>>> +     test_path_is_file clone_no_checkout/.git/index &&
>>> +     check_files clone_no_checkout a folder1
> 
> However, when I did have files in the directory, then I used your
> check_files function as seen here.  :-)
> 
> 
> Does that make sense, or is there a better alternative?

Unfortunately, we'll still have the platform issue, since there
_is_ a .git directory. To repeat the commit message for 761e3d26:

    On FreeBSD, when executed by root ls enables the '-A' option:
    
      -A  Include directory entries whose names begin with a dot (`.')
          except for . and ...  Automatically set for the super-user unless
          -I is specified.
    
    As a result the .git directory appeared in the output when run as root.
    Simulate no-dotfile ls behaviour using a shell glob.

So maybe be purposeful and include the -a option and expect only the
.git dir (along with . and ..)?

Something like:

	cat >expect <<-\EOF &&
	.
	..
	.git
	EOF
	ls -a clone_no_checkout >actual &&
	test_cmp expect actual &&

Thanks,
-Stolee

Re: [PATCH] sparse-checkout: avoid staging deletions of all files

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> ...  Thus, instead of all the files appearing deleted in `git
> status` being known to git as a special artifact of not yet being on a
> branch, our recording of an empty index made it suddenly look to git as
> though it was definitely on a branch with ALL files staged for deletion!
> A subsequent checkout or switch then had to contend with the fact that
> it wasn't on an initial_checkout but had a bunch of staged deletions.

Nicely analysed and explained.

> Make sure that sparse-checkout changes nothing in the index other than
> the SKIP_WORKTREE bit; in particular, when the index is unborn we do not
> have any branch checked out so there is no sparsification or
> de-sparsification work to do.  Simply return from
> update_working_directory() early.

OK, and that would avoid writing out an empty index, leaving the
"is_index_unborn()" still true after we are done.  Makes sense.

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>     sparse-checkout: avoid staging deletions of all files
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-801%2Fnewren%2Fsparse-checkout-and-unborn-index-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-801/newren/sparse-checkout-and-unborn-index-v1
> Pull-Request: https://github.com/git/git/pull/801
>
>  builtin/sparse-checkout.c          |  4 ++++
>  t/t1091-sparse-checkout-builtin.sh | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 95d08824172..595463be68e 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -99,6 +99,10 @@ static int update_working_directory(struct pattern_list *pl)
>  	struct lock_file lock_file = LOCK_INIT;
>  	struct repository *r = the_repository;
>  
> +	/* If no branch has been checked out, there are no updates to make. */
> +	if (is_index_unborn(r->index))
> +		return UPDATE_SPARSITY_SUCCESS;
> +
>  	memset(&o, 0, sizeof(o));
>  	o.verbose_update = isatty(2);
>  	o.update = 1;
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 88cdde255cd..bc287e5c1fa 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -100,6 +100,25 @@ test_expect_success 'clone --sparse' '
>  	check_files clone a
>  '
>  
> +test_expect_success 'interaction with clone --no-checkout (unborn index)' '
> +	git clone --no-checkout "file://$(pwd)/repo" clone_no_checkout &&
> +	git -C clone_no_checkout sparse-checkout init --cone &&
> +	git -C clone_no_checkout sparse-checkout set folder1 &&
> +	git -C clone_no_checkout sparse-checkout list >actual &&
> +	cat >expect <<-\EOF &&
> +	folder1
> +	EOF
> +	test_cmp expect actual &&
> +	ls clone_no_checkout >actual &&
> +	test_must_be_empty actual &&
> +	test_path_is_missing clone_no_checkout/.git/index &&
> +
> +	# No branch is checked out until we manually switch to one
> +	git -C clone_no_checkout switch master &&
> +	test_path_is_file clone_no_checkout/.git/index &&
> +	check_files clone_no_checkout a folder1
> +'
> +
>  test_expect_success 'set enables config' '
>  	git init empty-config &&
>  	(
>
> base-commit: 20514004ddf1a3528de8933bc32f284e175e1012

Re: [PATCH] sparse-checkout: avoid staging deletions of all files

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> So maybe be purposeful and include the -a option and expect only the
> .git dir (along with . and ..)?
>
> Something like:
>
> 	cat >expect <<-\EOF &&
> 	.
> 	..
> 	.git
> 	EOF
> 	ls -a clone_no_checkout >actual &&
> 	test_cmp expect actual &&

Is it guaranteed that everybody has dot and dot-dot in their
directory?  

  https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html

has this to describe the "-A" option:

    Write out all directory entries, including those whose names
    begin with a <period> ( '.' ) but excluding the entries dot and
    dot-dot (if they exist).

whose "if they exist" scares me ;-)

Re: [PATCH] sparse-checkout: avoid staging deletions of all files

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> My only comment on the test case is to see if you could use
>> the "check_files" macro instead of "ls". See 761e3d26
>> (sparse-checkout: improve OS ls compatibility, 2019-12-20)
>> for details.
>
> I attempted to do so initially, but that function fails badly when
> there are no files (other than the "hidden" files '.git', '.', and
> '..') in the directory.  The reason for this comes from the "printf
> '%s\n' *" -- the glob won't match anything and so it prints a literal
> asterisk, which is not helpful.
>
> I thought about writing an asterisk out to the expected file for
> comparison, but that just made the testcase look confusing.

Maybe.  It is clear that the implementation ignores the possibility
that it can be asked to check for an empty directory.

    list_files() {
            # Do not replace this with 'ls "$1"', as "ls" with BSD-lineage
            # enables "-A" by default for root and ends up including ".git" and
            # such in its output. (Note, though, that running the test suite as
            # root is generally not recommended.)
            (cd "$1" && printf '%s\n' *)
    }

The comment does not even talk about the possibility that glob '*'
may fail to glob anything.

Instead of letting the macro spend an extra subprocess, using ls
ourselves but in a bit more defensive way would help?  I.e.

	...
	# nothing checked out, expect "No such file or directory"
	! ls clone_no_checkout/* >actual &&
	test_must_be_empty actual
	test_path_is_missing clone_no_checkout/.git/index
	...

Re: [PATCH] sparse-checkout: avoid staging deletions of all files

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>     list_files() {
>             # Do not replace this with 'ls "$1"', as "ls" with BSD-lineage
>             # enables "-A" by default for root and ends up including ".git" and
>             # such in its output. (Note, though, that running the test suite as
>             # root is generally not recommended.)
>             (cd "$1" && printf '%s\n' *)
>     }

An unrelated tangent, but it took me more time than necessary to
find where these "macros" are defined because

	git grep 'check_files () {'

found no matches.  That is why I prefer to stress about styles (no,
looking for "check_files *()" is not a solution).

[PATCH v2] sparse-checkout: avoid staging deletions of all files

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

sparse-checkout's purpose is to update the working tree to have it
reflect a subset of the tracked files.  As such, it shouldn't be
switching branches, making commits, downloading or uploading data, or
staging or unstaging changes.  Other than updating the worktree, the
only thing sparse-checkout should touch is the SKIP_WORKTREE bit of the
index.  In particular, this sets up a nice invariant: running
sparse-checkout will never change the status of any file in `git status`
(reflecting the fact that we only set the SKIP_WORKTREE bit if the file
is safe to delete, i.e. if the file is unmodified).

Traditionally, we did a _really_ bad job with this goal.  The
predecessor to sparse-checkout involved manual editing of
.git/info/sparse-checkout and running `git read-tree -mu HEAD`.  That
command would stage and unstage changes and overwrite dirty changes in
the working tree.

The initial implementation of the sparse-checkout command was no better;
it simply invoked `git read-tree -mu HEAD` as a subprocess and had the
same caveats, though this issue came up repeatedly in review comments
and workarounds for the problems were put in place before the feature
was merged[1, 2, 3, 4, 5, 6; especially see 4 & 6].

[1] https://lore.kernel.org/git/CABPp-BFT9A5n=_bx5LsjCvbogqwSjiwgr5amcjgbU1iAk4KLJg@mail.gmail.com/
[2] https://lore.kernel.org/git/CABPp-BEmwSwg4tgJg6nVG8a3Hpn_g-=ZjApZF4EiJO+qVgu4uw@mail.gmail.com/
[3] https://lore.kernel.org/git/CABPp-BFV7TA0qwZCQpHCqx9N+JifyRyuBQ-pZ_oGfe-NOgyh7A@mail.gmail.com/
[4] https://lore.kernel.org/git/CABPp-BHYCCD+Vx5fq35jH82eHc1-P53Lz_aGNpHJNcx9kg2K-A@mail.gmail.com/
[5] https://lore.kernel.org/git/CABPp-BF+JWYZfDqp2Tn4AEKVp4b0YMA=Mbz4Nz62D-gGgiduYQ@mail.gmail.com/
[6] https://lore.kernel.org/git/20191121163706.GV23183@szeder.dev/

However, these workarounds, in addition to disabling the feature in a
number of important cases, also missed one special case.  I'll get back
to it later.

In the 2.27.0 cycle, the disabling of the feature was lifted by finally
replacing the internal equivalent of `git read-tree -mu HEAD` with
something that did what we wanted: the new update_sparsity() function in
unpack-trees.c that only ever updates SKIP_WORKTREE bits in the index
and updates the working tree to match.  This new function handles all
the cases that were problematic for the old implementation, except that
it breaks the same special case that avoided the workarounds of the old
implementation, but broke it in a different way.

So...that brings us to the special case: a git clone performed with
--no-checkout.  As per the meaning of the flag, --no-checkout does not
check out any branch, with the implication that you aren't on one and
need to switch to one after the clone.  Implementationally, HEAD is
still set (so in some sense you are partially on a branch), but
  * the index is "unborn" (non-existent)
  * there are no files in the working tree (other than .git/)
  * the next time git switch (or git checkout) is run it will run
    unpack_trees with `initial_checkout` flag set to true.
It is not until you run, e.g. `git switch <somebranch>` that the index
will be written and files in the working tree populated.

With this special --no-checkout case, the traditional `read-tree -mu
HEAD` behavior would have done the equivalent of acting like checkout --
switch to the default branch (HEAD), write out an index that matches
HEAD, and update the working tree to match.  This special case slipped
through the avoid-making-changes checks in the original sparse-checkout
command and thus continued there.

After update_sparsity() was introduced and used (see commit f56f31af03
("sparse-checkout: use new update_sparsity() function", 2020-03-27)),
the behavior for the --no-checkout case changed:  Due to git's
auto-vivification of an empty in-memory index (see do_read_index() and
note that `must_exist` is false), and due to sparse-checkout's
update_working_directory() code to always write out the index after it
was done, we got a new bug.  That made it so that sparse-checkout would
switch the repository from a clone with an "unborn" index (i.e. still
needing an initial_checkout), to one that had a recorded index with no
entries.  Thus, instead of all the files appearing deleted in `git
status` being known to git as a special artifact of not yet being on a
branch, our recording of an empty index made it suddenly look to git as
though it was definitely on a branch with ALL files staged for deletion!
A subsequent checkout or switch then had to contend with the fact that
it wasn't on an initial_checkout but had a bunch of staged deletions.

Make sure that sparse-checkout changes nothing in the index other than
the SKIP_WORKTREE bit; in particular, when the index is unborn we do not
have any branch checked out so there is no sparsification or
de-sparsification work to do.  Simply return from
update_working_directory() early.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    sparse-checkout: avoid staging deletions of all files
    
    Changes since v1:
    
     * Fixed the ls command for portability, using the construct Junio
       suggested

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-801%2Fnewren%2Fsparse-checkout-and-unborn-index-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-801/newren/sparse-checkout-and-unborn-index-v2
Pull-Request: https://github.com/git/git/pull/801

Range-diff vs v1:

 1:  4c8b512506e ! 1:  8be6550d11b sparse-checkout: avoid staging deletions of all files
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'clone --sparse' '
      +	git clone --no-checkout "file://$(pwd)/repo" clone_no_checkout &&
      +	git -C clone_no_checkout sparse-checkout init --cone &&
      +	git -C clone_no_checkout sparse-checkout set folder1 &&
     ++
      +	git -C clone_no_checkout sparse-checkout list >actual &&
      +	cat >expect <<-\EOF &&
      +	folder1
      +	EOF
      +	test_cmp expect actual &&
     -+	ls clone_no_checkout >actual &&
     ++
     ++	# nothing checked out, expect "No such file or directory"
     ++	! ls clone_no_checkout/* >actual &&
      +	test_must_be_empty actual &&
      +	test_path_is_missing clone_no_checkout/.git/index &&
      +


 builtin/sparse-checkout.c          |  4 ++++
 t/t1091-sparse-checkout-builtin.sh | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 95d08824172..595463be68e 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -99,6 +99,10 @@ static int update_working_directory(struct pattern_list *pl)
 	struct lock_file lock_file = LOCK_INIT;
 	struct repository *r = the_repository;
 
+	/* If no branch has been checked out, there are no updates to make. */
+	if (is_index_unborn(r->index))
+		return UPDATE_SPARSITY_SUCCESS;
+
 	memset(&o, 0, sizeof(o));
 	o.verbose_update = isatty(2);
 	o.update = 1;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 88cdde255cd..7cd45fc1394 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -100,6 +100,28 @@ test_expect_success 'clone --sparse' '
 	check_files clone a
 '
 
+test_expect_success 'interaction with clone --no-checkout (unborn index)' '
+	git clone --no-checkout "file://$(pwd)/repo" clone_no_checkout &&
+	git -C clone_no_checkout sparse-checkout init --cone &&
+	git -C clone_no_checkout sparse-checkout set folder1 &&
+
+	git -C clone_no_checkout sparse-checkout list >actual &&
+	cat >expect <<-\EOF &&
+	folder1
+	EOF
+	test_cmp expect actual &&
+
+	# nothing checked out, expect "No such file or directory"
+	! ls clone_no_checkout/* >actual &&
+	test_must_be_empty actual &&
+	test_path_is_missing clone_no_checkout/.git/index &&
+
+	# No branch is checked out until we manually switch to one
+	git -C clone_no_checkout switch master &&
+	test_path_is_file clone_no_checkout/.git/index &&
+	check_files clone_no_checkout a folder1
+'
+
 test_expect_success 'set enables config' '
 	git init empty-config &&
 	(

base-commit: 20514004ddf1a3528de8933bc32f284e175e1012
-- 
gitgitgadget