On Thu, Nov 21, 2019 at 8:37 AM SZEDER Gábor <szeder.dev@xxxxxxxxx> wrote: > > On Thu, Nov 21, 2019 at 10:36:10AM -0500, Derrick Stolee wrote: > > > But wait, I thought that only changes to files that are excluded from > > > the sparse-checkout are thrown away, but as it turns out it throws > > > away changes to files that are included in the sparse-checkout: > > For completeness, 'git sparse-checkout disable' throws away staged > changes as well, as it, too, runs 'git read-tree -um HEAD' (or its > equivalent). > > > Thanks for the additional details. > > > > This series intended to make the existing sparse-checkout behavior > > more useful to users by not requiring manual edits of the sparse-checkout > > file followed by 'git read-tree' commands. However, there do appear > > to be some serious improvements that we can make in the future. > > > > Keeping staged changes seems important, and we can address that in > > the near future. > > I think that at least for now 'git sparse-checkout' should flat out > refuse to init/set/disable if the working tree is not clean (but still > allow 'list', as that's a read-only operation), like the patch below. > Yeah, that way it wouldn't work in cases that appear to be safe > (unstaged changes), but it would prevent the data loss until we > carefully consider the circumstances under which these operations can > be safely allowed. A big +1 for this from me. We had an unfortunately large number of mis-merging and dataloss bugs in the merge machinery that were slowly fixed over the course of more than a decade[1], due to the fact that builtin/merge required index==HEAD and did so by placing a comment in the code notifying folks that the individual merge strategies were responsible to enforce it -- and, in practice, they *all* forgot to do so unless and until we discovered bugs. So, count me as a strongly in favor of just preventatively enforcing safe conditions and then later relaxing them in special conditions if it can be proven safe. [1] https://public-inbox.org/git/20190725174611.14802-4-newren@xxxxxxxxx/ > -- >8 -- > > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c > index 76f65d8edd..4b05625c4c 100644 > --- a/builtin/sparse-checkout.c > +++ b/builtin/sparse-checkout.c > @@ -12,6 +12,7 @@ > #include "lockfile.h" > #include "resolve-undo.h" > #include "unpack-trees.h" > +#include "wt-status.h" > > static char const * const builtin_sparse_checkout_usage[] = { > N_("git sparse-checkout (init|list|set|disable) <options>"), > @@ -188,6 +189,10 @@ static int sparse_checkout_init(int argc, const char **argv) > builtin_sparse_checkout_init_options, > builtin_sparse_checkout_init_usage, 0); > > + repo_read_index(the_repository); > + require_clean_work_tree(the_repository, > + N_("initialize sparse-checkout"), NULL, 1, 0); > + > if (init_opts.cone_mode) { > mode = MODE_CONE_PATTERNS; > core_sparse_checkout_cone = 1; > @@ -386,6 +391,10 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix) > builtin_sparse_checkout_set_usage, > PARSE_OPT_KEEP_UNKNOWN); > > + repo_read_index(the_repository); > + require_clean_work_tree(the_repository, > + N_("set up sparse-checkout"), NULL, 1, 0); > + > if (core_sparse_checkout_cone) { > struct strbuf line = STRBUF_INIT; > hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0); > @@ -437,6 +446,10 @@ static int sparse_checkout_disable(int argc, const char **argv) > char *sparse_filename; > FILE *fp; > > + repo_read_index(the_repository); > + require_clean_work_tree(the_repository, > + N_("disable sparse-checkout"), NULL, 1, 0); > + > if (sc_set_config(MODE_ALL_PATTERNS)) > die(_("failed to change config"));
