| Message ID | bb150483bcfd0469cd88bab735bc1178fb6628f5.1629841966.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sparse Index: Integrate with merge, cherry-pick, rebase, and revert | expand |
On Tue, Aug 24, 2021 at 2:52 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > Allow 'git merge' to operate without expanding a sparse index, at least > not immediately. The index still will be expanded in a few cases: > > 1. If the merge strategy is 'recursive', then we enable > command_requires_full_index at the start of the merge_recursive() > method. We expect sparse-index users to also have the 'ort' strategy > enabled. > > 2. With the 'ort' strategy, if the merge results in a conflicted file, > then we expand the index before updating the working tree. The loop > that iterates over the worktree replaces index entries and tracks > 'origintal_cache_nr' which can become completely wrong if the index > expands in the middle of the operation. This safety valve is > important before that loop starts. A later change will focus this > to only expand if we indeed have a conflict outside of the > sparse-checkout cone. > > 3. Other merge strategies are executed as a 'git merge-X' subcommand, > and those strategies are currently protected with the > 'command_requires_full_index' guard. Oh, indeed, it appears ag/merge-strategies-in-c didn't complete but was discarded, as per the July 6 "What's cooking in git.git" email. Well, that certainly makes things easier for you; thanks for mentioning them in this item #3. > > Some test updates are required, including a mistaken 'git checkout -b' > that did not specify the base branch, causing merges to be fast-forward > merges. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > builtin/merge.c | 3 +++ > merge-ort.c | 8 ++++++++ > merge-recursive.c | 3 +++ > t/t1092-sparse-checkout-compatibility.sh | 12 ++++++++++-- > 4 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/builtin/merge.c b/builtin/merge.c > index 22f23990b37..926de328fbb 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -1276,6 +1276,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > if (argc == 2 && !strcmp(argv[1], "-h")) > usage_with_options(builtin_merge_usage, builtin_merge_options); > > + prepare_repo_settings(the_repository); > + the_repository->settings.command_requires_full_index = 0; > + > /* > * Check if we are _not_ on a detached HEAD, i.e. if there is a > * current branch. > diff --git a/merge-ort.c b/merge-ort.c > index 6eb910d6f0c..8e754b769e1 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -4058,6 +4058,14 @@ static int record_conflicted_index_entries(struct merge_options *opt) > if (strmap_empty(&opt->priv->conflicted)) > return 0; > > + /* > + * We are in a conflicted state. These conflicts might be inside > + * sparse-directory entries, so expand the index preemtively. Same typo I pointed out in v1. > + * Also, we set original_cache_nr below, but that might change if > + * index_name_pos() calls ask for paths within sparse directories. > + */ > + ensure_full_index(index); > + > /* If any entries have skip_worktree set, we'll have to check 'em out */ > state.force = 1; > state.quiet = 1; > diff --git a/merge-recursive.c b/merge-recursive.c > index 3355d50e8ad..1f563cd6874 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -3750,6 +3750,9 @@ int merge_recursive(struct merge_options *opt, > assert(opt->ancestor == NULL || > !strcmp(opt->ancestor, "constructed merge base")); > > + prepare_repo_settings(opt->repo); > + opt->repo->settings.command_requires_full_index = 1; > + > if (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) > return -1; > clean = merge_recursive_internal(opt, h1, h2, merge_bases, result); > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index ddc86bb4152..dc56252865c 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -47,7 +47,7 @@ test_expect_success 'setup' ' > git checkout -b base && > for dir in folder1 folder2 deep > do > - git checkout -b update-$dir && > + git checkout -b update-$dir base && > echo "updated $dir" >$dir/a && > git commit -a -m "update $dir" || return 1 > done && > @@ -647,7 +647,15 @@ test_expect_success 'sparse-index is not expanded' ' > echo >>sparse-index/extra.txt && > ensure_not_expanded add extra.txt && > echo >>sparse-index/untracked.txt && > - ensure_not_expanded add . > + ensure_not_expanded add . && > + > + ensure_not_expanded checkout -f update-deep && > + ( > + sane_unset GIT_TEST_MERGE_ALGORITHM && > + git -C sparse-index config pull.twohead ort && > + ensure_not_expanded merge -m merge update-folder1 && > + ensure_not_expanded merge -m merge update-folder2 > + ) > ' Should you use test_config rather than git config here? More importantly, why the subshell and unsetting of GIT_TEST_MERGE_ALGORITHM and the special worrying about pull.twohead? Wouldn't it be simpler to just set GIT_TEST_MERGE_ALGORITHM=ort, perhaps at the beginning of the file?
On 8/27/2021 6:43 PM, Elijah Newren wrote: > On Tue, Aug 24, 2021 at 2:52 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: ... >> + /* >> + * We are in a conflicted state. These conflicts might be inside >> + * sparse-directory entries, so expand the index preemtively. > > Same typo I pointed out in v1. Sorry, this comment is edited in a later patch and I fixed it there. Will fix here, too. >> + ensure_not_expanded checkout -f update-deep && >> + ( >> + sane_unset GIT_TEST_MERGE_ALGORITHM && >> + git -C sparse-index config pull.twohead ort && >> + ensure_not_expanded merge -m merge update-folder1 && >> + ensure_not_expanded merge -m merge update-folder2 >> + ) >> ' > > Should you use test_config rather than git config here? That's a better pattern. It's not technically _required_ for these tests because the repositories are completely rewritten at the start of each new test, but it's best to be a good example. > More importantly, why the subshell and unsetting of > GIT_TEST_MERGE_ALGORITHM and the special worrying about pull.twohead? > Wouldn't it be simpler to just set GIT_TEST_MERGE_ALGORITHM=ort, > perhaps at the beginning of the file? I don't set GIT_TEST_MERGE_ALGORITHM at the beginning of the file so the rest of the tests are covered with both 'ort' and 'recursive' in the CI test suite. Using the config more carefully matches how I expect the 'ort' strategy to be specified in practice (very temporarily, as it will soon be the default). Thanks, -Stolee
On 8/30/2021 1:18 PM, Derrick Stolee wrote: > On 8/27/2021 6:43 PM, Elijah Newren wrote: >> On Tue, Aug 24, 2021 at 2:52 PM Derrick Stolee via GitGitGadget >>> + ensure_not_expanded checkout -f update-deep && >>> + ( >>> + sane_unset GIT_TEST_MERGE_ALGORITHM && >>> + git -C sparse-index config pull.twohead ort && >>> + ensure_not_expanded merge -m merge update-folder1 && >>> + ensure_not_expanded merge -m merge update-folder2 >>> + ) >>> ' >> >> Should you use test_config rather than git config here? > > That's a better pattern. It's not technically _required_ for these > tests because the repositories are completely rewritten at the start > of each new test, but it's best to be a good example. Actually, test_config runs test_when_finished, and that results in the following message and failure on macOS and Windows: BUG 'test_when_finished does nothing in a subshell' So, I'll leave this as-is. Thanks, -Stolee
diff --git a/builtin/merge.c b/builtin/merge.c index 22f23990b37..926de328fbb 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1276,6 +1276,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (argc == 2 && !strcmp(argv[1], "-h")) usage_with_options(builtin_merge_usage, builtin_merge_options); + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; + /* * Check if we are _not_ on a detached HEAD, i.e. if there is a * current branch. diff --git a/merge-ort.c b/merge-ort.c index 6eb910d6f0c..8e754b769e1 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4058,6 +4058,14 @@ static int record_conflicted_index_entries(struct merge_options *opt) if (strmap_empty(&opt->priv->conflicted)) return 0; + /* + * We are in a conflicted state. These conflicts might be inside + * sparse-directory entries, so expand the index preemtively. + * Also, we set original_cache_nr below, but that might change if + * index_name_pos() calls ask for paths within sparse directories. + */ + ensure_full_index(index); + /* If any entries have skip_worktree set, we'll have to check 'em out */ state.force = 1; state.quiet = 1; diff --git a/merge-recursive.c b/merge-recursive.c index 3355d50e8ad..1f563cd6874 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3750,6 +3750,9 @@ int merge_recursive(struct merge_options *opt, assert(opt->ancestor == NULL || !strcmp(opt->ancestor, "constructed merge base")); + prepare_repo_settings(opt->repo); + opt->repo->settings.command_requires_full_index = 1; + if (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) return -1; clean = merge_recursive_internal(opt, h1, h2, merge_bases, result); diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index ddc86bb4152..dc56252865c 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -47,7 +47,7 @@ test_expect_success 'setup' ' git checkout -b base && for dir in folder1 folder2 deep do - git checkout -b update-$dir && + git checkout -b update-$dir base && echo "updated $dir" >$dir/a && git commit -a -m "update $dir" || return 1 done && @@ -647,7 +647,15 @@ test_expect_success 'sparse-index is not expanded' ' echo >>sparse-index/extra.txt && ensure_not_expanded add extra.txt && echo >>sparse-index/untracked.txt && - ensure_not_expanded add . + ensure_not_expanded add . && + + ensure_not_expanded checkout -f update-deep && + ( + sane_unset GIT_TEST_MERGE_ALGORITHM && + git -C sparse-index config pull.twohead ort && + ensure_not_expanded merge -m merge update-folder1 && + ensure_not_expanded merge -m merge update-folder2 + ) ' # NEEDSWORK: a sparse-checkout behaves differently from a full checkout