| Message ID | pull.1148.v2.git.1647054681.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | sparse-checkout: make cone mode the default | expand |
On 3/11/2022 10:11 PM, Elijah Newren via GitGitGadget wrote: > == Updates Log == > > Changes since v1: > > * rebased on master, to remove dependence on en/present-despite-skipped > (which has since merged to master). Earlier version of series wasn't > picked up anyway, so rebasing should be safe. > * Wording and code style tweaks suggested by Stolee in his review Thanks for these updates. This version looks good to me. Thanks, -Stolee
Elijah Newren via GitGitGadget wrote: > == Updates Log == > > Changes since v1: > > * rebased on master, to remove dependence on en/present-despite-skipped > (which has since merged to master). Earlier version of series wasn't > picked up anyway, so rebasing should be safe. > * Wording and code style tweaks suggested by Stolee in his review > > == Overview == > > This patch changes the default mode for sparse-checkout from non-cone mode > to cone-mode, and marks non-cone mode as deprecated. There is no plan to > remove non-cone mode, we are merely recommending against its use. > > The code change is pretty small, and most of this series is about > documentation updates -- to focus on directories rather than patterns, to > explain the new default, to explain why we are deprecating non-cone mode > (the final patch), and to make other related cleanups to simplify the > manual. > > Patch 1: Update tests to not assume cone-mode is the default Patch 2: Make > cone-mode the default Patches 3-9: Various updates to > git-sparse-checkout.txt, divided up for ease of review > After looking over the patches (and Stolee's prior feedback), I don't have any additional comments - it all looks great! I also ran through some scenarios going from "non-cone" to "cone mode" that have been buggy in the past (e.g., non-cone directory patterns missing a leading '/', then 'git sparse-checkout init --cone'), but it looks like the checks added back in a3eca58445 (sparse-checkout: refuse to add to bad patterns, 2021-12-16) prevent any unexplained behavior for a user that might not be aware of the new default. In other words, changing the default looks to be safe and overall pushes users towards a better experience using sparse-checkout. > == Alternative == > > There is one primary alternative to this series: make sparse-checkout error > when neither --cone nor --no-cone are specified (so that there is no > default), and wait until a future date to make --cone the default. That'd be > reasonable, but I had three reason to avoid going this route (note that item > 2 means there's little practical difference between cone-mode-as-default and > no-mode-is-default): > > 1. git-sparse-checkout.txt has the following huge warning early in the > manual: > > """ THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE BEHAVIOR OF OTHER > COMMANDS IN THE PRESENCE OF SPARSE-CHECKOUTS, WILL LIKELY CHANGE IN THE > FUTURE. """ > > 2. If users are unaware of the default change and attempt to provide > patterns instead of directories, then they will get error messages added > from en/sparse-checkout-fixes. They can learn at that time to get around > the error messages by providing --no-cone. > > 3. If users are unaware of the default change and provide directories, then > that's where non-cone mode and cone mode overlap and things happen to > work. (There is a slight difference in that cone mode will include files > from parent directories of any specified directory, but that means the > user gets a few more files in their sparse-checkout with cone mode than > they would with non-cone mode.) > Agreed with your comments here and Stolee's earlier that changing the default is a better approach. > == CCs == > > Elijah Newren (9): > tests: stop assuming --no-cone is the default mode for sparse-checkout > sparse-checkout: make --cone the default > git-sparse-checkout.txt: wording updates for the cone mode default > git-sparse-checkout.txt: update docs for deprecation of 'init' > git-sparse-checkout.txt: shuffle some sections and mark as internal > git-sparse-checkout.txt: add a new EXAMPLES section > git-sparse-checkout.txt: flesh out non-cone mode pattern discussion a > bit > git-sparse-checkout.txt: mark non-cone mode as deprecated > Documentation: some sparsity wording clarifications > > Documentation/git-read-tree.txt | 9 +- > Documentation/git-sparse-checkout.txt | 249 +++++++++++++++++++------- > builtin/sparse-checkout.c | 2 +- > t/t1091-sparse-checkout-builtin.sh | 15 +- > t/t3602-rm-sparse-checkout.sh | 6 +- > t/t3705-add-sparse-checkout.sh | 4 +- > t/t6428-merge-conflicts-sparse.sh | 4 +- > t/t7002-mv-sparse-checkout.sh | 2 +- > t/t7012-skip-worktree-writing.sh | 2 +- > 9 files changed, 207 insertions(+), 86 deletions(-) > > > base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1148%2Fnewren%2Fsparse-checkout-default-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1148/newren/sparse-checkout-default-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/1148 > > Range-diff vs v1: > > 1: 8c3e730e86b = 1: 05dba7069c5 tests: stop assuming --no-cone is the default mode for sparse-checkout > 2: b174b42ed82 ! 2: a53179764bc sparse-checkout: make --cone the default > @@ builtin/sparse-checkout.c: static int update_modes(int *cone_mode, int *sparse_i > /* Set cone/non-cone mode appropriately */ > core_apply_sparse_checkout = 1; > - if (*cone_mode == 1) { > -+ if (*cone_mode == 1 || *cone_mode == -1) { > ++ if (*cone_mode) { /* also handles "not specified" (value of -1) */ > mode = MODE_CONE_PATTERNS; > core_sparse_checkout_cone = 1; > } else { > 3: f98b3fac78a = 3: 8eab21996c7 git-sparse-checkout.txt: wording updates for the cone mode default > 4: cd871966c06 = 4: eb3b318b39e git-sparse-checkout.txt: update docs for deprecation of 'init' > 5: fe37a966699 = 5: 7333198b778 git-sparse-checkout.txt: shuffle some sections and mark as internal > 6: 3265f41bcab ! 6: a814ea9e8d2 git-sparse-checkout.txt: add a new EXAMPLES section > @@ Documentation/git-sparse-checkout.txt: paths to pass to a subsequent 'set' or 'a > + > +`git sparse-checkout reapply`:: > + > -+ It is possible for commands to update the working tree in a way > -+ that does not respect the selected sparsity directories, either > -+ because of special cases (such as hitting conflicts when > -+ merging/rebasing), or because some commands didn't fully support > -+ sparse checkouts (e.g. the old `recursive` merge backend had > -+ only limited support). This command reapplies the existing > -+ sparse directory specifications to make the working directory > -+ match. > ++ It is possible for commands to update the working tree in a > ++ way that does not respect the selected sparsity directories. > ++ This can come from tools external to Git writing files, or > ++ even affect Git commands because of either special cases (such > ++ as hitting conflicts when merging/rebasing), or because some > ++ commands didn't fully support sparse checkouts (e.g. the old > ++ `recursive` merge backend had only limited support). This > ++ command reapplies the existing sparse directory specifications > ++ to make the working directory match. > + > INTERNALS -- SPARSE CHECKOUT > ---------------------------- > 7: bdbf61ee6a0 = 7: 78028ecaa58 git-sparse-checkout.txt: flesh out non-cone mode pattern discussion a bit > 8: 3e35d62c5ee = 8: 2d2b81986a5 git-sparse-checkout.txt: mark non-cone mode as deprecated > 9: e7d3ff70914 = 9: 4b89a3392b0 Documentation: some sparsity wording clarifications >
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > == Updates Log == > > Changes since v1: > > * rebased on master, to remove dependence on en/present-despite-skipped > (which has since merged to master). Earlier version of series wasn't > picked up anyway, so rebasing should be safe. > * Wording and code style tweaks suggested by Stolee in his review > > == Overview == > > This patch changes the default mode for sparse-checkout from non-cone mode > to cone-mode, and marks non-cone mode as deprecated. There is no plan to > remove non-cone mode, we are merely recommending against its use. Hopefully there are some convincing explanation to help users understand why the cone mode is recommended in the documentation in these patches. Was there a discussion on list to give our unanimous support for this plan, or is this only preference by a developer or two who have worked on the sparse-checkout code recently?
Junio C Hamano <gitster@pobox.com> writes: > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> == Updates Log == >> >> Changes since v1: >> >> * rebased on master, to remove dependence on en/present-despite-skipped >> (which has since merged to master). Earlier version of series wasn't >> picked up anyway, so rebasing should be safe. >> * Wording and code style tweaks suggested by Stolee in his review >> >> == Overview == >> >> This patch changes the default mode for sparse-checkout from non-cone mode >> to cone-mode, and marks non-cone mode as deprecated. There is no plan to >> remove non-cone mode, we are merely recommending against its use. > > Hopefully there are some convincing explanation to help users > understand why the cone mode is recommended in the documentation in > these patches. Was there a discussion on list to give our unanimous > support for this plan, or is this only preference by a developer or > two who have worked on the sparse-checkout code recently? OK. There are a few things that I found questionable, but they were mostly documentation issues (and lack of updates to two places, one in read-tree and the other in config/core.txt). Thanks.
== Updates Log == Changes since v1: * rebased on master, to remove dependence on en/present-despite-skipped (which has since merged to master). Earlier version of series wasn't picked up anyway, so rebasing should be safe. * Wording and code style tweaks suggested by Stolee in his review == Overview == This patch changes the default mode for sparse-checkout from non-cone mode to cone-mode, and marks non-cone mode as deprecated. There is no plan to remove non-cone mode, we are merely recommending against its use. The code change is pretty small, and most of this series is about documentation updates -- to focus on directories rather than patterns, to explain the new default, to explain why we are deprecating non-cone mode (the final patch), and to make other related cleanups to simplify the manual. Patch 1: Update tests to not assume cone-mode is the default Patch 2: Make cone-mode the default Patches 3-9: Various updates to git-sparse-checkout.txt, divided up for ease of review == Alternative == There is one primary alternative to this series: make sparse-checkout error when neither --cone nor --no-cone are specified (so that there is no default), and wait until a future date to make --cone the default. That'd be reasonable, but I had three reason to avoid going this route (note that item 2 means there's little practical difference between cone-mode-as-default and no-mode-is-default): 1. git-sparse-checkout.txt has the following huge warning early in the manual: """ THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE BEHAVIOR OF OTHER COMMANDS IN THE PRESENCE OF SPARSE-CHECKOUTS, WILL LIKELY CHANGE IN THE FUTURE. """ 2. If users are unaware of the default change and attempt to provide patterns instead of directories, then they will get error messages added from en/sparse-checkout-fixes. They can learn at that time to get around the error messages by providing --no-cone. 3. If users are unaware of the default change and provide directories, then that's where non-cone mode and cone mode overlap and things happen to work. (There is a slight difference in that cone mode will include files from parent directories of any specified directory, but that means the user gets a few more files in their sparse-checkout with cone mode than they would with non-cone mode.) == CCs == Elijah Newren (9): tests: stop assuming --no-cone is the default mode for sparse-checkout sparse-checkout: make --cone the default git-sparse-checkout.txt: wording updates for the cone mode default git-sparse-checkout.txt: update docs for deprecation of 'init' git-sparse-checkout.txt: shuffle some sections and mark as internal git-sparse-checkout.txt: add a new EXAMPLES section git-sparse-checkout.txt: flesh out non-cone mode pattern discussion a bit git-sparse-checkout.txt: mark non-cone mode as deprecated Documentation: some sparsity wording clarifications Documentation/git-read-tree.txt | 9 +- Documentation/git-sparse-checkout.txt | 249 +++++++++++++++++++------- builtin/sparse-checkout.c | 2 +- t/t1091-sparse-checkout-builtin.sh | 15 +- t/t3602-rm-sparse-checkout.sh | 6 +- t/t3705-add-sparse-checkout.sh | 4 +- t/t6428-merge-conflicts-sparse.sh | 4 +- t/t7002-mv-sparse-checkout.sh | 2 +- t/t7012-skip-worktree-writing.sh | 2 +- 9 files changed, 207 insertions(+), 86 deletions(-) base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1148%2Fnewren%2Fsparse-checkout-default-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1148/newren/sparse-checkout-default-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1148 Range-diff vs v1: 1: 8c3e730e86b = 1: 05dba7069c5 tests: stop assuming --no-cone is the default mode for sparse-checkout 2: b174b42ed82 ! 2: a53179764bc sparse-checkout: make --cone the default @@ builtin/sparse-checkout.c: static int update_modes(int *cone_mode, int *sparse_i /* Set cone/non-cone mode appropriately */ core_apply_sparse_checkout = 1; - if (*cone_mode == 1) { -+ if (*cone_mode == 1 || *cone_mode == -1) { ++ if (*cone_mode) { /* also handles "not specified" (value of -1) */ mode = MODE_CONE_PATTERNS; core_sparse_checkout_cone = 1; } else { 3: f98b3fac78a = 3: 8eab21996c7 git-sparse-checkout.txt: wording updates for the cone mode default 4: cd871966c06 = 4: eb3b318b39e git-sparse-checkout.txt: update docs for deprecation of 'init' 5: fe37a966699 = 5: 7333198b778 git-sparse-checkout.txt: shuffle some sections and mark as internal 6: 3265f41bcab ! 6: a814ea9e8d2 git-sparse-checkout.txt: add a new EXAMPLES section @@ Documentation/git-sparse-checkout.txt: paths to pass to a subsequent 'set' or 'a + +`git sparse-checkout reapply`:: + -+ It is possible for commands to update the working tree in a way -+ that does not respect the selected sparsity directories, either -+ because of special cases (such as hitting conflicts when -+ merging/rebasing), or because some commands didn't fully support -+ sparse checkouts (e.g. the old `recursive` merge backend had -+ only limited support). This command reapplies the existing -+ sparse directory specifications to make the working directory -+ match. ++ It is possible for commands to update the working tree in a ++ way that does not respect the selected sparsity directories. ++ This can come from tools external to Git writing files, or ++ even affect Git commands because of either special cases (such ++ as hitting conflicts when merging/rebasing), or because some ++ commands didn't fully support sparse checkouts (e.g. the old ++ `recursive` merge backend had only limited support). This ++ command reapplies the existing sparse directory specifications ++ to make the working directory match. + INTERNALS -- SPARSE CHECKOUT ---------------------------- 7: bdbf61ee6a0 = 7: 78028ecaa58 git-sparse-checkout.txt: flesh out non-cone mode pattern discussion a bit 8: 3e35d62c5ee = 8: 2d2b81986a5 git-sparse-checkout.txt: mark non-cone mode as deprecated 9: e7d3ff70914 = 9: 4b89a3392b0 Documentation: some sparsity wording clarifications