{"thread":{"id":"53603","subject":"Possible regression for sparse-checkout in git 2.27.0","startedAt":"2020-06-03T01:09:06Z","lastAt":"2020-06-05T13:07:57Z","messageCount":8,"participants":["Shaun Case","Elijah Newren","Derrick Stolee","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"399057","messageId":"CAD3+_6CUX0RPr-dgfUnfGDNNfqu80SYCskioYnu=MS6aJv2dEQ@mail.gmail.com","threadId":"53603","inReplyTo":null,"subject":"Possible regression for sparse-checkout in git 2.27.0","fromName":"Shaun Case","fromEmail":"warmsocks@gmail.com","sentAt":"2020-06-03T01:08:52Z","receivedAt":"2020-06-03T01:09:06Z","isPatch":false,"sender":{"key":"warmsocks@gmail.com","avatar":null},"body":"I'm seeing an unexpected change in behavior of git sparse-checkout\nbetween 2.26.2 and 2.27.0 on CentOS 7.8.  Problem manifests with both\ngithub and multiple recent versions of gitlab.\n\nSources came from\nhttps://mirrors.edge.kernel.org/pub/software/scm/git/, I verified the\nsha256 hashes for both 2.26.2 and 2.27.0 matched.\n\nBuilt with\ngcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3)\n\nBy doing:\n\n./configure \\\n    --prefix=/usr/local \\\n    --with-python=python3\nmake -j 10\nsudo make -j 10 install install-doc install-html\n\nProblem detail:\n\nIn 2.26.2, git sparse-checkout commands...\n\n  init --cone\n  set\n  add\n\n... all bring in files.  In 2.27.0, no files appear except the .git subdir\nuntil I do:\n\ngit read-tree -mu HEAD\n\nHowever, according to my reading of this, git read-tree should not be\nnecessary in 2.27.0:\nhttps://stackoverflow.com/questions/28760940/why-does-one-call-git-read-tree-after-a-sparse-checkout/\n\nWhen I do export GIT_TRACE_PACKET=1, I see that the init --cone, set\nand add sparse-checkout commands all talk to the server.  In 2.27.0,\nthese commands do not result in any packets sent to the server.  It\nsounds like they should be calling update_sparsity() themselves, but\naren't.  Is that wrong?  I couldn't find a git CLI client command to\ninvoke it directly except for read-tree.\n\nBelow is a small bash script and its output that shows the problem\nwith a small random public repo on github.\n\nBest regards,\nShaun\n\nScript:\n==============================================\n#!/bin/bash -x\n\nexport GIT_TRACE_PACKET=0\ngit --version\ngit clone --filter=blob:none --no-checkout\nhttps://github.com/r-spacex/launch-timeline.git\ncd launch-timeline\n\n# Note no server traffic at from here...\nexport GIT_TRACE_PACKET=1\ngit sparse-checkout init --cone\ngit sparse-checkout set README.md\ngit sparse-checkout add css\n# ... to here.\n\n# There is nothing here yet except .git/\nls -las\n\n# This brings in the files as expected, uncomment to verify.\n# git read-tree -mu HEAD\n==============================================\n\n\nOutput:\n==============================================\n3sc3396:/tmp>demo-git-sparse-checkout-bug.sh\n+ export GIT_TRACE_PACKET=0\n+ GIT_TRACE_PACKET=0\n+ git --version\ngit version 2.27.0\n+ git clone --filter=blob:none --no-checkout\nhttps://github.com/r-spacex/launch-timeline.git\nCloning into 'launch-timeline'...\nremote: Enumerating objects: 529, done.\nremote: Total 529 (delta 0), reused 0 (delta 0), pack-reused 529\nReceiving objects: 100% (529/529), 67.49 KiB | 987.00 KiB/s, done.\nResolving deltas: 100% (168/168), done.\n+ cd launch-timeline\n+ export GIT_TRACE_PACKET=1\n+ GIT_TRACE_PACKET=1\n+ git sparse-checkout init --cone\n+ git sparse-checkout set README.md\n+ git sparse-checkout add css\n+ ls -las\ntotal 68\n 4 drwxrwxr-x   3 scase scase  4096 Jun  2 17:46 .\n60 drwxrwxrwt. 34 root  root  57344 Jun  2 17:46 ..\n 4 drwxrwxr-x   8 scase scase  4096 Jun  2 17:46 .git\n==============================================\n"},{"id":"399066","messageId":"CABPp-BGvc3GZfFuiXaqDk6391ZexQ7D3x9gr5JK6-L+rDQQ4sg@mail.gmail.com","threadId":"53603","inReplyTo":"CAD3+_6CUX0RPr-dgfUnfGDNNfqu80SYCskioYnu=MS6aJv2dEQ@mail.gmail.com","subject":"Re: Possible regression for sparse-checkout in git 2.27.0","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-06-03T04:37:05Z","receivedAt":"2020-06-03T04:37:20Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Tue, Jun 2, 2020 at 6:12 PM Shaun Case <warmsocks@gmail.com> wrote:\n>\n> I'm seeing an unexpected change in behavior of git sparse-checkout\n> between 2.26.2 and 2.27.0 on CentOS 7.8.  Problem manifests with both\n> github and multiple recent versions of gitlab.\n>\n> Sources came from\n> https://mirrors.edge.kernel.org/pub/software/scm/git/, I verified the\n> sha256 hashes for both 2.26.2 and 2.27.0 matched.\n>\n> Built with\n> gcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3)\n>\n> By doing:\n>\n> ./configure \\\n>     --prefix=/usr/local \\\n>     --with-python=python3\n> make -j 10\n> sudo make -j 10 install install-doc install-html\n>\n> Problem detail:\n>\n> In 2.26.2, git sparse-checkout commands...\n>\n>   init --cone\n>   set\n>   add\n>\n> ... all bring in files.  In 2.27.0, no files appear except the .git subdir\n> until I do:\n>\n> git read-tree -mu HEAD\n>\n> However, according to my reading of this, git read-tree should not be\n> necessary in 2.27.0:\n> https://stackoverflow.com/questions/28760940/why-does-one-call-git-read-tree-after-a-sparse-checkout/\n>\n> When I do export GIT_TRACE_PACKET=1, I see that the init --cone, set\n> and add sparse-checkout commands all talk to the server.  In 2.27.0,\n> these commands do not result in any packets sent to the server.  It\n> sounds like they should be calling update_sparsity() themselves, but\n> aren't.  Is that wrong?  I couldn't find a git CLI client command to\n> invoke it directly except for read-tree.\n>\n> Below is a small bash script and its output that shows the problem\n> with a small random public repo on github.\n>\n> Best regards,\n> Shaun\n>\n> Script:\n> ==============================================\n> #!/bin/bash -x\n>\n> export GIT_TRACE_PACKET=0\n> git --version\n> git clone --filter=blob:none --no-checkout\n> https://github.com/r-spacex/launch-timeline.git\n> cd launch-timeline\n>\n> # Note no server traffic at from here...\n> export GIT_TRACE_PACKET=1\n> git sparse-checkout init --cone\n> git sparse-checkout set README.md\n> git sparse-checkout add css\n> # ... to here.\n>\n> # There is nothing here yet except .git/\n> ls -las\n>\n> # This brings in the files as expected, uncomment to verify.\n> # git read-tree -mu HEAD\n> ==============================================\n>\n>\n> Output:\n> ==============================================\n> 3sc3396:/tmp>demo-git-sparse-checkout-bug.sh\n> + export GIT_TRACE_PACKET=0\n> + GIT_TRACE_PACKET=0\n> + git --version\n> git version 2.27.0\n> + git clone --filter=blob:none --no-checkout\n> https://github.com/r-spacex/launch-timeline.git\n> Cloning into 'launch-timeline'...\n> remote: Enumerating objects: 529, done.\n> remote: Total 529 (delta 0), reused 0 (delta 0), pack-reused 529\n> Receiving objects: 100% (529/529), 67.49 KiB | 987.00 KiB/s, done.\n> Resolving deltas: 100% (168/168), done.\n> + cd launch-timeline\n> + export GIT_TRACE_PACKET=1\n> + GIT_TRACE_PACKET=1\n> + git sparse-checkout init --cone\n> + git sparse-checkout set README.md\n> + git sparse-checkout add css\n> + ls -las\n> total 68\n>  4 drwxrwxr-x   3 scase scase  4096 Jun  2 17:46 .\n> 60 drwxrwxrwt. 34 root  root  57344 Jun  2 17:46 ..\n>  4 drwxrwxr-x   8 scase scase  4096 Jun  2 17:46 .git\n> ==============================================\n\nThanks for the detailed report.  This change came from commit\nf56f31af03 (\"sparse-checkout: use new update_sparsity() function\",\n2020-03-27), made by me.  After playing with it for a bit, the\n--filter=blob:none turns out to not be relevant to reproducing the\nbehavior of not getting any files in the working tree, other than\nperhaps providing you a way to measure what was or wasn't happening.\n\nI think it'd be more natural to run\n  git clone --filter=blob:none --sparse\nhttps://github.com/r-spacex/launch-timeline.git\nin place of the combination of\n  git clone --filter=blob:none --no-checkout\nhttps://github.com/r-spacex/launch-timeline.git\n  git sparse-checkout init --cone\nsince the --sparse flag was added just for this kind of case -- to do\na clone but start with only a few things checked out.  It's easier, is\nthe route we're moving towards, and as a bonus also happens to work.\n;-)\n\nA bit of a side note, or a few of them, but this command of yours is broken:\n  git sparse-checkout set README.md\nbecause --cone mode means you are specifying *directories* that should\nbe checked out.  Currently, this gives no error, it instead silently\ndrops you back to non-cone mode, which seems bad to me.\nsparse-checkout should provide some kind of error -- or at very least\na warning -- when you make that mistake.\n\nNow let's talk about the commit in question that changed behavior\nhere.  The point of sparse-checkout is never to switch branches or\ncheckout new commits; all it does is update which paths are in the\ncurrent working directory.  A related point to this is it should never\nadd or remove entries from the index and shouldn't change any hashes\nof files in the index.  It used to violate this, at first via an\nimplementation that was literally invoking `git read-tree -mu HEAD` in\na subprocess, and then later using internal code equivalent to\ninvoking that command in a subprocess.  But by violating the\nleave-index-entries-alone mandate, it left folks who were in the\nmiddle of a rebase and wanted to update their sparse-checkout to\ninclude some more directories in their working tree in a precarious\nspot -- if they didn't update, then they didn't have the files\nnecessary to build, and if they did forcibly update via `git read-tree\n-mu HEAD` then their staged changes would all get wiped out.  I spent\nsome quality time helping users recover their files and teaching them\nabout the git storage model.\n\nSo that brings us back to your original question.  When you said\n--no-checkout, it means that there is no commit checked out and the\nindex is empty.  update_sparsity() is correctly toggling the\nSKIP_WORKTREE bits for the existing index entries that don't match the\nsparsity patterns, and it is correctly calling check_updates().\ncheck_updates() is correctly checking for files currently in the index\nwhich have toggled to being needed in the current worktree so that it\ncan issue downloads related to promisor packs.  The problem is just\nthat there aren't any index entries to begin with, so there are no\nSKIP_WORKTREE bits to update, and thus no files that need to be\ndownloaded.\n\nIt seems a bit risky to make sparse-checkout start doing\ncheckout/switch behavior and adding entries to the index.  There's a\ncouple ways forward.  One, we could decide this is a special edge or\ncorner case where we allow it: if the index is completely empty, then\nthere's no data to lose and thus we could make `git sparse-checkout\ninit [--cone]` in that one case use the old 'read-tree -mu HEAD'\nlogic.  Alternatively, we could just require users to run 'git reset\n--hard' at the end of your script.\n\nStolee: Thoughts?\n"},{"id":"399078","messageId":"c1f9d76a-bf39-8508-1f4f-b34be77450a0@gmail.com","threadId":"53603","inReplyTo":"CABPp-BGvc3GZfFuiXaqDk6391ZexQ7D3x9gr5JK6-L+rDQQ4sg@mail.gmail.com","subject":"Re: Possible regression for sparse-checkout in git 2.27.0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-03T15:36:09Z","receivedAt":"2020-06-03T15:36:13Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/3/2020 12:37 AM, Elijah Newren wrote:\n> I think it'd be more natural to run\n\n>   git clone --filter=blob:none --sparse\n> https://github.com/r-spacex/launch-timeline.git\n\n> in place of the combination of\n\n>   git clone --filter=blob:none --no-checkout\n> https://github.com/r-spacex/launch-timeline.git\n>   git sparse-checkout init --cone\n\n> since the --sparse flag was added just for this kind of case -- to do\n> a clone but start with only a few things checked out.  It's easier, is\n> the route we're moving towards, and as a bonus also happens to work.\n\nJust one warning: the --sparse option in \"git clone\" does not currently\nenable core.sparseCheckoutCone, so running \"git sparse-checkout init --cone\"\nafterwards is a good idea, or else your \"git sparse-checkout (set|add)\"\ncommands will not behave the way you expect.\n\n(I think that I will propose a change in behavior to make it do so during\nthis release cycle.)\n\n> A bit of a side note, or a few of them, but this command of yours is broken:\n>   git sparse-checkout set README.md\n> because --cone mode means you are specifying *directories* that should\n> be checked out.  Currently, this gives no error, it instead silently\n> drops you back to non-cone mode, which seems bad to me.\n> sparse-checkout should provide some kind of error -- or at very least\n> a warning -- when you make that mistake.\n \n> Now let's talk about the commit in question that changed behavior\n> here.  The point of sparse-checkout is never to switch branches or\n> checkout new commits; all it does is update which paths are in the\n> current working directory.  A related point to this is it should never\n> add or remove entries from the index and shouldn't change any hashes\n> of files in the index.  It used to violate this, at first via an\n> implementation that was literally invoking `git read-tree -mu HEAD` in\n> a subprocess, and then later using internal code equivalent to\n> invoking that command in a subprocess.  But by violating the\n> leave-index-entries-alone mandate, it left folks who were in the\n> middle of a rebase and wanted to update their sparse-checkout to\n> include some more directories in their working tree in a precarious\n> spot -- if they didn't update, then they didn't have the files\n> necessary to build, and if they did forcibly update via `git read-tree\n> -mu HEAD` then their staged changes would all get wiped out.  I spent\n> some quality time helping users recover their files and teaching them\n> about the git storage model.\n> \n> So that brings us back to your original question.  When you said\n> --no-checkout, it means that there is no commit checked out and the\n> index is empty.  update_sparsity() is correctly toggling the\n> SKIP_WORKTREE bits for the existing index entries that don't match the\n> sparsity patterns, and it is correctly calling check_updates().\n> check_updates() is correctly checking for files currently in the index\n> which have toggled to being needed in the current worktree so that it\n> can issue downloads related to promisor packs.  The problem is just\n> that there aren't any index entries to begin with, so there are no\n> SKIP_WORKTREE bits to update, and thus no files that need to be\n> downloaded.\n> \n> It seems a bit risky to make sparse-checkout start doing\n> checkout/switch behavior and adding entries to the index.  There's a\n> couple ways forward.  One, we could decide this is a special edge or\n> corner case where we allow it: if the index is completely empty, then\n> there's no data to lose and thus we could make `git sparse-checkout\n> init [--cone]` in that one case use the old 'read-tree -mu HEAD'\n> logic.  Alternatively, we could just require users to run 'git reset\n> --hard' at the end of your script.\n> \n> Stolee: Thoughts?\n\nI agree that using \"--sparse\" instead of \"--no-checkout\" is the\nbest way forward for now, but I'll classify that as a workaround\nand not necessarily the end of the conversation.\n\nIn general, the commit in question is doing something extremely\nvaluable for common situations, like rebase that you mention.\nI also think that this change in behavior is warranted by the\nclear warning placed at the top of the docs [1]:\n\n\tTHIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE\n\tBEHAVIOR OF OTHER COMMANDS IN THE PRESENCE OF SPARSE-\n\tCHECKOUTS, WILL LIKELY CHANGE IN THE FUTURE.\n\n[1] https://git-scm.com/docs/git-sparse-checkout#_description\n\nOf course, that's just us covering our butts as we push the\nfeature forward. It doesn't stop users (especially those that\nare bravely using the feature) from feeling pain that might\nbe avoidable.\n\nTo wrap up, it's unfortunate that the behavior changed. If it\nis easy to detect and handle this special case, then it _may_\nbe worth doing to avoid disrupting users. Elijah: could you\nspend at most an hour trying to see how difficult this would be?\n\nThanks,\n-Stolee\n"},{"id":"399099","messageId":"CABPp-BHyDUrOLg6-VONewbmXNFHUvKoDXfRTLgg8aEhJFtJLuQ@mail.gmail.com","threadId":"53603","inReplyTo":"c1f9d76a-bf39-8508-1f4f-b34be77450a0@gmail.com","subject":"Re: Possible regression for sparse-checkout in git 2.27.0","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-06-04T07:27:15Z","receivedAt":"2020-06-04T07:27:29Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Wed, Jun 3, 2020 at 8:36 AM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 6/3/2020 12:37 AM, Elijah Newren wrote:\n> > I think it'd be more natural to run\n>\n> >   git clone --filter=blob:none --sparse\n> > https://github.com/r-spacex/launch-timeline.git\n>\n> > in place of the combination of\n>\n> >   git clone --filter=blob:none --no-checkout\n> > https://github.com/r-spacex/launch-timeline.git\n> >   git sparse-checkout init --cone\n>\n> > since the --sparse flag was added just for this kind of case -- to do\n> > a clone but start with only a few things checked out.  It's easier, is\n> > the route we're moving towards, and as a bonus also happens to work.\n>\n> Just one warning: the --sparse option in \"git clone\" does not currently\n> enable core.sparseCheckoutCone, so running \"git sparse-checkout init --cone\"\n> afterwards is a good idea, or else your \"git sparse-checkout (set|add)\"\n> commands will not behave the way you expect.\n>\n> (I think that I will propose a change in behavior to make it do so during\n> this release cycle.)\n>\n> > A bit of a side note, or a few of them, but this command of yours is broken:\n> >   git sparse-checkout set README.md\n> > because --cone mode means you are specifying *directories* that should\n> > be checked out.  Currently, this gives no error, it instead silently\n> > drops you back to non-cone mode, which seems bad to me.\n> > sparse-checkout should provide some kind of error -- or at very least\n> > a warning -- when you make that mistake.\n>\n> > Now let's talk about the commit in question that changed behavior\n> > here.  The point of sparse-checkout is never to switch branches or\n> > checkout new commits; all it does is update which paths are in the\n> > current working directory.  A related point to this is it should never\n> > add or remove entries from the index and shouldn't change any hashes\n> > of files in the index.  It used to violate this, at first via an\n> > implementation that was literally invoking `git read-tree -mu HEAD` in\n> > a subprocess, and then later using internal code equivalent to\n> > invoking that command in a subprocess.  But by violating the\n> > leave-index-entries-alone mandate, it left folks who were in the\n> > middle of a rebase and wanted to update their sparse-checkout to\n> > include some more directories in their working tree in a precarious\n> > spot -- if they didn't update, then they didn't have the files\n> > necessary to build, and if they did forcibly update via `git read-tree\n> > -mu HEAD` then their staged changes would all get wiped out.  I spent\n> > some quality time helping users recover their files and teaching them\n> > about the git storage model.\n> >\n> > So that brings us back to your original question.  When you said\n> > --no-checkout, it means that there is no commit checked out and the\n> > index is empty.  update_sparsity() is correctly toggling the\n> > SKIP_WORKTREE bits for the existing index entries that don't match the\n> > sparsity patterns, and it is correctly calling check_updates().\n> > check_updates() is correctly checking for files currently in the index\n> > which have toggled to being needed in the current worktree so that it\n> > can issue downloads related to promisor packs.  The problem is just\n> > that there aren't any index entries to begin with, so there are no\n> > SKIP_WORKTREE bits to update, and thus no files that need to be\n> > downloaded.\n> >\n> > It seems a bit risky to make sparse-checkout start doing\n> > checkout/switch behavior and adding entries to the index.  There's a\n> > couple ways forward.  One, we could decide this is a special edge or\n> > corner case where we allow it: if the index is completely empty, then\n> > there's no data to lose and thus we could make `git sparse-checkout\n> > init [--cone]` in that one case use the old 'read-tree -mu HEAD'\n> > logic.  Alternatively, we could just require users to run 'git reset\n> > --hard' at the end of your script.\n> >\n> > Stolee: Thoughts?\n>\n> I agree that using \"--sparse\" instead of \"--no-checkout\" is the\n> best way forward for now, but I'll classify that as a workaround\n> and not necessarily the end of the conversation.\n\nAgreed.\n\n> In general, the commit in question is doing something extremely\n> valuable for common situations, like rebase that you mention.\n> I also think that this change in behavior is warranted by the\n> clear warning placed at the top of the docs [1]:\n>\n>         THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE\n>         BEHAVIOR OF OTHER COMMANDS IN THE PRESENCE OF SPARSE-\n>         CHECKOUTS, WILL LIKELY CHANGE IN THE FUTURE.\n>\n> [1] https://git-scm.com/docs/git-sparse-checkout#_description\n>\n> Of course, that's just us covering our butts as we push the\n> feature forward. It doesn't stop users (especially those that\n> are bravely using the feature) from feeling pain that might\n> be avoidable.\n>\n> To wrap up, it's unfortunate that the behavior changed. If it\n> is easy to detect and handle this special case, then it _may_\n> be worth doing to avoid disrupting users. Elijah: could you\n> spend at most an hour trying to see how difficult this would be?\n\nI actually spent...um...I don't know, but quite a bit longer than an\nhour on this today (er, um, yesterday now).  I kept feeling that we\nwould be painting ourselves into a bad corner, or violating some\nimportant invariant, and was also worried for a bit that this was just\na canary pointing out other important bugs that were ready to spring\non us once partial clones and sparse-checkouts were used more heavily\ntogether.  So I dug around a fair amount.  jrnieder's post today[1]\nwas very timely; it gave me a good way to frame what bothered me so\nmuch.  It seems to explain why I tend to hate working in dir.c, but\ndon't mind merge-recursive.c quite as bad: merge-recursive.c is a bit\nof a mess but it has intended invariants that can be discovered if you\ndig far enough (which means, it should be fixable or at least\nreplaceable); dir.c by contrast is mostly just a pile of empirical\nbehavior bolted on as we went and attempting to modify or replace or\nduplicate it is going to always be a big uphill battle.  I'm worried\nabout moving sparse-checkout in the direction of a pile of bolted on\nbehaviors and losing sight of high-level design principles.  In\nparticular, I think that restoring the old behavior would move us in\nthat direction, and make us more inconsistent with other aspects of\ngit.  However, that said, the new behavior is not good either; it's\njust buggy in a new way (though it's limited to this unborn index\nspecial case).\n\nI've got a patch that I'll post soon; it's a simple two-line change,\naccompanied by an 87-line commit message -- because it took a while to\nexplain the history, the intended invariants, how those invariants\nwere messed up in different ways historically, how we can restore the\ndesired properties that'll make it easier to reason about the code,\nand how the new behavior after the patch also makes us more consistent\nwith other behavior already found in git.  I'll submit it as soon as\nit passes testsuites on the relevant machines.\n\n\n[1] https://lore.kernel.org/git/20200603212336.GD253041@google.com/\n"},{"id":"399152","messageId":"CAD3+_6AK390F5iVqCmP-FY8MxJtcUoS7HsbunhZ0qNzOdT53CQ@mail.gmail.com","threadId":"53603","inReplyTo":"CABPp-BHyDUrOLg6-VONewbmXNFHUvKoDXfRTLgg8aEhJFtJLuQ@mail.gmail.com","subject":"Re: Possible regression for sparse-checkout in git 2.27.0","fromName":"Shaun Case","fromEmail":"warmsocks@gmail.com","sentAt":"2020-06-04T20:50:08Z","receivedAt":"2020-06-04T20:50:22Z","isPatch":false,"sender":{"key":"warmsocks@gmail.com","avatar":null},"body":"Hi Elijah, Derrick,\n\nOn Thu, Jun 4, 2020 at 12:27 AM Elijah Newren <newren@gmail.com> wrote:\n>\n> Hi,\n>\n> On Wed, Jun 3, 2020 at 8:36 AM Derrick Stolee <stolee@gmail.com> wrote:\n> >\n> > On 6/3/2020 12:37 AM, Elijah Newren wrote:\n> > > I think it'd be more natural to run\n> >\n> > >   git clone --filter=blob:none --sparse\n> > > https://github.com/r-spacex/launch-timeline.git\n> >\n> > > in place of the combination of\n> >\n> > >   git clone --filter=blob:none --no-checkout\n> > > https://github.com/r-spacex/launch-timeline.git\n> > >   git sparse-checkout init --cone\n> >\n> > > since the --sparse flag was added just for this kind of case -- to do\n> > > a clone but start with only a few things checked out.  It's easier, is\n> > > the route we're moving towards, and as a bonus also happens to work.\n> >\n> > Just one warning: the --sparse option in \"git clone\" does not currently\n> > enable core.sparseCheckoutCone, so running \"git sparse-checkout init --cone\"\n> > afterwards is a good idea, or else your \"git sparse-checkout (set|add)\"\n> > commands will not behave the way you expect.\n\nOk, great.  My production script now does this:\n\ngit clone --filter=blob:none --sparse <URL>\ngit sparse-checkout init --cone\ngit sparse-checkout add <PATH1>\ngit sparse-checkout add <PATH2>\n[ ... ]\n\n... and everything works as expected in 2.26.2 and 2.27.0. Perfection!\n\nSee below for why I was doing it the other way.\n\n> > (I think that I will propose a change in behavior to make it do so during\n> > this release cycle.)\n\nThat sounds good to me.  Could you also consider adding an error\nmessage for git sparse-checkout init, if something unexpected follows?  My\nrationale is that my script initially contained this:\n\n     git sparse-checkout init cone\n\nNote the missing \"--\" in front of cone.  This failed silently, so I\nthought I was in\ncone mode, but wasn't.  An error message would have helped.  Your proposal,\nhaving it happen automatically, would too.\n\n> > > A bit of a side note, or a few of them, but this command of yours is broken:\n> > >   git sparse-checkout set README.md\n> > > because --cone mode means you are specifying *directories* that should\n> > > be checked out.\n\nThank you for pointing this out.  This is a surprise.  I'm migrating some large\nSubversion repos and workspace setup scripts to git (thus the interest\nin partial\ncloning and sparse checkouts), and svn has the ability to sparse checkout\nindividual files as well as directories, so I expected it to work the\nsame way in git.\n\nIs this limitation a design decision, a technical limitation, a\nplanned feature?  It\nseems to be fairly popular in svn, and  there is even more interest\nfor it in git:\n\nhttps://stackoverflow.com/questions/122107/checkout-one-file-from-subversion\nhttps://stackoverflow.com/questions/2466735/how-to-sparsely-checkout-only-one-single-file-from-a-git-repository\n\n> > > Currently, this gives no error, it instead silently\n> > > drops you back to non-cone mode, which seems bad to me.\n> > > sparse-checkout should provide some kind of error -- or at very least\n> > > a warning -- when you make that mistake.\n\nAgreed.\n\nI'm going to leave the following here for context, but please scroll down.\n\n> > > Now let's talk about the commit in question that changed behavior\n> > > here.  The point of sparse-checkout is never to switch branches or\n> > > checkout new commits; all it does is update which paths are in the\n> > > current working directory.  A related point to this is it should never\n> > > add or remove entries from the index and shouldn't change any hashes\n> > > of files in the index.  It used to violate this, at first via an\n> > > implementation that was literally invoking `git read-tree -mu HEAD` in\n> > > a subprocess, and then later using internal code equivalent to\n> > > invoking that command in a subprocess.  But by violating the\n> > > leave-index-entries-alone mandate, it left folks who were in the\n> > > middle of a rebase and wanted to update their sparse-checkout to\n> > > include some more directories in their working tree in a precarious\n> > > spot -- if they didn't update, then they didn't have the files\n> > > necessary to build, and if they did forcibly update via `git read-tree\n> > > -mu HEAD` then their staged changes would all get wiped out.  I spent\n> > > some quality time helping users recover their files and teaching them\n> > > about the git storage model.\n> > >\n> > > So that brings us back to your original question.  When you said\n> > > --no-checkout, it means that there is no commit checked out and the\n> > > index is empty.  update_sparsity() is correctly toggling the\n> > > SKIP_WORKTREE bits for the existing index entries that don't match the\n> > > sparsity patterns, and it is correctly calling check_updates().\n> > > check_updates() is correctly checking for files currently in the index\n> > > which have toggled to being needed in the current worktree so that it\n> > > can issue downloads related to promisor packs.  The problem is just\n> > > that there aren't any index entries to begin with, so there are no\n> > > SKIP_WORKTREE bits to update, and thus no files that need to be\n> > > downloaded.\n> > >\n> > > It seems a bit risky to make sparse-checkout start doing\n> > > checkout/switch behavior and adding entries to the index.  There's a\n> > > couple ways forward.  One, we could decide this is a special edge or\n> > > corner case where we allow it: if the index is completely empty, then\n> > > there's no data to lose and thus we could make `git sparse-checkout\n> > > init [--cone]` in that one case use the old 'read-tree -mu HEAD'\n> > > logic.  Alternatively, we could just require users to run 'git reset\n> > > --hard' at the end of your script.\n> > >\n> > > Stolee: Thoughts?\n> >\n> > I agree that using \"--sparse\" instead of \"--no-checkout\" is the\n> > best way forward for now, but I'll classify that as a workaround\n> > and not necessarily the end of the conversation.\n>\n> Agreed.\n\nAgree too.  I also agree that Elijah's changes are necessary, and will save me\na ton of headaches going forward.  I don't want them backed out.\n\nSo, it seems --sparse instead of --no-checkout is what I should have been doing\nfrom the beginning, I had to fiddle a bit to get what I had working,\nby following\nsome online resources and experimenting, and what I ended up with working\nsatisfactorily was --no-checkout, with 2.26.2.\n\nWhy?\n\nPerhaps the messaging has inadvertently become a little muddled.  I started\nby working on getting partial cloning working; when I searched, these are the\nresources I found and followed:\n\nhttps://about.gitlab.com/blog/2020/03/13/partial-clone-for-massive-repositories/\nhttps://github.blog/2020-01-13-highlights-from-git-2-25/\nhttps://stackoverflow.com/questions/4114887/is-it-possible-to-do-a-sparse-checkout-without-checking-out-the-whole-repository\nhttps://github.blog/2020-01-17-bring-your-monorepo-down-to-size-with-sparse-checkout/\nhttps://docs.gitlab.com/ce/topics/git/partial_clone.html\n\nThe first four mention --no-checkout, but do not mention --sparse.  The last one\nactually gives proper guidance, but by the time I came across it, I had the\n--no-checkout method working and I came away not even realizing --sparse\nexists.  Perhaps I'm the only one who went down this path.  If not, the\n--no-checkout case may be a bit more common than you might expect.\n\n> > In general, the commit in question is doing something extremely\n> > valuable for common situations, like rebase that you mention.\n> > I also think that this change in behavior is warranted by the\n> > clear warning placed at the top of the docs [1]:\n> >\n> >         THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE\n> >         BEHAVIOR OF OTHER COMMANDS IN THE PRESENCE OF SPARSE-\n> >         CHECKOUTS, WILL LIKELY CHANGE IN THE FUTURE.\n> >\n> > [1] https://git-scm.com/docs/git-sparse-checkout#_description\n\nGot it.  Consider your butts covered.  :)\n\nThanks for the guidance and explanation.\n\nShaun\n"},{"id":"399158","messageId":"0aab7056-7176-01bc-ca7b-01356cbace4c@gmail.com","threadId":"53603","inReplyTo":"CAD3+_6AK390F5iVqCmP-FY8MxJtcUoS7HsbunhZ0qNzOdT53CQ@mail.gmail.com","subject":"Re: Possible regression for sparse-checkout in git 2.27.0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-05T00:32:42Z","receivedAt":"2020-06-05T00:32:49Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/4/2020 4:50 PM, Shaun Case wrote:\n> Hi Elijah, Derrick,\n> \n> On Thu, Jun 4, 2020 at 12:27 AM Elijah Newren <newren@gmail.com> wrote:\n>>\n>> Hi,\n>>\n>> On Wed, Jun 3, 2020 at 8:36 AM Derrick Stolee <stolee@gmail.com> wrote:\n>>>\n>>> On 6/3/2020 12:37 AM, Elijah Newren wrote:\n>>>> I think it'd be more natural to run\n>>>\n>>>>   git clone --filter=blob:none --sparse\n>>>> https://github.com/r-spacex/launch-timeline.git\n>>>\n>>>> in place of the combination of\n>>>\n>>>>   git clone --filter=blob:none --no-checkout\n>>>> https://github.com/r-spacex/launch-timeline.git\n>>>>   git sparse-checkout init --cone\n>>>\n>>>> since the --sparse flag was added just for this kind of case -- to do\n>>>> a clone but start with only a few things checked out.  It's easier, is\n>>>> the route we're moving towards, and as a bonus also happens to work.\n>>>\n>>> Just one warning: the --sparse option in \"git clone\" does not currently\n>>> enable core.sparseCheckoutCone, so running \"git sparse-checkout init --cone\"\n>>> afterwards is a good idea, or else your \"git sparse-checkout (set|add)\"\n>>> commands will not behave the way you expect.\n> \n> Ok, great.  My production script now does this:\n> \n> git clone --filter=blob:none --sparse <URL>\n> git sparse-checkout init --cone\n> git sparse-checkout add <PATH1>\n> git sparse-checkout add <PATH2>\n> [ ... ]\n> \n> ... and everything works as expected in 2.26.2 and 2.27.0. Perfection!\n> \n> See below for why I was doing it the other way.\n> \n>>> (I think that I will propose a change in behavior to make it do so during\n>>> this release cycle.)\n> \n> That sounds good to me.  Could you also consider adding an error\n> message for git sparse-checkout init, if something unexpected follows?  My\n> rationale is that my script initially contained this:\n> \n>      git sparse-checkout init cone\n> \n> Note the missing \"--\" in front of cone.  This failed silently, so I\n> thought I was in\n> cone mode, but wasn't.  An error message would have helped.  Your proposal,\n> having it happen automatically, would too.\n\nYes, this is probably a bug that's my fault. The option parser should\nbe more strict in the \"init\" case.\n\n>>>> A bit of a side note, or a few of them, but this command of yours is broken:\n>>>>   git sparse-checkout set README.md\n>>>> because --cone mode means you are specifying *directories* that should\n>>>> be checked out.\n> \n> Thank you for pointing this out.  This is a surprise.  I'm migrating some large\n> Subversion repos and workspace setup scripts to git (thus the interest\n> in partial\n> cloning and sparse checkouts), and svn has the ability to sparse checkout\n> individual files as well as directories, so I expected it to work the\n> same way in git.\n> \n> Is this limitation a design decision, a technical limitation, a\n> planned feature?  It\n> seems to be fairly popular in svn, and  there is even more interest\n> for it in git:\n> \n> https://stackoverflow.com/questions/122107/checkout-one-file-from-subversion\n> https://stackoverflow.com/questions/2466735/how-to-sparsely-checkout-only-one-single-file-from-a-git-repository\n\nThere are a couple reasons why this is tricky in Git.\n\nThe first is due to the .gitignore syntax. The syntax allows exact\nmatches for _directories_ using a trailing slash \"/\". For example,\nwe can match everything in A/B/C with the pattern\n\n\tA/B/C/\n\nThis would match the files in A/B/C/ and its subdirectories, but will\nnot match a file A/B/C.txt or A/B/C1/. There is no equivalent matching\nfor files, so A/B/C _will_ match a file A/B/C and A/B/C.txt. Whether this\nmatters to you or not depends on your file structure.\n\nThe second is how the pattern-matching works for cone mode. Everything\nis based on prefix matches of directories, but also that if a directory\nis included, then all of its parent directories are included, but not\nrecursively. All files contained directly in one of the parent directories\nare included automatically. Thus, if A/B/C/ is recursively added, then\nA/B/X.txt, A/Y.txt, and Z.txt all match in cone mode. This allows us some\nsignificant performance gains by allowing us to stop walking trees when\nwe reach a directory that isn't one of these \"parent\" directories. We\nknow that A/D/ is not in our list of \"parents\" so we don't need to look\nin there.\n\nWhat does this have to do with file matches? If we include A/B/C.txt as\na file, then we will need to match all sibling files in A/B/! This is\nvery similar to matching A/B/* or even A/B/C/*.\n\nThis idea was brought up and debated shortly after 2.25.0 released [1]\n\n[1] https://lore.kernel.org/git/CADSBhNbbO=aq-Oo2MpzDMN2VAX4m6f9Jb-eCtVVX1NfWKE9zJw@mail.gmail.com/\n\n>>>> Currently, this gives no error, it instead silently\n>>>> drops you back to non-cone mode, which seems bad to me.\n>>>> sparse-checkout should provide some kind of error -- or at very least\n>>>> a warning -- when you make that mistake.\n> \n> Agreed.\n> \n> I'm going to leave the following here for context, but please scroll down.\n> \n>>>> Now let's talk about the commit in question that changed behavior\n>>>> here.  The point of sparse-checkout is never to switch branches or\n>>>> checkout new commits; all it does is update which paths are in the\n>>>> current working directory.  A related point to this is it should never\n>>>> add or remove entries from the index and shouldn't change any hashes\n>>>> of files in the index.  It used to violate this, at first via an\n>>>> implementation that was literally invoking `git read-tree -mu HEAD` in\n>>>> a subprocess, and then later using internal code equivalent to\n>>>> invoking that command in a subprocess.  But by violating the\n>>>> leave-index-entries-alone mandate, it left folks who were in the\n>>>> middle of a rebase and wanted to update their sparse-checkout to\n>>>> include some more directories in their working tree in a precarious\n>>>> spot -- if they didn't update, then they didn't have the files\n>>>> necessary to build, and if they did forcibly update via `git read-tree\n>>>> -mu HEAD` then their staged changes would all get wiped out.  I spent\n>>>> some quality time helping users recover their files and teaching them\n>>>> about the git storage model.\n>>>>\n>>>> So that brings us back to your original question.  When you said\n>>>> --no-checkout, it means that there is no commit checked out and the\n>>>> index is empty.  update_sparsity() is correctly toggling the\n>>>> SKIP_WORKTREE bits for the existing index entries that don't match the\n>>>> sparsity patterns, and it is correctly calling check_updates().\n>>>> check_updates() is correctly checking for files currently in the index\n>>>> which have toggled to being needed in the current worktree so that it\n>>>> can issue downloads related to promisor packs.  The problem is just\n>>>> that there aren't any index entries to begin with, so there are no\n>>>> SKIP_WORKTREE bits to update, and thus no files that need to be\n>>>> downloaded.\n>>>>\n>>>> It seems a bit risky to make sparse-checkout start doing\n>>>> checkout/switch behavior and adding entries to the index.  There's a\n>>>> couple ways forward.  One, we could decide this is a special edge or\n>>>> corner case where we allow it: if the index is completely empty, then\n>>>> there's no data to lose and thus we could make `git sparse-checkout\n>>>> init [--cone]` in that one case use the old 'read-tree -mu HEAD'\n>>>> logic.  Alternatively, we could just require users to run 'git reset\n>>>> --hard' at the end of your script.\n>>>>\n>>>> Stolee: Thoughts?\n>>>\n>>> I agree that using \"--sparse\" instead of \"--no-checkout\" is the\n>>> best way forward for now, but I'll classify that as a workaround\n>>> and not necessarily the end of the conversation.\n>>\n>> Agreed.\n> \n> Agree too.  I also agree that Elijah's changes are necessary, and will save me\n> a ton of headaches going forward.  I don't want them backed out.\n> \n> So, it seems --sparse instead of --no-checkout is what I should have been doing\n> from the beginning, I had to fiddle a bit to get what I had working,\n> by following\n> some online resources and experimenting, and what I ended up with working\n> satisfactorily was --no-checkout, with 2.26.2.\n> \n> Why?\n> \n> Perhaps the messaging has inadvertently become a little muddled.  I started\n> by working on getting partial cloning working; when I searched, these are the\n> resources I found and followed:\n> \n> https://about.gitlab.com/blog/2020/03/13/partial-clone-for-massive-repositories/\n> https://github.blog/2020-01-13-highlights-from-git-2-25/\n> https://stackoverflow.com/questions/4114887/is-it-possible-to-do-a-sparse-checkout-without-checking-out-the-whole-repository\n> https://github.blog/2020-01-17-bring-your-monorepo-down-to-size-with-sparse-checkout/\n> https://docs.gitlab.com/ce/topics/git/partial_clone.html\n> \n> The first four mention --no-checkout, but do not mention --sparse.  The last one\n> actually gives proper guidance, but by the time I came across it, I had the\n> --no-checkout method working and I came away not even realizing --sparse\n> exists.  Perhaps I'm the only one who went down this path.  If not, the\n> --no-checkout case may be a bit more common than you might expect.\n\nThere's a good reason for this. In v2.25.0, the \"--sparse\" option was\npresent but broken. See [1] for the fix which was released in v2.26.0\nas 47dbf10d8 (clone: fix --sparse option with URLs, 2020-01-24).\n\n[1] https://lore.kernel.org/git/4991a51f6d5d840eaa3bb830e68f1530c2ee08e4.1579900782.git.gitgitgadget@gmail.com/\n\n>>> In general, the commit in question is doing something extremely\n>>> valuable for common situations, like rebase that you mention.\n>>> I also think that this change in behavior is warranted by the\n>>> clear warning placed at the top of the docs [1]:\n>>>\n>>>         THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE\n>>>         BEHAVIOR OF OTHER COMMANDS IN THE PRESENCE OF SPARSE-\n>>>         CHECKOUTS, WILL LIKELY CHANGE IN THE FUTURE.\n>>>\n>>> [1] https://git-scm.com/docs/git-sparse-checkout#_description\n> \n> Got it.  Consider your butts covered.  :)\n\n:D\n\nThanks,\n-Stolee\n\n"},{"id":"399159","messageId":"xmqqtuzqnxdm.fsf@gitster.c.googlers.com","threadId":"53603","inReplyTo":"0aab7056-7176-01bc-ca7b-01356cbace4c@gmail.com","subject":"Re: Possible regression for sparse-checkout in git 2.27.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-05T00:56:37Z","receivedAt":"2020-06-05T00:59:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> The first is due to the .gitignore syntax. The syntax allows exact\n> matches for _directories_ using a trailing slash \"/\". For example,\n> we can match everything in A/B/C with the pattern\n>\n> \tA/B/C/\n>\n> This would match the files in A/B/C/ and its subdirectories, but will\n> not match a file A/B/C.txt or A/B/C1/. There is no equivalent matching\n> for files, so A/B/C _will_ match a file A/B/C and A/B/C.txt. Whether this\n> matters to you or not depends on your file structure.\n\nThe pattern A/B/C _will_ match a file A/B/C and a directory A/B/C/,\nbut I do not think it matches a file A/B/C.txt (or a path with any\nother suffix).\n\nI suspect that for the purpose of your explanation, the pattern\nA/B/C does not have to match A/B/C.txt to cause trouble; the fact\nthat it matches directory A/B/C would be sufficient, I guess.\n"},{"id":"399183","messageId":"7a6e14b2-84ce-0a78-3644-ffd0e647df05@gmail.com","threadId":"53603","inReplyTo":"xmqqtuzqnxdm.fsf@gitster.c.googlers.com","subject":"Re: Possible regression for sparse-checkout in git 2.27.0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-05T13:07:52Z","receivedAt":"2020-06-05T13:07:57Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/4/2020 8:56 PM, Junio C Hamano wrote:\n> Derrick Stolee <stolee@gmail.com> writes:\n> \n>> The first is due to the .gitignore syntax. The syntax allows exact\n>> matches for _directories_ using a trailing slash \"/\". For example,\n>> we can match everything in A/B/C with the pattern\n>>\n>> \tA/B/C/\n>>\n>> This would match the files in A/B/C/ and its subdirectories, but will\n>> not match a file A/B/C.txt or A/B/C1/. There is no equivalent matching\n>> for files, so A/B/C _will_ match a file A/B/C and A/B/C.txt. Whether this\n>> matters to you or not depends on your file structure.\n> \n> The pattern A/B/C _will_ match a file A/B/C and a directory A/B/C/,\n> but I do not think it matches a file A/B/C.txt (or a path with any\n> other suffix).\n\nYou are correct! I am mistaken. The directory/file confusion would\nlead to some interesting logic in an implementation that allowed\nfile matches.\n\n> I suspect that for the purpose of your explanation, the pattern\n> A/B/C does not have to match A/B/C.txt to cause trouble; the fact\n> that it matches directory A/B/C would be sufficient, I guess.\n\nYes. It certainly complicates things.\n\nThe thing I neglected to mention is that this file-based matching\nis usually necessary when there is a directory containing _all_ of\nthe \"big files\" instead of organizing those files into directories\nbased on who needs those files. It is possible to reorganize the\ndirectory structure to use cone mode and achieve similar goals.\n\nThanks,\n-Stolee\n\n"}]}