{"thread":{"id":"52719","subject":"Sparse checkout inconsistency with non-folder paths between cone mode and full matching (2.5.0)","startedAt":"2020-01-30T15:26:11Z","lastAt":"2020-01-31T19:59:34Z","messageCount":5,"participants":["Finn Bryant","Derrick Stolee","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"390812","messageId":"CADSBhNbbO=aq-Oo2MpzDMN2VAX4m6f9Jb-eCtVVX1NfWKE9zJw@mail.gmail.com","threadId":"52719","inReplyTo":null,"subject":"Sparse checkout inconsistency with non-folder paths between cone mode and full matching (2.5.0)","fromName":"Finn Bryant","fromEmail":"finnbryant@gmail.com","sentAt":"2020-01-30T15:25:56Z","receivedAt":"2020-01-30T15:26:11Z","isPatch":false,"sender":{"key":"finnbryant@gmail.com","avatar":null},"body":"Hi,\n\nWith cone mode enabled for a sparse checkout, a pattern like the\nfollowing is accepted:\n\n/*\n!/*/\n/a_file_or_folder/\n\nAs the name suggests, a_file_or_folder might be a file, or might be a\ndirectory. If it's a directory, then this is a \"valid\" recursive match\nfor the directory and everything works as expected.\nBut if the path leads to an ordinary file, it appears that cone mode\nwill *exclude* the file from the worktree (ie, it'll have the\nskip-worktree flag set).\nThis seems bizarre and unexpected behaviour.\n\nMy suspicion is that cone mode is supposed to be a pure subset of full\npattern matching, such that if cone mode is ever disabled, the\nsparseness of the worktree will be unchanged. Clearly, this scenario\nis breaking that pattern.\n\nI think the correct behaviour is that recursive matches for a\npotential directory do not have any effect on a non-directory file\nwith the same name. Alternatively, you could forbid any patterns which\nmatch non-directory files instead (and downgrade to full pattern\nmatching), though I'd not be a fan of this, since it'd mean the\nvalidity of a cone-mode sparse configuration file is dependent on the\ncontents of the repo, and is thus much harder to ascertain (scripts\ncan't simply prove if it's a valid cone mode file by parsing it,\nthey'd need to have access to any repo it may be applied to, and\ninspect the type of any matching file/folder paths, and its validity\ncould be changed simply by replacing a folder with a file in the\nrepo).\n\nIf matching behaviour with full pattern mode is a non-goal for cone\nmode, I'd still question the logic of this behaviour, though I suppose\nit does have the benefit of (accidentally?) adding support for\nexcluding individual files from a sparse checkout, which I imagine\nsome could find useful. Personally I'd prefer that was instead added\nwith a more sane syntax, if needed.\n\nFull test case:\n\n$ git init test_repo\nInitialized empty Git repository in [path]/test_repo/.git/\n$ cd test_repo/\n$ touch some_file a_file_or_folder\n$ git add some_file a_file_or_folder\n$ git commit -m \"some files\"\n[master (root-commit) 80d5918] some files\n 2 files changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 a_file_or_folder\n create mode 100644 some_file\n$ git sparse-checkout init --cone\n$ git read-tree -mu HEAD\n$ ls -1\na_file_or_folder\nsome_file\n$ git sparse-checkout set a_file_or_folder\n$ git read-tree -mu HEAD\n$ ls -1\nsome_file\n$ cat .git/info/sparse-checkout\n/*\n!/*/\n/a_file_or_folder/\n$\n\n\nRight now I'm trying to integrate cone mode with my company's existing\nmanifest infrastructure, which doesn't differentiate between files and\nfolders, so this is forcing me to perform a lot of repo checks to\nconfirm we aren't accidentally excluding files we were supposed to\ninclude. Just in case you needed another example of why this behaviour\nis unhelpful.\n"},{"id":"390826","messageId":"ef39f8c5-ce0b-a48b-940d-821df563b292@gmail.com","threadId":"52719","inReplyTo":"CADSBhNbbO=aq-Oo2MpzDMN2VAX4m6f9Jb-eCtVVX1NfWKE9zJw@mail.gmail.com","subject":"Re: Sparse checkout inconsistency with non-folder paths between cone mode and full matching (2.5.0)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-30T18:52:27Z","receivedAt":"2020-01-30T18:52:36Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/30/2020 10:25 AM, Finn Bryant wrote:\n> Hi,\n\nHello! Thanks for chiming in with feedback.\n\n> With cone mode enabled for a sparse checkout, a pattern like the\n> following is accepted:\n> \n> /*\n> !/*/\n> /a_file_or_folder/\n\nAs you mention below, you say this path is a file OR a directory.\nHowever, the trailing slash means _only_ match directories with this\nname. This is a specific choice made in creating cone mode to restrict\nonly by directories.\n\n> My suspicion is that cone mode is supposed to be a pure subset of full\n> pattern matching, such that if cone mode is ever disabled, the\n> sparseness of the worktree will be unchanged. Clearly, this scenario\n> is breaking that pattern.\n\nBy \"pure subset\" you must mean \"strict subset\". Cone mode is definitely\na smaller set of patterns.\n\n> I think the correct behaviour is that recursive matches for a\n> potential directory do not have any effect on a non-directory file\n> with the same name. Alternatively, you could forbid any patterns which\n> match non-directory files instead (and downgrade to full pattern\n> matching), though I'd not be a fan of this, since it'd mean the\n> validity of a cone-mode sparse configuration file is dependent on the\n> contents of the repo, and is thus much harder to ascertain (scripts\n> can't simply prove if it's a valid cone mode file by parsing it,\n> they'd need to have access to any repo it may be applied to, and\n> inspect the type of any matching file/folder paths, and its validity\n> could be changed simply by replacing a folder with a file in the\n> repo).\n> \n> If matching behaviour with full pattern mode is a non-goal for cone\n> mode, I'd still question the logic of this behaviour, though I suppose\n> it does have the benefit of (accidentally?) adding support for\n> excluding individual files from a sparse checkout, which I imagine\n> some could find useful. Personally I'd prefer that was instead added\n> with a more sane syntax, if needed.\n\nCone mode is specifically designed for performance using a hashset-based\npattern matching. This naturally restricts the possible patterns since\nwe cannot match the full pattern set [1] this quickly. This means there\nare some trade-offs that are required, but they were made with some\ninformation of real-world repositories organized in a way that they\ncould take advantage of this pattern set.\n\n[1] https://git-scm.com/docs/gitignore#_pattern_format\n \n> $ git sparse-checkout init --cone\n> $ git read-tree -mu HEAD\n> $ ls -1\n> a_file_or_folder\n> some_file\n> $ git sparse-checkout set a_file_or_folder\n> $ git read-tree -mu HEAD\n> $ ls -1\n> some_file\n> $ cat .git/info/sparse-checkout\n> /*\n> !/*/\n> /a_file_or_folder/\n\nThis is an interesting test, because I would expect the /a_file_or_folder/\npattern to not cause the _file_ to not match. It does match the first two\npatterns, and just because it doesn't match the third pattern shouldn't\nremove it.\n\nThis is actually a bug in the hashset-based pattern-matching algorithm,\nsince setting core.sparseCheckoutCone=false does not have this behavior.\nI'll make a patch to fix this.\n\n> Right now I'm trying to integrate cone mode with my company's existing\n> manifest infrastructure, which doesn't differentiate between files and\n> folders, so this is forcing me to perform a lot of repo checks to\n> confirm we aren't accidentally excluding files we were supposed to\n> include. Just in case you needed another example of why this behaviour\n> is unhelpful.\n\nI apologize that this wrong behavior is causing you some issues. If you\nare able to identify which paths are files instead of directories, then\nyou can remove the filename from the path to include its parent directory.\nThis is only a workaround until the bug is fixed, but the end-result will\nbe the same: you'll have all sibling files in your working directory as\nwell as the files you specified.\n\nThis is important: if you have a directory with many large files, but\nintend to include only a subset of those files, then you will be\ndisappointed by the size of your working directory.\n\nThis requirement of specifying directories instead of files is part\nof the cone mode design for two reasons: one philosophical and another\nmore practical:\n\nPhilisophical: Filenames change more often than directories.\n\nThat is, as users are interacting with your repo, the overall directory\nstructure at a high level is typically static. If new cross-component\ndependencies are introduced, then those are usually big architecture-level\nchanges. However, at an individual file level dependencies change at\na higher rate, causing users to react by changing their sparse-checkout\ndefinition more frequently.\n\nPractical: How do we specify if an input is a directory or a file?\n\nAs you mentioned, if you run \"git sparse-checkout set X\" then you get\na sparse-checkout file containing:\n\n\t/*\n\t!/*/\n\t/X/\n\nBut if X is actually a _file_, then we should write this pattern:\n\n\t/*\n\t!/*/\n\t/X\n\nSo the input does not provide enough information to say which pattern\nshould be written.\n\nThe other concern about this second set of patterns is that \"/X\" is\nonly a _prefix_ match, not an exact path name match. That means we\ncannot use the existing hashset matching to result in the same\npattern matching as non-cone-mode.\n\nNow, there is perhaps a way to decide between the two: inspect the\ncurrent index or tree for whether the input corresponds to a directory\nor a file. This adds a layer of complexity that is not currently in\nthe sparse-checkout builtin, but it is not insurmountable. The only\nthing to consider is what happens when the input path is not in the\nindex/tree at all! It may be that we want to specify the patterns\nbefore moving HEAD to a commit that _does_ contain the path, and then\nwe would do the wrong thing at this point.\n\nSo I'll put out a question to the community: is this practical issue\nsomething worth overcoming? Is my concern about a non-existent path\na true problem, or something more?\n\nThanks,\n-Stolee\n\n\n"},{"id":"390830","messageId":"967f96e0-d26b-1ec0-f39f-617d95615d6f@gmail.com","threadId":"52719","inReplyTo":"ef39f8c5-ce0b-a48b-940d-821df563b292@gmail.com","subject":"Re: Sparse checkout inconsistency with non-folder paths between cone mode and full matching (2.5.0)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-30T19:22:49Z","receivedAt":"2020-01-30T19:22:54Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/30/2020 1:52 PM, Derrick Stolee wrote:\n> On 1/30/2020 10:25 AM, Finn Bryant wrote:\n>> $ git sparse-checkout init --cone\n>> $ git read-tree -mu HEAD\n>> $ ls -1\n>> a_file_or_folder\n>> some_file\n>> $ git sparse-checkout set a_file_or_folder\n>> $ git read-tree -mu HEAD\n>> $ ls -1\n>> some_file\n>> $ cat .git/info/sparse-checkout\n>> /*\n>> !/*/\n>> /a_file_or_folder/\n> \n> This is an interesting test, because I would expect the /a_file_or_folder/\n> pattern to not cause the _file_ to not match. It does match the first two\n> patterns, and just because it doesn't match the third pattern shouldn't\n> remove it.\n> \n> This is actually a bug in the hashset-based pattern-matching algorithm,\n> since setting core.sparseCheckoutCone=false does not have this behavior.\n> I'll make a patch to fix this.\n\nAnd here is a patch. I'll add this on top of my \"Harden the sparse-checkout\nbuiltin\" series in its v4.\n\n-->8--\n\nFrom bf2115670e8ef79844ec7e87f3ba99d1f776a44d Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee <dstolee@microsoft.com>\nDate: Thu, 30 Jan 2020 19:11:26 +0000\nSubject: [PATCH] sparse-checkout: fix cone mode behavior mismatch\n\nThe intention of the special \"cone mode\" in the sparse-checkout feature\nis to always match the same patterns that are matched by the same\nsparse-checkout file as when cone mode is disabled.\n\nWhen a file path is given to \"git sparse-checkout set\" in cone mode,\nthen the cone mode improperly matches the file as a recursive path.\nWhen setting the skip-worktree bits, files were not expecting the\nMATCHED_RECURSIVE response, and hence these were left out of the\nmatched cone.\n\nFix this bug by checking for MATCHED_RECURSIVE in addition to MATCHED\nand add a test that prevents regression.\n\nReported-by: Finn Bryant <finnbryant@gmail.com>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t1091-sparse-checkout-builtin.sh | 12 ++++++++++++\n unpack-trees.c                     |  2 +-\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 37e9304ef3..7d982096fb 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -305,6 +305,18 @@ test_expect_success 'different sparse-checkouts with worktrees' '\n \tcheck_files worktree a deep\n '\n \n+test_expect_success 'set using filename keeps file on-disk' '\n+\tgit -C repo sparse-checkout set a deep &&\n+\tcat >expect <<-\\EOF &&\n+\t/*\n+\t!/*/\n+\t/a/\n+\t/deep/\n+\tEOF\n+\ttest_cmp expect repo/.git/info/sparse-checkout &&\n+\tcheck_files repo a deep\n+'\n+\n check_read_tree_errors () {\n \tREPO=$1\n \tFILES=$2\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 3789a22cf0..78425ce74b 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1416,7 +1416,7 @@ static int clear_ce_flags_1(struct index_state *istate,\n \t\t\t\t\t\tname, &dtype, pl, istate);\n \t\tif (ret == UNDECIDED)\n \t\t\tret = default_match;\n-\t\tif (ret == MATCHED)\n+\t\tif (ret == MATCHED || ret == MATCHED_RECURSIVE)\n \t\t\tce->ce_flags &= ~clear_mask;\n \t\tcache++;\n \t\tprogress_nr++;\n-- \n2.25.0.vfs.1.1\n\n\n\n"},{"id":"390909","messageId":"CADSBhNZ7g_eqUSRczGJPkoiG57OL4DXray3_kTTtj-DF4=aM1g@mail.gmail.com","threadId":"52719","inReplyTo":"ef39f8c5-ce0b-a48b-940d-821df563b292@gmail.com","subject":"Re: Sparse checkout inconsistency with non-folder paths between cone mode and full matching (2.5.0)","fromName":"Finn Bryant","fromEmail":"finnbryant@gmail.com","sentAt":"2020-01-31T15:13:23Z","receivedAt":"2020-01-31T15:13:39Z","isPatch":false,"sender":{"key":"finnbryant@gmail.com","avatar":null},"body":"(resent, I forgot to reply all)\n\nOn Thu, 30 Jan 2020 at 18:52, Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 1/30/2020 10:25 AM, Finn Bryant wrote:\n> > Hi,\n>\n> Hello! Thanks for chiming in with feedback.\n>\n> > With cone mode enabled for a sparse checkout, a pattern like the\n> > following is accepted:\n> >\n> > /*\n> > !/*/\n> > /a_file_or_folder/\n>\n> As you mention below, you say this path is a file OR a directory.\n> However, the trailing slash means _only_ match directories with this\n> name. This is a specific choice made in creating cone mode to restrict\n> only by directories.\n\nRight, I thought it was supposed to work that way, but the bug you've\npatched left me uncertain.\n\n> > If matching behaviour with full pattern mode is a non-goal for cone\n> > mode, I'd still question the logic of this behaviour, though I suppose\n> > it does have the benefit of (accidentally?) adding support for\n> > excluding individual files from a sparse checkout, which I imagine\n> > some could find useful. Personally I'd prefer that was instead added\n> > with a more sane syntax, if needed.\n>\n> Cone mode is specifically designed for performance using a hashset-based\n> pattern matching. This naturally restricts the possible patterns since\n> we cannot match the full pattern set [1] this quickly. This means there\n> are some trade-offs that are required, but they were made with some\n> information of real-world repositories organized in a way that they\n> could take advantage of this pattern set.\n\nAbsolutely, that makes sense to me, and I think this fits the model\nI'm working with well, or will once your patch filters through.\n\n> > $ git sparse-checkout init --cone\n> > $ git read-tree -mu HEAD\n> > $ ls -1\n> > a_file_or_folder\n> > some_file\n> > $ git sparse-checkout set a_file_or_folder\n> > $ git read-tree -mu HEAD\n> > $ ls -1\n> > some_file\n> > $ cat .git/info/sparse-checkout\n> > /*\n> > !/*/\n> > /a_file_or_folder/\n>\n> This is an interesting test, because I would expect the /a_file_or_folder/\n> pattern to not cause the _file_ to not match. It does match the first two\n> patterns, and just because it doesn't match the third pattern shouldn't\n> remove it.\n>\n> This is actually a bug in the hashset-based pattern-matching algorithm,\n> since setting core.sparseCheckoutCone=false does not have this behavior.\n> I'll make a patch to fix this.\n\nI was hoping you'd say that!\n\n> > Right now I'm trying to integrate cone mode with my company's existing\n> > manifest infrastructure, which doesn't differentiate between files and\n> > folders, so this is forcing me to perform a lot of repo checks to\n> > confirm we aren't accidentally excluding files we were supposed to\n> > include. Just in case you needed another example of why this behaviour\n> > is unhelpful.\n>\n> I apologize that this wrong behavior is causing you some issues. If you\n> are able to identify which paths are files instead of directories, then\n> you can remove the filename from the path to include its parent directory.\n> This is only a workaround until the bug is fixed, but the end-result will\n> be the same: you'll have all sibling files in your working directory as\n> well as the files you specified.\n\nNo need for apologies, I just wanted to convey why I thought the bug\nwas worth attention.\nI realise this is a brand new feature and appreciate the impressively\nfast feedback!\nI am loving the cone mode improvements by the way, I've been living\nwith the poor performance from full matching for a long time now, and\nthis is a huge improvement.\n\n> This is important: if you have a directory with many large files, but\n> intend to include only a subset of those files, then you will be\n> disappointed by the size of your working directory.\n\nUnderstood, this thankfully isn't a huge issue for my use-case, our\nfolder structures are mostly not too shallow, so there's only a few\ncases where the lack of file matching hurts us, and the benefits far\noutweigh the costs.\n\n> This requirement of specifying directories instead of files is part\n> of the cone mode design for two reasons: one philosophical and another\n> more practical:\n>\n> Philisophical: Filenames change more often than directories.\n>\n> That is, as users are interacting with your repo, the overall directory\n> structure at a high level is typically static. If new cross-component\n> dependencies are introduced, then those are usually big architecture-level\n> changes. However, at an individual file level dependencies change at\n> a higher rate, causing users to react by changing their sparse-checkout\n> definition more frequently.\n\nIt's not too much of a concern for me, but the few cases where it has\nan impact for me tend to follow the pattern of:\nproject A needs file X, but X lives in a folder with 1000 other files\nthat aren't needed by project A.\n\n> Practical: How do we specify if an input is a directory or a file?\n>\n> As you mentioned, if you run \"git sparse-checkout set X\" then you get\n> a sparse-checkout file containing:\n>\n>         /*\n>         !/*/\n>         /X/\n>\n> But if X is actually a _file_, then we should write this pattern:\n>\n>         /*\n>         !/*/\n>         /X\n>\n> So the input does not provide enough information to say which pattern\n> should be written.\n>\n> The other concern about this second set of patterns is that \"/X\" is\n> only a _prefix_ match, not an exact path name match. That means we\n> cannot use the existing hashset matching to result in the same\n> pattern matching as non-cone-mode.\n\nThis is an interesting point, currently (at least in 2.25) both `/X/`\nand `/X` formats are allowed, and appear to be treated the same way.\n\nGiven `/X` is only a prefix match, perhaps it should be invalid for\ncone mode, and force the full matching fallback?\n\nThis wouldn't have any effect on the matching capabilities of cone\nmode, I think, but it would reduce confusion and ensure cone mode and\nfull matching have as close to the same results as possible.\nAdditionally, by preventing it now, you can effectively reserve that\nsyntax in case it proves useful for later extensions to cone mode.\n\n> Now, there is perhaps a way to decide between the two: inspect the\n> current index or tree for whether the input corresponds to a directory\n> or a file. This adds a layer of complexity that is not currently in\n> the sparse-checkout builtin, but it is not insurmountable. The only\n> thing to consider is what happens when the input path is not in the\n> index/tree at all! It may be that we want to specify the patterns\n> before moving HEAD to a commit that _does_ contain the path, and then\n> we would do the wrong thing at this point.\n\nAgreed, this is a nasty problem.\n\n> So I'll put out a question to the community: is this practical issue\n> something worth overcoming? Is my concern about a non-existent path\n> a true problem, or something more?\n\nPersonally I think it's a reasonable compromise. Many will benefit\nwith only folder matching support, and the cases where file matching\nis needed (or at least a significant benefit) are going to be quite a\nbit less common.\n\n> Thanks,\n> -Stolee\n\nThank you for the quick response and the patch!\n\nFinn\n"},{"id":"390938","messageId":"CABPp-BHNxGFFgMMABto0y0LA6xxqqDEcVGw4FSN=1JdH53+V9Q@mail.gmail.com","threadId":"52719","inReplyTo":"ef39f8c5-ce0b-a48b-940d-821df563b292@gmail.com","subject":"Re: Sparse checkout inconsistency with non-folder paths between cone mode and full matching (2.5.0)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-31T19:59:20Z","receivedAt":"2020-01-31T19:59:34Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jan 30, 2020 at 10:52 AM Derrick Stolee <stolee@gmail.com> wrote:\n> Practical: How do we specify if an input is a directory or a file?\n>\n> As you mentioned, if you run \"git sparse-checkout set X\" then you get\n> a sparse-checkout file containing:\n>\n>         /*\n>         !/*/\n>         /X/\n>\n> But if X is actually a _file_, then we should write this pattern:\n>\n>         /*\n>         !/*/\n>         /X\n>\n> So the input does not provide enough information to say which pattern\n> should be written.\n>\n> The other concern about this second set of patterns is that \"/X\" is\n> only a _prefix_ match, not an exact path name match. That means we\n> cannot use the existing hashset matching to result in the same\n> pattern matching as non-cone-mode.\n>\n> Now, there is perhaps a way to decide between the two: inspect the\n> current index or tree for whether the input corresponds to a directory\n> or a file. This adds a layer of complexity that is not currently in\n> the sparse-checkout builtin, but it is not insurmountable. The only\n> thing to consider is what happens when the input path is not in the\n> index/tree at all! It may be that we want to specify the patterns\n> before moving HEAD to a commit that _does_ contain the path, and then\n> we would do the wrong thing at this point.\n\nAnd once people ran into the difference because their repo was weird,\nthey'd be bewildered.  I don't want to deal with those support\nquestions; let's just disallow the latter form and require the\ntrailing slash.  There are three alternate universes we could have\nsupported such an ability under:\n\n   * If we had decided to use regexes instead of fnmatch for\nsparse-checkout/gitignore then we could have a /<path>$ pattern to\nmatch just files\n  * If fnmatch had an end-of-input marker like regex's '$', then we\ncould use it to mark exact files and be able to translate that into\nhashes.\n  * If we had decided that cone mode uses an input file other than\n.git/info/sparse-checkout with a different format, we wouldn't have to\nworry about strict compatibility of the format with non-cone mode and\nsupport different types of abilities.\n\nBut I think it's too late now.  Let's just leave the trailing slash\nthere and say we only support directory-based matching in cone mode.\n\n> So I'll put out a question to the community: is this practical issue\n> something worth overcoming? Is my concern about a non-existent path\n> a true problem, or something more?\n>\n> Thanks,\n> -Stolee\n"}]}