{"thread":{"id":"53696","subject":"[PATCH] unpack-trees: do not set SKIP_WORKTREE on submodules","startedAt":"2020-06-17T01:21:29Z","lastAt":"2020-06-18T20:29:42Z","messageCount":6,"participants":["Elijah Newren via GitGitGadget","Matheus Tavares Bernardino","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"399949","messageId":"pull.809.git.git.1592356884310.gitgitgadget@gmail.com","threadId":"53696","inReplyTo":null,"subject":"[PATCH] unpack-trees: do not set SKIP_WORKTREE on submodules","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-17T01:21:24Z","receivedAt":"2020-06-17T01:21:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nAs noted in commit e7d7c73249 (\"git-sparse-checkout: clarify\ninteractions with submodules\", 2020-06-10), sparse-checkout cannot\nremove submodules even if they don't match the sparsity patterns,\nbecause doing so would risk data loss -- unpushed, uncommitted, or\nuntracked changes could all be lost.  That commit also updated the\ndocumentation to point out that submodule initialization state was a\nparallel, orthogonal reason that entries in the index might not be\npresent in the working tree.\n\nHowever, sparsity and submodule initialization weren't actually fully\northogonal yet.  The SKIP_WORKTREE handling in unpack_trees would\nattempt to set the SKIP_WORKTREE bit on submodules when the submodule\ndid not match the sparsity patterns.  This resulted in innocuous but\npotentially alarming warning messages:\n\n    warning: unable to rmdir 'sha1collisiondetection': Directory not empty\n\nIt could also make things confusing since the entry would be marked as\nSKIP_WORKTREE in the index but actually still be present in the working\ntree:\n\n    $ git ls-files -t | grep sha1collisiondetection\n    S sha1collisiondetection\n    $ ls -al sha1collisiondetection/ | wc -l\n    13\n\nSubmodules have always been their own form of \"partial checkout\"\nbehavior, with their own \"present or not\" state determined by running\n\"git submodule [init|deinit|update]\".  Enforce that separation by having\nthe SKIP_WORKTREE logic not touch submodules and allow submodules to\ncontinue using their own initialization state for determining if the\nsubmodule is present.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    unpack-trees: do not set SKIP_WORKTREE on submodules\n    \n    Interactions between submodules and sparsity patterns have come up a few\n    times, with a certain edge case coming up multiple times recently:\n    should a submodule have the SKIP_WORKTREE bit set if the submodule path\n    does not match the sparsity patterns?[1][2][3].\n    \n    Here I try to resolve the question, with the answer of 'no'.\n    \n    This patch depends on en/sparse-with-submodule-doc lightly -- the commit\n    message in this patch references the commit from that other series. It\n    could possibly be considered an addition to that other topic, but\n    \"sparse-with-submodule-doc\" implies the other topic is only a\n    documentation change, whereas this patch involves a code change.\n    \n    [1] \n    https://lore.kernel.org/git/pull.805.git.git.1591831009762.gitgitgadget@gmail.com/\n    \n    > Since submodules may have unpushed changes or untracked files,\n    > removing them could result in data loss. Thus, changing sparse\n    > inclusion/exclusion rules will not cause an already checked out\n    > submodule to be removed from the working copy. Said another way, just\n    > as checkout will not cause submodules to be automatically removed or\n    > initialized even when switching between branches that remove or add\n    > submodules, using sparse-checkout to reduce or expand the scope of\n    > \"interesting\" files will not cause submodules to be automatically\n    > deinitialized or initialized either.\n    \n    [2] \n    https://lore.kernel.org/git/CABPp-BE+BL3Nq=Co=-kNB_wr=6gqX8zcGwa0ega_pGBpk6xYsg@mail.gmail.com/\n    \n    > If sparsity patterns would exclude a submodule that is initialized,\n    > sparse-checkout clearly can't remove the submodule. However, should it\n    > set the SKIP_WORKTREE bit for that submodule if it's not going to\n    > remove it?\n    \n    [3] \n    https://lore.kernel.org/git/CABPp-BFJG7uFAZNidDPK2o7eHv-eYBsmcdnVxkOnKcZo7WzmFQ@mail.gmail.com/\n    \n    >> Or if you don't deinitialize a submodule that is excluded by the\n    >> sparsity patterns (thus remaining in the working copy, anyway).\n    >\n    > This case requires more thought. If a submodule doesn't match the\n    > sparsity patterns, we already said elsewhere that sparse-checkout\n    > should not remove the submodule (since doing so would risk data loss).\n    > But do we set the SKIP_WORKTREE bit for it? Generally, sparse-checkout\n    > avoids removing files with modifications, and if it doesn't remove\n    > them it also doesn't set the SKIP_WORKTREE bit. For consistency,\n    > should sparse-checkout not set SKIP_WORKTREE for initialized\n    > submodules?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-809%2Fnewren%2Fsparse-submodule-no-skip-worktree-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-809/newren/sparse-submodule-no-skip-worktree-v1\nPull-Request: https://github.com/git/git/pull/809\n\n unpack-trees.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 4be5fc30754..9922522b29d 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1524,7 +1524,7 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n \tint i;\n \n \t/*\n-\t * 1. Pretend the narrowest worktree: only unmerged entries\n+\t * 1. Pretend the narrowest worktree: only unmerged files and symlinks\n \t * are checked out\n \t */\n \tfor (i = 0; i < istate->cache_nr; i++) {\n@@ -1533,7 +1533,8 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n \t\tif (select_flag && !(ce->ce_flags & select_flag))\n \t\t\tcontinue;\n \n-\t\tif (!ce_stage(ce) && !(ce->ce_flags & CE_CONFLICTED))\n+\t\tif (!ce_stage(ce) && !(ce->ce_flags & CE_CONFLICTED) &&\n+\t\t    !S_ISGITLINK(ce->ce_mode))\n \t\t\tce->ce_flags |= skip_wt_flag;\n \t\telse\n \t\t\tce->ce_flags &= ~skip_wt_flag;\n\nbase-commit: eebb51ba8cab97c0b3f3f18eaab7796803b8494b\n-- \ngitgitgadget\n"},{"id":"399981","messageId":"CAHd-oW5gTJO=6pYXvg3v=JfjffcajPyMpsUOoqXnozwYrg3WwQ@mail.gmail.com","threadId":"53696","inReplyTo":"pull.809.git.git.1592356884310.gitgitgadget@gmail.com","subject":"Re: [PATCH] unpack-trees: do not set SKIP_WORKTREE on submodules","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-17T17:58:41Z","receivedAt":"2020-06-17T17:58:58Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Jun 16, 2020 at 10:21 PM Elijah Newren via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Elijah Newren <newren@gmail.com>\n>\n> As noted in commit e7d7c73249 (\"git-sparse-checkout: clarify\n> interactions with submodules\", 2020-06-10), sparse-checkout cannot\n> remove submodules even if they don't match the sparsity patterns,\n> because doing so would risk data loss -- unpushed, uncommitted, or\n> untracked changes could all be lost.  That commit also updated the\n> documentation to point out that submodule initialization state was a\n> parallel, orthogonal reason that entries in the index might not be\n> present in the working tree.\n>\n> However, sparsity and submodule initialization weren't actually fully\n> orthogonal yet.  The SKIP_WORKTREE handling in unpack_trees would\n> attempt to set the SKIP_WORKTREE bit on submodules when the submodule\n> did not match the sparsity patterns.  This resulted in innocuous but\n> potentially alarming warning messages:\n>\n>     warning: unable to rmdir 'sha1collisiondetection': Directory not empty\n>\n> It could also make things confusing since the entry would be marked as\n> SKIP_WORKTREE in the index but actually still be present in the working\n> tree:\n>\n>     $ git ls-files -t | grep sha1collisiondetection\n>     S sha1collisiondetection\n>     $ ls -al sha1collisiondetection/ | wc -l\n>     13\n>\n> Submodules have always been their own form of \"partial checkout\"\n> behavior, with their own \"present or not\" state determined by running\n> \"git submodule [init|deinit|update]\".  Enforce that separation by having\n> the SKIP_WORKTREE logic not touch submodules and allow submodules to\n> continue using their own initialization state for determining if the\n> submodule is present.\n\nMakes sense to me.\n\nI'm just thinking about the possible implications in grep (with\nmt/grep-sparse-checkout). As you mentioned in [1], users might think\nof \"git grep $rev $pat\" as an optimized version of \"git checkout $rev\n&& git grep $pat\". And, in this sense, they probably expect the output\nof these two operations to be equal. But if we don't set the\nSKIP_WORKTREE bit for submodules they might diverge.\n\nAs an example, if we have a repository like:\n.\n|-- A\n|   `-- sub\n`-- B\n\nAnd the [cone-mode] sparsity rules:\n/*\n!/*/\n/B/\n\nThen, \"git grep --recurse-submodules $rev $pat\" would search only in B\n(as A doesn't match the sparsity patterns and thus, is not recursed\ninto). But \"git checkout $rev && git grep --recurse-submodules $pat\"\nwould search in both B and A/sub (as the latter would not have the\nSKIP_WORKTREE bit set).\n\nThis might be a problem for git-grep, not git-sparse-checkout. But I'm\nnot sure how we could solve it efficiently, as the submodule might be\ndeep down in a path whose first dir was already ignored for not\nmatching the sparsity patterns. Is this a problem we should consider,\nor is it OK if the outputs of these two operations diverge?\n\n[1]: https://lore.kernel.org/git/CABPp-BFKHivKffBPO0M_s-JtpiLyEMLZr+sX9_yZk9ZX7ULtbw@mail.gmail.com/\n"},{"id":"400015","messageId":"CABPp-BHtwifTHXxoxTKvz0mx45e2N-4SBTTfoRePcmMFAn1O2g@mail.gmail.com","threadId":"53696","inReplyTo":"CAHd-oW5gTJO=6pYXvg3v=JfjffcajPyMpsUOoqXnozwYrg3WwQ@mail.gmail.com","subject":"Re: [PATCH] unpack-trees: do not set SKIP_WORKTREE on submodules","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-06-18T00:24:26Z","receivedAt":"2020-06-18T00:25:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Matheus,\n\nOn Wed, Jun 17, 2020 at 10:58 AM Matheus Tavares Bernardino\n<matheus.bernardino@usp.br> wrote:\n>\n> On Tue, Jun 16, 2020 at 10:21 PM Elijah Newren via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> >\n> > From: Elijah Newren <newren@gmail.com>\n> >\n> > As noted in commit e7d7c73249 (\"git-sparse-checkout: clarify\n> > interactions with submodules\", 2020-06-10), sparse-checkout cannot\n> > remove submodules even if they don't match the sparsity patterns,\n> > because doing so would risk data loss -- unpushed, uncommitted, or\n> > untracked changes could all be lost.  That commit also updated the\n> > documentation to point out that submodule initialization state was a\n> > parallel, orthogonal reason that entries in the index might not be\n> > present in the working tree.\n> >\n> > However, sparsity and submodule initialization weren't actually fully\n> > orthogonal yet.  The SKIP_WORKTREE handling in unpack_trees would\n> > attempt to set the SKIP_WORKTREE bit on submodules when the submodule\n> > did not match the sparsity patterns.  This resulted in innocuous but\n> > potentially alarming warning messages:\n> >\n> >     warning: unable to rmdir 'sha1collisiondetection': Directory not empty\n> >\n> > It could also make things confusing since the entry would be marked as\n> > SKIP_WORKTREE in the index but actually still be present in the working\n> > tree:\n> >\n> >     $ git ls-files -t | grep sha1collisiondetection\n> >     S sha1collisiondetection\n> >     $ ls -al sha1collisiondetection/ | wc -l\n> >     13\n> >\n> > Submodules have always been their own form of \"partial checkout\"\n> > behavior, with their own \"present or not\" state determined by running\n> > \"git submodule [init|deinit|update]\".  Enforce that separation by having\n> > the SKIP_WORKTREE logic not touch submodules and allow submodules to\n> > continue using their own initialization state for determining if the\n> > submodule is present.\n>\n> Makes sense to me.\n>\n> I'm just thinking about the possible implications in grep (with\n> mt/grep-sparse-checkout). As you mentioned in [1], users might think\n> of \"git grep $rev $pat\" as an optimized version of \"git checkout $rev\n> && git grep $pat\". And, in this sense, they probably expect the output\n> of these two operations to be equal. But if we don't set the\n> SKIP_WORKTREE bit for submodules they might diverge.\n>\n> As an example, if we have a repository like:\n> .\n> |-- A\n> |   `-- sub\n> `-- B\n>\n> And the [cone-mode] sparsity rules:\n> /*\n> !/*/\n> /B/\n>\n> Then, \"git grep --recurse-submodules $rev $pat\" would search only in B\n> (as A doesn't match the sparsity patterns and thus, is not recursed\n> into). But \"git checkout $rev && git grep --recurse-submodules $pat\"\n> would search in both B and A/sub (as the latter would not have the\n> SKIP_WORKTREE bit set).\n>\n> This might be a problem for git-grep, not git-sparse-checkout. But I'm\n> not sure how we could solve it efficiently, as the submodule might be\n> deep down in a path whose first dir was already ignored for not\n> matching the sparsity patterns. Is this a problem we should consider,\n> or is it OK if the outputs of these two operations diverge?\n\nYou always have an eagle eye for catching these things.  Good point.\n\nSo, ignoring submodules for a second... In the case of unmerged files\nthat do not match the sparsity patterns, we previously felt it was\nfine that\n    git grep $revision $pattern\nwon't search through $revision:${currently_unmerged_file} despite the fact that\n    git grep $pattern\nwill search through ${currently_unmerged_file} from the working directory.\n\nIf we follow that analogy, then we're fine having those two \"almost\ncommand equivalents\" diverge.\n\nLet's dig a little further anyway and compare these \"almost command\nequivalents\" a bit more, which as a reminder are (1) \"git grep\n--recurse-submodules $rev $pat\" vs. (2) \"git checkout $rev && git grep\n--recurse-submodules $pat\".  (I just wanted to give them simple\nnumerical labels.)  In all cases, we'll be considering a $FILENAME\nwhich does not match sparsity patterns but is present in the working\ndirectory for reasons listed below:\n\nFirst case: $FILENAME has uncommitted changes:\n(Command 1) This will search only through files in $REV that match\nsparsity patterns; $REV:$FILENAME will be excluded\n(Command 2) If $FILENAME is different between HEAD and $REV this fails\nand shows nothing, even if other paths in the sparsity patterns had\nmatches.  Otherwise, it searches through $FILENAME.\n\nIf the command is modified to add a -m to the `git checkout` command\nto make the checkout not fail, then the second command will always\nsearch through $REV:$FILENAME, as modified by the dirty changes being\nmerged in.  If the user cleans up the file with uncommitted changes\nfirst, then the checkout would make the file go away and thus it\nwouldn't be searched after the switch.\n\nSecond case: $FILENAME has unmerged changes:\n(Command 1) This will search through files in $REV that match sparsity\npatterns; $REV:$FILENAME will be excluded.\n(Command 2) This will always fail and show nothing, regardless of any\nother matches.\n\nHere the checkout command always fails due to the unmerged entries and\nwon't even allow the user to switch with -m.\n\nSo, my comparison of commands #1 and #2 isn't quite right, even for\nnon-submodule cases.  But let's dig a little further.  If someone were\nto try do change their sparsity patterns or even just run a \"git\nsparse-checkout reapply\" when they had the above issues, they'd see\nsomething like:\n\n    $ git sparse-checkout reapply\n    warning: The following paths are unmerged and were left despite\nsparse patterns:\n            filename_with_conflicts\n\n    After fixing the above paths, you may want to run `git\nsparse-checkout reapply`.\n\nThis basically suggests that we consider uncommitted and unmerged\nfiles to be \"unclean\" in some way (sparse-checkout wants to set the\nSKIP_WORKTREE bit on all files that do not match the sparsity\nspecification, so \"clean\" means sparse-checkout is able to do so).  So\nI could amend my earlier comparison and say that IF the user has a\nclean directory, then \"git grep --recurse-submodules $REVISION\n$PATTERN\" should be equivalent to \"git checkout $REVISION && git grep\n--recurse-submodules $PATTERN\".  I could also say that given the big\nwarnings we give users when we can't set the SKIP_WORKTREE bit, that\nwe expect it to be a transient state and thus that we expect them to\nmore likely than not clear it out by the time they do switch branches.\nThat would lead us to the follow-up rule that if the user does not\nhave a clean directory then \"git grep --recurse-submodules $REVISION\n$PATTERN\" should be equivalent to what you would get if the unclean\nentries were ignored (expecting them to be cleaned before the any `git\ncheckout` could be run) and you then otherwise ran \"git checkout\n$REVISION && git grep --recurse-submodules $PATTERN\".\n\nThat suggests that grep's implementation we agreed on earlier is still\ncorrect (when given a $REVISION ignore submodulees that do not match\nthe sparsity patterns), but that unpack-trees/sparse-checkout still\nneed an update:\n\nWhen we notice an initialized submodule that does not match the\nsparsity patterns, we should print a warning just like we do for\nunmerged and dirty entries.\n\nSo my patch needs a bit more work.\n"},{"id":"400060","messageId":"CAHd-oW7tT7GhHz7mCH9Y66B=kriBKv8ZyrTXT-VTJgcXGa1Jzw@mail.gmail.com","threadId":"53696","inReplyTo":"CABPp-BHtwifTHXxoxTKvz0mx45e2N-4SBTTfoRePcmMFAn1O2g@mail.gmail.com","subject":"Re: [PATCH] unpack-trees: do not set SKIP_WORKTREE on submodules","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-18T14:34:11Z","receivedAt":"2020-06-18T14:34:30Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Jun 17, 2020 at 9:24 PM Elijah Newren <newren@gmail.com> wrote:\n>\n> If someone were\n> to try do change their sparsity patterns or even just run a \"git\n> sparse-checkout reapply\" when they had the above issues, they'd see\n> something like:\n>\n>     $ git sparse-checkout reapply\n>     warning: The following paths are unmerged and were left despite\n> sparse patterns:\n>             filename_with_conflicts\n>\n>     After fixing the above paths, you may want to run `git\n> sparse-checkout reapply`.\n>\n> This basically suggests that we consider uncommitted and unmerged\n> files to be \"unclean\" in some way (sparse-checkout wants to set the\n> SKIP_WORKTREE bit on all files that do not match the sparsity\n> specification, so \"clean\" means sparse-checkout is able to do so).  So\n> I could amend my earlier comparison and say that IF the user has a\n> clean directory, then \"git grep --recurse-submodules $REVISION\n> $PATTERN\" should be equivalent to \"git checkout $REVISION && git grep\n> --recurse-submodules $PATTERN\".  I could also say that given the big\n> warnings we give users when we can't set the SKIP_WORKTREE bit, that\n> we expect it to be a transient state and thus that we expect them to\n> more likely than not clear it out by the time they do switch branches.\n> That would lead us to the follow-up rule that if the user does not\n> have a clean directory then \"git grep --recurse-submodules $REVISION\n> $PATTERN\" should be equivalent to what you would get if the unclean\n> entries were ignored (expecting them to be cleaned before the any `git\n> checkout` could be run) and you then otherwise ran \"git checkout\n> $REVISION && git grep --recurse-submodules $PATTERN\".\n\nMakes sense, thanks! We haven't mentioned \"git grep --cached\" yet, but\nit would behave in the same way of the worktree grep, in this case.\n(I.e. searching the submodules, as their SKIP_WORTREE bit was not\nset.) So I guess it should be fine, as well.\n\n> That suggests that grep's implementation we agreed on earlier is still\n> correct (when given a $REVISION ignore submodulees that do not match\n> the sparsity patterns), but that unpack-trees/sparse-checkout still\n> need an update:\n>\n> When we notice an initialized submodule that does not match the\n> sparsity patterns, we should print a warning just like we do for\n> unmerged and dirty entries.\n\nYeah, seems like a good approach. Thanks for the explanations. Some of\nthe test cases in mt/grep-sparse-checkout will have to be adjusted\nwith this change. Should I reroll the series based on the patch you\nwill send or do you prefer to adjust them in your patch (and make it\ndependent on mt/grep-sparse-checkout)?\n\nThanks,\nMatheus\n"},{"id":"400074","messageId":"CABPp-BHV6iKH4PS6Gqfhe6piBdgmZ9D+cgXvSJ+dNcw-UHD-+w@mail.gmail.com","threadId":"53696","inReplyTo":"CAHd-oW7tT7GhHz7mCH9Y66B=kriBKv8ZyrTXT-VTJgcXGa1Jzw@mail.gmail.com","subject":"Re: [PATCH] unpack-trees: do not set SKIP_WORKTREE on submodules","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-06-18T19:19:49Z","receivedAt":"2020-06-18T19:20:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jun 18, 2020 at 7:34 AM Matheus Tavares Bernardino\n<matheus.bernardino@usp.br> wrote:\n>\n> On Wed, Jun 17, 2020 at 9:24 PM Elijah Newren <newren@gmail.com> wrote:\n> >\n> > If someone were\n> > to try do change their sparsity patterns or even just run a \"git\n> > sparse-checkout reapply\" when they had the above issues, they'd see\n> > something like:\n> >\n> >     $ git sparse-checkout reapply\n> >     warning: The following paths are unmerged and were left despite\n> > sparse patterns:\n> >             filename_with_conflicts\n> >\n> >     After fixing the above paths, you may want to run `git\n> > sparse-checkout reapply`.\n> >\n> > This basically suggests that we consider uncommitted and unmerged\n> > files to be \"unclean\" in some way (sparse-checkout wants to set the\n> > SKIP_WORKTREE bit on all files that do not match the sparsity\n> > specification, so \"clean\" means sparse-checkout is able to do so).  So\n> > I could amend my earlier comparison and say that IF the user has a\n> > clean directory, then \"git grep --recurse-submodules $REVISION\n> > $PATTERN\" should be equivalent to \"git checkout $REVISION && git grep\n> > --recurse-submodules $PATTERN\".  I could also say that given the big\n> > warnings we give users when we can't set the SKIP_WORKTREE bit, that\n> > we expect it to be a transient state and thus that we expect them to\n> > more likely than not clear it out by the time they do switch branches.\n> > That would lead us to the follow-up rule that if the user does not\n> > have a clean directory then \"git grep --recurse-submodules $REVISION\n> > $PATTERN\" should be equivalent to what you would get if the unclean\n> > entries were ignored (expecting them to be cleaned before the any `git\n> > checkout` could be run) and you then otherwise ran \"git checkout\n> > $REVISION && git grep --recurse-submodules $PATTERN\".\n>\n> Makes sense, thanks! We haven't mentioned \"git grep --cached\" yet, but\n> it would behave in the same way of the worktree grep, in this case.\n> (I.e. searching the submodules, as their SKIP_WORTREE bit was not\n> set.) So I guess it should be fine, as well.\n\n:-)\n\n> > That suggests that grep's implementation we agreed on earlier is still\n> > correct (when given a $REVISION ignore submodulees that do not match\n> > the sparsity patterns), but that unpack-trees/sparse-checkout still\n> > need an update:\n> >\n> > When we notice an initialized submodule that does not match the\n> > sparsity patterns, we should print a warning just like we do for\n> > unmerged and dirty entries.\n>\n> Yeah, seems like a good approach. Thanks for the explanations. Some of\n> the test cases in mt/grep-sparse-checkout will have to be adjusted\n> with this change. Should I reroll the series based on the patch you\n> will send or do you prefer to adjust them in your patch (and make it\n> dependent on mt/grep-sparse-checkout)?\n\nAh, good catch.  Your series came first, is longer, and is reviewed\nother than the submodule/config stuff that needs someone more familiar\nwith that area than me.  Since my patch needs more work anyway, how\nabout I rebase my patch on top of your work, and make sure to ping you\nas a reviewer to make sure I don't mess anything up?\n\nThanks,\nElijah\n"},{"id":"400078","messageId":"CAHd-oW7S1NKtwy=LdV95Nw-FPbLyUDV34BXLzd1D7Y6g29AtbQ@mail.gmail.com","threadId":"53696","inReplyTo":"CABPp-BHV6iKH4PS6Gqfhe6piBdgmZ9D+cgXvSJ+dNcw-UHD-+w@mail.gmail.com","subject":"Re: [PATCH] unpack-trees: do not set SKIP_WORKTREE on submodules","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-18T20:29:28Z","receivedAt":"2020-06-18T20:29:42Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Jun 18, 2020 at 4:20 PM Elijah Newren <newren@gmail.com> wrote:\n>\n> On Thu, Jun 18, 2020 at 7:34 AM Matheus Tavares Bernardino\n> <matheus.bernardino@usp.br> wrote:\n> >\n> > On Wed, Jun 17, 2020 at 9:24 PM Elijah Newren <newren@gmail.com> wrote:\n> > >\n> > > When we notice an initialized submodule that does not match the\n> > > sparsity patterns, we should print a warning just like we do for\n> > > unmerged and dirty entries.\n> >\n> > Yeah, seems like a good approach. Thanks for the explanations. Some of\n> > the test cases in mt/grep-sparse-checkout will have to be adjusted\n> > with this change. Should I reroll the series based on the patch you\n> > will send or do you prefer to adjust them in your patch (and make it\n> > dependent on mt/grep-sparse-checkout)?\n>\n> Ah, good catch.  Your series came first, is longer, and is reviewed\n> other than the submodule/config stuff that needs someone more familiar\n> with that area than me.  Since my patch needs more work anyway, how\n> about I rebase my patch on top of your work, and make sure to ping you\n> as a reviewer to make sure I don't mess anything up?\n\nSounds good :) Thanks\n"}]}