{"thread":{"id":"57010","subject":"2.34 regression (and workaround): deleting untracked files both outside *and inside* desired sparsity cone","startedAt":"2021-12-01T17:16:52Z","lastAt":"2021-12-02T00:43:47Z","messageCount":4,"participants":["Elijah Newren","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"442782","messageId":"CABPp-BE8TJ8QGAQWsSGT7S+9Xp-XmApcC9PSw3K=RQOP0rt+PQ@mail.gmail.com","threadId":"57010","inReplyTo":null,"subject":"2.34 regression (and workaround): deleting untracked files both outside *and inside* desired sparsity cone","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-01T17:16:28Z","receivedAt":"2021-12-01T17:16:52Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nI've got a proposal for changing the sparse-checkout command slightly;\nbut it probably doesn't make sense without the context of the bugs\n(old and new) we are facing.  Consider this an RFC, with the final\nbullet point particularly in need of comment and ideas.\n\n== Background ==\n\nsparse-checkouts in cone mode are documented as being obtained either\nby using the `--sparse` flag to `git clone`, or by running the\nsequence:\n\n    git sparse-checkout init --cone [--sparse-index]\n    git sparse-checkout set ...\n\nThe first step has traditionally deleted all the tracked files from\nthe working tree, except in the toplevel directory, and the second\nrestores all the tracked files that are wanted.\n\n(Usage context:)\nMy understanding is Microsoft never uses this sequence, instead using\nthe --sparse flag to `git clone`.  In contrast, at Palantir the\n--sparse flag to clone is rarely used.\n\n(An aside on pre-existing bugs/warts of the above sequence:)\nThis has always been bad from a performance point of view (especially\nin more extreme not-so-sparse cases when the desired sparsity paths\nrepresent roughly half of all paths), and has been suboptimal from a\nUI point of view due to the dual progress meters (other wrappers, such\nas an in-house `sparsify`, can have a single user-facing command for\nswitching to a sparse-checkout that has to call both of these git\ncommands under the hood; that wrapper would prefer one progress meter\nto two).  But it never quite arose to the level of needing to be\nfixed.\n\n== The (New) Bug ==\n\nStarting with Git 2.34, each step will delete all ignored files\noutside the sparsity paths specified to the individual command in\nquestion.  We are totally onboard with deleting ignored files outside\nthe sparsity paths the user wants, but the first command is required\naccording to the documentation and does not allow specifying any\nsparsity paths.  Since it does not allow specifying any sparsity\npaths, it treats *everything* as outside and essentially deletes all\nignored files everywhere.  That's not workable for us.  We want a\nsingle command for changing to a sparse-checkout.\n\n== The Current Workaround ==\n\nLuckily, having these two commands separate isn't enforced, and the\nfirst command is basically roughly equivalent to setting a few config\nvariables and then running `sparse-checkout set` with an empty set of\npaths.  So, currently, we can just do the config setting part of init\nmanually, and then skip the rest of init, and then call our desired\n`set` command:\n    git config extensions.worktreeConfig true\n    git config --worktree core.sparseCheckout true\n    git config --worktree core.sparseCheckoutCone true\n    git sparse-checkout set ...\n\nSince we're using a wrapper anyway (for computing dependencies and\ndetermining the list of directories to include), it was relatively\neasy for us to add this workaround.\n\nHowever, it is not clear that our current workaround will continue\nfunctioning with future versions of git, particularly if\n`sparse-checkout init` gains more options.  In fact, it already\ndoesn't handle --sparse-index.\n\n== Long term proposal ==\n\nMake `set` do both the work of `init` and `set`.\n\nThis means:\n  * `set` gains the ability to parse both --cone and --sparse-index\n(in addition to --stdin, etc.)\n  * If the sparse-index is not initialized, `set` does the\ninitialization work of `init`.\n  * Modify the `init` documentation to mark it as deprecated,\nmentioning the 2-3 bugs above as reasons why.\n  * We could effectively just turn `git sparse-checkout init ...` into\nan alias for `git sparse-checkout set ...`, since init's parameters\nwould be a subset of those that `set` accepts.  However, the latter\nmight interact badly with allowing a user to toggle sparse-index on\nand off in the middle of a sparse-checkout...so maybe we need\nsomething more?  Alternatively, we could leave `init` as-is and just\nconsider it set in concrete, possibly risking it becoming\nnon-functional in a future upgrade.  Hmm...\n\n\nThoughts?\n"},{"id":"442788","messageId":"9c7afd37-9940-e6e7-da41-8a44840defb3@gmail.com","threadId":"57010","inReplyTo":"CABPp-BE8TJ8QGAQWsSGT7S+9Xp-XmApcC9PSw3K=RQOP0rt+PQ@mail.gmail.com","subject":"Re: 2.34 regression (and workaround): deleting untracked files both outside *and inside* desired sparsity cone","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-12-01T19:19:32Z","receivedAt":"2021-12-01T19:19:36Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/1/2021 12:16 PM, Elijah Newren wrote:\n> Hi,\n> \n> I've got a proposal for changing the sparse-checkout command slightly;\n> but it probably doesn't make sense without the context of the bugs\n> (old and new) we are facing.  Consider this an RFC, with the final\n> bullet point particularly in need of comment and ideas.\n> \n> == Background ==\n> \n> sparse-checkouts in cone mode are documented as being obtained either\n> by using the `--sparse` flag to `git clone`, ...\n\nThe `--sparse` flag doesn't initialize cone mode, unfortunately.\n\n>     git sparse-checkout init --cone [--sparse-index]\n>     git sparse-checkout set ...\n> \n> The first step has traditionally deleted all the tracked files from\n> the working tree, except in the toplevel directory, and the second\n> restores all the tracked files that are wanted.\n> \n> (Usage context:)\n> My understanding is Microsoft never uses this sequence, instead using\n> the --sparse flag to `git clone`.  In contrast, at Palantir the\n> --sparse flag to clone is rarely used.\n\nWe use the sparse-checkout builtin. From the Scalar patch series,\nyou can see that we don't call \"git clone\" at all, but instead\n\"scalar clone\" does a lot with \"git init\" and works from there by\nsetting config and fetching at the correct time.\n \n> == The (New) Bug ==\n> \n> Starting with Git 2.34, each step will delete all ignored files\n> outside the sparsity paths specified to the individual command in\n> question.  We are totally onboard with deleting ignored files outside\n> the sparsity paths the user wants, but the first command is required\n> according to the documentation and does not allow specifying any\n> sparsity paths.  Since it does not allow specifying any sparsity\n> paths, it treats *everything* as outside and essentially deletes all\n> ignored files everywhere.  That's not workable for us.  We want a\n> single command for changing to a sparse-checkout.\n\nAh, since 'git sparse-checkout set' would work differently if not\nin cone mode, I see the problem. It's a little too much to manually\nset core.sparseCheckoutCone=true before running the 'set' command,\nprobably.\n\n> == The Current Workaround ==\n> \n> Luckily, having these two commands separate isn't enforced, and the\n> first command is basically roughly equivalent to setting a few config\n> variables and then running `sparse-checkout set` with an empty set of\n> paths.  So, currently, we can just do the config setting part of init\n> manually, and then skip the rest of init, and then call our desired\n> `set` command:\n>     git config extensions.worktreeConfig true\n>     git config --worktree core.sparseCheckout true\n>     git config --worktree core.sparseCheckoutCone true\n>     git sparse-checkout set ...\n\nWhich you have already worked out.\n\n> Since we're using a wrapper anyway (for computing dependencies and\n> determining the list of directories to include), it was relatively\n> easy for us to add this workaround.\n> \n> However, it is not clear that our current workaround will continue\n> functioning with future versions of git, particularly if\n> `sparse-checkout init` gains more options.  In fact, it already\n> doesn't handle --sparse-index.\n\nRight. It's _yet another_ config option to tweak.\n\n> == Long term proposal ==\n> \n> Make `set` do both the work of `init` and `set`.\n> \n> This means:\n>   * `set` gains the ability to parse both --cone and --sparse-index\n> (in addition to --stdin, etc.)\n>   * If the sparse-index is not initialized, `set` does the\n> initialization work of `init`.\n>   * Modify the `init` documentation to mark it as deprecated,\n> mentioning the 2-3 bugs above as reasons why.\n>   * We could effectively just turn `git sparse-checkout init ...` into\n> an alias for `git sparse-checkout set ...`, since init's parameters\n> would be a subset of those that `set` accepts.  However, the latter\n> might interact badly with allowing a user to toggle sparse-index on\n> and off in the middle of a sparse-checkout...so maybe we need\n> something more?  Alternatively, we could leave `init` as-is and just\n> consider it set in concrete, possibly risking it becoming\n> non-functional in a future upgrade.  Hmm...\n\nI think this is a good plan. Making 'init' the same as 'set' with\nno paths makes sense to me. We would want to be careful now that\n\"--option\" could be interpreted as a path to recommend using\n\n  git sparse-checkout set <options> -- <path1> ... <pathN>\n\nWhile you are here, I would be interested in making 'git clone\n--sparse' default to cone mode. Or, should it be 'git clone\n--sparse=cone' or something? Not making it default to cone mode\nis a big regret of mine.\n\nThanks,\n-Stolee\n\n"},{"id":"442829","messageId":"CABPp-BHSPQMUw8a6E7Kz+7L6NuD8c9GrHdTMf_rBzQR+2VGs-Q@mail.gmail.com","threadId":"57010","inReplyTo":"9c7afd37-9940-e6e7-da41-8a44840defb3@gmail.com","subject":"Re: 2.34 regression (and workaround): deleting untracked files both outside *and inside* desired sparsity cone","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-01T23:40:35Z","receivedAt":"2021-12-01T23:40:55Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Dec 1, 2021 at 11:19 AM Derrick Stolee <stolee@gmail.com> wrote:\n>\n...\n> We use the sparse-checkout builtin. From the Scalar patch series,\n> you can see that we don't call \"git clone\" at all, but instead\n> \"scalar clone\" does a lot with \"git init\" and works from there by\n> setting config and fetching at the correct time.\n\nAh, thanks for the correction.  I had seen that, but forgotten.\n\n...\n> > == Long term proposal ==\n> >\n> > Make `set` do both the work of `init` and `set`.\n> >\n> > This means:\n> >   * `set` gains the ability to parse both --cone and --sparse-index\n> > (in addition to --stdin, etc.)\n> >   * If the sparse-index is not initialized, `set` does the\n> > initialization work of `init`.\n> >   * Modify the `init` documentation to mark it as deprecated,\n> > mentioning the 2-3 bugs above as reasons why.\n> >   * We could effectively just turn `git sparse-checkout init ...` into\n> > an alias for `git sparse-checkout set ...`, since init's parameters\n> > would be a subset of those that `set` accepts.  However, the latter\n> > might interact badly with allowing a user to toggle sparse-index on\n> > and off in the middle of a sparse-checkout...so maybe we need\n> > something more?  Alternatively, we could leave `init` as-is and just\n> > consider it set in concrete, possibly risking it becoming\n> > non-functional in a future upgrade.  Hmm...\n>\n> I think this is a good plan. Making 'init' the same as 'set' with\n> no paths makes sense to me.\n\nCool, I'll get to work on it.\n\n> We would want to be careful now that\n> \"--option\" could be interpreted as a path to recommend using\n>\n>   git sparse-checkout set <options> -- <path1> ... <pathN>\n\nMakes sense.  However, wasn't this already an issue when you added\n`--stdin` as an option for the `set` command?\n\n> While you are here, I would be interested in making 'git clone\n> --sparse' default to cone mode. Or, should it be 'git clone\n> --sparse=cone' or something? Not making it default to cone mode\n> is a big regret of mine.\n\nI agree it'd be much nicer to have it default to cone mode, and the\nbig warning in git-sparse-checkout.txt might permit us to do so.  A\nfew related questions:\n\n* Should we document how to change from cone mode to non-cone mode?\nWe have --sparse-index, --no-sparse-index, and --cone flags, but no\n--no-cone one.  Should we?  (Do these flags belong somewhere other\nthan `init` since it's toggling some other flag while already using a\nsparse-checkout?)\n\n* Should we clean up the wording in clone's --sparse option?  In particular:\n\n--sparse::\nInitialize the sparse-checkout file so the working\ndirectory starts with only the files in the root\nof the repository. The sparse-checkout file can be\nmodified to grow the working directory as needed.\n\nThis wording seems to suggest direct editing of\n.git/info/sparse-checkout, and might confuse users.  Perhaps the last\nsentence could change \"sparse-checkout file can be modified\" ->\n\"sparse-checkout command can be used\" or something like that?\n"},{"id":"442839","messageId":"d981adbf-7f6a-32f8-2d3e-8af5c36a6f8f@gmail.com","threadId":"57010","inReplyTo":"CABPp-BHSPQMUw8a6E7Kz+7L6NuD8c9GrHdTMf_rBzQR+2VGs-Q@mail.gmail.com","subject":"Re: 2.34 regression (and workaround): deleting untracked files both outside *and inside* desired sparsity cone","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-12-02T00:41:40Z","receivedAt":"2021-12-02T00:43:47Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/1/2021 6:40 PM, Elijah Newren wrote:\n> On Wed, Dec 1, 2021 at 11:19 AM Derrick Stolee <stolee@gmail.com> wrote:\n>>\n>> We would want to be careful now that\n>> \"--option\" could be interpreted as a path to recommend using\n>>\n>>   git sparse-checkout set <options> -- <path1> ... <pathN>\n> \n> Makes sense.  However, wasn't this already an issue when you added\n> `--stdin` as an option for the `set` command?\n\nYou are right. This should already be handled in a sane way.\n \n>> While you are here, I would be interested in making 'git clone\n>> --sparse' default to cone mode. Or, should it be 'git clone\n>> --sparse=cone' or something? Not making it default to cone mode\n>> is a big regret of mine.\n> \n> I agree it'd be much nicer to have it default to cone mode, and the\n> big warning in git-sparse-checkout.txt might permit us to do so.  A\n> few related questions:\n> \n> * Should we document how to change from cone mode to non-cone mode?\n> We have --sparse-index, --no-sparse-index, and --cone flags, but no\n> --no-cone one.  Should we?  (Do these flags belong somewhere other\n> than `init` since it's toggling some other flag while already using a\n> sparse-checkout?)\n\n--no-cone exists, it probably just isn't in the docs. Our 'init'\noptions are defined as follows:\n\n\tstatic struct option builtin_sparse_checkout_init_options[] = {\n\t\tOPT_BOOL(0, \"cone\", &init_opts.cone_mode,\n\t\t\t N_(\"initialize the sparse-checkout in cone mode\")),\n\t\tOPT_BOOL(0, \"sparse-index\", &init_opts.sparse_index,\n\t\t\t N_(\"toggle the use of a sparse index\")),\n\t\tOPT_END(),\n\t};\n\n\nso --no-cone exists the same way --no-sparse-index does.\n\n> * Should we clean up the wording in clone's --sparse option?  In particular:\n> \n> --sparse::\n> Initialize the sparse-checkout file so the working\n> directory starts with only the files in the root\n> of the repository. The sparse-checkout file can be\n> modified to grow the working directory as needed.\n> \n> This wording seems to suggest direct editing of\n> .git/info/sparse-checkout, and might confuse users.  Perhaps the last\n> sentence could change \"sparse-checkout file can be modified\" ->\n> \"sparse-checkout command can be used\" or something like that?\n\nThis is probably just too old. It could use updates to link to\ngit-sparse-checkout.\n\nThanks,\n-Stolee\n"}]}