[PATCH 0/5] nd/fix-sparse-checkout v2

[PATCH 0/5] nd/fix-sparse-checkout v2

[Nguyễn Thái Ngọc Duy]

Discussion of patch 1/3 from the first series resulted in 2/5 in this
series. 2/3 and 3/3 from the former have commit messages revised,
code change remains the same.

Jonathan Nieder (1):
  t1011 (sparse checkout): style nitpicks

Nguyễn Thái Ngọc Duy (4):
  unpack-trees: only clear CE_UPDATE|CE_REMOVE when skip-worktree is always set
  unpack-trees: let read-tree -u remove index entries outside sparse area
  unpack-trees: do not check for conflict entries too early
  unpack-trees: mark new entries skip-worktree appropriately

 cache.h                              |    3 +-
 t/t1011-read-tree-sparse-checkout.sh |  123 +++++++++++++++++++++-------------
 unpack-trees.c                       |   56 +++++++++-------
 3 files changed, 108 insertions(+), 74 deletions(-)

[PATCH 1/5] t1011 (sparse checkout): style nitpicks

[Nguyễn Thái Ngọc Duy]

From: Jonathan Nieder <jrnieder@gmail.com>

Tweak the rest of the script to more closely follow the test
style guide.  Guarding setup commands with test_expect_success
makes it easy to see the scope in which some particular data is
used; removal of whitespace after >redirection operators is just
for consistency.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh |  102 ++++++++++++++++++----------------
 1 files changed, 55 insertions(+), 47 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 62246db..9189de8 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -1,16 +1,30 @@
 #!/bin/sh
 
-test_description='sparse checkout tests'
+test_description='sparse checkout tests
+
+* (tag: removed, master) removed
+| D	sub/added
+* (HEAD, tag: top) modified and added
+| M	init.t
+| A	sub/added
+* (tag: init) init
+  A	init.t
+'
 
 . ./test-lib.sh
 
-cat >expected <<EOF
-100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0	init.t
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/added
-EOF
 test_expect_success 'setup' '
+	cat >expected <<-\EOF &&
+	100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0	init.t
+	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/added
+	EOF
+	cat >expected.swt <<-\EOF &&
+	H init.t
+	H sub/added
+	EOF
+
 	test_commit init &&
-	echo modified >> init.t &&
+	echo modified >>init.t &&
 	mkdir sub &&
 	touch sub/added &&
 	git add init.t sub/added &&
@@ -20,26 +34,22 @@ test_expect_success 'setup' '
 	git commit -m removed &&
 	git tag removed &&
 	git checkout top &&
-	git ls-files --stage > result &&
+	git ls-files --stage >result &&
 	test_cmp expected result
 '
 
-cat >expected.swt <<EOF
-H init.t
-H sub/added
-EOF
 test_expect_success 'read-tree without .git/info/sparse-checkout' '
 	git read-tree -m -u HEAD &&
-	git ls-files --stage > result &&
+	git ls-files --stage >result &&
 	test_cmp expected result &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result
 '
 
 test_expect_success 'read-tree with .git/info/sparse-checkout but disabled' '
-	echo > .git/info/sparse-checkout
+	echo >.git/info/sparse-checkout
 	git read-tree -m -u HEAD &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result &&
 	test -f init.t &&
 	test -f sub/added
@@ -47,9 +57,9 @@ test_expect_success 'read-tree with .git/info/sparse-checkout but disabled' '
 
 test_expect_success 'read-tree --no-sparse-checkout with empty .git/info/sparse-checkout and enabled' '
 	git config core.sparsecheckout true &&
-	echo > .git/info/sparse-checkout &&
+	echo >.git/info/sparse-checkout &&
 	git read-tree --no-sparse-checkout -m -u HEAD &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result &&
 	test -f init.t &&
 	test -f sub/added
@@ -57,92 +67,90 @@ test_expect_success 'read-tree --no-sparse-checkout with empty .git/info/sparse-
 
 test_expect_success 'read-tree with empty .git/info/sparse-checkout' '
 	git config core.sparsecheckout true &&
-	echo > .git/info/sparse-checkout &&
+	echo >.git/info/sparse-checkout &&
 	test_must_fail git read-tree -m -u HEAD &&
-	git ls-files --stage > result &&
+	git ls-files --stage >result &&
 	test_cmp expected result &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result &&
 	test -f init.t &&
 	test -f sub/added
 '
 
-cat >expected.swt <<EOF
-S init.t
-H sub/added
-EOF
 test_expect_success 'match directories with trailing slash' '
