Re: [PATCH v5 02/17] sparse-checkout: create 'init' subcommand
- From
Elijah Newren <newren@gmail.com>
- Date
- Nov 21, 2019, 17:01 UTC
- Message-ID
- <CABPp-BEVFyd9+22MhtREh57FYUJhBSj2vz3piUuk_hX8=Aq3UA@mail.gmail.com>
- In-Reply-To
- <20191121163706.GV23183@szeder.dev>
On Thu, Nov 21, 2019 at 8:37 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
Show 27 quoted lines
> > 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@gmail.com/
Show 46 quoted lines
> -- >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"));