{"thread":{"id":"24524","subject":"[PATCH 1/3] Fix sparse checkout not removing files from index","startedAt":"2010-07-26T09:08:28Z","lastAt":"2010-07-31T03:48:36Z","messageCount":17,"participants":["Nguyễn Thái Ngọc Duy","Jonathan Nieder","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"146360","messageId":"1280135310-2347-1-git-send-email-pclouds@gmail.com","threadId":"24524","inReplyTo":null,"subject":"[PATCH 1/3] Fix sparse checkout not removing files from index","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-07-26T09:08:28Z","receivedAt":"2010-07-26T09:08:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When unpack_trees() nearly completes, sparse pattern is applied and\nwill prevent any updates (CE_UPDATE, CE_WT_REMOVE, CE_REMOVE) outside\nsparse checkout area by clearing these flags.\n\nUnfortunately CE_REMOVE also instructs write_index() to remove files from\nthe index. By clearing this flag, index may keep unwanted files.\n\nThis patch turns CE_REMOVE in unpack-trees.c to CE_REMOVE|CE_WT_REMOVE\nand only remove files from worktree if CE_WT_REMOVE is set. CE_REMOVE\nis now only meaningful to index operations and CE_WT_REMOVE to\nworktree ones.\n\nThis fault made the test suite itself faulty. With this fault fixed,\ntwo other faults are revealed :(\n\nReported in http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=583699\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n cache.h                              |    3 +--\n t/t1011-read-tree-sparse-checkout.sh |    9 ++++++++-\n unpack-trees.c                       |   22 ++++++++++++----------\n 3 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0d101e4..4e7bb62 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -179,8 +179,7 @@ struct cache_entry {\n #define CE_UNHASHED  (0x200000)\n #define CE_CONFLICTED (0x800000)\n \n-/* Only remove in work directory, not index */\n-#define CE_WT_REMOVE (0x400000)\n+#define CE_WT_REMOVE (0x400000) /* remove in work directory */\n \n #define CE_UNPACKED  (0x1000000)\n \ndiff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh\nindex 62246db..885578a 100755\n--- a/t/t1011-read-tree-sparse-checkout.sh\n+++ b/t/t1011-read-tree-sparse-checkout.sh\n@@ -138,7 +138,7 @@ test_expect_success 'read-tree adds to worktree, absent case' '\n \ttest ! -f sub/added\n '\n \n-test_expect_success 'read-tree adds to worktree, dirty case' '\n+test_expect_failure 'read-tree adds to worktree, dirty case' '\n \techo init.t > .git/info/sparse-checkout &&\n \tgit checkout -f removed &&\n \tmkdir sub &&\n@@ -147,4 +147,11 @@ test_expect_success 'read-tree adds to worktree, dirty case' '\n \tgrep -q dirty sub/added\n '\n \n+test_expect_success 'read-tree --reset removes outside worktree' '\n+\techo init.t > .git/info/sparse-checkout &&\n+\tgit checkout -f top &&\n+\tgit reset --hard removed &&\n+\ttest -z \"$(git ls-files sub/added)\"\n+'\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e8f03f5..f2d148c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -53,6 +53,9 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n \n \tclear |= CE_HASHED | CE_UNHASHED;\n \n+\tif (set & CE_REMOVE)\n+\t\tset |= CE_WT_REMOVE;\n+\n \tmemcpy(new, ce, size);\n \tnew->next = NULL;\n \tnew->ce_flags = (new->ce_flags & ~clear) | set;\n@@ -84,7 +87,7 @@ static int check_updates(struct unpack_trees_options *o)\n \tif (o->update && o->verbose_update) {\n \t\tfor (total = cnt = 0; cnt < index->cache_nr; cnt++) {\n \t\t\tstruct cache_entry *ce = index->cache[cnt];\n-\t\t\tif (ce->ce_flags & (CE_UPDATE | CE_REMOVE | CE_WT_REMOVE))\n+\t\t\tif (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))\n \t\t\t\ttotal++;\n \t\t}\n \n@@ -104,12 +107,6 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\t\tunlink_entry(ce);\n \t\t\tcontinue;\n \t\t}\n-\n-\t\tif (ce->ce_flags & CE_REMOVE) {\n-\t\t\tdisplay_progress(progress, ++cnt);\n-\t\t\tif (o->update)\n-\t\t\t\tunlink_entry(ce);\n-\t\t}\n \t}\n \tremove_marked_cache_entries(&o->result);\n \tremove_scheduled_dirs();\n@@ -799,10 +796,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t\t/*\n \t\t\t * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout\n \t\t\t * area as a result of ce_skip_worktree() shortcuts in\n-\t\t\t * verify_absent() and verify_uptodate(). Clear them.\n+\t\t\t * verify_absent() and verify_uptodate().\n+\t\t\t * Make sure they don't modify worktree.\n \t\t\t */\n-\t\t\tif (ce_skip_worktree(ce))\n-\t\t\t\tce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);\n+\t\t\tif (ce_skip_worktree(ce)) {\n+\t\t\t\tce->ce_flags &= ~CE_UPDATE;\n+\n+\t\t\t\tif (ce->ce_flags & CE_REMOVE)\n+\t\t\t\t\tce->ce_flags &= ~CE_WT_REMOVE;\n+\t\t\t}\n \t\t\telse\n \t\t\t\tempty_worktree = 0;\n \n-- \n1.7.1.rc1.69.g24c2f7\n"},{"id":"146362","messageId":"1280135310-2347-2-git-send-email-pclouds@gmail.com","threadId":"24524","inReplyTo":"1280135310-2347-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/3] unpack-trees.c: Do not check ce_stage in will_have_skip_worktree()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-07-26T09:08:29Z","receivedAt":"2010-07-26T09:08:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The idea of sparse checkout is conflicted entries should always stay\nin worktree, regardless $GIT_DIR/info/sparse-checkout. Therefore,\nce_stage(ce) usually means no CE_SKIP_WORKTREE. This is true when all\nentries have been merged into the index, and identical staged entries\ncollapsed.\n\nHowever, will_have_skip_worktree() since f1f523e (unpack-trees():\nignore worktree check outside checkout area) is also used earlier in\nverify_* functions, when entries have not been merged to index yet\nand ce_stage() may not be zero. Checking ce_stage() then can make\nunnecessary verification on entries outside checkout area and error out.\n\nThis fixes part of test case \"read-tree adds to worktree, dirty case\".\nThe error is changed from\n\nerror: Untracked working tree file 'sub/added' would be overwritten by merge.\n\nbefore this patch to\n\ngrep: sub/added: No such file or directory\n\nwhich will be addressed in the next patch.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n unpack-trees.c |    5 +----\n 1 files changed, 1 insertions(+), 4 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f2d148c..7ea8c5f 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -135,9 +135,6 @@ static int will_have_skip_worktree(const struct cache_entry *ce, struct unpack_t\n {\n \tconst char *basename;\n \n-\tif (ce_stage(ce))\n-\t\treturn 0;\n-\n \tbasename = strrchr(ce->name, '/');\n \tbasename = basename ? basename+1 : ce->name;\n \treturn excluded_from_list(ce->name, ce_namelen(ce), basename, NULL, o->el) <= 0;\n@@ -147,7 +144,7 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt\n {\n \tint was_skip_worktree = ce_skip_worktree(ce);\n \n-\tif (will_have_skip_worktree(ce, o))\n+\tif (!ce_stage(ce) && will_have_skip_worktree(ce, o))\n \t\tce->ce_flags |= CE_SKIP_WORKTREE;\n \telse\n \t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n-- \n1.7.1.rc1.69.g24c2f7\n"},{"id":"146361","messageId":"1280135310-2347-3-git-send-email-pclouds@gmail.com","threadId":"24524","inReplyTo":"1280135310-2347-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/3] Mark new entries skip-worktree appropriately","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-07-26T09:08:30Z","receivedAt":"2010-07-26T09:08:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Sparse checkout updates worktree based on the old and new\nskip-worktree status when $GIT_DIR/info/sparse-checkout changes:\n\nold = ce_skip_worktree(ce);           // current skip-worktree\nnew = will_have_skip_work_tree(ce);   // from $GIT..sparse-checkout\nif (old && !new) add_file_back(ce);   // shrink checkout area\nif (!old && new) remove_file_out(ce); [1] // enlarge checkout area\n\nNew entries after merging will always have skip-worktree unset\n(i.e. old = 0). If those files are filtered out by\n$GIT_DIR/info/sparse-checkout (i.e. new != 0), then case [1] will\nhappen. But there is nothing to remove because they're new.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n t/t1011-read-tree-sparse-checkout.sh |    2 +-\n unpack-trees.c                       |    2 ++\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh\nindex 885578a..86a6246 100755\n--- a/t/t1011-read-tree-sparse-checkout.sh\n+++ b/t/t1011-read-tree-sparse-checkout.sh\n@@ -138,7 +138,7 @@ test_expect_success 'read-tree adds to worktree, absent case' '\n \ttest ! -f sub/added\n '\n \n-test_expect_failure 'read-tree adds to worktree, dirty case' '\n+test_expect_success 'read-tree adds to worktree, dirty case' '\n \techo init.t > .git/info/sparse-checkout &&\n \tgit checkout -f removed &&\n \tmkdir sub &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7ea8c5f..f98b1cb 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1091,6 +1091,8 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \tif (!old) {\n \t\tif (verify_absent(merge, \"overwritten\", o))\n \t\t\treturn -1;\n+\t\tif (!o->skip_sparse_checkout && will_have_skip_worktree(merge, o))\n+\t\t\tupdate |= CE_SKIP_WORKTREE;\n \t\tinvalidate_ce_path(merge, o);\n \t} else if (!(old->ce_flags & CE_CONFLICTED)) {\n \t\t/*\n-- \n1.7.1.rc1.69.g24c2f7\n"},{"id":"146749","messageId":"20100730013534.GB2182@burratino","threadId":"24524","inReplyTo":"1280135310-2347-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/3] Fix sparse checkout not removing files from index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-30T01:35:34Z","receivedAt":"2010-07-30T01:35:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\nThe log message left me confused.  Let’s see if the code is easier.\n\n> +++ b/cache.h\n> @@ -179,8 +179,7 @@ struct cache_entry {\n>  #define CE_UNHASHED  (0x200000)\n>  #define CE_CONFLICTED (0x800000)\n>  \n> -/* Only remove in work directory, not index */\n> -#define CE_WT_REMOVE (0x400000)\n> +#define CE_WT_REMOVE (0x400000) /* remove in work directory */\n\nBefore[1], CE_REMOVE was used by read-tree et al in core to represent\nsomething to be removed from the index file and work tree (e.g., when\nswitching branches).\n\nIn the new order, one uses CE_REMOVE|CE_WT_REMOVE for that, right?\n\n> +++ b/t/t1011-read-tree-sparse-checkout.sh\n> @@ -147,4 +147,11 @@ test_expect_success 'read-tree adds to worktree, dirty case' '\n>  \tgrep -q dirty sub/added\n>  '\n>  \n> +test_expect_success 'read-tree --reset removes outside worktree' '\n> +\techo init.t > .git/info/sparse-checkout &&\n> +\tgit checkout -f top &&\n> +\tgit reset --hard removed &&\n> +\ttest -z \"$(git ls-files sub/added)\"\n> +'\n> +\n\nUsing reset --hard to remove a file outside the sparse checkout.\nA sane, simple test; thanks.  (Nitpick: I would have used\n\n\t>empty &&\n\tgit ls-files sub/added >output &&\n\ttest_cmp empty output\n\neven though that does not produce any more helpful output in this\ncase.)\n\n> +++ b/unpack-trees.c\n> @@ -53,6 +53,9 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n>  \n>  \tclear |= CE_HASHED | CE_UNHASHED;\n>  \n> +\tif (set & CE_REMOVE)\n> +\t\tset |= CE_WT_REMOVE;\n> +\n\nA bridge between the old and new worlds.\n\nI would have added CE_WT_REMOVE to callers instead (they all pass\nconstants more or less), but I suppose this way makes the patch\nshorter.\n\n[...]\n> @@ -84,7 +87,7 @@ static int check_updates(struct unpack_trees_options *o)\n>  \tif (o->update && o->verbose_update) {\n>  \t\tfor (total = cnt = 0; cnt < index->cache_nr; cnt++) {\n>  \t\t\tstruct cache_entry *ce = index->cache[cnt];\n> -\t\t\tif (ce->ce_flags & (CE_UPDATE | CE_REMOVE | CE_WT_REMOVE))\n> +\t\t\tif (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))\n\nOnly entries marked CE_WT_REMOVE will be touched by read-tree -u.\n\n> @@ -104,12 +107,6 @@ static int check_updates(struct unpack_trees_options *o)\n>  \t\t\t\tunlink_entry(ce);\n>  \t\t\tcontinue;\n>  \t\t}\n> -\n> -\t\tif (ce->ce_flags & CE_REMOVE) {\n[...]\n\nand the CE_WT_REMOVE case takes care of that.  Makes sense.\n\n> @@ -799,10 +796,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>  \t\t\t/*\n>  \t\t\t * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout\n>  \t\t\t * area as a result of ce_skip_worktree() shortcuts in\n> -\t\t\t * verify_absent() and verify_uptodate(). Clear them.\n> +\t\t\t * verify_absent() and verify_uptodate().\n> +\t\t\t * Make sure they don't modify worktree.\n>  \t\t\t */\n> -\t\t\tif (ce_skip_worktree(ce))\n> -\t\t\t\tce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);\n> +\t\t\tif (ce_skip_worktree(ce)) {\n> +\t\t\t\tce->ce_flags &= ~CE_UPDATE;\n> +\n> +\t\t\t\tif (ce->ce_flags & CE_REMOVE)\n> +\t\t\t\t\tce->ce_flags &= ~CE_WT_REMOVE;\n> +\t\t\t}\n>  \t\t\telse\n>  \t\t\t\tempty_worktree = 0;\n\nAh, and at last we come to the fix. :)\n\nThis is a little tricky: the CE_WT_REMOVE case (without CE_REMOVE)\nrepresents a narrowing of the checkout and should be preserved,\nwhile CE_WT_REMOVE|CE_REMOVE represents a removed index entry and\nshould be changed to just CE_REMOVE.\n\nBut it is a clear improvement over the code from before and should\nbehave as advertised now.  So aside from the log message,\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\n[1] v1.6.6.1~23^2~1 (Make on-disk index representation separate from\nin-core one, 2008-01-14) and v0.99~295 (git-read-tree: remove deleted\nfiles in the working directory, 2005-06-09).\n"},{"id":"146758","messageId":"AANLkTi=3pX=k=Pf5SnWt8s=hbvwAXcZdmcqP_+kgCyE5@mail.gmail.com","threadId":"24524","inReplyTo":"20100730013534.GB2182@burratino","subject":"Re: [PATCH 1/3] Fix sparse checkout not removing files from index","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-07-30T08:24:59Z","receivedAt":"2010-07-30T08:24:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/7/30 Jonathan Nieder <jrnieder@gmail.com>:\n>> +++ b/cache.h\n>> @@ -179,8 +179,7 @@ struct cache_entry {\n>>  #define CE_UNHASHED  (0x200000)\n>>  #define CE_CONFLICTED (0x800000)\n>>\n>> -/* Only remove in work directory, not index */\n>> -#define CE_WT_REMOVE (0x400000)\n>> +#define CE_WT_REMOVE (0x400000) /* remove in work directory */\n>\n> Before[1], CE_REMOVE was used by read-tree et al in core to represent\n> something to be removed from the index file and work tree (e.g., when\n> switching branches).\n>\n> In the new order, one uses CE_REMOVE|CE_WT_REMOVE for that, right?\n\nFor unpack_trees() only, yes. You made me grep for CE_REMOVE and found\nthat do_diff_cache() may turn CE_REMOVE on. The function is used by\nblame and reset.\n\nHmm.. It's for removing staged entries. So it's probably fine.\n\n>> +++ b/t/t1011-read-tree-sparse-checkout.sh\n>> @@ -147,4 +147,11 @@ test_expect_success 'read-tree adds to worktree, dirty case' '\n>>       grep -q dirty sub/added\n>>  '\n>>\n>> +test_expect_success 'read-tree --reset removes outside worktree' '\n>> +     echo init.t > .git/info/sparse-checkout &&\n>> +     git checkout -f top &&\n>> +     git reset --hard removed &&\n>> +     test -z \"$(git ls-files sub/added)\"\n>> +'\n>> +\n>\n> Using reset --hard to remove a file outside the sparse checkout.\n> A sane, simple test; thanks.  (Nitpick: I would have used\n>\n>        >empty &&\n>        git ls-files sub/added >output &&\n>        test_cmp empty output\n>\n> even though that does not produce any more helpful output in this\n> case.)\n\nThanks. That looks better.\n\n>\n>> +++ b/unpack-trees.c\n>> @@ -53,6 +53,9 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n>>\n>>       clear |= CE_HASHED | CE_UNHASHED;\n>>\n>> +     if (set & CE_REMOVE)\n>> +             set |= CE_WT_REMOVE;\n>> +\n>\n> A bridge between the old and new worlds.\n>\n> I would have added CE_WT_REMOVE to callers instead (they all pass\n> constants more or less), but I suppose this way makes the patch\n> shorter.\n\nIt'd better be done in one place. I don't expect anybody to set\nCE_REMOVE without CE_WT_REMOVE any time soon. Adding it the the\ncallers, the new callers might miss it.\n\n>> @@ -799,10 +796,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>>                       /*\n>>                        * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout\n>>                        * area as a result of ce_skip_worktree() shortcuts in\n>> -                      * verify_absent() and verify_uptodate(). Clear them.\n>> +                      * verify_absent() and verify_uptodate().\n>> +                      * Make sure they don't modify worktree.\n>>                        */\n>> -                     if (ce_skip_worktree(ce))\n>> -                             ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);\n>> +                     if (ce_skip_worktree(ce)) {\n>> +                             ce->ce_flags &= ~CE_UPDATE;\n>> +\n>> +                             if (ce->ce_flags & CE_REMOVE)\n>> +                                     ce->ce_flags &= ~CE_WT_REMOVE;\n>> +                     }\n>>                       else\n>>                               empty_worktree = 0;\n>\n> Ah, and at last we come to the fix. :)\n>\n> This is a little tricky: the CE_WT_REMOVE case (without CE_REMOVE)\n> represents a narrowing of the checkout and should be preserved,\n> while CE_WT_REMOVE|CE_REMOVE represents a removed index entry and\n> should be changed to just CE_REMOVE.\n\nYeah. I did wonder if it's worth a comment to explain. I forget why I\ndid not add that comment now.\n-- \nDuy\n"},{"id":"146798","messageId":"20100730195022.GB2448@burratino","threadId":"24524","inReplyTo":"AANLkTi=3pX=k=Pf5SnWt8s=hbvwAXcZdmcqP_+kgCyE5@mail.gmail.com","subject":"Re: [PATCH 1/3] Fix sparse checkout not removing files from index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-30T19:50:22Z","receivedAt":"2010-07-30T19:50:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n\n> For unpack_trees() only, yes.\n\nAh, that is what I was missing.\n\nSo would the following be correct?\n\n\tSubject: unpack-trees: let read-tree -u remove index entries outside sparse area\n\n\tTo avoid touching the worktree outside a sparse checkout,\n\twhen the update flag is enabled unpack_trees() clears the\n\tCE_UPDATE and CE_REMOVE flags on entries that do not match the\n\tsparse pattern before actually committing any updates to the\n\tindex file or worktree.\n\n\tThe effect on the index was unintentional; sparse checkout was\n\tnever meant to prevent index updates outside the area checked\n\tout.  And the result is very confusing: for example, after a\n\tfailed merge, currently \"git reset --hard\" does not reset the\n\tstate completely but an additional \"git reset --mixed\" will.\n\n\tSo stop clearing the CE_REMOVE flag.  Instead, maintain a\n\tCE_WT_REMOVE flag to separately track whether a particular\n\tfile removal should apply to the worktree in addition to the\n\tindex or not.\n\n\tThe CE_WT_REMOVE flag is used already to mark files that\n\tshould be removed because of a narrowing checkout area.  That\n\tusage will still apply; do not clear the CE_WT_REMOVE flag\n\tin that case (detectable because the CE_REMOVE flag is not\n\tset).\n\n\tThis bug masked some other bugs illustrated by the test\n\tsuite, which will be addressed by later patches.\n\n\tReported-by: Frédéric Brière <fbriere@fbriere.net>\n\tFixes: http://bugs.debian.org/583699\n\n> It'd better be done in one place.\n\nI think I understand better now, and now I agree.  (This patch is not\na change in the semantics of CE_REMOVE, but just some new unpack-trees\nbookkeeping.)\n\n> 2010/7/30 Jonathan Nieder <jrnieder@gmail.com>:\n\n>> This is a little tricky: the CE_WT_REMOVE case (without CE_REMOVE)\n>> represents a narrowing of the checkout and should be preserved,\n>> while CE_WT_REMOVE|CE_REMOVE represents a removed index entry and\n>> should be changed to just CE_REMOVE.\n>\n> Yeah. I did wonder if it's worth a comment to explain. I forget why I\n> did not add that comment now.\n\nWhat happens if an index entry is removed at the same time as the\ncheckout is narrowed?\n"},{"id":"146813","messageId":"20100731010439.GB5817@burratino","threadId":"24524","inReplyTo":"20100730195022.GB2448@burratino","subject":"Re: [PATCH 1/3] Fix sparse checkout not removing files from index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-31T01:04:40Z","receivedAt":"2010-07-31T01:04:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> What happens if an index entry is removed at the same time as the\n> checkout is narrowed?\n\nHm, maybe something like this would help.\n\nJonathan Nieder (2):\n  t1011 (sparse checkout): style nitpicks\n  read-tree -m -u: always remove relevant files when narrowing checkout\n\n cache.h                              |    3 +-\n t/t1011-read-tree-sparse-checkout.sh |  129 +++++++++++++++++++++-------------\n unpack-trees.c                       |   42 +++--------\n 3 files changed, 94 insertions(+), 80 deletions(-)\n"},{"id":"146814","messageId":"20100731010540.GC5817@burratino","threadId":"24524","inReplyTo":"20100731010439.GB5817@burratino","subject":"[PATCH 1/2] t1011 (sparse checkout): style nitpicks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-31T01:05:40Z","receivedAt":"2010-07-31T01:05:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The exit status of \"git ls-files\" in\n\n\ttest -z \"$(git ls-files sub/added)\"\n\nis ignored.  So redirect output to a file and compare to an\nempty file with test_cmp instead.\n\nWhile changing that, tweak the rest of the script to more closely\nfollow the test style guide.  Guarding setup commands with\ntest_expect_success makes it easy to see the scope in which some\nparticular data is used; removal of whitespace after >redirection\noperators is just for consistency.\n\nCc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n t/t1011-read-tree-sparse-checkout.sh |  108 ++++++++++++++++++---------------\n 1 files changed, 59 insertions(+), 49 deletions(-)\n\ndiff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh\nindex 885578a..2955071 100755\n--- a/t/t1011-read-tree-sparse-checkout.sh\n+++ b/t/t1011-read-tree-sparse-checkout.sh\n@@ -1,16 +1,30 @@\n #!/bin/sh\n \n-test_description='sparse checkout tests'\n+test_description='sparse checkout tests\n+\n+* (tag: removed, master) removed\n+| D\tsub/added\n+* (HEAD, tag: top) modified and added\n+| M\tinit.t\n+| A\tsub/added\n+* (tag: init) init\n+  A\tinit.t\n+'\n \n . ./test-lib.sh\n \n-cat >expected <<EOF\n-100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0\tinit.t\n-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tsub/added\n-EOF\n test_expect_success 'setup' '\n+\tcat >expected <<-\\EOF &&\n+\t100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0\tinit.t\n+\t100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tsub/added\n+\tEOF\n+\tcat >expected.swt <<-\\EOF &&\n+\tH init.t\n+\tH sub/added\n+\tEOF\n+\n \ttest_commit init &&\n-\techo modified >> init.t &&\n+\techo modified >>init.t &&\n \tmkdir sub &&\n \ttouch sub/added &&\n \tgit add init.t sub/added &&\n@@ -20,26 +34,22 @@ test_expect_success 'setup' '\n \tgit commit -m removed &&\n \tgit tag removed &&\n \tgit checkout top &&\n-\tgit ls-files --stage > result &&\n+\tgit ls-files --stage >result &&\n \ttest_cmp expected result\n '\n \n-cat >expected.swt <<EOF\n-H init.t\n-H sub/added\n-EOF\n test_expect_success 'read-tree without .git/info/sparse-checkout' '\n \tgit read-tree -m -u HEAD &&\n-\tgit ls-files --stage > result &&\n+\tgit ls-files --stage >result &&\n \ttest_cmp expected result &&\n-\tgit ls-files -t > result &&\n+\tgit ls-files -t >result &&\n \ttest_cmp expected.swt result\n '\n \n test_expect_success 'read-tree with .git/info/sparse-checkout but disabled' '\n-\techo > .git/info/sparse-checkout\n+\techo >.git/info/sparse-checkout\n \tgit read-tree -m -u HEAD &&\n-\tgit ls-files -t > result &&\n+\tgit ls-files -t >result &&\n \ttest_cmp expected.swt result &&\n \ttest -f init.t &&\n \ttest -f sub/added\n@@ -47,9 +57,9 @@ test_expect_success 'read-tree with .git/info/sparse-checkout but disabled' '\n \n test_expect_success 'read-tree --no-sparse-checkout with empty .git/info/sparse-checkout and enabled' '\n \tgit config core.sparsecheckout true &&\n-\techo > .git/info/sparse-checkout &&\n+\techo >.git/info/sparse-checkout &&\n \tgit read-tree --no-sparse-checkout -m -u HEAD &&\n-\tgit ls-files -t > result &&\n+\tgit ls-files -t >result &&\n \ttest_cmp expected.swt result &&\n \ttest -f init.t &&\n \ttest -f sub/added\n@@ -57,101 +67,101 @@ test_expect_success 'read-tree --no-sparse-checkout with empty .git/info/sparse-\n \n test_expect_success 'read-tree with empty .git/info/sparse-checkout' '\n \tgit config core.sparsecheckout true &&\n-\techo > .git/info/sparse-checkout &&\n+\techo >.git/info/sparse-checkout &&\n \ttest_must_fail git read-tree -m -u HEAD &&\n-\tgit ls-files --stage > result &&\n+\tgit ls-files --stage >result &&\n \ttest_cmp expected result &&\n-\tgit ls-files -t > result &&\n+\tgit ls-files -t >result &&\n \ttest_cmp expected.swt result &&\n \ttest -f init.t &&\n \ttest -f sub/added\n '\n \n-cat >expected.swt <<EOF\n-S init.t\n-H sub/added\n-EOF\n test_expect_success 'match directories with trailing slash' '\n+\tcat >expected.swt-noinit <<-\\EOF &&\n+\tS init.t\n+\tH sub/added\n+\tEOF\n+\n \techo sub/ > .git/info/sparse-checkout &&\n \tgit read-tree -m -u HEAD &&\n \tgit ls-files -t > result &&\n-\ttest_cmp expected.swt result &&\n+\ttest_cmp expected.swt-noinit result &&\n \ttest ! -f init.t &&\n \ttest -f sub/added\n '\n \n-cat >expected.swt <<EOF\n-H init.t\n-H sub/added\n-EOF\n test_expect_failure 'match directories without trailing slash' '\n-\techo init.t > .git/info/sparse-checkout &&\n-\techo sub >> .git/info/sparse-checkout &&\n+\techo init.t >.git/info/sparse-checkout &&\n+\techo sub >>.git/info/sparse-checkout &&\n \tgit read-tree -m -u HEAD &&\n-\tgit ls-files -t > result &&\n+\tgit ls-files -t >result &&\n \ttest_cmp expected.swt result &&\n \ttest ! -f init.t &&\n \ttest -f sub/added\n '\n \n-cat >expected.swt <<EOF\n-H init.t\n-S sub/added\n-EOF\n test_expect_success 'checkout area changes' '\n-\techo init.t > .git/info/sparse-checkout &&\n+\tcat >expected.swt-nosub <<-\\EOF &&\n+\tH init.t\n+\tS sub/added\n+\tEOF\n+\n+\techo init.t >.git/info/sparse-checkout &&\n \tgit read-tree -m -u HEAD &&\n-\tgit ls-files -t > result &&\n-\ttest_cmp expected.swt result &&\n+\tgit ls-files -t >result &&\n+\ttest_cmp expected.swt-nosub result &&\n \ttest -f init.t &&\n \ttest ! -f sub/added\n '\n \n test_expect_success 'read-tree updates worktree, absent case' '\n-\techo sub/added > .git/info/sparse-checkout &&\n+\techo sub/added >.git/info/sparse-checkout &&\n \tgit checkout -f top &&\n \tgit read-tree -m -u HEAD^ &&\n \ttest ! -f init.t\n '\n \n test_expect_success 'read-tree updates worktree, dirty case' '\n-\techo sub/added > .git/info/sparse-checkout &&\n+\techo sub/added >.git/info/sparse-checkout &&\n \tgit checkout -f top &&\n-\techo dirty > init.t &&\n+\techo dirty >init.t &&\n \tgit read-tree -m -u HEAD^ &&\n \tgrep -q dirty init.t &&\n \trm init.t\n '\n \n test_expect_success 'read-tree removes worktree, dirty case' '\n-\techo init.t > .git/info/sparse-checkout &&\n+\techo init.t >.git/info/sparse-checkout &&\n \tgit checkout -f top &&\n-\techo dirty > added &&\n+\techo dirty >added &&\n \tgit read-tree -m -u HEAD^ &&\n \tgrep -q dirty added\n '\n \n test_expect_success 'read-tree adds to worktree, absent case' '\n-\techo init.t > .git/info/sparse-checkout &&\n+\techo init.t >.git/info/sparse-checkout &&\n \tgit checkout -f removed &&\n \tgit read-tree -u -m HEAD^ &&\n \ttest ! -f sub/added\n '\n \n test_expect_failure 'read-tree adds to worktree, dirty case' '\n-\techo init.t > .git/info/sparse-checkout &&\n+\techo init.t >.git/info/sparse-checkout &&\n \tgit checkout -f removed &&\n \tmkdir sub &&\n-\techo dirty > sub/added &&\n+\techo dirty >sub/added &&\n \tgit read-tree -u -m HEAD^ &&\n \tgrep -q dirty sub/added\n '\n \n test_expect_success 'read-tree --reset removes outside worktree' '\n-\techo init.t > .git/info/sparse-checkout &&\n+\t>empty &&\n+\techo init.t >.git/info/sparse-checkout &&\n \tgit checkout -f top &&\n \tgit reset --hard removed &&\n-\ttest -z \"$(git ls-files sub/added)\"\n+\tgit ls-files sub/added >result &&\n+\ttest_cmp empty result\n '\n \n test_done\n-- \n1.7.2.1.544.ga752d.dirty\n"},{"id":"146815","messageId":"20100731010920.GD5817@burratino","threadId":"24524","inReplyTo":"20100731010439.GB5817@burratino","subject":"[PATCH 2/2] read-tree -m -u: always remove relevant files when narrowing checkout","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-31T01:09:20Z","receivedAt":"2010-07-31T01:09:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When narrowing the checkout pattern to exclude a file that the merge\nwas going to delete, read-tree decides two wrongs make a right and\nleaves the file behind.\n\nOr that’s what it looks like.  Actually, for some reason\napply_sparse_checkout() is not paying proper attention to entries\nremoved by the merge, so they look like ordinary deletions outside\nthe sparse area.\n\nWhile fixing that, clarify the semantics of the unpack-trees flags:\n\n - CE_UPDATE marks an entry for which the worktree might need\n   to be updated to match.  If CE_UPDATE is not set, changes\n   only apply to the index file.\n\n - CE_REMOVE marks an entry removed by the merge (just like\n   it did before).\n\n - CE_WT_REMOVE marks an entry that no longer matches the sparse\n   pattern.\n\nCc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nSorry for the lack of clarity before.  This is just to give the\nidea; I have no confidence that I am not breaking something,\nespecially because I have not checked why the \"if\" I am removing\nin apply_sparse_checkout was added.\n\n cache.h                              |    3 +-\n t/t1011-read-tree-sparse-checkout.sh |   21 +++++++++++++++++\n unpack-trees.c                       |   42 +++++++++------------------------\n 3 files changed, 35 insertions(+), 31 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3c7fb6f..c9fa3df 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -179,7 +179,8 @@ struct cache_entry {\n #define CE_UNHASHED  (0x200000)\n #define CE_CONFLICTED (0x800000)\n \n-#define CE_WT_REMOVE (0x400000) /* remove in work directory */\n+/* Only remove in work directory, not index */\n+#define CE_WT_REMOVE (0x400000)\n \n #define CE_UNPACKED  (0x1000000)\n \ndiff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh\nindex 2955071..247f295 100755\n--- a/t/t1011-read-tree-sparse-checkout.sh\n+++ b/t/t1011-read-tree-sparse-checkout.sh\n@@ -155,6 +155,27 @@ test_expect_failure 'read-tree adds to worktree, dirty case' '\n \tgrep -q dirty sub/added\n '\n \n+test_expect_success 'narrow checkout while removing file' '\n+\techo \"H init.t\" >expected.swt-after &&\n+\tprintf \"%s\\n\" init.t sub/added | sort >expected.ls-before\n+\techo init.t >expected.ls-after &&\n+\n+\tprintf \"%s\\n\" init.t sub/added >.git/info/sparse-checkout &&\n+\tgit checkout -f top &&\n+\tgit ls-files -t >index.before &&\n+\tls init.t sub/added | sort >worktree.before &&\n+\n+\techo init.t >.git/info/sparse-checkout &&\n+\tgit read-tree -m -u HEAD^ &&\n+\tgit ls-files -t >index.after &&\n+\tls init.t sub/added | sort >worktree.after &&\n+\n+\ttest_cmp expected.swt index.before &&\n+\ttest_cmp expected.swt-after index.after &&\n+\ttest_cmp expected.ls-before worktree.before &&\n+\ttest_cmp expected.ls-after worktree.after\n+'\n+\n test_expect_success 'read-tree --reset removes outside worktree' '\n \t>empty &&\n \techo init.t >.git/info/sparse-checkout &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 523e1de..deabfd9 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -53,9 +53,6 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n \n \tclear |= CE_HASHED | CE_UNHASHED;\n \n-\tif (set & CE_REMOVE)\n-\t\tset |= CE_WT_REMOVE;\n-\n \tmemcpy(new, ce, size);\n \tnew->next = NULL;\n \tnew->ce_flags = (new->ce_flags & ~clear) | set;\n@@ -87,7 +84,7 @@ static int check_updates(struct unpack_trees_options *o)\n \tif (o->update && o->verbose_update) {\n \t\tfor (total = cnt = 0; cnt < index->cache_nr; cnt++) {\n \t\t\tstruct cache_entry *ce = index->cache[cnt];\n-\t\t\tif (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))\n+\t\t\tif (ce->ce_flags & CE_UPDATE)\n \t\t\t\ttotal++;\n \t\t}\n \n@@ -101,11 +98,12 @@ static int check_updates(struct unpack_trees_options *o)\n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n \n-\t\tif (ce->ce_flags & CE_WT_REMOVE) {\n+\t\tif (ce->ce_flags & CE_UPDATE &&\n+\t\t    ce->ce_flags & (CE_REMOVE | CE_WT_REMOVE)) {\n \t\t\tdisplay_progress(progress, ++cnt);\n+\t\t\tce->ce_flags &= ~CE_UPDATE & ~CE_WT_REMOVE;\n \t\t\tif (o->update)\n \t\t\t\tunlink_entry(ce);\n-\t\t\tcontinue;\n \t\t}\n \t}\n \tremove_marked_cache_entries(&o->result);\n@@ -152,15 +150,6 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt\n \telse\n \t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \n-\t/*\n-\t * We only care about files getting into the checkout area\n-\t * If merge strategies want to remove some, go ahead, this\n-\t * flag will be removed eventually in unpack_trees() if it's\n-\t * outside checkout area.\n-\t */\n-\tif (ce->ce_flags & CE_REMOVE)\n-\t\treturn 0;\n-\n \tif (!was_skip_worktree && ce_skip_worktree(ce)) {\n \t\t/*\n \t\t * If CE_UPDATE is set, verify_uptodate() must be called already\n@@ -169,7 +158,7 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt\n \t\t */\n \t\tif (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o))\n \t\t\treturn -1;\n-\t\tce->ce_flags |= CE_WT_REMOVE;\n+\t\tce->ce_flags |= CE_WT_REMOVE | CE_UPDATE;\n \t}\n \tif (was_skip_worktree && !ce_skip_worktree(ce)) {\n \t\tif (verify_absent_sparse(ce, \"overwritten\", o))\n@@ -796,20 +785,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t\t\tgoto done;\n \t\t\t}\n \t\t\t/*\n-\t\t\t * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout\n-\t\t\t * area as a result of ce_skip_worktree() shortcuts in\n-\t\t\t * verify_absent() and verify_uptodate().\n-\t\t\t * Make sure they don't modify worktree.\n+\t\t\t * Do not modify worktree outside of sparse checkout area,\n+\t\t\t * except to make the checkout more narrow.\n \t\t\t */\n-\t\t\tif (ce_skip_worktree(ce)) {\n-\t\t\t\tce->ce_flags &= ~CE_UPDATE;\n-\n-\t\t\t\tif (ce->ce_flags & CE_REMOVE)\n-\t\t\t\t\tce->ce_flags &= ~CE_WT_REMOVE;\n-\t\t\t}\n-\t\t\telse\n+\t\t\tif (!ce_skip_worktree(ce))\n \t\t\t\tempty_worktree = 0;\n-\n+\t\t\telse if (!(ce->ce_flags & CE_WT_REMOVE))\n+\t\t\t\tce->ce_flags &= ~CE_UPDATE;\n \t\t}\n \t\tif (o->result.cache_nr && empty_worktree) {\n \t\t\tret = unpack_failed(o, \"Sparse checkout leaves no entry on working directory\");\n@@ -968,7 +950,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \t\tif (!ce_stage(ce2)) {\n \t\t\tif (verify_uptodate(ce2, o))\n \t\t\t\treturn -1;\n-\t\t\tadd_entry(o, ce2, CE_REMOVE, 0);\n+\t\t\tadd_entry(o, ce2, CE_REMOVE|CE_UPDATE, 0);\n \t\t\tmark_ce_used(ce2, o);\n \t\t}\n \t\tcnt++;\n@@ -1138,7 +1120,7 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,\n \t}\n \tif (!(old->ce_flags & CE_CONFLICTED) && verify_uptodate(old, o))\n \t\treturn -1;\n-\tadd_entry(o, ce, CE_REMOVE, 0);\n+\tadd_entry(o, ce, CE_REMOVE|CE_UPDATE, 0);\n \tinvalidate_ce_path(ce, o);\n \treturn 1;\n }\n-- \n1.7.2.1.544.ga752d.dirty\n"},{"id":"146822","messageId":"20100731021144.GA906@burratino","threadId":"24524","inReplyTo":"1280135310-2347-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/3] unpack-trees.c: Do not check ce_stage in will_have_skip_worktree()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-31T02:11:44Z","receivedAt":"2010-07-31T02:11:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> The idea of sparse checkout is conflicted entries should always stay\n> in worktree, regardless $GIT_DIR/info/sparse-checkout. Therefore,\n> ce_stage(ce) usually means no CE_SKIP_WORKTREE. This is true when all\n> entries have been merged into the index, and identical staged entries\n> collapsed.\n> \n> However, will_have_skip_worktree() since f1f523e (unpack-trees():\n> ignore worktree check outside checkout area) is also used earlier in\n> verify_* functions, when entries have not been merged to index yet\n> and ce_stage() may not be zero. Checking ce_stage() then can make\n> unnecessary verification on entries outside checkout area and error out.\n\ns/make/provoke/?\n\nSo: conflicts from unmerged index entries are supposed to be kept in\nthe work tree; but unpack_trees() is checking too early and seeing\nconflicts where there are none.  Do I understand correctly?\n\n> This fixes part of test case \"read-tree adds to worktree, dirty case\".\n\nA summary might help people who do not remember the test:\n\n\tSo attempting to add a file outside the checkout area \n\twhere there is a file already present (as in\n\tt1011.13 \"read-tree adds to worktree, dirty case\")\n\twill no longer error out with\n\n\terror: Untracked working tree file 'sub/added' would be overwritten by merge.\n\n\tbut instead will remove the file.  (The latter problem will be\n\taddressed in a later patch.)\n\n> +++ a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -135,9 +135,6 @@ static int will_have_skip_worktree(const struct cache_entry *ce, struct unpack_t\n>  {\n>  \tconst char *basename;\n>  \n> -\tif (ce_stage(ce))\n> -\t\treturn 0;\n> -\n>  \tbasename = strrchr(ce->name, '/');\n>  \tbasename = basename ? basename+1 : ce->name;\n>  \treturn excluded_from_list(ce->name, ce_namelen(ce), basename, NULL, o->el) <= 0;\n> @@ -147,7 +144,7 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt\n>  {\n>  \tint was_skip_worktree = ce_skip_worktree(ce);\n>  \n> -\tif (will_have_skip_worktree(ce, o))\n> +\tif (!ce_stage(ce) && will_have_skip_worktree(ce, o))\n>  \t\tce->ce_flags |= CE_SKIP_WORKTREE;\n>  \telse\n>  \t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n\nMakes sense.\n\nThanks,\nJonathan\n"},{"id":"146826","messageId":"20100731023219.GB906@burratino","threadId":"24524","inReplyTo":"1280135310-2347-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/3] Mark new entries skip-worktree appropriately","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-31T02:32:20Z","receivedAt":"2010-07-31T02:32:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> Sparse checkout updates worktree based on the old and new\n> skip-worktree status when $GIT_DIR/info/sparse-checkout changes:\n> \n> old = ce_skip_worktree(ce);           // current skip-worktree\n> new = will_have_skip_work_tree(ce);   // from $GIT..sparse-checkout\n> if (old && !new) add_file_back(ce);   // shrink checkout area\n> if (!old && new) remove_file_out(ce); [1] // enlarge checkout area\n> \n> New entries after merging will always have skip-worktree unset\n> (i.e. old = 0). If those files are filtered out by\n> $GIT_DIR/info/sparse-checkout (i.e. new != 0), then case [1] will\n> happen. But there is nothing to remove because they're new.\n\nWhen using unpack_trees to add a new file, there is no in-file\nindex entry to grab the previous skip worktree bit from.  If it\nis outside the checkout area, we should pretend it was skipped before,\ntoo; otherwise a checkout can cause a file on disk whose name\ncoincides with a newly added outside-worktree file to be deleted.\n\nDo I understand correctly?\n\n> +++ b/unpack-trees.c\n> @@ -1091,6 +1091,8 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n>  \tif (!old) {\n>  \t\tif (verify_absent(merge, \"overwritten\", o))\n>  \t\t\treturn -1;\n> +\t\tif (!o->skip_sparse_checkout && will_have_skip_worktree(merge, o))\n> +\t\t\tupdate |= CE_SKIP_WORKTREE;\n>  \t\tinvalidate_ce_path(merge, o);\n>  \t} else if (!(old->ce_flags & CE_CONFLICTED)) {\n>  \t\t/*\n\nLooks sane.\n\nThanks for the fixes,\nJonathan\n"},{"id":"146828","messageId":"AANLkTi=K1Q+BkMvTPxKCo_VFgWY-ZLRznFcBEN2qQ3Yk@mail.gmail.com","threadId":"24524","inReplyTo":"20100731021144.GA906@burratino","subject":"Re: [PATCH 2/3] unpack-trees.c: Do not check ce_stage in will_have_skip_worktree()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-07-31T03:12:59Z","receivedAt":"2010-07-31T03:12:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/7/31 Jonathan Nieder <jrnieder@gmail.com>:\n> Nguyễn Thái Ngọc Duy wrote:\n>\n>> The idea of sparse checkout is conflicted entries should always stay\n>> in worktree, regardless $GIT_DIR/info/sparse-checkout. Therefore,\n>> ce_stage(ce) usually means no CE_SKIP_WORKTREE. This is true when all\n>> entries have been merged into the index, and identical staged entries\n>> collapsed.\n>>\n>> However, will_have_skip_worktree() since f1f523e (unpack-trees():\n>> ignore worktree check outside checkout area) is also used earlier in\n>> verify_* functions, when entries have not been merged to index yet\n>> and ce_stage() may not be zero. Checking ce_stage() then can make\n>> unnecessary verification on entries outside checkout area and error out.\n>\n> s/make/provoke/?\n>\n> So: conflicts from unmerged index entries are supposed to be kept in\n> the work tree; but unpack_trees() is checking too early and seeing\n> conflicts where there are none.  Do I understand correctly?\n\nCorrect.\n-- \nDuy\n"},{"id":"146829","messageId":"AANLkTikhXy1KjA5=SJ9ZwS_XtV99aHPkeo3XoPXqxeKh@mail.gmail.com","threadId":"24524","inReplyTo":"20100731023219.GB906@burratino","subject":"Re: [PATCH 3/3] Mark new entries skip-worktree appropriately","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-07-31T03:13:32Z","receivedAt":"2010-07-31T03:13:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/7/31 Jonathan Nieder <jrnieder@gmail.com>:\n> Nguyễn Thái Ngọc Duy wrote:\n>\n>> Sparse checkout updates worktree based on the old and new\n>> skip-worktree status when $GIT_DIR/info/sparse-checkout changes:\n>>\n>> old = ce_skip_worktree(ce);           // current skip-worktree\n>> new = will_have_skip_work_tree(ce);   // from $GIT..sparse-checkout\n>> if (old && !new) add_file_back(ce);   // shrink checkout area\n>> if (!old && new) remove_file_out(ce); [1] // enlarge checkout area\n>>\n>> New entries after merging will always have skip-worktree unset\n>> (i.e. old = 0). If those files are filtered out by\n>> $GIT_DIR/info/sparse-checkout (i.e. new != 0), then case [1] will\n>> happen. But there is nothing to remove because they're new.\n>\n> When using unpack_trees to add a new file, there is no in-file\n> index entry to grab the previous skip worktree bit from.  If it\n> is outside the checkout area, we should pretend it was skipped before,\n> too; otherwise a checkout can cause a file on disk whose name\n> coincides with a newly added outside-worktree file to be deleted.\n>\n> Do I understand correctly?\n\nYes.\n-- \nDuy\n"},{"id":"146830","messageId":"AANLkTi=n-LYGiYw=g4QsO=9GtvzHc8Vn=m99pcDR5r00@mail.gmail.com","threadId":"24524","inReplyTo":"20100731010439.GB5817@burratino","subject":"Re: [PATCH 1/3] Fix sparse checkout not removing files from index","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-07-31T03:28:01Z","receivedAt":"2010-07-31T03:28:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 31, 2010 at 11:04 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Jonathan Nieder wrote:\n>\n>> What happens if an index entry is removed at the same time as the\n>> checkout is narrowed?\n>\n> Hm, maybe something like this would help.\n\nOr maybe we should move the CE_UPDATE/CE_WT_REMOVE removal code into\napply_sparse_checkout(). That function knows when checkout area is\nnarrowed and CE_WT_REMOVE should not be removed in that case,\nregardless CE_REMOVE|CE_WT_REMOVE combination.\n-- \nDuy\n"},{"id":"146831","messageId":"20100731032902.GA1607@burratino","threadId":"24524","inReplyTo":"AANLkTikhXy1KjA5=SJ9ZwS_XtV99aHPkeo3XoPXqxeKh@mail.gmail.com","subject":"Re: [PATCH 3/3] Mark new entries skip-worktree appropriately","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-31T03:29:02Z","receivedAt":"2010-07-31T03:29:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n> 2010/7/31 Jonathan Nieder <jrnieder@gmail.com>:\n\n>> Do I understand correctly?\n>\n> Yes.\n\nAh, good.\n\nThis is fun code to play with.  Thanks for putting it together.\n"},{"id":"146832","messageId":"20100731033339.GB1607@burratino","threadId":"24524","inReplyTo":"AANLkTi=n-LYGiYw=g4QsO=9GtvzHc8Vn=m99pcDR5r00@mail.gmail.com","subject":"Re: [PATCH 1/3] Fix sparse checkout not removing files from index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-31T03:33:39Z","receivedAt":"2010-07-31T03:33:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n\n> maybe we should move the CE_UPDATE/CE_WT_REMOVE removal code into\n> apply_sparse_checkout().\n\nNot a bad idea regardless.\n"},{"id":"146833","messageId":"AANLkTi=HGqwg1HUMej-NeYgCf2454erOo1EYp7z-LT1D@mail.gmail.com","threadId":"24524","inReplyTo":"20100731033339.GB1607@burratino","subject":"Re: [PATCH 1/3] Fix sparse checkout not removing files from index","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-07-31T03:48:36Z","receivedAt":"2010-07-31T03:48:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 31, 2010 at 1:33 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Nguyen Thai Ngoc Duy wrote:\n>\n>> maybe we should move the CE_UPDATE/CE_WT_REMOVE removal code into\n>> apply_sparse_checkout().\n>\n> Not a bad idea regardless.\n\nYeah, something like this, perhaps. It's not tested. I'll repost the\npatch later, incorporating all your suggestions.\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f2d148c..48654b3 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -153,6 +153,26 @@ static int apply_sparse_checkout(struct\ncache_entry *ce, struct unpack_trees_opt\n \t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n\n \t/*\n+\t * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout\n+\t * area as a result of ce_skip_worktree() shortcuts in\n+\t * verify_absent() and verify_uptodate().\n+\t * Make sure they don't modify worktree if they are already\n+\t * outside checkout area\n+\t */\n+\tif (was_skip_worktree && ce_skip_worktree(ce)) {\n+\t\tce->ce_flags &= ~CE_UPDATE;\n+\n+\t\t/*\n+\t\t * By default, when CE_REMOVE is on, CE_WT_REMOVE is also\n+\t\t * on to get that file removed from both index and worktree.\n+\t\t * If that file is already outside worktree area, don't\n+\t\t * bother remove it.\n+\t\t */\n+\t\tif (ce->ce_flags & CE_REMOVE)\n+\t\t\tce->ce_flags &= ~CE_WT_REMOVE;\n+\t}\n+\n+\t/*\n \t * We only care about files getting into the checkout area\n \t * If merge strategies want to remove some, go ahead, this\n \t * flag will be removed eventually in unpack_trees() if it's\n@@ -793,19 +813,7 @@ int unpack_trees(unsigned len, struct tree_desc\n*t, struct unpack_trees_options\n \t\t\t\tret = -1;\n \t\t\t\tgoto done;\n \t\t\t}\n-\t\t\t/*\n-\t\t\t * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout\n-\t\t\t * area as a result of ce_skip_worktree() shortcuts in\n-\t\t\t * verify_absent() and verify_uptodate().\n-\t\t\t * Make sure they don't modify worktree.\n-\t\t\t */\n-\t\t\tif (ce_skip_worktree(ce)) {\n-\t\t\t\tce->ce_flags &= ~CE_UPDATE;\n-\n-\t\t\t\tif (ce->ce_flags & CE_REMOVE)\n-\t\t\t\t\tce->ce_flags &= ~CE_WT_REMOVE;\n-\t\t\t}\n-\t\t\telse\n+\t\t\tif (!ce_skip_worktree(ce))\n \t\t\t\tempty_worktree = 0;\n\n \t\t}\n-- \nDuy\n"}]}