+	cat >expected.swt-noinit <<-\EOF &&
+	S init.t
+	H sub/added
+	EOF
+
 	echo sub/ > .git/info/sparse-checkout &&
 	git read-tree -m -u HEAD &&
 	git ls-files -t > result &&
-	test_cmp expected.swt result &&
+	test_cmp expected.swt-noinit result &&
 	test ! -f init.t &&
 	test -f sub/added
 '
 
-cat >expected.swt <<EOF
-H init.t
-H sub/added
-EOF
 test_expect_failure 'match directories without trailing slash' '
-	echo init.t > .git/info/sparse-checkout &&
-	echo sub >> .git/info/sparse-checkout &&
+	echo init.t >.git/info/sparse-checkout &&
+	echo sub >>.git/info/sparse-checkout &&
 	git read-tree -m -u HEAD &&
-	git ls-files -t > result &&
+	git ls-files -t >result &&
 	test_cmp expected.swt result &&
 	test ! -f init.t &&
 	test -f sub/added
 '
 
-cat >expected.swt <<EOF
-H init.t
-S sub/added
-EOF
 test_expect_success 'checkout area changes' '
-	echo init.t > .git/info/sparse-checkout &&
+	cat >expected.swt-nosub <<-\EOF &&
+	H init.t
+	S sub/added
+	EOF
+
+	echo init.t >.git/info/sparse-checkout &&
 	git read-tree -m -u HEAD &&
-	git ls-files -t > result &&
-	test_cmp expected.swt result &&
+	git ls-files -t >result &&
+	test_cmp expected.swt-nosub result &&
 	test -f init.t &&
 	test ! -f sub/added
 '
 
 test_expect_success 'read-tree updates worktree, absent case' '
-	echo sub/added > .git/info/sparse-checkout &&
+	echo sub/added >.git/info/sparse-checkout &&
 	git checkout -f top &&
 	git read-tree -m -u HEAD^ &&
 	test ! -f init.t
 '
 
 test_expect_success 'read-tree updates worktree, dirty case' '
-	echo sub/added > .git/info/sparse-checkout &&
+	echo sub/added >.git/info/sparse-checkout &&
 	git checkout -f top &&
-	echo dirty > init.t &&
+	echo dirty >init.t &&
 	git read-tree -m -u HEAD^ &&
 	grep -q dirty init.t &&
 	rm init.t
 '
 
 test_expect_success 'read-tree removes worktree, dirty case' '
-	echo init.t > .git/info/sparse-checkout &&
+	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f top &&
-	echo dirty > added &&
+	echo dirty >added &&
 	git read-tree -m -u HEAD^ &&
 	grep -q dirty added
 '
 
 test_expect_success 'read-tree adds to worktree, absent case' '
-	echo init.t > .git/info/sparse-checkout &&
+	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f removed &&
 	git read-tree -u -m HEAD^ &&
 	test ! -f sub/added
 '
 
 test_expect_success 'read-tree adds to worktree, dirty case' '
-	echo init.t > .git/info/sparse-checkout &&
+	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f removed &&
 	mkdir sub &&
-	echo dirty > sub/added &&
+	echo dirty >sub/added &&
 	git read-tree -u -m HEAD^ &&
 	grep -q dirty sub/added
 '
-- 
1.7.1.rc1.69.g24c2f7

[PATCH 2/5] unpack-trees: only clear CE_UPDATE|CE_REMOVE when skip-worktree is always set

[Nguyễn Thái Ngọc Duy]

The purpose of this clearing is, as explained in comment, because
verify_*() may set those bits before apply_sparse_checkout() is
called. By that time, it's not clear whether an entry will stay in
checkout area or out. After $GIT_DIR/info/sparse-checkout is applied,
we know what entries will be in finally. It's time to clean unwanted
bits.

That works perfectly when checkout area remains unchanged. When
checkout area changes, apply_sparse_checkout() may set CE_UPDATE
or CE_WT_REMOVE to widen/narrow checkout area. Doing the clearing
after apply_sparse_checkout() may clear those widening/narrowing
bits unexpectedly.

So, only do that on entries that are not affected by checkout area
changes (i.e. skip-worktree bit does not change after
apply_sparse_checkout).

This code does not actually fix anything though, just
future-proof. The removed code and the narrow/widen code inside
apply_sparse_checkout are currently independent (narrow code never
sets CE_REMOVE, widen code sets CE_UPDATE, but ce_skip_worktree()
would be false).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh |   12 ++++++++++++
 unpack-trees.c                       |   26 ++++++++++++--------------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 9189de8..2876daf 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -155,4 +155,16 @@ test_expect_success 'read-tree adds to worktree, dirty case' '
 	grep -q dirty sub/added
 '
 
