# [PATCH] unpack-trees: don't update files flagged for deletion 8 messages from 2015-07-17 to 2015-07-21. Participants: David Turner, Junio C Hamano, Duy Nguyen. Thread: https://gitlist.dev/t/39878 ## David Turner, 2015-07-17 19:48 Subject: [PATCH] unpack-trees: don't update files flagged for deletion Message-ID: <1437162532-31933-1-git-send-email-dturner@twopensource.com> URL: https://gitlist.dev/e/1437162532-31933-1-git-send-email-dturner%40twopensource.com ``` Don't update files in the worktree from cache entries which are flagged with CE_WT_REMOVE. This is fixes merges in sparse checkouts. Signed-off-by: Anatole Shaw Signed-off-by: David Turner --- This patch was written by my colleague Anatole Shaw for Twitter's git; I just rebased it on mainstream git. Anatole has given me permission to send this upstream. --- t/t1090-sparse-checkout-scope.sh | 52 ++++++++++++++++++++++++++++++++++++++++ unpack-trees.c | 2 +- 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100755 t/t1090-sparse-checkout-scope.sh diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh new file mode 100755 index 0000000..1f61eb3 --- /dev/null +++ b/t/t1090-sparse-checkout-scope.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +test_description='sparse checkout scope tests' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo "initial" >a && + echo "initial" >b && + echo "initial" >c && + git add a b c && + git commit -m "initial commit" +' + +test_expect_success 'create feature branch' ' + git checkout -b feature && + echo "modified" >b && + echo "modified" >c && + git add b c && + git commit -m "modification" +' + +test_expect_success 'perform sparse checkout of master' ' + git config --local --bool core.sparsecheckout true && + echo "!/*" >.git/info/sparse-checkout && + echo "/a" >>.git/info/sparse-checkout && + echo "/c" >>.git/info/sparse-checkout && + git checkout master && + test_path_is_file a && + test_path_is_missing b && + test_path_is_file c +' + +test_expect_success 'merge feature branch into sparse checkout of master' ' + git merge feature && + test_path_is_file a && + test_path_is_missing b && + test_path_is_file c && + test "$(cat c)" = "modified" +' + +test_expect_success 'return to full checkout of master' ' + git checkout feature && + echo "/*" >.git/info/sparse-checkout && + git checkout master && + test_path_is_file a && + test_path_is_file b && + test_path_is_file c && + test "$(cat b)" = "modified" +' + +test_done diff --git a/unpack-trees.c b/unpack-trees.c index 2927660..11a5300 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -223,7 +223,7 @@ static int check_updates(struct unpack_trees_options *o) for (i = 0; i < index->cache_nr; i++) { struct cache_entry *ce = index->cache[i]; - if (ce->ce_flags & CE_UPDATE) { + if (ce->ce_flags & CE_UPDATE && !(ce->ce_flags & CE_WT_REMOVE)) { display_progress(progress, ++cnt); ce->ce_flags &= ~CE_UPDATE; if (o->update && !o->dry_run) { -- 2.0.4.315.gad8727a-twtrsrc ``` ## Junio C Hamano, 2015-07-17 20:05 Subject: Re: [PATCH] unpack-trees: don't update files flagged for deletion Message-ID: URL: https://gitlist.dev/e/xmqqk2ty1reo.fsf%40gitster.dls.corp.google.com In-Reply-To: <1437162532-31933-1-git-send-email-dturner@twopensource.com> ``` David Turner writes: > Don't update files in the worktree from cache entries which are > flagged with CE_WT_REMOVE. This is fixes merges in sparse > checkouts. s/This is/This/; But more importantly, what is missing is "why" it is a good fix. i.e. things like: - why is it wrong to update what is marked with CE_UPDATE bit to be updated in the working tree when CE_WT_REMOVE bit is on? - how that incorrect operation breaks which operation in what way? - how does the changed code fix that breakage? ``` ## David Turner, 2015-07-17 21:19 Subject: [PATCH v2] unpack-trees: don't update files with CE_WT_REMOVE set Message-ID: <1437167967-5933-1-git-send-email-dturner@twopensource.com> URL: https://gitlist.dev/e/1437167967-5933-1-git-send-email-dturner%40twopensource.com In-Reply-To: ``` Don't update files in the worktree from cache entries which are flagged with CE_WT_REMOVE. When a user does a sparse checkout, git removes files that are marked with CE_WT_REMOVE (because they are out-of-scope for the sparse checkout). If those files are also marked CE_UPDATE (for instance, because they differ in the branch that is being checked out and the outgoing branch), git would previously recreate them. This patch prevents them from being recreated. These erroneously-created files would also interfere with merges, causing pre-merge revisions of out-of-scope files to appear in the worktree. Signed-off-by: Anatole Shaw Signed-off-by: David Turner --- t/t1090-sparse-checkout-scope.sh | 52 ++++++++++++++++++++++++++++++++++++++++ unpack-trees.c | 2 +- 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100755 t/t1090-sparse-checkout-scope.sh diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh new file mode 100755 index 0000000..1f61eb3 --- /dev/null +++ b/t/t1090-sparse-checkout-scope.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +test_description='sparse checkout scope tests' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo "initial" >a && + echo "initial" >b && + echo "initial" >c && + git add a b c && + git commit -m "initial commit" +' + +test_expect_success 'create feature branch' ' + git checkout -b feature && + echo "modified" >b && + echo "modified" >c && + git add b c && + git commit -m "modification" +' + +test_expect_success 'perform sparse checkout of master' ' + git config --local --bool core.sparsecheckout true && + echo "!/*" >.git/info/sparse-checkout && + echo "/a" >>.git/info/sparse-checkout && + echo "/c" >>.git/info/sparse-checkout && + git checkout master && + test_path_is_file a && + test_path_is_missing b && + test_path_is_file c +' + +test_expect_success 'merge feature branch into sparse checkout of master' ' + git merge feature && + test_path_is_file a && + test_path_is_missing b && + test_path_is_file c && + test "$(cat c)" = "modified" +' + +test_expect_success 'return to full checkout of master' ' + git checkout feature && + echo "/*" >.git/info/sparse-checkout && + git checkout master && + test_path_is_file a && + test_path_is_file b && + test_path_is_file c && + test "$(cat b)" = "modified" +' + +test_done diff --git a/unpack-trees.c b/unpack-trees.c index 2927660..11a5300 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -223,7 +223,7 @@ static int check_updates(struct unpack_trees_options *o) for (i = 0; i < index->cache_nr; i++) { struct cache_entry *ce = index->cache[i]; - if (ce->ce_flags & CE_UPDATE) { + if (ce->ce_flags & CE_UPDATE && !(ce->ce_flags & CE_WT_REMOVE)) { display_progress(progress, ++cnt); ce->ce_flags &= ~CE_UPDATE; if (o->update && !o->dry_run) { -- 2.0.4.315.gad8727a-twtrsrc ``` ## Junio C Hamano, 2015-07-17 21:34 Subject: Re: [PATCH v2] unpack-trees: don't update files with CE_WT_REMOVE set Message-ID: URL: https://gitlist.dev/e/xmqqbnfa1na6.fsf%40gitster.dls.corp.google.com In-Reply-To: <1437167967-5933-1-git-send-email-dturner@twopensource.com> ``` David Turner writes: > Don't update files in the worktree from cache entries which are > flagged with CE_WT_REMOVE. > > When a user does a sparse checkout, git removes files that are marked > with CE_WT_REMOVE (because they are out-of-scope for the sparse > checkout). If those files are also marked CE_UPDATE (for instance, > because they differ in the branch that is being checked out and the > outgoing branch), git would previously recreate them. This patch > prevents them from being recreated. > > These erroneously-created files would also interfere with merges, > causing pre-merge revisions of out-of-scope files to appear in the > worktree. > > Signed-off-by: Anatole Shaw > Signed-off-by: David Turner > --- That's much more readable and understandable ;-) Duy Cc'ed for final sanity check and possiible extra comments. Thanks. > t/t1090-sparse-checkout-scope.sh | 52 ++++++++++++++++++++++++++++++++++++++++ > unpack-trees.c | 2 +- > 2 files changed, 53 insertions(+), 1 deletion(-) > create mode 100755 t/t1090-sparse-checkout-scope.sh > > diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh > new file mode 100755 > index 0000000..1f61eb3 > --- /dev/null > +++ b/t/t1090-sparse-checkout-scope.sh > @@ -0,0 +1,52 @@ > +#!/bin/sh > + > +test_description='sparse checkout scope tests' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' ' > + echo "initial" >a && > + echo "initial" >b && > + echo "initial" >c && > + git add a b c && > + git commit -m "initial commit" > +' > + > +test_expect_success 'create feature branch' ' > + git checkout -b feature && > + echo "modified" >b && > + echo "modified" >c && > + git add b c && > + git commit -m "modification" > +' > + > +test_expect_success 'perform sparse checkout of master' ' > + git config --local --bool core.sparsecheckout true && > + echo "!/*" >.git/info/sparse-checkout && > + echo "/a" >>.git/info/sparse-checkout && > + echo "/c" >>.git/info/sparse-checkout && > + git checkout master && > + test_path_is_file a && > + test_path_is_missing b && > + test_path_is_file c > +' > + > +test_expect_success 'merge feature branch into sparse checkout of master' ' > + git merge feature && > + test_path_is_file a && > + test_path_is_missing b && > + test_path_is_file c && > + test "$(cat c)" = "modified" > +' > + > +test_expect_success 'return to full checkout of master' ' > + git checkout feature && > + echo "/*" >.git/info/sparse-checkout && > + git checkout master && > + test_path_is_file a && > + test_path_is_file b && > + test_path_is_file c && > + test "$(cat b)" = "modified" > +' > + > +test_done > diff --git a/unpack-trees.c b/unpack-trees.c > index 2927660..11a5300 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -223,7 +223,7 @@ static int check_updates(struct unpack_trees_options *o) > for (i = 0; i < index->cache_nr; i++) { > struct cache_entry *ce = index->cache[i]; > > - if (ce->ce_flags & CE_UPDATE) { > + if (ce->ce_flags & CE_UPDATE && !(ce->ce_flags & CE_WT_REMOVE)) { > display_progress(progress, ++cnt); > ce->ce_flags &= ~CE_UPDATE; > if (o->update && !o->dry_run) { ``` ## Duy Nguyen, 2015-07-18 08:37 Subject: Re: [PATCH v2] unpack-trees: don't update files with CE_WT_REMOVE set Message-ID: <20150718083719.GA19676@lanh> URL: https://gitlist.dev/e/20150718083719.GA19676%40lanh In-Reply-To: <1437167967-5933-1-git-send-email-dturner@twopensource.com> ``` On Fri, Jul 17, 2015 at 05:19:27PM -0400, David Turner wrote: > Don't update files in the worktree from cache entries which are > flagged with CE_WT_REMOVE. > > When a user does a sparse checkout, git removes files that are marked > with CE_WT_REMOVE (because they are out-of-scope for the sparse > checkout). If those files are also marked CE_UPDATE (for instance, > because they differ in the branch that is being checked out and the > outgoing branch), git would previously recreate them. This patch > prevents them from being recreated. > > These erroneously-created files would also interfere with merges, > causing pre-merge revisions of out-of-scope files to appear in the > worktree. Thank you both for catching this. Just a small suggestion. Perhaps we should do this instead. apply_sparse_checkout() is the function where all "action" manipulation (add, delete, update files..) for sparse checkout occurs and it should not ask to delete and update both at the same time. -- 8< -- diff --git a/unpack-trees.c b/unpack-trees.c index 2927660..d6cf849 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -224,6 +224,9 @@ static int check_updates(struct unpack_trees_options *o) struct cache_entry *ce = index->cache[i]; if (ce->ce_flags & CE_UPDATE) { + if (ce->ce_flags & CE_WT_REMOVE) + die("BUG: both update and delete flags are set on %s", + ce->name); display_progress(progress, ++cnt); ce->ce_flags &= ~CE_UPDATE; if (o->update && !o->dry_run) { @@ -293,6 +296,7 @@ static int apply_sparse_checkout(struct index_state *istate, if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o)) return -1; ce->ce_flags |= CE_WT_REMOVE; + ce->ce_flags &= ~CE_UPDATE; } if (was_skip_worktree && !ce_skip_worktree(ce)) { if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) -- 8< -- -- Duy ``` ## Junio C Hamano, 2015-07-18 21:20 Subject: Re: [PATCH v2] unpack-trees: don't update files with CE_WT_REMOVE set Message-ID: URL: https://gitlist.dev/e/xmqqpp3pyxh0.fsf%40gitster.dls.corp.google.com In-Reply-To: <20150718083719.GA19676@lanh> ``` Duy Nguyen writes: > Thank you both for catching this. Just a small suggestion. Perhaps we > should do this instead. apply_sparse_checkout() is the function where > all "action" manipulation (add, delete, update files..) for sparse > checkout occurs and it should not ask to delete and update both at the > same time. Sounds good. The first hunk may merely be a noise, but the second one is the true bugfix for the issue observed, I think. > > -- 8< -- > diff --git a/unpack-trees.c b/unpack-trees.c > index 2927660..d6cf849 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -224,6 +224,9 @@ static int check_updates(struct unpack_trees_options *o) > struct cache_entry *ce = index->cache[i]; > > if (ce->ce_flags & CE_UPDATE) { > + if (ce->ce_flags & CE_WT_REMOVE) > + die("BUG: both update and delete flags are set on %s", > + ce->name); > display_progress(progress, ++cnt); > ce->ce_flags &= ~CE_UPDATE; > if (o->update && !o->dry_run) { > @@ -293,6 +296,7 @@ static int apply_sparse_checkout(struct index_state *istate, > if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o)) > return -1; > ce->ce_flags |= CE_WT_REMOVE; > + ce->ce_flags &= ~CE_UPDATE; > } > if (was_skip_worktree && !ce_skip_worktree(ce)) { > if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) > -- 8< -- > > -- > Duy ``` ## David Turner, 2015-07-20 17:40 Subject: Re: [PATCH v2] unpack-trees: don't update files with CE_WT_REMOVE set Message-ID: <1437414048.30911.10.camel@twopensource.com> URL: https://gitlist.dev/e/1437414048.30911.10.camel%40twopensource.com In-Reply-To: <20150718083719.GA19676@lanh> ``` Anatole tells me that this works for us. Thanks. On Sat, 2015-07-18 at 15:37 +0700, Duy Nguyen wrote: > On Fri, Jul 17, 2015 at 05:19:27PM -0400, David Turner wrote: > > Don't update files in the worktree from cache entries which are > > flagged with CE_WT_REMOVE. > > > > When a user does a sparse checkout, git removes files that are marked > > with CE_WT_REMOVE (because they are out-of-scope for the sparse > > checkout). If those files are also marked CE_UPDATE (for instance, > > because they differ in the branch that is being checked out and the > > outgoing branch), git would previously recreate them. This patch > > prevents them from being recreated. > > > > These erroneously-created files would also interfere with merges, > > causing pre-merge revisions of out-of-scope files to appear in the > > worktree. > > Thank you both for catching this. Just a small suggestion. Perhaps we > should do this instead. apply_sparse_checkout() is the function where > all "action" manipulation (add, delete, update files..) for sparse > checkout occurs and it should not ask to delete and update both at the > same time. > > -- 8< -- > diff --git a/unpack-trees.c b/unpack-trees.c > index 2927660..d6cf849 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -224,6 +224,9 @@ static int check_updates(struct unpack_trees_options *o) > struct cache_entry *ce = index->cache[i]; > > if (ce->ce_flags & CE_UPDATE) { > + if (ce->ce_flags & CE_WT_REMOVE) > + die("BUG: both update and delete flags are set on %s", > + ce->name); > display_progress(progress, ++cnt); > ce->ce_flags &= ~CE_UPDATE; > if (o->update && !o->dry_run) { > @@ -293,6 +296,7 @@ static int apply_sparse_checkout(struct index_state *istate, > if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o)) > return -1; > ce->ce_flags |= CE_WT_REMOVE; > + ce->ce_flags &= ~CE_UPDATE; > } > if (was_skip_worktree && !ce_skip_worktree(ce)) { > if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) > -- 8< -- > > -- > Duy ``` ## Junio C Hamano, 2015-07-21 20:52 Subject: Re: [PATCH v2] unpack-trees: don't update files with CE_WT_REMOVE set Message-ID: URL: https://gitlist.dev/e/xmqqlhe9ters.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Duy Nguyen writes: > >> Thank you both for catching this. Just a small suggestion. Perhaps we >> should do this instead. apply_sparse_checkout() is the function where >> all "action" manipulation (add, delete, update files..) for sparse >> checkout occurs and it should not ask to delete and update both at the >> same time. > > Sounds good. The first hunk may merely be a noise, but the second > one is the true bugfix for the issue observed, I think. I've replaced as/sparse-checkout-removal with the following. Thanks, all. -- >8 -- From: David Turner Date: Fri, 17 Jul 2015 17:19:27 -0400 Subject: [PATCH] unpack-trees: don't update files with CE_WT_REMOVE set Don't update files in the worktree from cache entries which are flagged with CE_WT_REMOVE. When a user does a sparse checkout, git removes files that are marked with CE_WT_REMOVE (because they are out-of-scope for the sparse checkout). If those files are also marked CE_UPDATE (for instance, because they differ in the branch that is being checked out and the outgoing branch), git would previously recreate them. This patch prevents them from being recreated. These erroneously-created files would also interfere with merges, causing pre-merge revisions of out-of-scope files to appear in the worktree. apply_sparse_checkout() is the function where all "action" manipulation (add, delete, update files..) for sparse checkout occurs; it should not ask to delete and update both at the same time. Signed-off-by: Anatole Shaw Signed-off-by: David Turner Helped-by: Duy Nguyen Signed-off-by: Junio C Hamano --- t/t1090-sparse-checkout-scope.sh | 52 ++++++++++++++++++++++++++++++++++++++++ unpack-trees.c | 4 ++++ 2 files changed, 56 insertions(+) create mode 100755 t/t1090-sparse-checkout-scope.sh diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh new file mode 100755 index 0000000..1f61eb3 --- /dev/null +++ b/t/t1090-sparse-checkout-scope.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +test_description='sparse checkout scope tests' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo "initial" >a && + echo "initial" >b && + echo "initial" >c && + git add a b c && + git commit -m "initial commit" +' + +test_expect_success 'create feature branch' ' + git checkout -b feature && + echo "modified" >b && + echo "modified" >c && + git add b c && + git commit -m "modification" +' + +test_expect_success 'perform sparse checkout of master' ' + git config --local --bool core.sparsecheckout true && + echo "!/*" >.git/info/sparse-checkout && + echo "/a" >>.git/info/sparse-checkout && + echo "/c" >>.git/info/sparse-checkout && + git checkout master && + test_path_is_file a && + test_path_is_missing b && + test_path_is_file c +' + +test_expect_success 'merge feature branch into sparse checkout of master' ' + git merge feature && + test_path_is_file a && + test_path_is_missing b && + test_path_is_file c && + test "$(cat c)" = "modified" +' + +test_expect_success 'return to full checkout of master' ' + git checkout feature && + echo "/*" >.git/info/sparse-checkout && + git checkout master && + test_path_is_file a && + test_path_is_file b && + test_path_is_file c && + test "$(cat b)" = "modified" +' + +test_done diff --git a/unpack-trees.c b/unpack-trees.c index 02f69ae..f177c0e 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -227,6 +227,9 @@ static int check_updates(struct unpack_trees_options *o) struct cache_entry *ce = index->cache[i]; if (ce->ce_flags & CE_UPDATE) { + if (ce->ce_flags & CE_WT_REMOVE) + die("BUG: both update and delete flags are set on %s", + ce->name); display_progress(progress, ++cnt); ce->ce_flags &= ~CE_UPDATE; if (o->update && !o->dry_run) { @@ -290,6 +293,7 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o)) return -1; ce->ce_flags |= CE_WT_REMOVE; + ce->ce_flags &= ~CE_UPDATE; } if (was_skip_worktree && !ce_skip_worktree(ce)) { if (verify_absent_sparse(ce, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) -- 2.5.0-rc2-392-g7dbe568 ```