On 10/12/2019 6:57 PM, Elijah Newren wrote:
> On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
>>
>> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>>
>> The sparse-checkout builtin used 'git read-tree -mu HEAD' to update the
>> skip-worktree bits in the index and to update the working directory.
>> This extra process is overly complex, and prone to failure. It also
>> requires that we write our changes to the sparse-checkout file before
>> trying to update the index.
>>
>> Remove this extra process call by creating a direct call to
>> unpack_trees() in the same way 'git read-tree -mu HEAD' does. In
>> adition, provide an in-memory list of patterns so we can avoid
>
> s/adition/addition/
>
>> reading from the sparse-checkout file. This allows us to test a
>> proposed change to the file before writing to it.
>>
>> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>> ---
>> builtin/read-tree.c | 2 +-
>> builtin/sparse-checkout.c | 85 +++++++++++++++++++++++++-----
>> t/t1091-sparse-checkout-builtin.sh | 17 ++++++
>> unpack-trees.c | 5 +-
>> unpack-trees.h | 3 +-
>> 5 files changed, 95 insertions(+), 17 deletions(-)
>>
>> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
>> index 69963d83dc..d7eeaa26ec 100644
>> --- a/builtin/read-tree.c
>> +++ b/builtin/read-tree.c
>> @@ -186,7 +186,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>>
>> if (opts.reset || opts.merge || opts.prefix) {
>> if (read_cache_unmerged() && (opts.prefix || opts.merge))
>> - die("You need to resolve your current index first");
>> + die(_("You need to resolve your current index first"));
>
> A good change, but isn't this unrelated to the current commit?
It's related because I'm repeating the error in the sparse-checkout builtin, but
it should be localized in both places.
>> stage = opts.merge = 1;
>> }
>> resolve_undo_clear();
>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
>> index 25786f8bb0..542d57fac6 100644
>> --- a/builtin/sparse-checkout.c
>> +++ b/builtin/sparse-checkout.c
>> @@ -7,6 +7,11 @@
>> #include "run-command.h"
>> #include "strbuf.h"
>> #include "string-list.h"
>> +#include "cache.h"
>> +#include "cache-tree.h"
>> +#include "lockfile.h"
>> +#include "resolve-undo.h"
>> +#include "unpack-trees.h"
>>
>> static char const * const builtin_sparse_checkout_usage[] = {
>> N_("git sparse-checkout [init|list|set|disable] <options>"),
>> @@ -60,18 +65,53 @@ static int sparse_checkout_list(int argc, const char **argv)
>> return 0;
>> }
>>
>> -static int update_working_directory(void)
>> +static int update_working_directory(struct pattern_list *pl)
>> {
>> - struct argv_array argv = ARGV_ARRAY_INIT;
>> int result = 0;
>> - argv_array_pushl(&argv, "read-tree", "-m", "-u", "HEAD", NULL);
>> + struct unpack_trees_options o;
>> + struct lock_file lock_file = LOCK_INIT;
>> + struct object_id oid;
>> + struct tree *tree;
>> + struct tree_desc t;
>> + struct repository *r = the_repository;
>>
>> - if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
>> - error(_("failed to update index with new sparse-checkout paths"));
>> - result = 1;
>> + if (repo_read_index_unmerged(r))
>> + die(_("You need to resolve your current index first"));
>
> Well, at least that ensures that the user gets a good error message.
> I'm not sure I like the error, because e.g. if a user hits a conflict
> while merging in a sparse checkout and wants to return to a non-sparse
> checkout because they think other files might help them resolve the
> conflicts, then they ought to be able to do it. Basically, unless
> they are trying use sparsification to remove entries from the working
> directory that differ from the index (and conflicted entries always
> differ), then it seems like we should be able to support
> sparsification despite the presence of conflicts.
>
> Your series is long enough, doesn't make this problem any worse (and
> appears to make it slightly better), and so you really don't need to
> tackle that problem in this series. I'm just stating a gripe with
> sparse checkouts again. :-)
Absolutely, we should revisit the entire feature and how it handles these
conflicts in the best possible ways. As far as I can see, the only way these
conflicts arise is if the user creates conflicting files _outside_ their
sparse cone and then expand their cone. Finding all the strange cases
will require experimentation.
> [...]
>
>> static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *path)
>> {
>> - struct pattern_entry *e = xmalloc(sizeof(struct pattern_entry));
>> + struct pattern_entry *e = xmalloc(sizeof(*e));
>
> This is a good fix, but shouldn't it be squashed into the
> "sparse-checkout: init and set in cone mode" commit from earlier in
> your series?
Yeah, I think I mis-applied a few fixups to this commit instead of an earlier one.
>> @@ -262,12 +308,21 @@ static int write_patterns_and_update(struct pattern_list *pl)
>> {
>> char *sparse_filename;
>> FILE *fp;
>> -
>> + int result;
>> +
>
> Trailing whitespace that should be cleaned up.
Thanks. Will do.
>
>> if (!core_apply_sparse_checkout) {
>> warning(_("core.sparseCheckout is disabled, so changes to the sparse-checkout file will have no effect"));
>> warning(_("run 'git sparse-checkout init' to enable the sparse-checkout feature"));
>> }
>>
>> + result = update_working_directory(pl);
>> +
>> + if (result) {
>> + clear_pattern_list(pl);
>> + update_working_directory(NULL);
>> + return result;
>> + }
>> +
>> sparse_filename = get_sparse_checkout_filename();
>> fp = fopen(sparse_filename, "w");
>>
>> @@ -277,9 +332,11 @@ static int write_patterns_and_update(struct pattern_list *pl)
>> write_patterns_to_file(fp, pl);
>>
>> fclose(fp);
>> +
>> free(sparse_filename);
>> + clear_pattern_list(pl);
>>
>> - return update_working_directory();
>> + return 0;
>> }
>>
>> static void strbuf_to_cone_pattern(struct strbuf *line, struct pattern_list *pl)
>> @@ -330,6 +387,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>> struct strbuf line = STRBUF_INIT;
>> hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
>> hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
>> + pl.use_cone_patterns = 1;
>>
>> if (set_opts.use_stdin) {
>> while (!strbuf_getline(&line, stdin))
>> @@ -375,7 +433,8 @@ static int sparse_checkout_disable(int argc, const char **argv)
>> fprintf(fp, "/*\n");
>> fclose(fp);
>>
>> - if (update_working_directory())
>> + core_apply_sparse_checkout = 1;
>> + if (update_working_directory(NULL))
>> die(_("error while refreshing working directory"));
>>
>> unlink(sparse_filename);
>> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
>> index ee4d361787..82eb5fb2f8 100755
>> --- a/t/t1091-sparse-checkout-builtin.sh
>> +++ b/t/t1091-sparse-checkout-builtin.sh
>> @@ -199,11 +199,13 @@ test_expect_success 'cone mode: init and set' '
>> a
>> deep
>> EOF
>> + test_cmp dir expect &&
>> ls repo/deep >dir &&
>> cat >expect <<-EOF &&
>> a
>> deeper1
>> EOF
>> + test_cmp dir expect &&
>> ls repo/deep/deeper1 >dir &&
>> cat >expect <<-EOF &&
>> a
>> @@ -245,4 +247,19 @@ test_expect_success 'cone mode: set with nested folders' '
>> test_cmp repo/.git/info/sparse-checkout expect
>> '
>>
>> +test_expect_success 'revert to old sparse-checkout on bad update' '
>> + echo update >repo/deep/deeper2/a &&
>> + cp repo/.git/info/sparse-checkout expect &&
>> + test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
>> + test_i18ngrep "Cannot update sparse checkout" err &&
>> + test_cmp repo/.git/info/sparse-checkout expect &&
>> + ls repo/deep >dir &&
>> + cat >expect <<-EOF &&
>> + a
>> + deeper1
>> + deeper2
>> + EOF
>> + test_cmp dir expect
>> +'
>> +
>> test_done
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index edf0fb4673..f0fee5adf2 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -1508,7 +1508,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>> memset(&pl, 0, sizeof(pl));
>> if (!core_apply_sparse_checkout || !o->update)
>> o->skip_sparse_checkout = 1;
>> - if (!o->skip_sparse_checkout) {
>> + if (!o->skip_sparse_checkout && !o->pl) {
>> char *sparse = git_pathdup("info/sparse-checkout");
>> pl.use_cone_patterns = core_sparse_checkout_cone;
>> if (add_patterns_from_file_to_list(sparse, "", 0, &pl, NULL) < 0)
>> @@ -1681,7 +1681,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>>
>> done:
>> trace_performance_leave("unpack_trees");
>> - clear_pattern_list(&pl);
>> + if (!o->keep_pattern_list)
>> + clear_pattern_list(&pl);
>> return ret;
>>
>> return_failed:
>> diff --git a/unpack-trees.h b/unpack-trees.h
>> index f2eee0c7c5..ca94a421a5 100644
>> --- a/unpack-trees.h
>> +++ b/unpack-trees.h
>> @@ -59,7 +59,8 @@ struct unpack_trees_options {
>> quiet,
>> exiting_early,
>> show_all_errors,
>> - dry_run;
>> + dry_run,
>> + keep_pattern_list;
>> const char *prefix;
>> int cache_bottom;
>> struct dir_struct *dir;
>> --
>
> The rest looks reasonable.
>