+test_expect_success 'index removal and worktree narrowing at the same time' '
+	>empty &&
+	echo init.t >.git/info/sparse-checkout &&
+	echo sub/added >>.git/info/sparse-checkout &&
+	git checkout -f top &&
+	echo init.t >.git/info/sparse-checkout &&
+	git checkout removed &&
+	git ls-files sub/added >result
+	test ! -f sub/added &&
+	test_cmp empty result
+'
+
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index e8f03f5..6d0df99 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -156,13 +156,18 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 		ce->ce_flags &= ~CE_SKIP_WORKTREE;
 
 	/*
-	 * We only care about files getting into the checkout area
-	 * If merge strategies want to remove some, go ahead, this
-	 * flag will be removed eventually in unpack_trees() if it's
-	 * outside checkout area.
+	 * if (!was_skip_worktree && !ce_skip_worktree()) {
+	 *	This is perfectly normal. Move on;
+	 * }
 	 */
-	if (ce->ce_flags & CE_REMOVE)
-		return 0;
+
+	/*
+	 * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
+	 * area as a result of ce_skip_worktree() shortcuts in
+	 * verify_absent() and verify_uptodate(). Clear them.
+	 */
+	if (was_skip_worktree && ce_skip_worktree(ce))
+		ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
 
 	if (!was_skip_worktree && ce_skip_worktree(ce)) {
 		/*
@@ -796,14 +801,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 				ret = -1;
 				goto done;
 			}
-			/*
-			 * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
-			 * area as a result of ce_skip_worktree() shortcuts in
-			 * verify_absent() and verify_uptodate(). Clear them.
-			 */
-			if (ce_skip_worktree(ce))
-				ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
-			else
+			if (!ce_skip_worktree(ce))
 				empty_worktree = 0;
 
 		}
-- 
1.7.1.rc1.69.g24c2f7

[PATCH 3/5] unpack-trees: let read-tree -u remove index entries outside sparse area

[Nguyễn Thái Ngọc Duy]

To avoid touching the worktree outside a sparse checkout,
when the update flag is enabled unpack_trees() clears the
CE_UPDATE and CE_REMOVE flags on entries that do not match the
sparse pattern before actually committing any updates to the
index file or worktree.

The effect on the index was unintentional; sparse checkout was
never meant to prevent index updates outside the area checked
out.  And the result is very confusing: for example, after a
failed merge, currently "git reset --hard" does not reset the
state completely but an additional "git reset --mixed" will.

So stop clearing the CE_REMOVE flag.  Instead, maintain a
CE_WT_REMOVE flag to separately track whether a particular
file removal should apply to the worktree in addition to the
index or not.

The CE_WT_REMOVE flag is used already to mark files that
should be removed because of a narrowing checkout area.  That
usage will still apply; do not clear the CE_WT_REMOVE flag
in that case (detectable because the CE_REMOVE flag is not
set).

This bug masked some other bugs illustrated by the test
suite, which will be addressed by later patches.

Reported-by: Frédéric Brière <fbriere@fbriere.net>
Fixes: http://bugs.debian.org/583699

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h                              |    3 +--
 t/t1011-read-tree-sparse-checkout.sh |   11 ++++++++++-
 unpack-trees.c                       |   29 +++++++++++++++++++----------
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/cache.h b/cache.h
index 0d101e4..4e7bb62 100644
--- a/cache.h
+++ b/cache.h
@@ -179,8 +179,7 @@ struct cache_entry {
 #define CE_UNHASHED  (0x200000)
 #define CE_CONFLICTED (0x800000)
 
-/* Only remove in work directory, not index */
-#define CE_WT_REMOVE (0x400000)
+#define CE_WT_REMOVE (0x400000) /* remove in work directory */
 
 #define CE_UNPACKED  (0x1000000)
 
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 2876daf..3e01c6e 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -146,7 +146,7 @@ test_expect_success 'read-tree adds to worktree, absent case' '
 	test ! -f sub/added
 '
 
-test_expect_success 'read-tree adds to worktree, dirty case' '
+test_expect_failure 'read-tree adds to worktree, dirty case' '
 	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f removed &&
 	mkdir sub &&
@@ -167,4 +167,13 @@ test_expect_success 'index removal and worktree narrowing at the same time' '
 	test_cmp empty result
 '
 
+test_expect_success 'read-tree --reset removes outside worktree' '
+	>empty &&
+	echo init.t >.git/info/sparse-checkout &&
+	git checkout -f top &&
+	git reset --hard removed &&
+	git ls-files sub/added >result &&
+	test_cmp empty result
+'
+
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index 6d0df99..1413872 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -53,6 +53,9 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 
 	clear |= CE_HASHED | CE_UNHASHED;
 
+	if (set & CE_REMOVE)
+		set |= CE_WT_REMOVE;
+
 	memcpy(new, ce, size);
 	new->next = NULL;
 	new->ce_flags = (new->ce_flags & ~clear) | set;
@@ -84,7 +87,7 @@ static int check_updates(struct unpack_trees_options *o)
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
 			struct cache_entry *ce = index->cache[cnt];
-			if (ce->ce_flags & (CE_UPDATE | CE_REMOVE | CE_WT_REMOVE))
+			if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
 				total++;
 		}
 
@@ -104,12 +107,6 @@ static int check_updates(struct unpack_trees_options *o)
 				unlink_entry(ce);
 			continue;
 		}
-
-		if (ce->ce_flags & CE_REMOVE) {
-			display_progress(progress, ++cnt);
-			if (o->update)
-				unlink_entry(ce);
-		}
 	}
 	remove_marked_cache_entries(&o->result);
 	remove_scheduled_dirs();
@@ -164,10 +161,22 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 	/*
 	 * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
 	 * area as a result of ce_skip_worktree() shortcuts in
-	 * verify_absent() and verify_uptodate(). Clear them.
+	 * verify_absent() and verify_uptodate().
+	 * Make sure they don't modify worktree if they are already
+	 * outside checkout area
 	 */
-	if (was_skip_worktree && ce_skip_worktree(ce))
-		ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
+	if (was_skip_worktree && ce_skip_worktree(ce)) {
+		ce->ce_flags &= ~CE_UPDATE;
+
+		/*
+		 * By default, when CE_REMOVE is on, CE_WT_REMOVE is also
+		 * on to get that file removed from both index and worktree.
+		 * If that file is already outside worktree area, don't
+		 * bother remove it.
+		 */
+		if (ce->ce_flags & CE_REMOVE)
+			ce->ce_flags &= ~CE_WT_REMOVE;
+	}
 
 	if (!was_skip_worktree && ce_skip_worktree(ce)) {
 		/*
-- 
1.7.1.rc1.69.g24c2f7

[PATCH 4/5] unpack-trees: do not check for conflict entries too early

[Nguyễn Thái Ngọc Duy]

The idea of sparse checkout is conflict entries should always stay
in worktree, regardless $GIT_DIR/info/sparse-checkout. Therefore,
ce_stage(ce) usually means no CE_SKIP_WORKTREE. This is true when all
entries have been merged into the index, and identical staged entries
collapsed.

However, will_have_skip_worktree() since f1f523e (unpack-trees():
ignore worktree check outside checkout area) is also used earlier in
verify_* functions, where entries have not been merged to index yet
and ce_stage() is not zero. Checking ce_stage() then may provoke
unnecessary verification on entries outside checkout area and error
out.

This fixes part of test case "read-tree adds to worktree, dirty case".
The error

error: Untracked working tree file 'sub/added' would be overwritten by merge.

is now gone and (unfortunately) replaced by another error, which will
be addressed in the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 unpack-trees.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 1413872..b34ef46 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -135,9 +135,6 @@ static int will_have_skip_worktree(const struct cache_entry *ce, struct unpack_t
 {
 	const char *basename;
 
-	if (ce_stage(ce))
-		return 0;
-
 	basename = strrchr(ce->name, '/');
 	basename = basename ? basename+1 : ce->name;
 	return excluded_from_list(ce->name, ce_namelen(ce), basename, NULL, o->el) <= 0;
@@ -147,7 +144,7 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 {
 	int was_skip_worktree = ce_skip_worktree(ce);
 
-	if (will_have_skip_worktree(ce, o))
+	if (!ce_stage(ce) && will_have_skip_worktree(ce, o))
 		ce->ce_flags |= CE_SKIP_WORKTREE;
 	else
 		ce->ce_flags &= ~CE_SKIP_WORKTREE;
-- 
1.7.1.rc1.69.g24c2f7

[PATCH 5/5] unpack-trees: mark new entries skip-worktree appropriately

[Nguyễn Thái Ngọc Duy]

Sparse checkout narrows worktree down based on the skip-worktree bit
before and after $GIT_DIR/info/sparse-checkout application. If it does
not have that bit before but does after, a narrow is detected and the
file will be removed from worktree.

New files added by merge, however, does not have skip-worktree bit. If
those files appear to be outside checkout area, the same rule applies:
the file gets removed from worktree even though they don't exist in
worktree.

Just pretend they have skip-worktree before in that case, so the rule
is ignored.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh |    2 +-
 unpack-trees.c                       |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 3e01c6e..04d4450 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -146,7 +146,7 @@ test_expect_success 'read-tree adds to worktree, absent case' '
 	test ! -f sub/added
 '
 
-test_expect_failure 'read-tree adds to worktree, dirty case' '
+test_expect_success 'read-tree adds to worktree, dirty case' '
 	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f removed &&
 	mkdir sub &&
diff --git a/unpack-trees.c b/unpack-trees.c
index b34ef46..86e6409 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1096,6 +1096,8 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 	if (!old) {
 		if (verify_absent(merge, "overwritten", o))
 			return -1;
+		if (!o->skip_sparse_checkout && will_have_skip_worktree(merge, o))
+			update |= CE_SKIP_WORKTREE;
 		invalidate_ce_path(merge, o);
 	} else if (!(old->ce_flags & CE_CONFLICTED)) {
 		/*
-- 
1.7.1.rc1.69.g24c2f7

Re: [PATCH 1/5] t1011 (sparse checkout): style nitpicks

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> From: Jonathan Nieder <jrnieder@gmail.com>
>
> Tweak the rest of the script to more closely follow the test
> style guide.  Guarding setup commands with test_expect_success
> makes it easy to see the scope in which some particular data is
> used; removal of whitespace after >redirection operators is just
> for consistency.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

What happened to the 'test -z "$(cmd)" discards exit status from cmd' part
of Jonathan's patch?

Re: [PATCH 1/5] t1011 (sparse checkout): style nitpicks

[Junio C Hamano]

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

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> From: Jonathan Nieder <jrnieder@gmail.com>
>>
>> Tweak the rest of the script to more closely follow the test
>> style guide.  Guarding setup commands with test_expect_success
>> makes it easy to see the scope in which some particular data is
>> used; removal of whitespace after >redirection operators is just
>> for consistency.
>>
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> What happened to the 'test -z "$(cmd)" discards exit status from cmd' part
> of Jonathan's patch?

Sorry, I was confused.

Please disregard the above; it took me a while to realize that this
5-patch series is a re-roll of what I queued, which in turn Jonathan's
nitpick is based on, and you took the advice while redoing 2/5.

Thanks.

Re: [PATCH 0/5] nd/fix-sparse-checkout v2

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> Discussion of patch 1/3 from the first series resulted in 2/5 in this
> series. 2/3 and 3/3 from the former have commit messages revised,
> code change remains the same.
> 
> Jonathan Nieder (1):
>   t1011 (sparse checkout): style nitpicks
> 
> Nguyễn Thái Ngọc Duy (4):
>   unpack-trees: only clear CE_UPDATE|CE_REMOVE when skip-worktree is always set
>   unpack-trees: let read-tree -u remove index entries outside sparse area
>   unpack-trees: do not check for conflict entries too early
>   unpack-trees: mark new entries skip-worktree appropriately

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for the explanations.

Re: [PATCH 2/5] unpack-trees: only clear CE_UPDATE|CE_REMOVE when skip-worktree is always set

[Jonathan Nieder]

Hi Duy,

Nguyễn Thái Ngọc Duy wrote:

> +++ b/t/t1011-read-tree-sparse-checkout.sh
> @@ -155,4 +155,16 @@ test_expect_success 'read-tree adds to worktree, dirty case' '
>  	grep -q dirty sub/added
>  '
>  
> +test_expect_success 'index removal and worktree narrowing at the same time' '
> +	>empty &&
> +	echo init.t >.git/info/sparse-checkout &&
> +	echo sub/added >>.git/info/sparse-checkout &&
> +	git checkout -f top &&
> +	echo init.t >.git/info/sparse-checkout &&
> +	git checkout removed &&
> +	git ls-files sub/added >result
> +	test ! -f sub/added &&

Missing “&&”.  Sorry to miss this before.

Patch for squashing follows.

-- 8< --
Subject: t1011 (sparse checkout): fix &&-chaining

Make sure errors from “git checkout” are detected.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 04d4450..9a07de1 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -162,7 +162,7 @@ test_expect_success 'index removal and worktree narrowing at the same time' '
 	git checkout -f top &&
 	echo init.t >.git/info/sparse-checkout &&
 	git checkout removed &&
-	git ls-files sub/added >result
+	git ls-files sub/added >result &&
 	test ! -f sub/added &&
 	test_cmp empty result
 '
-- 
1.7.2.1.544.ga752d.dirty