{"thread":{"id":"57792","subject":"[PATCH 0/7] Sparse index: integrate with 'git stash'","startedAt":"2022-04-25T17:49:26Z","lastAt":"2022-05-12T16:56:42Z","messageCount":42,"participants":["Victoria Dye via GitGitGadget","Junio C Hamano","Derrick Stolee","Victoria Dye","Elijah Newren","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"454391","messageId":"pull.1171.git.1650908957.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":null,"subject":"[PATCH 0/7] Sparse index: integrate with 'git stash'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T17:49:10Z","receivedAt":"2022-04-25T17:49:26Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"This series, in combination with the sparse index integrations of reset [1],\nupdate-index [2], checkout-index [2], clean [2], and read-tree [3], allows\nmost subcommands of 'git stash' to use the sparse index end-to-end without\nindex expansion.\n\nLike the earlier series, this series starts with new tests ensuring\ncompatibility of the sparse index with non-sparse index full and sparse\ncheckouts [1/7]. Next, sparse index is trivially enabled [2/7].\nFunctionally, sparse index-enabled sparse-checkouts remain compatible with\nnon-sparse index sparse-checkouts, but there are still some cases where the\nindex (or a temporary index) is expanded unnecessarily. These cases are\nfixed in three parts:\n\n * First, 'git stash -u' is made sparse index-compatible by ensuring the\n \"temporary\" index holding the stashed, untracked files is created as a\n sparse index whenever possible (per repo settings &\n 'is_sparse_index_allowed()'). Patch [3/7] exposes\n 'is_sparse_index_allowed()' to files outside of 'sparse-index.c', then\n patch [4/7] uses that function to mark the temporary index sparse when\n appropriate.\n * Next, 'git stash (apply|pop)' are made sparse index-compatible by\n changing their internal \"merge\" function (executed via\n 'merge_recursive_generic()') from 'merge_recursive()' to\n 'merge_ort_recursive()'. This requires first allowing\n 'merge_recursive_generic()' to accept a merge function as an input\n (rather than hardcoding use of 'merge_recursive()') in patch [5/7], then\n changing the call in 'stash.c' to specify 'merge_ort_recursive()' in\n patch [6/7]. See note [4] for possible alternate implementations.\n * Finally, while patches 5 & 6 avoid index expansion for most cases of 'git\n stash (apply|pop)', applying a stash that includes untracked files still\n expands the index. This is a result of an internal 'read-tree' execution\n (specifically in its 'unpack_trees' call) creating a result index that is\n never sparse in-core, thus forcing the index to be unnecessarily\n collapsed and re-expanded in 'do_write_locked_index()'. In patch [7/7],\n 'unpack_trees' is updated to set the default sparsity of the resultant\n index to \"sparse\" if allowed by repo settings and\n 'is_sparse_index_allowed()' (similar to the change in patch 4).\n\nPerformance results (from the 'p2000' tests):\n\n(git stash &&\n git stash pop) master this series\n---------------------------------------------------------------------\nfull-v3 4.07(2.42+1.34) 3.98(2.42+1.32) -2.2%\nfull-v4 4.05(2.46+1.31) 4.00(2.49+1.29) -1.2%\nsparse-v3 7.48(4.81+2.57) 1.53(0.26+1.61) -79.5%\nsparse-v4 7.35(4.74+2.54) 1.59(0.27+1.63) -78.4%\n\n(echo >>new &&\n git stash -u &&\n git stash pop) master this series\n---------------------------------------------------------------------\nfull-v3 4.21(2.62+1.45) 4.11(2.55+1.44) -2.4%\nfull-v4 4.11(2.51+1.41) 4.02(2.49+1.41) -2.2%\nsparse-v3 7.35(4.64+2.66) 1.70(0.32+1.64) -76.9%\nsparse-v4 7.74(4.87+2.83) 1.70(0.32+1.66) -78.0%\n\n\n[1]\nhttps://lore.kernel.org/git/pull.1048.v6.git.1638201164.gitgitgadget@gmail.com/\n[2]\nhttps://lore.kernel.org/git/pull.1109.v2.git.1641924306.gitgitgadget@gmail.com/\n[3]\nhttps://lore.kernel.org/git/pull.1157.v3.git.1646166271.gitgitgadget@gmail.com/\n[4] I went with changing 'stash' to always use 'merge-ort' in\n'merge_recursive_generic()' as a sort of \"middle ground\" between \"replace\n'merge_recursive()' with 'merge_ort_recursive()' in all of its hardcoded\ninternal usage\" and \"only use 'merge-ort' if using a sparse index in 'git\nstash', otherwise 'merge-recursive'\". The former would extend the use of\n'merge-ort' to 'git am' and 'git merge-recursive', whereas the latter is a\nmore cautious/narrowly-focused option. If anyone has any other thoughts, I'm\ninterested in hearing them.\n\nThanks!\n\n-Victoria\n\nVictoria Dye (7):\n stash: expand sparse-checkout compatibility testing\n stash: integrate with sparse index\n sparse-index: expose 'is_sparse_index_allowed()'\n read-cache: set sparsity when index is new\n merge-recursive: add merge function arg to 'merge_recursive_generic'\n stash: merge applied stash with merge-ort\n unpack-trees: preserve index sparsity\n\n builtin/am.c | 2 +-\n builtin/merge-recursive.c | 2 +-\n builtin/stash.c | 6 +-\n merge-ort.c | 3 +-\n merge-recursive.c | 4 +-\n merge-recursive.h | 9 ++-\n read-cache.c | 18 ++++-\n sparse-index.c | 2 +-\n sparse-index.h | 1 +\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 94 +++++++++++++++++++++---\n unpack-trees.c | 6 ++\n 12 files changed, 131 insertions(+), 18 deletions(-)\n\n\nbase-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1171%2Fvdye%2Fsparse%2Fstash-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1171/vdye/sparse/stash-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1171\n-- \ngitgitgadget\n"},{"id":"454392","messageId":"994864852a020d4de5bbe9914908b84377acaecc.1650908958.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.git.1650908957.gitgitgadget@gmail.com","subject":"[PATCH 1/7] stash: expand sparse-checkout compatibility testing","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T17:49:11Z","receivedAt":"2022-04-25T17:49:27Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd tests verifying expected 'git stash' behavior in\n't1092-sparse-checkout-compatibility'. These cases establish the expected\nbehavior of 'git stash' in a sparse-checkout and verify consistency both\nwith and without a sparse index. Although no sparse index compatibility has\nbeen integrated into 'git stash' yet, the tests are all 'expect_success' -\nwe don't want the cone-mode sparse-checkout behavior to change depending on\nwhether it is using a sparse index or not. Therefore, we expect these tests\nto continue passing once sparse index is integrated with 'git stash'.\n\nAdditionally, add performance test cases for 'git stash' both with and\nwithout untracked files. Note that, unlike the other tests in\n'p2000-sparse-operations.sh', the tests added for 'stash' are combination\noperations. This is done to ensure the stash/unstash is not blocked by the\nmodification of '$SPARSE_CONE/a' performed as part of 'test_perf_on_all'.\n\nSigned-off-by: Victoria Dye \n---\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 49 ++++++++++++++++++++++++\n 2 files changed, 51 insertions(+)\n\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 382716cfca9..76710cbef35 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -106,6 +106,8 @@ test_perf_on_all () {\n }\n \n test_perf_on_all git status\n+test_perf_on_all 'git stash && git stash pop'\n+test_perf_on_all 'echo >>new && git stash -u && git stash pop'\n test_perf_on_all git add -A\n test_perf_on_all git add .\n test_perf_on_all git commit -a -m A\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 236ab530284..baf95906729 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1151,6 +1151,55 @@ test_expect_success 'clean' '\n \ttest_sparse_match test_path_is_dir folder1\n '\n \n+test_expect_success 'stash' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\t# Stash a sparse directory (folder1)\n+\ttest_all_match git checkout -b test-branch rename-base &&\n+\ttest_all_match git reset --soft rename-out-to-out &&\n+\ttest_all_match git stash &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Apply the sparse directory stash without reinstating the index\n+\ttest_all_match git stash apply -q &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Reset to state where stash can be applied\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git reset --hard rename-out-to-out &&\n+\n+\t# Apply the sparse directory stash *with* reinstating the index\n+\ttest_all_match git stash apply --index -q &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Reset to state where we will get a conflict applying the stash\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git reset --hard update-folder1 &&\n+\n+\t# Apply the sparse directory stash with conflicts\n+\ttest_all_match test_must_fail git stash apply --index -q &&\n+\ttest_all_match test_must_fail git stash apply -q &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Reset to base branch\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git reset --hard base &&\n+\n+\t# Stash & unstash an untracked file outside of the sparse checkout\n+\t# definition.\n+\trun_on_sparse mkdir -p folder1 &&\n+\trun_on_all ../edit-contents folder1/new &&\n+\ttest_all_match git stash -u &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git stash pop -q &&\n+\ttest_all_match git status --porcelain=v2\n+'\n+\n test_expect_success 'submodule handling' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"454393","messageId":"27d9920366f35f06ab2572a894acb6d2a3cffcd0.1650908958.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.git.1650908957.gitgitgadget@gmail.com","subject":"[PATCH 3/7] sparse-index: expose 'is_sparse_index_allowed()'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T17:49:13Z","receivedAt":"2022-04-25T17:49:47Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nExpose 'is_sparse_index_allowed()' publicly so that it may be used by\ncallers outside of 'sparse-index.c'. While no such callers exist yet, it\nwill be used in a subsequent commit.\n\nSigned-off-by: Victoria Dye \n---\n sparse-index.c | 2 +-\n sparse-index.h | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 8636af72de5..ffbab7d35f1 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -118,7 +118,7 @@ static int index_has_unmerged_entries(struct index_state *istate)\n \treturn 0;\n }\n \n-static int is_sparse_index_allowed(struct index_state *istate, int flags)\n+int is_sparse_index_allowed(struct index_state *istate, int flags)\n {\n \tif (!core_apply_sparse_checkout || !core_sparse_checkout_cone)\n \t\treturn 0;\ndiff --git a/sparse-index.h b/sparse-index.h\nindex 633d4fb7e31..f57c65d972f 100644\n--- a/sparse-index.h\n+++ b/sparse-index.h\n@@ -3,6 +3,7 @@\n \n struct index_state;\n #define SPARSE_INDEX_MEMORY_ONLY (1 << 0)\n+int is_sparse_index_allowed(struct index_state *istate, int flags);\n int convert_to_sparse(struct index_state *istate, int flags);\n void ensure_correct_sparsity(struct index_state *istate);\n void clear_skip_worktree_from_present_files(struct index_state *istate);\n-- \ngitgitgadget\n\n"},{"id":"454394","messageId":"1daecbe45c1c3026b9eeb03d55a590c3c457a619.1650908958.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.git.1650908957.gitgitgadget@gmail.com","subject":"[PATCH 7/7] unpack-trees: preserve index sparsity","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T17:49:17Z","receivedAt":"2022-04-25T17:49:48Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen unpacking trees, set the default sparsity of the resultant index based\non repo settings and 'is_sparse_index_allowed()'.\n\nNormally, when executing 'unpack_trees', the output index is marked sparse\nwhen (and only when) it unpacks a sparse directory. However, an index may be\n\"sparse\" even if it contains no sparse directories - when all files fall\ninside the sparse-checkout definition or otherwise have SKIP_WORKTREE\ndisabled. Therefore, the output index may be marked \"full\" even when it is\n\"sparse\", resulting in unnecessary 'ensure_full_index' calls when writing to\ndisk. Avoid this by setting the \"default\" index sparsity to match what is\nexpected for the repository.\n\nAs a consequence of this fix, the (non-merge) 'read-tree' performed when\napplying a stash with untracked files no longer expands the index. Update\nthe corresponding test in 't1092'.\n\nSigned-off-by: Victoria Dye \n---\n t/t1092-sparse-checkout-compatibility.sh | 2 +-\n unpack-trees.c | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 8545a865e04..8443c7e65ae 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1384,7 +1384,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \tensure_not_expanded stash -u &&\n \t(\n \t\tWITHOUT_UNTRACKED_TXT=1 &&\n-\t\t! ensure_not_expanded stash pop\n+\t\tensure_not_expanded stash pop\n \t) &&\n \n \tensure_not_expanded stash create &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7f528d35cc2..a1d0ff3a4d3 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -11,6 +11,7 @@\n #include \"refs.h\"\n #include \"attr.h\"\n #include \"split-index.h\"\n+#include \"sparse-index.h\"\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n #include \"fsmonitor.h\"\n@@ -1839,6 +1840,11 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->result.fsmonitor_last_update =\n \t\txstrdup_or_null(o->src_index->fsmonitor_last_update);\n \n+\tif (!o->src_index->initialized &&\n+\t !repo->settings.command_requires_full_index &&\n+\t is_sparse_index_allowed(&o->result, 0))\n+\t\to->result.sparse_index = 1;\n+\n \t/*\n \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n \t */\n-- \ngitgitgadget\n"},{"id":"454395","messageId":"76f2a9e87223e1c1ebf2de6629c46bdf7ad326f1.1650908958.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.git.1650908957.gitgitgadget@gmail.com","subject":"[PATCH 6/7] stash: merge applied stash with merge-ort","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T17:49:16Z","receivedAt":"2022-04-25T17:49:49Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nChange the merge function used in 'do_apply_stash()' from 'merge_recursive'\nto 'merge_ort_recursive'. In addition to aligning with the default merge\nstrategy used by 'git merge' (6a5fb96672 (Change default merge backend from\nrecursive to ort, 2021-08-04)), this allows 'git stash ' to\noperate without expanding the index by default. Update tests in 't1092'\nverifying index expansion for 'git stash' accordingly.\n\nSigned-off-by: Victoria Dye \n---\n builtin/stash.c | 3 ++-\n t/t1092-sparse-checkout-compatibility.sh | 4 ++--\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 16171eb1dab..cd77d448546 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -7,6 +7,7 @@\n #include \"cache-tree.h\"\n #include \"unpack-trees.h\"\n #include \"merge-recursive.h\"\n+#include \"merge-ort-wrappers.h\"\n #include \"strvec.h\"\n #include \"run-command.h\"\n #include \"dir.h\"\n@@ -554,7 +555,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tbases[0] = &info->b_tree;\n \n \tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n-\t\t\t\t merge_recursive, &result);\n+\t\t\t\t merge_ort_recursive, &result);\n \tif (ret) {\n \t\trerere(0);\n \ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex a8c1c345ab0..8545a865e04 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1377,7 +1377,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \tensure_not_expanded stash &&\n \tensure_not_expanded stash list &&\n \tensure_not_expanded stash show stash@{0} &&\n-\t! ensure_not_expanded stash apply stash@{0} &&\n+\tensure_not_expanded stash apply stash@{0} &&\n \tensure_not_expanded stash drop stash@{0} &&\n \n \techo >>sparse-index/deep/new &&\n@@ -1391,7 +1391,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \toid=$(git -C sparse-index stash create) &&\n \tensure_not_expanded stash store -m \"test\" $oid &&\n \tensure_not_expanded reset --hard &&\n-\t! ensure_not_expanded stash pop &&\n+\tensure_not_expanded stash pop &&\n \n \tensure_not_expanded checkout-index -f a &&\n \tensure_not_expanded checkout-index -f --all &&\n-- \ngitgitgadget\n\n"},{"id":"454396","messageId":"64edbed0f95623e307371cfc3905786e5b0a02f3.1650908958.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.git.1650908957.gitgitgadget@gmail.com","subject":"[PATCH 4/7] read-cache: set sparsity when index is new","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T17:49:14Z","receivedAt":"2022-04-25T17:49:50Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen the index read in 'do_read_index()' does not exist on-disk, mark the\nindex \"sparse\" if the executing command does not require a full index and\nsparse index is otherwise enabled.\n\nSome commands (such as 'git stash -u') implicitly create a new index (when\nthe 'GIT_INDEX_FILE' variable points to a non-existent file) and perform\nsome operation on it. However, when this index is created, it isn't created\nwith the same sparsity settings as the repo index. As a result, while these\nindexes may be sparse during the operation, they are always expanded before\nbeing written to disk. We can avoid that expansion by defaulting the index\nto \"sparse\", in which case it will only be expanded if the full index is\nneeded.\n\nNote that the function 'set_new_index_sparsity()' is created despite having\nonly a single caller because additional callers will be added in a\nsubsequent patch.\n\nSigned-off-by: Victoria Dye \n---\n read-cache.c | 18 +++++++++++++++++-\n t/t1092-sparse-checkout-compatibility.sh | 2 +-\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 4df97e185e9..60355f5ad6a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2260,6 +2260,20 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con\n \treturn consumed;\n }\n \n+static void set_new_index_sparsity(struct index_state *istate)\n+{\n+\t/*\n+\t * If the index's repo exists, mark it sparse according to\n+\t * repo settings.\n+\t */\n+\tif (istate->repo) {\n+\t\tprepare_repo_settings(istate->repo);\n+\t\tif (!istate->repo->settings.command_requires_full_index &&\n+\t\t is_sparse_index_allowed(istate, 0))\n+\t\t\tistate->sparse_index = 1;\n+\t}\n+}\n+\n /* remember to discard_cache() before reading a different cache! */\n int do_read_index(struct index_state *istate, const char *path, int must_exist)\n {\n@@ -2281,8 +2295,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \tistate->timestamp.nsec = 0;\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n-\t\tif (!must_exist && errno == ENOENT)\n+\t\tif (!must_exist && errno == ENOENT) {\n+\t\t\tset_new_index_sparsity(istate);\n \t\t\treturn 0;\n+\t\t}\n \t\tdie_errno(_(\"%s: index file open failed\"), path);\n \t}\n \ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex b00c65c7770..a8c1c345ab0 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1381,7 +1381,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \tensure_not_expanded stash drop stash@{0} &&\n \n \techo >>sparse-index/deep/new &&\n-\t! ensure_not_expanded stash -u &&\n+\tensure_not_expanded stash -u &&\n \t(\n \t\tWITHOUT_UNTRACKED_TXT=1 &&\n \t\t! ensure_not_expanded stash pop\n-- \ngitgitgadget\n\n"},{"id":"454397","messageId":"f6cf05a5bee9e4ebc174bab0385a13cc1cdb4014.1650908958.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.git.1650908957.gitgitgadget@gmail.com","subject":"[PATCH 2/7] stash: integrate with sparse index","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T17:49:12Z","receivedAt":"2022-04-25T17:49:53Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nEnable sparse index in 'git stash' by disabling\n'command_requires_full_index'.\n\nWith sparse index enabled, some subcommands of 'stash' work without\nexpanding the index, e.g., 'git stash', 'git stash list', 'git stash drop',\netc. Others ensure the index is expanded either directly (as in the case of\n'git stash [pop|apply]', where the call to 'merge_recursive_generic()' in\n'do_apply_stash()' triggers the expansion), or in a command called\ninternally by stash (e.g., 'git update-index' in 'git stash -u'). So, in\naddition to enabling sparse index, add tests to 't1092' demonstrating which\nvariants of 'git stash' expand the index, and which do not.\n\nFinally, add the option to skip writing 'untracked.txt' in\n'ensure_not_expanded', and use that option to successfully apply stashed\nuntracked files without a conflict in 'untracked.txt'.\n\nSigned-off-by: Victoria Dye \n---\n builtin/stash.c | 3 ++\n t/t1092-sparse-checkout-compatibility.sh | 45 +++++++++++++++++++-----\n 2 files changed, 39 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 0c7b6a95882..1bfba532044 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1770,6 +1770,9 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, git_stash_usage,\n \t\t\t PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \tindex_file = get_index_file();\n \tstrbuf_addf(&stash_index_path, \"%s.stash.%\" PRIuMAX, index_file,\n \t\t (uintmax_t)pid);\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex baf95906729..b00c65c7770 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1271,7 +1271,10 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n \n ensure_not_expanded () {\n \trm -f trace2.txt &&\n-\techo >>sparse-index/untracked.txt &&\n+\tif test -z $WITHOUT_UNTRACKED_TXT\n+\tthen\n+\t\techo >>sparse-index/untracked.txt\n+\tfi &&\n \n \tif test \"$1\" = \"!\"\n \tthen\n@@ -1314,14 +1317,6 @@ test_expect_success 'sparse-index is not expanded' '\n \techo >>sparse-index/untracked.txt &&\n \tensure_not_expanded add . &&\n \n-\tensure_not_expanded checkout-index -f a &&\n-\tensure_not_expanded checkout-index -f --all &&\n-\tfor ref in update-deep update-folder1 update-folder2 update-deep\n-\tdo\n-\t\techo >>sparse-index/README.md &&\n-\t\tensure_not_expanded reset --hard $ref || return 1\n-\tdone &&\n-\n \tensure_not_expanded reset --mixed base &&\n \tensure_not_expanded reset --hard update-deep &&\n \tensure_not_expanded reset --keep base &&\n@@ -1375,6 +1370,38 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' '\n \t)\n '\n \n+test_expect_success 'sparse-index is not expanded: stash' '\n+\tinit_repos &&\n+\n+\techo >>sparse-index/a &&\n+\tensure_not_expanded stash &&\n+\tensure_not_expanded stash list &&\n+\tensure_not_expanded stash show stash@{0} &&\n+\t! ensure_not_expanded stash apply stash@{0} &&\n+\tensure_not_expanded stash drop stash@{0} &&\n+\n+\techo >>sparse-index/deep/new &&\n+\t! ensure_not_expanded stash -u &&\n+\t(\n+\t\tWITHOUT_UNTRACKED_TXT=1 &&\n+\t\t! ensure_not_expanded stash pop\n+\t) &&\n+\n+\tensure_not_expanded stash create &&\n+\toid=$(git -C sparse-index stash create) &&\n+\tensure_not_expanded stash store -m \"test\" $oid &&\n+\tensure_not_expanded reset --hard &&\n+\t! ensure_not_expanded stash pop &&\n+\n+\tensure_not_expanded checkout-index -f a &&\n+\tensure_not_expanded checkout-index -f --all &&\n+\tfor ref in update-deep update-folder1 update-folder2 update-deep\n+\tdo\n+\t\techo >>sparse-index/README.md &&\n+\t\tensure_not_expanded reset --hard $ref || return 1\n+\tdone\n+'\n+\n test_expect_success 'sparse index is not expanded: diff' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"454398","messageId":"80c25c7587484ca32afcfd6cc054eab78dd1348a.1650908958.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.git.1650908957.gitgitgadget@gmail.com","subject":"[PATCH 5/7] merge-recursive: add merge function arg to 'merge_recursive_generic'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T17:49:15Z","receivedAt":"2022-04-25T17:49:55Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nReplace the hardcoded 'merge_recursive()' function used by the\n'merge_recursive_generic()' with a caller-specific merge function. This will\nallow us to use 'merge_ort_recursive()' (and therefore avoid the index\nexpansion of 'merge_recursive()') in commands that perform merges with\n'merge_recursive_generic()', such as 'git stash pop'.\n\nNote that this patch is strictly a refactor; all callers still use\n'merge_recursive()', and any changing to 'merge_ort_recursive()' will be\ndone in a later commit.\n\nSigned-off-by: Victoria Dye \n---\n builtin/am.c | 2 +-\n builtin/merge-recursive.c | 2 +-\n builtin/stash.c | 2 +-\n merge-ort.c | 3 ++-\n merge-recursive.c | 4 ++--\n merge-recursive.h | 9 ++++++++-\n 6 files changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0f4111bafa0..6d01185d122 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1614,7 +1614,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \tif (state->quiet)\n \t\to.verbosity = 0;\n \n-\tif (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {\n+\tif (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, merge_recursive, &result)) {\n \t\trepo_rerere(the_repository, state->allow_rerere_autoupdate);\n \t\tfree(their_tree_name);\n \t\treturn error(_(\"Failed to merge in the changes.\"));\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex b9acbf5d342..687ed1e527b 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -81,7 +81,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n \tif (o.verbosity >= 3)\n \t\tprintf(_(\"Merging %s with %s\\n\"), o.branch1, o.branch2);\n \n-\tfailed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, &result);\n+\tfailed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, merge_recursive, &result);\n \n \tfree(better1);\n \tfree(better2);\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 1bfba532044..16171eb1dab 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -554,7 +554,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tbases[0] = &info->b_tree;\n \n \tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n-\t\t\t\t &result);\n+\t\t\t\t merge_recursive, &result);\n \tif (ret) {\n \t\trerere(0);\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8545354dafd..4bccdfcf355 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4737,7 +4737,8 @@ void merge_incore_recursive(struct merge_options *opt,\n \ttrace2_region_enter(\"merge\", \"incore_recursive\", opt->repo);\n \n \t/* We set the ancestor label based on the merge_bases */\n-\tassert(opt->ancestor == NULL);\n+\tassert(opt->ancestor == NULL ||\n+\t !strcmp(opt->ancestor, \"constructed merge base\"));\n \n \ttrace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n \tmerge_start(opt, result);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1ee6364e8b1..2088f5c5fb3 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3806,6 +3806,7 @@ int merge_recursive_generic(struct merge_options *opt,\n \t\t\t const struct object_id *merge,\n \t\t\t int num_merge_bases,\n \t\t\t const struct object_id **merge_bases,\n+\t\t\t recursive_merge_fn_t merge_fn,\n \t\t\t struct commit **result)\n {\n \tint clean;\n@@ -3829,8 +3830,7 @@ int merge_recursive_generic(struct merge_options *opt,\n \t}\n \n \trepo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);\n-\tclean = merge_recursive(opt, head_commit, next_commit, ca,\n-\t\t\t\tresult);\n+\tclean = merge_fn(opt, head_commit, next_commit, ca, result);\n \tif (clean < 0) {\n \t\trollback_lock_file(&lock);\n \t\treturn clean;\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex b88000e3c25..6a21f2da538 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -53,6 +53,12 @@ struct merge_options {\n \tstruct merge_options_internal *priv;\n };\n \n+typedef int (*recursive_merge_fn_t)(struct merge_options *opt,\n+\t\t\t\t struct commit *h1,\n+\t\t\t\t struct commit *h2,\n+\t\t\t\t struct commit_list *merge_bases,\n+\t\t\t\t struct commit **result);\n+\n void init_merge_options(struct merge_options *opt, struct repository *repo);\n \n /* parse the option in s and update the relevant field of opt */\n@@ -105,7 +111,7 @@ int merge_recursive(struct merge_options *opt,\n \n /*\n * merge_recursive_generic can operate on trees instead of commits, by\n- * wrapping the trees into virtual commits, and calling merge_recursive().\n+ * wrapping the trees into virtual commits, and calling the provided merge_fn.\n * It also writes out the in-memory index to disk if the merge is successful.\n *\n * Outputs:\n@@ -120,6 +126,7 @@ int merge_recursive_generic(struct merge_options *opt,\n \t\t\t const struct object_id *merge,\n \t\t\t int num_merge_bases,\n \t\t\t const struct object_id **merge_bases,\n+\t\t\t recursive_merge_fn_t merge_fn,\n \t\t\t struct commit **result);\n \n #endif\n-- \ngitgitgadget\n\n"},{"id":"454411","messageId":"xmqq1qxkvoav.fsf@gitster.g","threadId":"57792","inReplyTo":"64edbed0f95623e307371cfc3905786e5b0a02f3.1650908958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/7] read-cache: set sparsity when index is new","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-25T21:35:52Z","receivedAt":"2022-04-25T22:21:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> When the index read in 'do_read_index()' does not exist on-disk, mark the\n> index \"sparse\" if the executing command does not require a full index and\n> sparse index is otherwise enabled.\n>\n> Some commands (such as 'git stash -u') implicitly create a new index (when\n> the 'GIT_INDEX_FILE' variable points to a non-existent file) and perform\n> some operation on it. However, when this index is created, it isn't created\n> with the same sparsity settings as the repo index. As a result, while these\n> indexes may be sparse during the operation, they are always expanded before\n> being written to disk. We can avoid that expansion by defaulting the index\n> to \"sparse\", in which case it will only be expanded if the full index is\n> needed.\n\nMakes sense.\n"},{"id":"454412","messageId":"xmqq5ymwvod1.fsf@gitster.g","threadId":"57792","inReplyTo":"f6cf05a5bee9e4ebc174bab0385a13cc1cdb4014.1650908958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/7] stash: integrate with sparse index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-25T21:34:34Z","receivedAt":"2022-04-25T22:21:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> Enable sparse index in 'git stash' by disabling\n> 'command_requires_full_index'.\n>\n> With sparse index enabled, some subcommands of 'stash' work without\n> expanding the index, e.g., 'git stash', 'git stash list', 'git stash drop',\n> etc. Others ensure the index is expanded either directly (as in the case of\n> 'git stash [pop|apply]', where the call to 'merge_recursive_generic()' in\n> 'do_apply_stash()' triggers the expansion), or in a command called\n> internally by stash (e.g., 'git update-index' in 'git stash -u'). So, in\n> addition to enabling sparse index, add tests to 't1092' demonstrating which\n> variants of 'git stash' expand the index, and which do not.\n\nAs always, it is suprising that the change id deceptively easy, but\nit is only true because various components like the merge machinery\nused by the code have been taught to expand the sparse index entries\nas needed. Looking good so far.\n"},{"id":"454413","messageId":"xmqqwnfcu9mi.fsf@gitster.g","threadId":"57792","inReplyTo":"80c25c7587484ca32afcfd6cc054eab78dd1348a.1650908958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/7] merge-recursive: add merge function arg to 'merge_recursive_generic'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-25T21:38:13Z","receivedAt":"2022-04-25T22:22:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> -\tif (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {\n> +\tif (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, merge_recursive, &result)) {\n\nThis one does need input from Elijah to properly evaluate. Off the\ntop of my head, I wonder if the choice of the merge function should\nbe part of \"struct merge_options\".\n\n"},{"id":"454426","messageId":"9ff9f199-baec-80f5-41c2-17034744e527@github.com","threadId":"57792","inReplyTo":"pull.1171.git.1650908957.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] Sparse index: integrate with 'git stash'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-26T12:49:28Z","receivedAt":"2022-04-26T12:49:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/25/2022 1:49 PM, Victoria Dye via GitGitGadget wrote:\n> This series, in combination with the sparse index integrations of reset [1],\n> update-index [2], checkout-index [2], clean [2], and read-tree [3], allows\n> most subcommands of 'git stash' to use the sparse index end-to-end without\n> index expansion.\n\nI'm really excited to see this culmination of your work in this area!\n\n> Performance results (from the 'p2000' tests):\n> \n> (git stash &&\n> git stash pop) master this series\n> ---------------------------------------------------------------------\n> full-v3 4.07(2.42+1.34) 3.98(2.42+1.32) -2.2%\n> full-v4 4.05(2.46+1.31) 4.00(2.49+1.29) -1.2%\n> sparse-v3 7.48(4.81+2.57) 1.53(0.26+1.61) -79.5%\n> sparse-v4 7.35(4.74+2.54) 1.59(0.27+1.63) -78.4%\n> \n> (echo >>new &&\n> git stash -u &&\n> git stash pop) master this series\n> ---------------------------------------------------------------------\n> full-v3 4.21(2.62+1.45) 4.11(2.55+1.44) -2.4%\n> full-v4 4.11(2.51+1.41) 4.02(2.49+1.41) -2.2%\n> sparse-v3 7.35(4.64+2.66) 1.70(0.32+1.64) -76.9%\n> sparse-v4 7.74(4.87+2.83) 1.70(0.32+1.66) -78.0%\n\nI wanted to add some performance results from real-world users of\nan internal monorepo. These are numbers for all runs of \"git stash\",\nso it is not broken down by subcommand:\n\nIndex Type P50 P80 P99\n---------------------------------------\nFull Index 13.3s 26.3s 47.5s\nSparse Index 2.1s 5.8s 9.4s\n\nThanks,\n-Stolee\n"},{"id":"454427","messageId":"f332fec9-da00-0063-4c88-765fa3639ea1@github.com","threadId":"57792","inReplyTo":"f6cf05a5bee9e4ebc174bab0385a13cc1cdb4014.1650908958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/7] stash: integrate with sparse index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-26T12:53:52Z","receivedAt":"2022-04-26T12:53:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/25/2022 1:49 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Enable sparse index in 'git stash' by disabling\n> 'command_requires_full_index'.\n\n> ensure_not_expanded () {\n> \trm -f trace2.txt &&\n> -\techo >>sparse-index/untracked.txt &&\n> +\tif test -z $WITHOUT_UNTRACKED_TXT\n\nDo we need quotes around \"$WITHOUT_UNTRACKED_TXT\"?\n\nI mean, I suppose we don't since the tests pass when this variable\nis unset, but I think it is a good practice to be careful about\nempty variables. Or am I wrong?\n\n> +\tthen\n> +\t\techo >>sparse-index/untracked.txt\n> +\tfi &&\n\n\n> -\tensure_not_expanded checkout-index -f a &&\n> -\tensure_not_expanded checkout-index -f --all &&\n> -\tfor ref in update-deep update-folder1 update-folder2 update-deep\n> -\tdo\n> -\t\techo >>sparse-index/README.md &&\n> -\t\tensure_not_expanded reset --hard $ref || return 1\n> -\tdone &&\n> -\n\nMoving these to be within the stash tests is interesting.\n\n> +test_expect_success 'sparse-index is not expanded: stash' '\n> +\tinit_repos &&\n> +\n> +\techo >>sparse-index/a &&\n> +\tensure_not_expanded stash &&\n> +\tensure_not_expanded stash list &&\n> +\tensure_not_expanded stash show stash@{0} &&\n> +\t! ensure_not_expanded stash apply stash@{0} &&\n> +\tensure_not_expanded stash drop stash@{0} &&\n> +\n> +\techo >>sparse-index/deep/new &&\n> +\t! ensure_not_expanded stash -u &&\n> +\t(\n> +\t\tWITHOUT_UNTRACKED_TXT=1 &&\n> +\t\t! ensure_not_expanded stash pop\n> +\t) &&\n> +\n> +\tensure_not_expanded stash create &&\n> +\toid=$(git -C sparse-index stash create) &&\n> +\tensure_not_expanded stash store -m \"test\" $oid &&\n> +\tensure_not_expanded reset --hard &&\n> +\t! ensure_not_expanded stash pop &&\n> +\n> +\tensure_not_expanded checkout-index -f a &&\n> +\tensure_not_expanded checkout-index -f --all &&\n> +\tfor ref in update-deep update-folder1 update-folder2 update-deep\n> +\tdo\n> +\t\techo >>sparse-index/README.md &&\n> +\t\tensure_not_expanded reset --hard $ref || return 1\n> +\tdone\n\nIt is not obvious why that's necessary here. Perhaps a later\nchange will build upon these checkout-index commands within\nthis test?\n\nThanks,\n-Stolee\n"},{"id":"454428","messageId":"6db07cef-8536-62fd-ad56-f6ef6736ab22@github.com","threadId":"57792","inReplyTo":"xmqqwnfcu9mi.fsf@gitster.g","subject":"Re: [PATCH 5/7] merge-recursive: add merge function arg to 'merge_recursive_generic'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-26T12:57:42Z","receivedAt":"2022-04-26T12:57:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/25/2022 5:38 PM, Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" writes:\n> \n>> -\tif (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {\n>> +\tif (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, merge_recursive, &result)) {\n> \n> This one does need input from Elijah to properly evaluate. Off the\n> top of my head, I wonder if the choice of the merge function should\n> be part of \"struct merge_options\".\n \nI agree that when we have an options struct, it is better to add a\nmember to the struct than to update the prototype of the function,\nif only for the simplicity of not updating all callers.\n\nI am interested in the effect that moving from recursive to ORT for\nall 'git stash' commands might have, regardless of sparse index. I\nexpect 'git stash' to improve on a number of dimensions with that\nchange.\n\nThanks,\n-Stolee\n"},{"id":"454429","messageId":"f2b4b885-b473-bea7-29e5-9e4f6002fc27@github.com","threadId":"57792","inReplyTo":"76f2a9e87223e1c1ebf2de6629c46bdf7ad326f1.1650908958.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/7] stash: merge applied stash with merge-ort","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-26T13:02:16Z","receivedAt":"2022-04-26T13:02:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/25/2022 1:49 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Change the merge function used in 'do_apply_stash()' from 'merge_recursive'\n> to 'merge_ort_recursive'. In addition to aligning with the default merge\n> strategy used by 'git merge' (6a5fb96672 (Change default merge backend from\n> recursive to ort, 2021-08-04)), this allows 'git stash ' to\n> operate without expanding the index by default. Update tests in 't1092'\n> verifying index expansion for 'git stash' accordingly.\n\nThis is an interesting change, not just in the sparse index sense. Yes,\nusing ORT by default is a good idea to align with our default merge\nstrategy _and_ allowing us to avoid expanding the sparse index.\n\nBut: should we allow this algorithm to change via our pull.twoHead\nconfig value? By default, it will be fast with the sparse index. Would\nthere be value in allowing a user to change back to the recursive\nstrategy if they want?\n\nThis also seems like we should have a measurable performance impact\nfor 'git stash ' when the stash contains a lot of renames.\nIt would be interesting to have that documented here. Elijah might\nhave some thoughts on how to measure this performance impact.\n\nThanks,\n-Stolee\n"},{"id":"454430","messageId":"b7752094-1f70-20a4-3217-502803817264@github.com","threadId":"57792","inReplyTo":"9ff9f199-baec-80f5-41c2-17034744e527@github.com","subject":"Re: [PATCH 0/7] Sparse index: integrate with 'git stash'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-26T13:09:22Z","receivedAt":"2022-04-26T13:09:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/26/2022 8:49 AM, Derrick Stolee wrote:\n> On 4/25/2022 1:49 PM, Victoria Dye via GitGitGadget wrote:\n>> This series, in combination with the sparse index integrations of reset [1],\n>> update-index [2], checkout-index [2], clean [2], and read-tree [3], allows\n>> most subcommands of 'git stash' to use the sparse index end-to-end without\n>> index expansion.\n> \n> I'm really excited to see this culmination of your work in this area!\n\nAfter taking a careful read of this series, I'm really pleased with how\nsimple the actual code changes are at this point. This is a testament\nto the hard work you've done on the child processes and also some luck\nin how we've added protections to different portions of the lower-level\nAPIs.\n\nThe biggest interesting points from my review and Junio's comments are:\n\n1. The merge_recursive_generic() change could maybe be done by modifying\n the options struct instead of changing the prototype.\n\n2. Using merge ORT for applying stashes could maybe be changed by config,\n allowing users to select recursive if they want. This change could\n also use some testing to show the merits of ORT even without a sparse\n index.\n\nAnd then I had some nitpicks:\n\n* A script variable could use quoting, probably.\n\n* You moved the checkout-index tests to be next to the stash tests, but\n I don't see the value of that. (Perhaps I just need to be enlightened.)\n\nThanks!\n-Stolee\n"},{"id":"454436","messageId":"f4409521-9812-60d1-bfd1-1dfef0fa0265@github.com","threadId":"57792","inReplyTo":"f332fec9-da00-0063-4c88-765fa3639ea1@github.com","subject":"Re: [PATCH 2/7] stash: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-04-26T15:26:08Z","receivedAt":"2022-04-26T15:26:16Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee wrote:\n> On 4/25/2022 1:49 PM, Victoria Dye via GitGitGadget wrote:\n>> From: Victoria Dye \n>>\n>> Enable sparse index in 'git stash' by disabling\n>> 'command_requires_full_index'.\n> \n>> ensure_not_expanded () {\n>> \trm -f trace2.txt &&\n>> -\techo >>sparse-index/untracked.txt &&\n>> +\tif test -z $WITHOUT_UNTRACKED_TXT\n> \n> Do we need quotes around \"$WITHOUT_UNTRACKED_TXT\"?\n> \n> I mean, I suppose we don't since the tests pass when this variable\n> is unset, but I think it is a good practice to be careful about\n> empty variables. Or am I wrong?\n> \n>> +\tthen\n>> +\t\techo >>sparse-index/untracked.txt\n>> +\tfi &&\n> \n> \n>> -\tensure_not_expanded checkout-index -f a &&\n>> -\tensure_not_expanded checkout-index -f --all &&\n>> -\tfor ref in update-deep update-folder1 update-folder2 update-deep\n>> -\tdo\n>> -\t\techo >>sparse-index/README.md &&\n>> -\t\tensure_not_expanded reset --hard $ref || return 1\n>> -\tdone &&\n>> -\n> \n> Moving these to be within the stash tests is interesting.\n> \n\nThat was unintentional. I originally had the 'stash' cases in the big\n'sparse-index is not expanded' test, but decided to move them into their own\ntest for more appropriate granularity. In doing that, I accidentally took\nsome of the 'checkout-index' tests with it. I'll move them back in V2.\n\nThanks for the catch!\n\n>> +test_expect_success 'sparse-index is not expanded: stash' '\n>> +\tinit_repos &&\n>> +\n>> +\techo >>sparse-index/a &&\n>> +\tensure_not_expanded stash &&\n>> +\tensure_not_expanded stash list &&\n>> +\tensure_not_expanded stash show stash@{0} &&\n>> +\t! ensure_not_expanded stash apply stash@{0} &&\n>> +\tensure_not_expanded stash drop stash@{0} &&\n>> +\n>> +\techo >>sparse-index/deep/new &&\n>> +\t! ensure_not_expanded stash -u &&\n>> +\t(\n>> +\t\tWITHOUT_UNTRACKED_TXT=1 &&\n>> +\t\t! ensure_not_expanded stash pop\n>> +\t) &&\n>> +\n>> +\tensure_not_expanded stash create &&\n>> +\toid=$(git -C sparse-index stash create) &&\n>> +\tensure_not_expanded stash store -m \"test\" $oid &&\n>> +\tensure_not_expanded reset --hard &&\n>> +\t! ensure_not_expanded stash pop &&\n>> +\n>> +\tensure_not_expanded checkout-index -f a &&\n>> +\tensure_not_expanded checkout-index -f --all &&\n>> +\tfor ref in update-deep update-folder1 update-folder2 update-deep\n>> +\tdo\n>> +\t\techo >>sparse-index/README.md &&\n>> +\t\tensure_not_expanded reset --hard $ref || return 1\n>> +\tdone\n> \n> It is not obvious why that's necessary here. Perhaps a later\n> change will build upon these checkout-index commands within\n> this test?\n> \n> Thanks,\n> -Stolee\n\n"},{"id":"454450","messageId":"xmqqy1zrrf2q.fsf@gitster.g","threadId":"57792","inReplyTo":"f332fec9-da00-0063-4c88-765fa3639ea1@github.com","subject":"Re: [PATCH 2/7] stash: integrate with sparse index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-26T16:21:01Z","receivedAt":"2022-04-26T16:21:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 4/25/2022 1:49 PM, Victoria Dye via GitGitGadget wrote:\n>> From: Victoria Dye \n>> \n>> Enable sparse index in 'git stash' by disabling\n>> 'command_requires_full_index'.\n>\n>> ensure_not_expanded () {\n>> \trm -f trace2.txt &&\n>> -\techo >>sparse-index/untracked.txt &&\n>> +\tif test -z $WITHOUT_UNTRACKED_TXT\n>\n> Do we need quotes around \"$WITHOUT_UNTRACKED_TXT\"?\n>\n> I mean, I suppose we don't since the tests pass when this variable\n> is unset, but I think it is a good practice to be careful about\n> empty variables. Or am I wrong?\n\nInteresting. I do not think the reason why\n\n\ttest -z && echo true\n\nsays \"true\" is \"-z\" noticed an empty string---it is \"test $string\"\nthat says \"true\" when \"$string\" is not a recognised command and is\nnot an empty string, no?\n\nYou are absolutely right that the above must be quoted.\n\nThanks for carefully reading.\n"},{"id":"454525","messageId":"8ea986cb249448f59078e2410e9c9daf13b6c714.1651083378.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","subject":"[PATCH v2 1/7] stash: expand sparse-checkout compatibility testing","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-27T18:16:12Z","receivedAt":"2022-04-27T18:16:28Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd tests verifying expected 'git stash' behavior in\n't1092-sparse-checkout-compatibility'. These cases establish the expected\nbehavior of 'git stash' in a sparse-checkout and verify consistency both\nwith and without a sparse index. Although no sparse index compatibility has\nbeen integrated into 'git stash' yet, the tests are all 'expect_success' -\nwe don't want the cone-mode sparse-checkout behavior to change depending on\nwhether it is using a sparse index or not. Therefore, we expect these tests\nto continue passing once sparse index is integrated with 'git stash'.\n\nAdditionally, add performance test cases for 'git stash' both with and\nwithout untracked files. Note that, unlike the other tests in\n'p2000-sparse-operations.sh', the tests added for 'stash' are combination\noperations. This is done to ensure the stash/unstash is not blocked by the\nmodification of '$SPARSE_CONE/a' performed as part of 'test_perf_on_all'.\n\nSigned-off-by: Victoria Dye \n---\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 49 ++++++++++++++++++++++++\n 2 files changed, 51 insertions(+)\n\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 382716cfca9..76710cbef35 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -106,6 +106,8 @@ test_perf_on_all () {\n }\n \n test_perf_on_all git status\n+test_perf_on_all 'git stash && git stash pop'\n+test_perf_on_all 'echo >>new && git stash -u && git stash pop'\n test_perf_on_all git add -A\n test_perf_on_all git add .\n test_perf_on_all git commit -a -m A\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 236ab530284..86312b30444 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1034,6 +1034,55 @@ test_expect_success 'cherry-pick with conflicts' '\n \ttest_all_match test_must_fail git cherry-pick to-cherry-pick\n '\n \n+test_expect_success 'stash' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\t# Stash a sparse directory (folder1)\n+\ttest_all_match git checkout -b test-branch rename-base &&\n+\ttest_all_match git reset --soft rename-out-to-out &&\n+\ttest_all_match git stash &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Apply the sparse directory stash without reinstating the index\n+\ttest_all_match git stash apply -q &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Reset to state where stash can be applied\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git reset --hard rename-out-to-out &&\n+\n+\t# Apply the sparse directory stash *with* reinstating the index\n+\ttest_all_match git stash apply --index -q &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Reset to state where we will get a conflict applying the stash\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git reset --hard update-folder1 &&\n+\n+\t# Apply the sparse directory stash with conflicts\n+\ttest_all_match test_must_fail git stash apply --index -q &&\n+\ttest_all_match test_must_fail git stash apply -q &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Reset to base branch\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git reset --hard base &&\n+\n+\t# Stash & unstash an untracked file outside of the sparse checkout\n+\t# definition.\n+\trun_on_sparse mkdir -p folder1 &&\n+\trun_on_all ../edit-contents folder1/new &&\n+\ttest_all_match git stash -u &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git stash pop -q &&\n+\ttest_all_match git status --porcelain=v2\n+'\n+\n test_expect_success 'checkout-index inside sparse definition' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"454526","messageId":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.git.1650908957.gitgitgadget@gmail.com","subject":"[PATCH v2 0/7] Sparse index: integrate with 'git stash'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-27T18:16:11Z","receivedAt":"2022-04-27T18:16:31Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"This series, in combination with the sparse index integrations of reset [1],\nupdate-index [2], checkout-index [2], clean [2], and read-tree [3], allows\nmost subcommands of 'git stash' to use the sparse index end-to-end without\nindex expansion.\n\nLike the earlier series, this series starts with new tests ensuring\ncompatibility of the sparse index with non-sparse index full and sparse\ncheckouts [1/7]. Next, sparse index is trivially enabled [2/7].\nFunctionally, sparse index-enabled sparse-checkouts remain compatible with\nnon-sparse index sparse-checkouts, but there are still some cases where the\nindex (or a temporary index) is expanded unnecessarily. These cases are\nfixed in three parts:\n\n * First, 'git stash -u' is made sparse index-compatible by ensuring the\n \"temporary\" index holding the stashed, untracked files is created as a\n sparse index whenever possible (per repo settings &\n 'is_sparse_index_allowed()'). Patch [3/7] exposes\n 'is_sparse_index_allowed()' to files outside of 'sparse-index.c', then\n patch [4/7] uses that function to mark the temporary index sparse when\n appropriate.\n * Next, 'git stash (apply|pop)' are made sparse index-compatible by\n changing their internal \"merge\" function (executed via\n 'merge_recursive_generic()') from 'merge_recursive()' to\n 'merge_ort_recursive()'. This requires first allowing\n 'merge_recursive_generic()' to accept a merge function as an input\n (rather than hardcoding use of 'merge_recursive()') in patch [5/7], then\n changing the call in 'stash.c' to specify 'merge_ort_recursive()' in\n patch [6/7]. See note [4] for possible alternate implementations.\n * Finally, while patches 5 & 6 avoid index expansion for most cases of 'git\n stash (apply|pop)', applying a stash that includes untracked files still\n expands the index. This is a result of an internal 'read-tree' execution\n (specifically in its 'unpack_trees' call) creating a result index that is\n never sparse in-core, thus forcing the index to be unnecessarily\n collapsed and re-expanded in 'do_write_locked_index()'. In patch [7/7],\n 'unpack_trees' is updated to set the default sparsity of the resultant\n index to \"sparse\" if allowed by repo settings and\n 'is_sparse_index_allowed()' (similar to the change in patch 4).\n\nPerformance results (from the 'p2000' tests):\n\n(git stash &&\n git stash pop) master this series\n---------------------------------------------------------------------\nfull-v3 4.07(2.42+1.34) 3.98(2.42+1.32) -2.2%\nfull-v4 4.05(2.46+1.31) 4.00(2.49+1.29) -1.2%\nsparse-v3 7.48(4.81+2.57) 1.53(0.26+1.61) -79.5%\nsparse-v4 7.35(4.74+2.54) 1.59(0.27+1.63) -78.4%\n\n(echo >>new &&\n git stash -u &&\n git stash pop) master this series\n---------------------------------------------------------------------\nfull-v3 4.21(2.62+1.45) 4.11(2.55+1.44) -2.4%\nfull-v4 4.11(2.51+1.41) 4.02(2.49+1.41) -2.2%\nsparse-v3 7.35(4.64+2.66) 1.70(0.32+1.64) -76.9%\nsparse-v4 7.74(4.87+2.83) 1.70(0.32+1.66) -78.0%\n\n\n\nChanges since V1\n================\n\n * Added quotes to the \"$WITHOUT_UNTRACKED_TXT\" when testing for it in\n 'ensure_not_expanded' (in 't/t1092-sparse-checkout-compatibility.sh')\n * Moved the 'stash' test in 't1092' elsewhere in the file, so that it\n doesn't conflict (even trivially) with the also-in-flight 'git show'\n integration\n * Moved the 'ensure_not_expended' tests for 'checkout-index' back to\n original location\n\n[1]\nhttps://lore.kernel.org/git/pull.1048.v6.git.1638201164.gitgitgadget@gmail.com/\n[2]\nhttps://lore.kernel.org/git/pull.1109.v2.git.1641924306.gitgitgadget@gmail.com/\n[3]\nhttps://lore.kernel.org/git/pull.1157.v3.git.1646166271.gitgitgadget@gmail.com/\n[4] I went with changing 'stash' to always use 'merge-ort' in\n'merge_recursive_generic()' as a sort of \"middle ground\" between \"replace\n'merge_recursive()' with 'merge_ort_recursive()' in all of its hardcoded\ninternal usage\" and \"only use 'merge-ort' if using a sparse index in 'git\nstash', otherwise 'merge-recursive'\". The former would extend the use of\n'merge-ort' to 'git am' and 'git merge-recursive', whereas the latter is a\nmore cautious/narrowly-focused option. If anyone has any other thoughts, I'm\ninterested in hearing them.\n\nThanks!\n\n-Victoria\n\nVictoria Dye (7):\n stash: expand sparse-checkout compatibility testing\n stash: integrate with sparse index\n sparse-index: expose 'is_sparse_index_allowed()'\n read-cache: set sparsity when index is new\n merge-recursive: add merge function arg to 'merge_recursive_generic'\n stash: merge applied stash with merge-ort\n unpack-trees: preserve index sparsity\n\n builtin/am.c | 2 +-\n builtin/merge-recursive.c | 2 +-\n builtin/stash.c | 6 +-\n merge-ort.c | 3 +-\n merge-recursive.c | 4 +-\n merge-recursive.h | 9 ++-\n read-cache.c | 18 +++++-\n sparse-index.c | 2 +-\n sparse-index.h | 1 +\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 78 +++++++++++++++++++++++-\n unpack-trees.c | 6 ++\n 12 files changed, 123 insertions(+), 10 deletions(-)\n\n\nbase-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1171%2Fvdye%2Fsparse%2Fstash-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1171/vdye/sparse/stash-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1171\n\nRange-diff vs v1:\n\n 1: 994864852a0 ! 1: 8ea986cb249 stash: expand sparse-checkout compatibility testing\n @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all () {\n test_perf_on_all git commit -a -m A\n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'clean' '\n - \ttest_sparse_match test_path_is_dir folder1\n +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'cherry-pick with conflicts' '\n + \ttest_all_match test_must_fail git cherry-pick to-cherry-pick\n '\n \n +test_expect_success 'stash' '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'clean' '\n +\ttest_all_match git status --porcelain=v2\n +'\n +\n - test_expect_success 'submodule handling' '\n + test_expect_success 'checkout-index inside sparse definition' '\n \tinit_repos &&\n \n 2: f6cf05a5bee ! 2: b3e3f0298fb stash: integrate with sparse index\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'index.sparse disa\n ensure_not_expanded () {\n \trm -f trace2.txt &&\n -\techo >>sparse-index/untracked.txt &&\n -+\tif test -z $WITHOUT_UNTRACKED_TXT\n ++\tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n +\tthen\n +\t\techo >>sparse-index/untracked.txt\n +\tfi &&\n \n \tif test \"$1\" = \"!\"\n \tthen\n -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded' '\n - \techo >>sparse-index/untracked.txt &&\n - \tensure_not_expanded add . &&\n - \n --\tensure_not_expanded checkout-index -f a &&\n --\tensure_not_expanded checkout-index -f --all &&\n --\tfor ref in update-deep update-folder1 update-folder2 update-deep\n --\tdo\n --\t\techo >>sparse-index/README.md &&\n --\t\tensure_not_expanded reset --hard $ref || return 1\n --\tdone &&\n --\n - \tensure_not_expanded reset --mixed base &&\n - \tensure_not_expanded reset --hard update-deep &&\n - \tensure_not_expanded reset --keep base &&\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded: merge conflict in cone' '\n \t)\n '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n +\toid=$(git -C sparse-index stash create) &&\n +\tensure_not_expanded stash store -m \"test\" $oid &&\n +\tensure_not_expanded reset --hard &&\n -+\t! ensure_not_expanded stash pop &&\n -+\n -+\tensure_not_expanded checkout-index -f a &&\n -+\tensure_not_expanded checkout-index -f --all &&\n -+\tfor ref in update-deep update-folder1 update-folder2 update-deep\n -+\tdo\n -+\t\techo >>sparse-index/README.md &&\n -+\t\tensure_not_expanded reset --hard $ref || return 1\n -+\tdone\n ++\t! ensure_not_expanded stash pop\n +'\n +\n test_expect_success 'sparse index is not expanded: diff' '\n 3: 27d9920366f = 3: 73f04e95400 sparse-index: expose 'is_sparse_index_allowed()'\n 4: 64edbed0f95 = 4: 42550f39a75 read-cache: set sparsity when index is new\n 5: 80c25c75874 = 5: 4537d473b93 merge-recursive: add merge function arg to 'merge_recursive_generic'\n 6: 76f2a9e8722 ! 6: 22fee0732ad stash: merge applied stash with merge-ort\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n \toid=$(git -C sparse-index stash create) &&\n \tensure_not_expanded stash store -m \"test\" $oid &&\n \tensure_not_expanded reset --hard &&\n --\t! ensure_not_expanded stash pop &&\n -+\tensure_not_expanded stash pop &&\n +-\t! ensure_not_expanded stash pop\n ++\tensure_not_expanded stash pop\n + '\n \n - \tensure_not_expanded checkout-index -f a &&\n - \tensure_not_expanded checkout-index -f --all &&\n + test_expect_success 'sparse index is not expanded: diff' '\n 7: 1daecbe45c1 = 7: 3179018a8cb unpack-trees: preserve index sparsity\n\n-- \ngitgitgadget\n"},{"id":"454527","messageId":"b3e3f0298fb99b9aecc26271eee6fb6eab842dd9.1651083378.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","subject":"[PATCH v2 2/7] stash: integrate with sparse index","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-27T18:16:13Z","receivedAt":"2022-04-27T18:16:33Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nEnable sparse index in 'git stash' by disabling\n'command_requires_full_index'.\n\nWith sparse index enabled, some subcommands of 'stash' work without\nexpanding the index, e.g., 'git stash', 'git stash list', 'git stash drop',\netc. Others ensure the index is expanded either directly (as in the case of\n'git stash [pop|apply]', where the call to 'merge_recursive_generic()' in\n'do_apply_stash()' triggers the expansion), or in a command called\ninternally by stash (e.g., 'git update-index' in 'git stash -u'). So, in\naddition to enabling sparse index, add tests to 't1092' demonstrating which\nvariants of 'git stash' expand the index, and which do not.\n\nFinally, add the option to skip writing 'untracked.txt' in\n'ensure_not_expanded', and use that option to successfully apply stashed\nuntracked files without a conflict in 'untracked.txt'.\n\nSigned-off-by: Victoria Dye \n---\n builtin/stash.c | 3 +++\n t/t1092-sparse-checkout-compatibility.sh | 29 +++++++++++++++++++++++-\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 0c7b6a95882..1bfba532044 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1770,6 +1770,9 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, git_stash_usage,\n \t\t\t PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \tindex_file = get_index_file();\n \tstrbuf_addf(&stash_index_path, \"%s.stash.%\" PRIuMAX, index_file,\n \t\t (uintmax_t)pid);\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 86312b30444..75d844cd71d 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1271,7 +1271,10 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n \n ensure_not_expanded () {\n \trm -f trace2.txt &&\n-\techo >>sparse-index/untracked.txt &&\n+\tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n+\tthen\n+\t\techo >>sparse-index/untracked.txt\n+\tfi &&\n \n \tif test \"$1\" = \"!\"\n \tthen\n@@ -1375,6 +1378,30 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' '\n \t)\n '\n \n+test_expect_success 'sparse-index is not expanded: stash' '\n+\tinit_repos &&\n+\n+\techo >>sparse-index/a &&\n+\tensure_not_expanded stash &&\n+\tensure_not_expanded stash list &&\n+\tensure_not_expanded stash show stash@{0} &&\n+\t! ensure_not_expanded stash apply stash@{0} &&\n+\tensure_not_expanded stash drop stash@{0} &&\n+\n+\techo >>sparse-index/deep/new &&\n+\t! ensure_not_expanded stash -u &&\n+\t(\n+\t\tWITHOUT_UNTRACKED_TXT=1 &&\n+\t\t! ensure_not_expanded stash pop\n+\t) &&\n+\n+\tensure_not_expanded stash create &&\n+\toid=$(git -C sparse-index stash create) &&\n+\tensure_not_expanded stash store -m \"test\" $oid &&\n+\tensure_not_expanded reset --hard &&\n+\t! ensure_not_expanded stash pop\n+'\n+\n test_expect_success 'sparse index is not expanded: diff' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"454528","messageId":"73f04e95400f7fc8ea43a48abeb29b5c70244fe8.1651083378.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","subject":"[PATCH v2 3/7] sparse-index: expose 'is_sparse_index_allowed()'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-27T18:16:14Z","receivedAt":"2022-04-27T18:16:35Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nExpose 'is_sparse_index_allowed()' publicly so that it may be used by\ncallers outside of 'sparse-index.c'. While no such callers exist yet, it\nwill be used in a subsequent commit.\n\nSigned-off-by: Victoria Dye \n---\n sparse-index.c | 2 +-\n sparse-index.h | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 8636af72de5..ffbab7d35f1 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -118,7 +118,7 @@ static int index_has_unmerged_entries(struct index_state *istate)\n \treturn 0;\n }\n \n-static int is_sparse_index_allowed(struct index_state *istate, int flags)\n+int is_sparse_index_allowed(struct index_state *istate, int flags)\n {\n \tif (!core_apply_sparse_checkout || !core_sparse_checkout_cone)\n \t\treturn 0;\ndiff --git a/sparse-index.h b/sparse-index.h\nindex 633d4fb7e31..f57c65d972f 100644\n--- a/sparse-index.h\n+++ b/sparse-index.h\n@@ -3,6 +3,7 @@\n \n struct index_state;\n #define SPARSE_INDEX_MEMORY_ONLY (1 << 0)\n+int is_sparse_index_allowed(struct index_state *istate, int flags);\n int convert_to_sparse(struct index_state *istate, int flags);\n void ensure_correct_sparsity(struct index_state *istate);\n void clear_skip_worktree_from_present_files(struct index_state *istate);\n-- \ngitgitgadget\n\n"},{"id":"454529","messageId":"42550f39a756c862222ab7ef2e665b87f727f8fa.1651083378.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","subject":"[PATCH v2 4/7] read-cache: set sparsity when index is new","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-27T18:16:15Z","receivedAt":"2022-04-27T18:16:43Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen the index read in 'do_read_index()' does not exist on-disk, mark the\nindex \"sparse\" if the executing command does not require a full index and\nsparse index is otherwise enabled.\n\nSome commands (such as 'git stash -u') implicitly create a new index (when\nthe 'GIT_INDEX_FILE' variable points to a non-existent file) and perform\nsome operation on it. However, when this index is created, it isn't created\nwith the same sparsity settings as the repo index. As a result, while these\nindexes may be sparse during the operation, they are always expanded before\nbeing written to disk. We can avoid that expansion by defaulting the index\nto \"sparse\", in which case it will only be expanded if the full index is\nneeded.\n\nNote that the function 'set_new_index_sparsity()' is created despite having\nonly a single caller because additional callers will be added in a\nsubsequent patch.\n\nSigned-off-by: Victoria Dye \n---\n read-cache.c | 18 +++++++++++++++++-\n t/t1092-sparse-checkout-compatibility.sh | 2 +-\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 4df97e185e9..60355f5ad6a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2260,6 +2260,20 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con\n \treturn consumed;\n }\n \n+static void set_new_index_sparsity(struct index_state *istate)\n+{\n+\t/*\n+\t * If the index's repo exists, mark it sparse according to\n+\t * repo settings.\n+\t */\n+\tif (istate->repo) {\n+\t\tprepare_repo_settings(istate->repo);\n+\t\tif (!istate->repo->settings.command_requires_full_index &&\n+\t\t is_sparse_index_allowed(istate, 0))\n+\t\t\tistate->sparse_index = 1;\n+\t}\n+}\n+\n /* remember to discard_cache() before reading a different cache! */\n int do_read_index(struct index_state *istate, const char *path, int must_exist)\n {\n@@ -2281,8 +2295,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \tistate->timestamp.nsec = 0;\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n-\t\tif (!must_exist && errno == ENOENT)\n+\t\tif (!must_exist && errno == ENOENT) {\n+\t\t\tset_new_index_sparsity(istate);\n \t\t\treturn 0;\n+\t\t}\n \t\tdie_errno(_(\"%s: index file open failed\"), path);\n \t}\n \ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 75d844cd71d..85c6a56f1b7 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1389,7 +1389,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \tensure_not_expanded stash drop stash@{0} &&\n \n \techo >>sparse-index/deep/new &&\n-\t! ensure_not_expanded stash -u &&\n+\tensure_not_expanded stash -u &&\n \t(\n \t\tWITHOUT_UNTRACKED_TXT=1 &&\n \t\t! ensure_not_expanded stash pop\n-- \ngitgitgadget\n\n"},{"id":"454530","messageId":"22fee0732ad7f4bb355a96bc514b084195f312bc.1651083378.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","subject":"[PATCH v2 6/7] stash: merge applied stash with merge-ort","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-27T18:16:17Z","receivedAt":"2022-04-27T18:16:44Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nChange the merge function used in 'do_apply_stash()' from 'merge_recursive'\nto 'merge_ort_recursive'. In addition to aligning with the default merge\nstrategy used by 'git merge' (6a5fb96672 (Change default merge backend from\nrecursive to ort, 2021-08-04)), this allows 'git stash ' to\noperate without expanding the index by default. Update tests in 't1092'\nverifying index expansion for 'git stash' accordingly.\n\nSigned-off-by: Victoria Dye \n---\n builtin/stash.c | 3 ++-\n t/t1092-sparse-checkout-compatibility.sh | 4 ++--\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 16171eb1dab..cd77d448546 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -7,6 +7,7 @@\n #include \"cache-tree.h\"\n #include \"unpack-trees.h\"\n #include \"merge-recursive.h\"\n+#include \"merge-ort-wrappers.h\"\n #include \"strvec.h\"\n #include \"run-command.h\"\n #include \"dir.h\"\n@@ -554,7 +555,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tbases[0] = &info->b_tree;\n \n \tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n-\t\t\t\t merge_recursive, &result);\n+\t\t\t\t merge_ort_recursive, &result);\n \tif (ret) {\n \t\trerere(0);\n \ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 85c6a56f1b7..aaf4d880dbc 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1385,7 +1385,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \tensure_not_expanded stash &&\n \tensure_not_expanded stash list &&\n \tensure_not_expanded stash show stash@{0} &&\n-\t! ensure_not_expanded stash apply stash@{0} &&\n+\tensure_not_expanded stash apply stash@{0} &&\n \tensure_not_expanded stash drop stash@{0} &&\n \n \techo >>sparse-index/deep/new &&\n@@ -1399,7 +1399,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \toid=$(git -C sparse-index stash create) &&\n \tensure_not_expanded stash store -m \"test\" $oid &&\n \tensure_not_expanded reset --hard &&\n-\t! ensure_not_expanded stash pop\n+\tensure_not_expanded stash pop\n '\n \n test_expect_success 'sparse index is not expanded: diff' '\n-- \ngitgitgadget\n\n"},{"id":"454531","messageId":"3179018a8cb72c09c091a58eca4f59f71f90845a.1651083378.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","subject":"[PATCH v2 7/7] unpack-trees: preserve index sparsity","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-27T18:16:18Z","receivedAt":"2022-04-27T18:16:47Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen unpacking trees, set the default sparsity of the resultant index based\non repo settings and 'is_sparse_index_allowed()'.\n\nNormally, when executing 'unpack_trees', the output index is marked sparse\nwhen (and only when) it unpacks a sparse directory. However, an index may be\n\"sparse\" even if it contains no sparse directories - when all files fall\ninside the sparse-checkout definition or otherwise have SKIP_WORKTREE\ndisabled. Therefore, the output index may be marked \"full\" even when it is\n\"sparse\", resulting in unnecessary 'ensure_full_index' calls when writing to\ndisk. Avoid this by setting the \"default\" index sparsity to match what is\nexpected for the repository.\n\nAs a consequence of this fix, the (non-merge) 'read-tree' performed when\napplying a stash with untracked files no longer expands the index. Update\nthe corresponding test in 't1092'.\n\nSigned-off-by: Victoria Dye \n---\n t/t1092-sparse-checkout-compatibility.sh | 2 +-\n unpack-trees.c | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex aaf4d880dbc..19221c14225 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1392,7 +1392,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \tensure_not_expanded stash -u &&\n \t(\n \t\tWITHOUT_UNTRACKED_TXT=1 &&\n-\t\t! ensure_not_expanded stash pop\n+\t\tensure_not_expanded stash pop\n \t) &&\n \n \tensure_not_expanded stash create &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7f528d35cc2..a1d0ff3a4d3 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -11,6 +11,7 @@\n #include \"refs.h\"\n #include \"attr.h\"\n #include \"split-index.h\"\n+#include \"sparse-index.h\"\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n #include \"fsmonitor.h\"\n@@ -1839,6 +1840,11 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->result.fsmonitor_last_update =\n \t\txstrdup_or_null(o->src_index->fsmonitor_last_update);\n \n+\tif (!o->src_index->initialized &&\n+\t !repo->settings.command_requires_full_index &&\n+\t is_sparse_index_allowed(&o->result, 0))\n+\t\to->result.sparse_index = 1;\n+\n \t/*\n \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n \t */\n-- \ngitgitgadget\n"},{"id":"454532","messageId":"4537d473b937b182cd79b2f3c5673b75d92cab23.1651083378.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","subject":"[PATCH v2 5/7] merge-recursive: add merge function arg to 'merge_recursive_generic'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-27T18:16:16Z","receivedAt":"2022-04-27T18:16:49Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nReplace the hardcoded 'merge_recursive()' function used by the\n'merge_recursive_generic()' with a caller-specific merge function. This will\nallow us to use 'merge_ort_recursive()' (and therefore avoid the index\nexpansion of 'merge_recursive()') in commands that perform merges with\n'merge_recursive_generic()', such as 'git stash pop'.\n\nNote that this patch is strictly a refactor; all callers still use\n'merge_recursive()', and any changing to 'merge_ort_recursive()' will be\ndone in a later commit.\n\nSigned-off-by: Victoria Dye \n---\n builtin/am.c | 2 +-\n builtin/merge-recursive.c | 2 +-\n builtin/stash.c | 2 +-\n merge-ort.c | 3 ++-\n merge-recursive.c | 4 ++--\n merge-recursive.h | 9 ++++++++-\n 6 files changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0f4111bafa0..6d01185d122 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1614,7 +1614,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \tif (state->quiet)\n \t\to.verbosity = 0;\n \n-\tif (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {\n+\tif (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, merge_recursive, &result)) {\n \t\trepo_rerere(the_repository, state->allow_rerere_autoupdate);\n \t\tfree(their_tree_name);\n \t\treturn error(_(\"Failed to merge in the changes.\"));\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex b9acbf5d342..687ed1e527b 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -81,7 +81,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n \tif (o.verbosity >= 3)\n \t\tprintf(_(\"Merging %s with %s\\n\"), o.branch1, o.branch2);\n \n-\tfailed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, &result);\n+\tfailed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, merge_recursive, &result);\n \n \tfree(better1);\n \tfree(better2);\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 1bfba532044..16171eb1dab 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -554,7 +554,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tbases[0] = &info->b_tree;\n \n \tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n-\t\t\t\t &result);\n+\t\t\t\t merge_recursive, &result);\n \tif (ret) {\n \t\trerere(0);\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8545354dafd..4bccdfcf355 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4737,7 +4737,8 @@ void merge_incore_recursive(struct merge_options *opt,\n \ttrace2_region_enter(\"merge\", \"incore_recursive\", opt->repo);\n \n \t/* We set the ancestor label based on the merge_bases */\n-\tassert(opt->ancestor == NULL);\n+\tassert(opt->ancestor == NULL ||\n+\t !strcmp(opt->ancestor, \"constructed merge base\"));\n \n \ttrace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n \tmerge_start(opt, result);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1ee6364e8b1..2088f5c5fb3 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3806,6 +3806,7 @@ int merge_recursive_generic(struct merge_options *opt,\n \t\t\t const struct object_id *merge,\n \t\t\t int num_merge_bases,\n \t\t\t const struct object_id **merge_bases,\n+\t\t\t recursive_merge_fn_t merge_fn,\n \t\t\t struct commit **result)\n {\n \tint clean;\n@@ -3829,8 +3830,7 @@ int merge_recursive_generic(struct merge_options *opt,\n \t}\n \n \trepo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);\n-\tclean = merge_recursive(opt, head_commit, next_commit, ca,\n-\t\t\t\tresult);\n+\tclean = merge_fn(opt, head_commit, next_commit, ca, result);\n \tif (clean < 0) {\n \t\trollback_lock_file(&lock);\n \t\treturn clean;\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex b88000e3c25..6a21f2da538 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -53,6 +53,12 @@ struct merge_options {\n \tstruct merge_options_internal *priv;\n };\n \n+typedef int (*recursive_merge_fn_t)(struct merge_options *opt,\n+\t\t\t\t struct commit *h1,\n+\t\t\t\t struct commit *h2,\n+\t\t\t\t struct commit_list *merge_bases,\n+\t\t\t\t struct commit **result);\n+\n void init_merge_options(struct merge_options *opt, struct repository *repo);\n \n /* parse the option in s and update the relevant field of opt */\n@@ -105,7 +111,7 @@ int merge_recursive(struct merge_options *opt,\n \n /*\n * merge_recursive_generic can operate on trees instead of commits, by\n- * wrapping the trees into virtual commits, and calling merge_recursive().\n+ * wrapping the trees into virtual commits, and calling the provided merge_fn.\n * It also writes out the in-memory index to disk if the merge is successful.\n *\n * Outputs:\n@@ -120,6 +126,7 @@ int merge_recursive_generic(struct merge_options *opt,\n \t\t\t const struct object_id *merge,\n \t\t\t int num_merge_bases,\n \t\t\t const struct object_id **merge_bases,\n+\t\t\t recursive_merge_fn_t merge_fn,\n \t\t\t struct commit **result);\n \n #endif\n-- \ngitgitgadget\n\n"},{"id":"454899","messageId":"CABPp-BHcWjOeVhRD_XKTko0OH2pwYsuCt8PzH=C_0u_gUWe0GQ@mail.gmail.com","threadId":"57792","inReplyTo":"4537d473b937b182cd79b2f3c5673b75d92cab23.1651083378.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/7] merge-recursive: add merge function arg to 'merge_recursive_generic'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-06T07:23:43Z","receivedAt":"2022-05-06T07:24:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 27, 2022 at 11:16 AM Victoria Dye via GitGitGadget\n wrote:\n>\n> From: Victoria Dye \n>\n> Replace the hardcoded 'merge_recursive()' function used by the\n> 'merge_recursive_generic()' with a caller-specific merge function. This will\n> allow us to use 'merge_ort_recursive()' (and therefore avoid the index\n> expansion of 'merge_recursive()') in commands that perform merges with\n> 'merge_recursive_generic()', such as 'git stash pop'.\n>\n> Note that this patch is strictly a refactor; all callers still use\n> 'merge_recursive()', and any changing to 'merge_ort_recursive()' will be\n> done in a later commit.\n\nI'm not sure if we can gut merge_recursive_generic(), but I don't\nthink stash should use it...\n\n> Signed-off-by: Victoria Dye \n> ---\n> builtin/am.c | 2 +-\n> builtin/merge-recursive.c | 2 +-\n> builtin/stash.c | 2 +-\n> merge-ort.c | 3 ++-\n> merge-recursive.c | 4 ++--\n> merge-recursive.h | 9 ++++++++-\n> 6 files changed, 15 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 0f4111bafa0..6d01185d122 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1614,7 +1614,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n> if (state->quiet)\n> o.verbosity = 0;\n>\n> - if (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {\n> + if (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, merge_recursive, &result)) {\n> repo_rerere(the_repository, state->allow_rerere_autoupdate);\n> free(their_tree_name);\n> return error(_(\"Failed to merge in the changes.\"));\n> diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\n> index b9acbf5d342..687ed1e527b 100644\n> --- a/builtin/merge-recursive.c\n> +++ b/builtin/merge-recursive.c\n> @@ -81,7 +81,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n> if (o.verbosity >= 3)\n> printf(_(\"Merging %s with %s\\n\"), o.branch1, o.branch2);\n>\n> - failed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, &result);\n> + failed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, merge_recursive, &result);\n>\n> free(better1);\n> free(better2);\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 1bfba532044..16171eb1dab 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -554,7 +554,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> bases[0] = &info->b_tree;\n>\n> ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n> - &result);\n> + merge_recursive, &result);\n> if (ret) {\n> rerere(0);\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 8545354dafd..4bccdfcf355 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -4737,7 +4737,8 @@ void merge_incore_recursive(struct merge_options *opt,\n> trace2_region_enter(\"merge\", \"incore_recursive\", opt->repo);\n>\n> /* We set the ancestor label based on the merge_bases */\n> - assert(opt->ancestor == NULL);\n> + assert(opt->ancestor == NULL ||\n> + !strcmp(opt->ancestor, \"constructed merge base\"));\n\n...and here's one of the reasons why. The fact that\nmerge_recursive_generic() uses this string when exactly one merge base\nis passed is something that is only correct for git-am; it is wrong\nand actively misleading for git-stash since it has a real merge base\nthat is not internally constructed by the operation using the merge\nmachinery. (The merge base it uses is something like $STASH^1, IIRC.)\n\nIn fact, this was half the coin around why merge_recursive_generic()\nwasn't converted when merge-ort was written; see\nhttps://lore.kernel.org/git/CABPp-BHW61zA+MefvWK47iVZKY97rxc2XZ-NjXzuJxEhgSLqUw@mail.gmail.com/\nand https://lore.kernel.org/git/CABPp-BFr=1iVY739cfh-1Hp82x-Mny-k4y0f3zZ_YuP3PxiGfQ@mail.gmail.com/\nfor more details.\n\nThe use of merge_recursive_generic() by stash is also a bit weird;\nmost of the time, stash is going to have actual commits instead of\njust trees. But stash dereferences those commits to trees, passes\nthem to merge_recursive_generic(), and then merge_recursive_generic()\nhas to create fake commits containing those trees, because the merge\nmachinery wants commits. It feels a bit like a Rube Goldberg machine.\nAlso, stash also always calls merge_recursive_generic() with exactly\none merge base, which together with having real commits both kind of\ndefeat the need for \"generic\". I think stash should just use\nmerge_trees()/merge_incore_nonrecursive() directly (much as\nsequencer.c does). The only special case to worry about with stash is\nwhen c_tree != HEAD^{tree}, i.e. when applying changes on top of\nalready present changes instead of just on top of HEAD. But in that\ncase, I think stash should be the thing to create a fake commit rather\nthan invoking some wrapper that will create fake commits for all three\ntrees.\n\n> trace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n> merge_start(opt, result);\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 1ee6364e8b1..2088f5c5fb3 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -3806,6 +3806,7 @@ int merge_recursive_generic(struct merge_options *opt,\n> const struct object_id *merge,\n> int num_merge_bases,\n> const struct object_id **merge_bases,\n> + recursive_merge_fn_t merge_fn,\n> struct commit **result)\n> {\n> int clean;\n> @@ -3829,8 +3830,7 @@ int merge_recursive_generic(struct merge_options *opt,\n> }\n>\n> repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);\n> - clean = merge_recursive(opt, head_commit, next_commit, ca,\n> - result);\n> + clean = merge_fn(opt, head_commit, next_commit, ca, result);\n> if (clean < 0) {\n> rollback_lock_file(&lock);\n> return clean;\n> diff --git a/merge-recursive.h b/merge-recursive.h\n> index b88000e3c25..6a21f2da538 100644\n> --- a/merge-recursive.h\n> +++ b/merge-recursive.h\n> @@ -53,6 +53,12 @@ struct merge_options {\n> struct merge_options_internal *priv;\n> };\n>\n> +typedef int (*recursive_merge_fn_t)(struct merge_options *opt,\n> + struct commit *h1,\n> + struct commit *h2,\n> + struct commit_list *merge_bases,\n> + struct commit **result);\n> +\n> void init_merge_options(struct merge_options *opt, struct repository *repo);\n>\n> /* parse the option in s and update the relevant field of opt */\n> @@ -105,7 +111,7 @@ int merge_recursive(struct merge_options *opt,\n>\n> /*\n> * merge_recursive_generic can operate on trees instead of commits, by\n> - * wrapping the trees into virtual commits, and calling merge_recursive().\n> + * wrapping the trees into virtual commits, and calling the provided merge_fn.\n> * It also writes out the in-memory index to disk if the merge is successful.\n> *\n> * Outputs:\n> @@ -120,6 +126,7 @@ int merge_recursive_generic(struct merge_options *opt,\n> const struct object_id *merge,\n> int num_merge_bases,\n> const struct object_id **merge_bases,\n> + recursive_merge_fn_t merge_fn,\n> struct commit **result);\n>\n> #endif\n> --\n> gitgitgadget\n"},{"id":"454901","messageId":"CABPp-BFANwZn73w8zrVySB7mh0bQQBdGJjBuSJy50UOeyYT6aA@mail.gmail.com","threadId":"57792","inReplyTo":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/7] Sparse index: integrate with 'git stash'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-06T07:46:49Z","receivedAt":"2022-05-06T07:47:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 27, 2022 at 11:16 AM Victoria Dye via GitGitGadget\n wrote:\n>\n> This series, in combination with the sparse index integrations of reset [1],\n> update-index [2], checkout-index [2], clean [2], and read-tree [3], allows\n> most subcommands of 'git stash' to use the sparse index end-to-end without\n> index expansion.\n\nI've read through the series. Like Stolee, I'm pleased with how\nsimple some of the code changes are (much due to prior hard work in\nthe area) and how nicely you describe the changes.\n\nAt first when I was reading through, I was again a bit disappointed\nthat we're just converting the subcommands stash uses instead of\nfixing stash to stop forking subprocesses, but...then I realized this\nserved as a forcing function of sorts to make more of Git sparse-index\ncompatible, so I think it's actually the better route. We can always\nmake the orthogonal fixes to stash's implementation design later. :-)\n\nHowever, there is one small issue around the way merging is handled...\n\n> Like the earlier series, this series starts with new tests ensuring\n> compatibility of the sparse index with non-sparse index full and sparse\n> checkouts [1/7]. Next, sparse index is trivially enabled [2/7].\n> Functionally, sparse index-enabled sparse-checkouts remain compatible with\n> non-sparse index sparse-checkouts, but there are still some cases where the\n> index (or a temporary index) is expanded unnecessarily. These cases are\n> fixed in three parts:\n>\n> * First, 'git stash -u' is made sparse index-compatible by ensuring the\n> \"temporary\" index holding the stashed, untracked files is created as a\n> sparse index whenever possible (per repo settings &\n> 'is_sparse_index_allowed()'). Patch [3/7] exposes\n> 'is_sparse_index_allowed()' to files outside of 'sparse-index.c', then\n> patch [4/7] uses that function to mark the temporary index sparse when\n> appropriate.\n> * Next, 'git stash (apply|pop)' are made sparse index-compatible by\n> changing their internal \"merge\" function (executed via\n> 'merge_recursive_generic()') from 'merge_recursive()' to\n> 'merge_ort_recursive()'. This requires first allowing\n> 'merge_recursive_generic()' to accept a merge function as an input\n> (rather than hardcoding use of 'merge_recursive()') in patch [5/7], then\n> changing the call in 'stash.c' to specify 'merge_ort_recursive()' in\n> patch [6/7]. See note [4] for possible alternate implementations.\n> * Finally, while patches 5 & 6 avoid index expansion for most cases of 'git\n> stash (apply|pop)', applying a stash that includes untracked files still\n> expands the index. This is a result of an internal 'read-tree' execution\n> (specifically in its 'unpack_trees' call) creating a result index that is\n> never sparse in-core, thus forcing the index to be unnecessarily\n> collapsed and re-expanded in 'do_write_locked_index()'. In patch [7/7],\n> 'unpack_trees' is updated to set the default sparsity of the resultant\n> index to \"sparse\" if allowed by repo settings and\n> 'is_sparse_index_allowed()' (similar to the change in patch 4).\n>\n> Performance results (from the 'p2000' tests):\n>\n> (git stash &&\n> git stash pop) master this series\n> ---------------------------------------------------------------------\n> full-v3 4.07(2.42+1.34) 3.98(2.42+1.32) -2.2%\n> full-v4 4.05(2.46+1.31) 4.00(2.49+1.29) -1.2%\n> sparse-v3 7.48(4.81+2.57) 1.53(0.26+1.61) -79.5%\n> sparse-v4 7.35(4.74+2.54) 1.59(0.27+1.63) -78.4%\n>\n> (echo >>new &&\n> git stash -u &&\n> git stash pop) master this series\n> ---------------------------------------------------------------------\n> full-v3 4.21(2.62+1.45) 4.11(2.55+1.44) -2.4%\n> full-v4 4.11(2.51+1.41) 4.02(2.49+1.41) -2.2%\n> sparse-v3 7.35(4.64+2.66) 1.70(0.32+1.64) -76.9%\n> sparse-v4 7.74(4.87+2.83) 1.70(0.32+1.66) -78.0%\n>\n>\n>\n> Changes since V1\n> ================\n>\n> * Added quotes to the \"$WITHOUT_UNTRACKED_TXT\" when testing for it in\n> 'ensure_not_expanded' (in 't/t1092-sparse-checkout-compatibility.sh')\n> * Moved the 'stash' test in 't1092' elsewhere in the file, so that it\n> doesn't conflict (even trivially) with the also-in-flight 'git show'\n> integration\n> * Moved the 'ensure_not_expended' tests for 'checkout-index' back to\n> original location\n>\n> [1]\n> https://lore.kernel.org/git/pull.1048.v6.git.1638201164.gitgitgadget@gmail.com/\n> [2]\n> https://lore.kernel.org/git/pull.1109.v2.git.1641924306.gitgitgadget@gmail.com/\n> [3]\n> https://lore.kernel.org/git/pull.1157.v3.git.1646166271.gitgitgadget@gmail.com/\n> [4] I went with changing 'stash' to always use 'merge-ort' in\n> 'merge_recursive_generic()' as a sort of \"middle ground\" between \"replace\n> 'merge_recursive()' with 'merge_ort_recursive()' in all of its hardcoded\n> internal usage\" and \"only use 'merge-ort' if using a sparse index in 'git\n> stash', otherwise 'merge-recursive'\". The former would extend the use of\n> 'merge-ort' to 'git am' and 'git merge-recursive', whereas the latter is a\n> more cautious/narrowly-focused option. If anyone has any other thoughts, I'm\n> interested in hearing them.\n\nI understand that you'd want to modify and use\nmerge_recursive_generic() in order to make the smallest code change\npossible, but merge_recursive_generic() has no equivalent in ort\nbecause during the review of ort, we discovered that porting over\nmerge_recursive_generic() meant porting bugs. We need to fix those\nbugs. However, in the case of stash, I think we can just sidestep\nthose bugs. stash probably only uses merge_recursive_generic() because\nit was the easiest transliteration of shell (which originally invoked\ngit-merge-recursive), as opposed to the best conversion. The best\nconversion at the time would have been to call merge_trees() instead\n(stash doesn't need recursiveness, and tends to have commits available\nrather than just trees). I'd like to see us modify stash to call\nmerge_incore_nonrecursive() directly (and/or merge_trees() if we're\nsupporting using both merge-ort and merge-recursive backends).\n\nNo need to worry about git-am or git-merge-recursive for now; we can\nreplace/fix/update their calls to merge_recursive_generic() later.\n"},{"id":"455050","messageId":"65dd1914-137e-d9ba-ee1f-7611e5f71d30@github.com","threadId":"57792","inReplyTo":"CABPp-BHcWjOeVhRD_XKTko0OH2pwYsuCt8PzH=C_0u_gUWe0GQ@mail.gmail.com","subject":"Re: [PATCH v2 5/7] merge-recursive: add merge function arg to 'merge_recursive_generic'","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-05-09T19:24:55Z","receivedAt":"2022-05-09T19:25:01Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Elijah Newren wrote:\n> On Wed, Apr 27, 2022 at 11:16 AM Victoria Dye via GitGitGadget\n> wrote:\n>>\n>> From: Victoria Dye \n>>\n>> Replace the hardcoded 'merge_recursive()' function used by the\n>> 'merge_recursive_generic()' with a caller-specific merge function. This will\n>> allow us to use 'merge_ort_recursive()' (and therefore avoid the index\n>> expansion of 'merge_recursive()') in commands that perform merges with\n>> 'merge_recursive_generic()', such as 'git stash pop'.\n>>\n>> Note that this patch is strictly a refactor; all callers still use\n>> 'merge_recursive()', and any changing to 'merge_ort_recursive()' will be\n>> done in a later commit.\n> \n> I'm not sure if we can gut merge_recursive_generic(), but I don't\n> think stash should use it...\n> \n>> Signed-off-by: Victoria Dye \n>> ---\n>> builtin/am.c | 2 +-\n>> builtin/merge-recursive.c | 2 +-\n>> builtin/stash.c | 2 +-\n>> merge-ort.c | 3 ++-\n>> merge-recursive.c | 4 ++--\n>> merge-recursive.h | 9 ++++++++-\n>> 6 files changed, 15 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index 0f4111bafa0..6d01185d122 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -1614,7 +1614,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n>> if (state->quiet)\n>> o.verbosity = 0;\n>>\n>> - if (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {\n>> + if (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, merge_recursive, &result)) {\n>> repo_rerere(the_repository, state->allow_rerere_autoupdate);\n>> free(their_tree_name);\n>> return error(_(\"Failed to merge in the changes.\"));\n>> diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\n>> index b9acbf5d342..687ed1e527b 100644\n>> --- a/builtin/merge-recursive.c\n>> +++ b/builtin/merge-recursive.c\n>> @@ -81,7 +81,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n>> if (o.verbosity >= 3)\n>> printf(_(\"Merging %s with %s\\n\"), o.branch1, o.branch2);\n>>\n>> - failed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, &result);\n>> + failed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, merge_recursive, &result);\n>>\n>> free(better1);\n>> free(better2);\n>> diff --git a/builtin/stash.c b/builtin/stash.c\n>> index 1bfba532044..16171eb1dab 100644\n>> --- a/builtin/stash.c\n>> +++ b/builtin/stash.c\n>> @@ -554,7 +554,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>> bases[0] = &info->b_tree;\n>>\n>> ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n>> - &result);\n>> + merge_recursive, &result);\n>> if (ret) {\n>> rerere(0);\n>>\n>> diff --git a/merge-ort.c b/merge-ort.c\n>> index 8545354dafd..4bccdfcf355 100644\n>> --- a/merge-ort.c\n>> +++ b/merge-ort.c\n>> @@ -4737,7 +4737,8 @@ void merge_incore_recursive(struct merge_options *opt,\n>> trace2_region_enter(\"merge\", \"incore_recursive\", opt->repo);\n>>\n>> /* We set the ancestor label based on the merge_bases */\n>> - assert(opt->ancestor == NULL);\n>> + assert(opt->ancestor == NULL ||\n>> + !strcmp(opt->ancestor, \"constructed merge base\"));\n> \n> ...and here's one of the reasons why. The fact that\n> merge_recursive_generic() uses this string when exactly one merge base\n> is passed is something that is only correct for git-am; it is wrong\n> and actively misleading for git-stash since it has a real merge base\n> that is not internally constructed by the operation using the merge\n> machinery. (The merge base it uses is something like $STASH^1, IIRC.)\n> \n> In fact, this was half the coin around why merge_recursive_generic()\n> wasn't converted when merge-ort was written; see\n> https://lore.kernel.org/git/CABPp-BHW61zA+MefvWK47iVZKY97rxc2XZ-NjXzuJxEhgSLqUw@mail.gmail.com/\n> and https://lore.kernel.org/git/CABPp-BFr=1iVY739cfh-1Hp82x-Mny-k4y0f3zZ_YuP3PxiGfQ@mail.gmail.com/\n> for more details.\n> \n\nAll of that makes sense, thanks for the context!\n\n> The use of merge_recursive_generic() by stash is also a bit weird;\n> most of the time, stash is going to have actual commits instead of\n> just trees. But stash dereferences those commits to trees, passes\n> them to merge_recursive_generic(), and then merge_recursive_generic()\n> has to create fake commits containing those trees, because the merge\n> machinery wants commits. It feels a bit like a Rube Goldberg machine.\n> Also, stash also always calls merge_recursive_generic() with exactly\n> one merge base, which together with having real commits both kind of\n> defeat the need for \"generic\". I think stash should just use\n> merge_trees()/merge_incore_nonrecursive() directly (much as\n> sequencer.c does). The only special case to worry about with stash is\n> when c_tree != HEAD^{tree}, i.e. when applying changes on top of\n> already present changes instead of just on top of HEAD. But in that\n> case, I think stash should be the thing to create a fake commit rather\n> than invoking some wrapper that will create fake commits for all three\n> trees.\n> \n\nI'm a bit confused about this. The non-recursive merge functions\n('merge_trees()' & 'merge_ort_nonrecursive()' or the lower-level\n'merge_incore_nonrecursive()') merge trees, not commits, so performing a\nnon-recursive merge requires dereferencing commits to trees anyway. I think\nI agree with your other message [1] that the 'stash' merge doesn't need to\nmerge recursively, but that would mean it also doesn't use the commits\n*directly* (i.e., as arguments in the merge). \n\nApologies if I'm missing something obvious, but I want to make sure I\nunderstand your suggestion.\n\n[1] https://lore.kernel.org/git/CABPp-BFANwZn73w8zrVySB7mh0bQQBdGJjBuSJy50UOeyYT6aA@mail.gmail.com/ \n\n\n>> trace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n>> merge_start(opt, result);\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index 1ee6364e8b1..2088f5c5fb3 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -3806,6 +3806,7 @@ int merge_recursive_generic(struct merge_options *opt,\n>> const struct object_id *merge,\n>> int num_merge_bases,\n>> const struct object_id **merge_bases,\n>> + recursive_merge_fn_t merge_fn,\n>> struct commit **result)\n>> {\n>> int clean;\n>> @@ -3829,8 +3830,7 @@ int merge_recursive_generic(struct merge_options *opt,\n>> }\n>>\n>> repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);\n>> - clean = merge_recursive(opt, head_commit, next_commit, ca,\n>> - result);\n>> + clean = merge_fn(opt, head_commit, next_commit, ca, result);\n>> if (clean < 0) {\n>> rollback_lock_file(&lock);\n>> return clean;\n>> diff --git a/merge-recursive.h b/merge-recursive.h\n>> index b88000e3c25..6a21f2da538 100644\n>> --- a/merge-recursive.h\n>> +++ b/merge-recursive.h\n>> @@ -53,6 +53,12 @@ struct merge_options {\n>> struct merge_options_internal *priv;\n>> };\n>>\n>> +typedef int (*recursive_merge_fn_t)(struct merge_options *opt,\n>> + struct commit *h1,\n>> + struct commit *h2,\n>> + struct commit_list *merge_bases,\n>> + struct commit **result);\n>> +\n>> void init_merge_options(struct merge_options *opt, struct repository *repo);\n>>\n>> /* parse the option in s and update the relevant field of opt */\n>> @@ -105,7 +111,7 @@ int merge_recursive(struct merge_options *opt,\n>>\n>> /*\n>> * merge_recursive_generic can operate on trees instead of commits, by\n>> - * wrapping the trees into virtual commits, and calling merge_recursive().\n>> + * wrapping the trees into virtual commits, and calling the provided merge_fn.\n>> * It also writes out the in-memory index to disk if the merge is successful.\n>> *\n>> * Outputs:\n>> @@ -120,6 +126,7 @@ int merge_recursive_generic(struct merge_options *opt,\n>> const struct object_id *merge,\n>> int num_merge_bases,\n>> const struct object_id **merge_bases,\n>> + recursive_merge_fn_t merge_fn,\n>> struct commit **result);\n>>\n>> #endif\n>> --\n>> gitgitgadget\n\n"},{"id":"455076","messageId":"CABPp-BGcxu8gkNtdKYsGErNapxKzcHugNxoz61=RwG-8dkOWGg@mail.gmail.com","threadId":"57792","inReplyTo":"65dd1914-137e-d9ba-ee1f-7611e5f71d30@github.com","subject":"Re: [PATCH v2 5/7] merge-recursive: add merge function arg to 'merge_recursive_generic'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-10T07:06:44Z","receivedAt":"2022-05-10T07:33:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, May 9, 2022 at 12:24 PM Victoria Dye wrote:\n>\n> Elijah Newren wrote:\n> > On Wed, Apr 27, 2022 at 11:16 AM Victoria Dye via GitGitGadget\n> > wrote:\n> >>\n> >> From: Victoria Dye \n> >>\n> >> Replace the hardcoded 'merge_recursive()' function used by the\n> >> 'merge_recursive_generic()' with a caller-specific merge function. This will\n> >> allow us to use 'merge_ort_recursive()' (and therefore avoid the index\n> >> expansion of 'merge_recursive()') in commands that perform merges with\n> >> 'merge_recursive_generic()', such as 'git stash pop'.\n> >>\n> >> Note that this patch is strictly a refactor; all callers still use\n> >> 'merge_recursive()', and any changing to 'merge_ort_recursive()' will be\n> >> done in a later commit.\n> >\n> > I'm not sure if we can gut merge_recursive_generic(), but I don't\n> > think stash should use it...\n> >\n> >> Signed-off-by: Victoria Dye \n> >> ---\n> >> builtin/am.c | 2 +-\n> >> builtin/merge-recursive.c | 2 +-\n> >> builtin/stash.c | 2 +-\n> >> merge-ort.c | 3 ++-\n> >> merge-recursive.c | 4 ++--\n> >> merge-recursive.h | 9 ++++++++-\n> >> 6 files changed, 15 insertions(+), 7 deletions(-)\n> >>\n> >> diff --git a/builtin/am.c b/builtin/am.c\n> >> index 0f4111bafa0..6d01185d122 100644\n> >> --- a/builtin/am.c\n> >> +++ b/builtin/am.c\n> >> @@ -1614,7 +1614,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n> >> if (state->quiet)\n> >> o.verbosity = 0;\n> >>\n> >> - if (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {\n> >> + if (merge_recursive_generic(&o, &our_tree, &their_tree, 1, bases, merge_recursive, &result)) {\n> >> repo_rerere(the_repository, state->allow_rerere_autoupdate);\n> >> free(their_tree_name);\n> >> return error(_(\"Failed to merge in the changes.\"));\n> >> diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\n> >> index b9acbf5d342..687ed1e527b 100644\n> >> --- a/builtin/merge-recursive.c\n> >> +++ b/builtin/merge-recursive.c\n> >> @@ -81,7 +81,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n> >> if (o.verbosity >= 3)\n> >> printf(_(\"Merging %s with %s\\n\"), o.branch1, o.branch2);\n> >>\n> >> - failed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, &result);\n> >> + failed = merge_recursive_generic(&o, &h1, &h2, bases_count, bases, merge_recursive, &result);\n> >>\n> >> free(better1);\n> >> free(better2);\n> >> diff --git a/builtin/stash.c b/builtin/stash.c\n> >> index 1bfba532044..16171eb1dab 100644\n> >> --- a/builtin/stash.c\n> >> +++ b/builtin/stash.c\n> >> @@ -554,7 +554,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> >> bases[0] = &info->b_tree;\n> >>\n> >> ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n> >> - &result);\n> >> + merge_recursive, &result);\n> >> if (ret) {\n> >> rerere(0);\n> >>\n> >> diff --git a/merge-ort.c b/merge-ort.c\n> >> index 8545354dafd..4bccdfcf355 100644\n> >> --- a/merge-ort.c\n> >> +++ b/merge-ort.c\n> >> @@ -4737,7 +4737,8 @@ void merge_incore_recursive(struct merge_options *opt,\n> >> trace2_region_enter(\"merge\", \"incore_recursive\", opt->repo);\n> >>\n> >> /* We set the ancestor label based on the merge_bases */\n> >> - assert(opt->ancestor == NULL);\n> >> + assert(opt->ancestor == NULL ||\n> >> + !strcmp(opt->ancestor, \"constructed merge base\"));\n> >\n> > ...and here's one of the reasons why. The fact that\n> > merge_recursive_generic() uses this string when exactly one merge base\n> > is passed is something that is only correct for git-am; it is wrong\n> > and actively misleading for git-stash since it has a real merge base\n> > that is not internally constructed by the operation using the merge\n> > machinery. (The merge base it uses is something like $STASH^1, IIRC.)\n> >\n> > In fact, this was half the coin around why merge_recursive_generic()\n> > wasn't converted when merge-ort was written; see\n> > https://lore.kernel.org/git/CABPp-BHW61zA+MefvWK47iVZKY97rxc2XZ-NjXzuJxEhgSLqUw@mail.gmail.com/\n> > and https://lore.kernel.org/git/CABPp-BFr=1iVY739cfh-1Hp82x-Mny-k4y0f3zZ_YuP3PxiGfQ@mail.gmail.com/\n> > for more details.\n> >\n>\n> All of that makes sense, thanks for the context!\n>\n> > The use of merge_recursive_generic() by stash is also a bit weird;\n> > most of the time, stash is going to have actual commits instead of\n> > just trees. But stash dereferences those commits to trees, passes\n> > them to merge_recursive_generic(), and then merge_recursive_generic()\n> > has to create fake commits containing those trees, because the merge\n> > machinery wants commits. It feels a bit like a Rube Goldberg machine.\n> > Also, stash also always calls merge_recursive_generic() with exactly\n> > one merge base, which together with having real commits both kind of\n> > defeat the need for \"generic\". I think stash should just use\n> > merge_trees()/merge_incore_nonrecursive() directly (much as\n> > sequencer.c does). The only special case to worry about with stash is\n> > when c_tree != HEAD^{tree}, i.e. when applying changes on top of\n> > already present changes instead of just on top of HEAD. But in that\n> > case, I think stash should be the thing to create a fake commit rather\n> > than invoking some wrapper that will create fake commits for all three\n> > trees.\n> >\n>\n> I'm a bit confused about this. The non-recursive merge functions\n> ('merge_trees()' & 'merge_ort_nonrecursive()' or the lower-level\n> 'merge_incore_nonrecursive()') merge trees, not commits, so performing a\n> non-recursive merge requires dereferencing commits to trees anyway. I think\n> I agree with your other message [1] that the 'stash' merge doesn't need to\n> merge recursively, but that would mean it also doesn't use the commits\n> *directly* (i.e., as arguments in the merge).\n>\n> Apologies if I'm missing something obvious, but I want to make sure I\n> understand your suggestion.\n>\n> [1] https://lore.kernel.org/git/CABPp-BFANwZn73w8zrVySB7mh0bQQBdGJjBuSJy50UOeyYT6aA@mail.gmail.com/\n\nOh, right, it's only the recursive merge that needs commits (so that\nit can find ancestors and ancestors of ancestors, etc.). So, ignore\nmy comments about making fake commits; that's not needed.\n"},{"id":"455132","messageId":"pull.1171.v3.git.1652225552.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v2.git.1651083378.gitgitgadget@gmail.com","subject":"[PATCH v3 0/6] Sparse index: integrate with 'git stash'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-10T23:32:26Z","receivedAt":"2022-05-10T23:32:44Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"This series, in combination with the sparse index integrations of reset [1],\nupdate-index [2], checkout-index [2], clean [2], and read-tree [3], allows\nmost subcommands of 'git stash' to use the sparse index end-to-end without\nindex expansion.\n\nLike the earlier series, this series starts with new tests ensuring\ncompatibility of the sparse index with non-sparse index full and sparse\ncheckouts [1/6]. Next, sparse index is trivially enabled [2/6].\nFunctionally, sparse index-enabled sparse-checkouts remain compatible with\nnon-sparse index sparse-checkouts, but there are still some cases where the\nindex (or a temporary index) is expanded unnecessarily. These cases are\nfixed in three parts:\n\n * First, 'git stash -u' is made sparse index-compatible by ensuring the\n \"temporary\" index holding the stashed, untracked files is created as a\n sparse index whenever possible (per repo settings &\n 'is_sparse_index_allowed()'). Patch [3/6] exposes\n 'is_sparse_index_allowed()' to files outside of 'sparse-index.c', then\n patch [4/6] uses that function to mark the temporary index sparse when\n appropriate.\n * Next, 'git stash (apply|pop)' are made sparse index-compatible by\n changing their internal merge function from 'merge_recursive_generic()'\n (which constructs \"fake\" commits from the working tree, stash, and stash\n base) to 'merge_ort_nonrecursive()' (which operates on the trees\n directly) in patch [5/6]. The use of the non-recursive 'merge-ort' helps\n us to avoid sparse index expansion associated with 'merge-recursive', as\n well as avoid the unused/unnecessary complexity of a recursive merge.\n * Finally, while patch 5 skips index expansion for most cases of 'git stash\n (apply|pop)', applying a stash that includes untracked files still\n expands the index. This is a result of an internal 'read-tree' execution\n (specifically in its 'unpack_trees' call) creating a result index that is\n never sparse in-core, thus forcing the index to be unnecessarily\n collapsed and re-expanded in 'do_write_locked_index()'. In patch [6/6],\n 'unpack_trees' is updated to set the default sparsity of the resultant\n index to \"sparse\" if allowed by repo settings and\n 'is_sparse_index_allowed()' (similar to the change in patch 4).\n\nPerformance results (from the 'p2000' tests):\n\n(git stash &&\n git stash pop) master this series\n---------------------------------------------------------------------\nfull-v3 4.07(2.42+1.34) 3.98(2.42+1.32) -2.2%\nfull-v4 4.05(2.46+1.31) 4.00(2.49+1.29) -1.2%\nsparse-v3 7.48(4.81+2.57) 1.53(0.26+1.61) -79.5%\nsparse-v4 7.35(4.74+2.54) 1.59(0.27+1.63) -78.4%\n\n(echo >>new &&\n git stash -u &&\n git stash pop) master this series\n---------------------------------------------------------------------\nfull-v3 4.21(2.62+1.45) 4.11(2.55+1.44) -2.4%\nfull-v4 4.11(2.51+1.41) 4.02(2.49+1.41) -2.2%\nsparse-v3 7.35(4.64+2.66) 1.70(0.32+1.64) -76.9%\nsparse-v4 7.74(4.87+2.83) 1.70(0.32+1.66) -78.0%\n\n\n\nChanges since V2\n================\n\n * Replaced use of 'merge_recursive_generic' with 'merge_ort_nonrecursive'\n in 'do_apply_stash()'\n * Rebased on top of 'master'\n\n\nChanges since V1\n================\n\n * Added quotes to the \"$WITHOUT_UNTRACKED_TXT\" when testing for it in\n 'ensure_not_expanded' (in 't/t1092-sparse-checkout-compatibility.sh')\n * Moved the 'stash' test in 't1092' elsewhere in the file, so that it\n doesn't conflict (even trivially) with the also-in-flight 'git show'\n integration\n * Moved the 'ensure_not_expended' tests for 'checkout-index' back to\n original location\n\n[1]\nhttps://lore.kernel.org/git/pull.1048.v6.git.1638201164.gitgitgadget@gmail.com/\n[2]\nhttps://lore.kernel.org/git/pull.1109.v2.git.1641924306.gitgitgadget@gmail.com/\n[3]\nhttps://lore.kernel.org/git/pull.1157.v3.git.1646166271.gitgitgadget@gmail.com/\n\nThanks!\n\n-Victoria\n\nVictoria Dye (6):\n stash: expand sparse-checkout compatibility testing\n stash: integrate with sparse index\n sparse-index: expose 'is_sparse_index_allowed()'\n read-cache: set sparsity when index is new\n stash: apply stash using 'merge_ort_nonrecursive()'\n unpack-trees: preserve index sparsity\n\n builtin/stash.c | 33 ++++++++--\n read-cache.c | 18 +++++-\n sparse-index.c | 2 +-\n sparse-index.h | 1 +\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 78 +++++++++++++++++++++++-\n unpack-trees.c | 6 ++\n 7 files changed, 131 insertions(+), 9 deletions(-)\n\n\nbase-commit: 0f828332d5ac36fc63b7d8202652efa152809856\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1171%2Fvdye%2Fsparse%2Fstash-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1171/vdye/sparse/stash-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1171\n\nRange-diff vs v2:\n\n 1: 8ea986cb249 = 1: 4e0a09f7a3c stash: expand sparse-checkout compatibility testing\n 2: b3e3f0298fb = 2: 7be484a8c0b stash: integrate with sparse index\n 3: 73f04e95400 = 3: 6f00fca9267 sparse-index: expose 'is_sparse_index_allowed()'\n 4: 42550f39a75 = 4: bb092c075f4 read-cache: set sparsity when index is new\n 5: 4537d473b93 < -: ----------- merge-recursive: add merge function arg to 'merge_recursive_generic'\n 6: 22fee0732ad ! 5: e733c2fd9f4 stash: merge applied stash with merge-ort\n @@ Metadata\n Author: Victoria Dye \n \n ## Commit message ##\n - stash: merge applied stash with merge-ort\n + stash: apply stash using 'merge_ort_nonrecursive()'\n \n - Change the merge function used in 'do_apply_stash()' from 'merge_recursive'\n - to 'merge_ort_recursive'. In addition to aligning with the default merge\n - strategy used by 'git merge' (6a5fb96672 (Change default merge backend from\n - recursive to ort, 2021-08-04)), this allows 'git stash ' to\n - operate without expanding the index by default. Update tests in 't1092'\n - verifying index expansion for 'git stash' accordingly.\n + Update 'stash' to use 'merge_ort_nonrecursive()' to apply a stash to the\n + current working tree. When 'git stash apply' was converted from its shell\n + script implementation to a builtin in 8a0fc8d19d (stash: convert apply to\n + builtin, 2019-02-25), 'merge_recursive_generic()' was used to merge a stash\n + into the working tree as part of 'git stash (apply|pop)'. However, with the\n + single merge base used in 'do_apply_stash()', the commit wrapping done by\n + 'merge_recursive_generic()' is not only unnecessary, but misleading (the\n + *real* merge base is labeled \"constructed merge base\"). Therefore, a\n + non-recursive merge of the working tree, stashed tree, and stash base tree\n + is more appropriate.\n +\n + There are two options for a non-recursive merge-then-update-worktree\n + function: 'merge_trees()' and 'merge_ort_nonrecursive()'. Use\n + 'merge_ort_nonrecursive()' to align with the default merge strategy used by\n + 'git merge' (6a5fb96672 (Change default merge backend from recursive to ort,\n + 2021-08-04)) and, because merge-ort does not operate in-place on the index,\n + avoid unnecessary index expansion. Update tests in 't1092' verifying index\n + expansion for 'git stash' accordingly.\n \n Signed-off-by: Victoria Dye \n \n @@ builtin/stash.c\n #include \"strvec.h\"\n #include \"run-command.h\"\n #include \"dir.h\"\n +@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)\n + static int do_apply_stash(const char *prefix, struct stash_info *info,\n + \t\t\t int index, int quiet)\n + {\n +-\tint ret;\n ++\tint clean, ret;\n + \tint has_index = index;\n + \tstruct merge_options o;\n + \tstruct object_id c_tree;\n + \tstruct object_id index_tree;\n +-\tstruct commit *result;\n +-\tconst struct object_id *bases[1];\n ++\tstruct tree *head, *merge, *merge_base;\n ++\tstruct lock_file lock = LOCK_INIT;\n + \n + \tread_cache_preload(NULL);\n + \tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n +@@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info *info,\n + \n + \to.branch1 = \"Updated upstream\";\n + \to.branch2 = \"Stashed changes\";\n ++\to.ancestor = \"Stash base\";\n + \n + \tif (oideq(&info->b_tree, &c_tree))\n + \t\to.branch1 = \"Version stash was based on\";\n @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info *info,\n - \tbases[0] = &info->b_tree;\n + \tif (o.verbosity >= 3)\n + \t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n + \n +-\tbases[0] = &info->b_tree;\n ++\thead = lookup_tree(o.repo, &c_tree);\n ++\tmerge = lookup_tree(o.repo, &info->w_tree);\n ++\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n ++\n ++\trepo_hold_locked_index(o.repo, &lock, LOCK_DIE_ON_ERROR);\n ++\tclean = merge_ort_nonrecursive(&o, head, merge, merge_base);\n ++\n ++\t/*\n ++\t * If 'clean' >= 0, reverse the value for 'ret' so 'ret' is 0 when the\n ++\t * merge was clean, and nonzero if the merge was unclean or encountered\n ++\t * an error.\n ++\t */\n ++\tret = clean >= 0 ? !clean : clean;\n ++\n ++\tif (ret < 0)\n ++\t\trollback_lock_file(&lock);\n ++\telse if (write_locked_index(o.repo->index, &lock,\n ++\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n ++\t\tret = error(_(\"could not write index\"));\n \n - \tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n --\t\t\t\t merge_recursive, &result);\n -+\t\t\t\t merge_ort_recursive, &result);\n +-\tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n +-\t\t\t\t &result);\n \tif (ret) {\n \t\trerere(0);\n \n 7: 3179018a8cb = 6: 4b4c38fcc03 unpack-trees: preserve index sparsity\n\n-- \ngitgitgadget\n"},{"id":"455133","messageId":"4e0a09f7a3ce55ea5478a3539c0c0e80188bf533.1652225552.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v3.git.1652225552.gitgitgadget@gmail.com","subject":"[PATCH v3 1/6] stash: expand sparse-checkout compatibility testing","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-10T23:32:27Z","receivedAt":"2022-05-10T23:32:46Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd tests verifying expected 'git stash' behavior in\n't1092-sparse-checkout-compatibility'. These cases establish the expected\nbehavior of 'git stash' in a sparse-checkout and verify consistency both\nwith and without a sparse index. Although no sparse index compatibility has\nbeen integrated into 'git stash' yet, the tests are all 'expect_success' -\nwe don't want the cone-mode sparse-checkout behavior to change depending on\nwhether it is using a sparse index or not. Therefore, we expect these tests\nto continue passing once sparse index is integrated with 'git stash'.\n\nAdditionally, add performance test cases for 'git stash' both with and\nwithout untracked files. Note that, unlike the other tests in\n'p2000-sparse-operations.sh', the tests added for 'stash' are combination\noperations. This is done to ensure the stash/unstash is not blocked by the\nmodification of '$SPARSE_CONE/a' performed as part of 'test_perf_on_all'.\n\nSigned-off-by: Victoria Dye \n---\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 49 ++++++++++++++++++++++++\n 2 files changed, 51 insertions(+)\n\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 382716cfca9..76710cbef35 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -106,6 +106,8 @@ test_perf_on_all () {\n }\n \n test_perf_on_all git status\n+test_perf_on_all 'git stash && git stash pop'\n+test_perf_on_all 'echo >>new && git stash -u && git stash pop'\n test_perf_on_all git add -A\n test_perf_on_all git add .\n test_perf_on_all git commit -a -m A\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 236ab530284..86312b30444 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1034,6 +1034,55 @@ test_expect_success 'cherry-pick with conflicts' '\n \ttest_all_match test_must_fail git cherry-pick to-cherry-pick\n '\n \n+test_expect_success 'stash' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\t# Stash a sparse directory (folder1)\n+\ttest_all_match git checkout -b test-branch rename-base &&\n+\ttest_all_match git reset --soft rename-out-to-out &&\n+\ttest_all_match git stash &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Apply the sparse directory stash without reinstating the index\n+\ttest_all_match git stash apply -q &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Reset to state where stash can be applied\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git reset --hard rename-out-to-out &&\n+\n+\t# Apply the sparse directory stash *with* reinstating the index\n+\ttest_all_match git stash apply --index -q &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Reset to state where we will get a conflict applying the stash\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git reset --hard update-folder1 &&\n+\n+\t# Apply the sparse directory stash with conflicts\n+\ttest_all_match test_must_fail git stash apply --index -q &&\n+\ttest_all_match test_must_fail git stash apply -q &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\t# Reset to base branch\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git reset --hard base &&\n+\n+\t# Stash & unstash an untracked file outside of the sparse checkout\n+\t# definition.\n+\trun_on_sparse mkdir -p folder1 &&\n+\trun_on_all ../edit-contents folder1/new &&\n+\ttest_all_match git stash -u &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git stash pop -q &&\n+\ttest_all_match git status --porcelain=v2\n+'\n+\n test_expect_success 'checkout-index inside sparse definition' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"455134","messageId":"7be484a8c0beb5787967e4c89fb3eab15006e3be.1652225552.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v3.git.1652225552.gitgitgadget@gmail.com","subject":"[PATCH v3 2/6] stash: integrate with sparse index","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-10T23:32:28Z","receivedAt":"2022-05-10T23:32:49Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nEnable sparse index in 'git stash' by disabling\n'command_requires_full_index'.\n\nWith sparse index enabled, some subcommands of 'stash' work without\nexpanding the index, e.g., 'git stash', 'git stash list', 'git stash drop',\netc. Others ensure the index is expanded either directly (as in the case of\n'git stash [pop|apply]', where the call to 'merge_recursive_generic()' in\n'do_apply_stash()' triggers the expansion), or in a command called\ninternally by stash (e.g., 'git update-index' in 'git stash -u'). So, in\naddition to enabling sparse index, add tests to 't1092' demonstrating which\nvariants of 'git stash' expand the index, and which do not.\n\nFinally, add the option to skip writing 'untracked.txt' in\n'ensure_not_expanded', and use that option to successfully apply stashed\nuntracked files without a conflict in 'untracked.txt'.\n\nSigned-off-by: Victoria Dye \n---\n builtin/stash.c | 3 +++\n t/t1092-sparse-checkout-compatibility.sh | 29 +++++++++++++++++++++++-\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 0c7b6a95882..1bfba532044 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1770,6 +1770,9 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, git_stash_usage,\n \t\t\t PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \tindex_file = get_index_file();\n \tstrbuf_addf(&stash_index_path, \"%s.stash.%\" PRIuMAX, index_file,\n \t\t (uintmax_t)pid);\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 86312b30444..75d844cd71d 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1271,7 +1271,10 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n \n ensure_not_expanded () {\n \trm -f trace2.txt &&\n-\techo >>sparse-index/untracked.txt &&\n+\tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n+\tthen\n+\t\techo >>sparse-index/untracked.txt\n+\tfi &&\n \n \tif test \"$1\" = \"!\"\n \tthen\n@@ -1375,6 +1378,30 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' '\n \t)\n '\n \n+test_expect_success 'sparse-index is not expanded: stash' '\n+\tinit_repos &&\n+\n+\techo >>sparse-index/a &&\n+\tensure_not_expanded stash &&\n+\tensure_not_expanded stash list &&\n+\tensure_not_expanded stash show stash@{0} &&\n+\t! ensure_not_expanded stash apply stash@{0} &&\n+\tensure_not_expanded stash drop stash@{0} &&\n+\n+\techo >>sparse-index/deep/new &&\n+\t! ensure_not_expanded stash -u &&\n+\t(\n+\t\tWITHOUT_UNTRACKED_TXT=1 &&\n+\t\t! ensure_not_expanded stash pop\n+\t) &&\n+\n+\tensure_not_expanded stash create &&\n+\toid=$(git -C sparse-index stash create) &&\n+\tensure_not_expanded stash store -m \"test\" $oid &&\n+\tensure_not_expanded reset --hard &&\n+\t! ensure_not_expanded stash pop\n+'\n+\n test_expect_success 'sparse index is not expanded: diff' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"455135","messageId":"6f00fca92671f23391ca92bfb529b8222a6e8406.1652225552.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v3.git.1652225552.gitgitgadget@gmail.com","subject":"[PATCH v3 3/6] sparse-index: expose 'is_sparse_index_allowed()'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-10T23:32:29Z","receivedAt":"2022-05-10T23:32:50Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nExpose 'is_sparse_index_allowed()' publicly so that it may be used by\ncallers outside of 'sparse-index.c'. While no such callers exist yet, it\nwill be used in a subsequent commit.\n\nSigned-off-by: Victoria Dye \n---\n sparse-index.c | 2 +-\n sparse-index.h | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 8636af72de5..ffbab7d35f1 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -118,7 +118,7 @@ static int index_has_unmerged_entries(struct index_state *istate)\n \treturn 0;\n }\n \n-static int is_sparse_index_allowed(struct index_state *istate, int flags)\n+int is_sparse_index_allowed(struct index_state *istate, int flags)\n {\n \tif (!core_apply_sparse_checkout || !core_sparse_checkout_cone)\n \t\treturn 0;\ndiff --git a/sparse-index.h b/sparse-index.h\nindex 633d4fb7e31..f57c65d972f 100644\n--- a/sparse-index.h\n+++ b/sparse-index.h\n@@ -3,6 +3,7 @@\n \n struct index_state;\n #define SPARSE_INDEX_MEMORY_ONLY (1 << 0)\n+int is_sparse_index_allowed(struct index_state *istate, int flags);\n int convert_to_sparse(struct index_state *istate, int flags);\n void ensure_correct_sparsity(struct index_state *istate);\n void clear_skip_worktree_from_present_files(struct index_state *istate);\n-- \ngitgitgadget\n\n"},{"id":"455136","messageId":"bb092c075f499acd5aa86debee0a23fa66ac523d.1652225552.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v3.git.1652225552.gitgitgadget@gmail.com","subject":"[PATCH v3 4/6] read-cache: set sparsity when index is new","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-10T23:32:30Z","receivedAt":"2022-05-10T23:32:52Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen the index read in 'do_read_index()' does not exist on-disk, mark the\nindex \"sparse\" if the executing command does not require a full index and\nsparse index is otherwise enabled.\n\nSome commands (such as 'git stash -u') implicitly create a new index (when\nthe 'GIT_INDEX_FILE' variable points to a non-existent file) and perform\nsome operation on it. However, when this index is created, it isn't created\nwith the same sparsity settings as the repo index. As a result, while these\nindexes may be sparse during the operation, they are always expanded before\nbeing written to disk. We can avoid that expansion by defaulting the index\nto \"sparse\", in which case it will only be expanded if the full index is\nneeded.\n\nNote that the function 'set_new_index_sparsity()' is created despite having\nonly a single caller because additional callers will be added in a\nsubsequent patch.\n\nSigned-off-by: Victoria Dye \n---\n read-cache.c | 18 +++++++++++++++++-\n t/t1092-sparse-checkout-compatibility.sh | 2 +-\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 4df97e185e9..60355f5ad6a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2260,6 +2260,20 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con\n \treturn consumed;\n }\n \n+static void set_new_index_sparsity(struct index_state *istate)\n+{\n+\t/*\n+\t * If the index's repo exists, mark it sparse according to\n+\t * repo settings.\n+\t */\n+\tif (istate->repo) {\n+\t\tprepare_repo_settings(istate->repo);\n+\t\tif (!istate->repo->settings.command_requires_full_index &&\n+\t\t is_sparse_index_allowed(istate, 0))\n+\t\t\tistate->sparse_index = 1;\n+\t}\n+}\n+\n /* remember to discard_cache() before reading a different cache! */\n int do_read_index(struct index_state *istate, const char *path, int must_exist)\n {\n@@ -2281,8 +2295,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \tistate->timestamp.nsec = 0;\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n-\t\tif (!must_exist && errno == ENOENT)\n+\t\tif (!must_exist && errno == ENOENT) {\n+\t\t\tset_new_index_sparsity(istate);\n \t\t\treturn 0;\n+\t\t}\n \t\tdie_errno(_(\"%s: index file open failed\"), path);\n \t}\n \ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 75d844cd71d..85c6a56f1b7 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1389,7 +1389,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \tensure_not_expanded stash drop stash@{0} &&\n \n \techo >>sparse-index/deep/new &&\n-\t! ensure_not_expanded stash -u &&\n+\tensure_not_expanded stash -u &&\n \t(\n \t\tWITHOUT_UNTRACKED_TXT=1 &&\n \t\t! ensure_not_expanded stash pop\n-- \ngitgitgadget\n\n"},{"id":"455137","messageId":"e733c2fd9f497a8d80555126ec2e166e182ab8db.1652225552.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v3.git.1652225552.gitgitgadget@gmail.com","subject":"[PATCH v3 5/6] stash: apply stash using 'merge_ort_nonrecursive()'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-10T23:32:31Z","receivedAt":"2022-05-10T23:32:55Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate 'stash' to use 'merge_ort_nonrecursive()' to apply a stash to the\ncurrent working tree. When 'git stash apply' was converted from its shell\nscript implementation to a builtin in 8a0fc8d19d (stash: convert apply to\nbuiltin, 2019-02-25), 'merge_recursive_generic()' was used to merge a stash\ninto the working tree as part of 'git stash (apply|pop)'. However, with the\nsingle merge base used in 'do_apply_stash()', the commit wrapping done by\n'merge_recursive_generic()' is not only unnecessary, but misleading (the\n*real* merge base is labeled \"constructed merge base\"). Therefore, a\nnon-recursive merge of the working tree, stashed tree, and stash base tree\nis more appropriate.\n\nThere are two options for a non-recursive merge-then-update-worktree\nfunction: 'merge_trees()' and 'merge_ort_nonrecursive()'. Use\n'merge_ort_nonrecursive()' to align with the default merge strategy used by\n'git merge' (6a5fb96672 (Change default merge backend from recursive to ort,\n2021-08-04)) and, because merge-ort does not operate in-place on the index,\navoid unnecessary index expansion. Update tests in 't1092' verifying index\nexpansion for 'git stash' accordingly.\n\nSigned-off-by: Victoria Dye \n---\n builtin/stash.c | 30 +++++++++++++++++++-----\n t/t1092-sparse-checkout-compatibility.sh | 4 ++--\n 2 files changed, 26 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 1bfba532044..3fe549f7d3c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -7,6 +7,7 @@\n #include \"cache-tree.h\"\n #include \"unpack-trees.h\"\n #include \"merge-recursive.h\"\n+#include \"merge-ort-wrappers.h\"\n #include \"strvec.h\"\n #include \"run-command.h\"\n #include \"dir.h\"\n@@ -492,13 +493,13 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)\n static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\t\t int index, int quiet)\n {\n-\tint ret;\n+\tint clean, ret;\n \tint has_index = index;\n \tstruct merge_options o;\n \tstruct object_id c_tree;\n \tstruct object_id index_tree;\n-\tstruct commit *result;\n-\tconst struct object_id *bases[1];\n+\tstruct tree *head, *merge, *merge_base;\n+\tstruct lock_file lock = LOCK_INIT;\n \n \tread_cache_preload(NULL);\n \tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n@@ -541,6 +542,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \n \to.branch1 = \"Updated upstream\";\n \to.branch2 = \"Stashed changes\";\n+\to.ancestor = \"Stash base\";\n \n \tif (oideq(&info->b_tree, &c_tree))\n \t\to.branch1 = \"Version stash was based on\";\n@@ -551,10 +553,26 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \tif (o.verbosity >= 3)\n \t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n \n-\tbases[0] = &info->b_tree;\n+\thead = lookup_tree(o.repo, &c_tree);\n+\tmerge = lookup_tree(o.repo, &info->w_tree);\n+\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n+\n+\trepo_hold_locked_index(o.repo, &lock, LOCK_DIE_ON_ERROR);\n+\tclean = merge_ort_nonrecursive(&o, head, merge, merge_base);\n+\n+\t/*\n+\t * If 'clean' >= 0, reverse the value for 'ret' so 'ret' is 0 when the\n+\t * merge was clean, and nonzero if the merge was unclean or encountered\n+\t * an error.\n+\t */\n+\tret = clean >= 0 ? !clean : clean;\n+\n+\tif (ret < 0)\n+\t\trollback_lock_file(&lock);\n+\telse if (write_locked_index(o.repo->index, &lock,\n+\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\tret = error(_(\"could not write index\"));\n \n-\tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n-\t\t\t\t &result);\n \tif (ret) {\n \t\trerere(0);\n \ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 85c6a56f1b7..aaf4d880dbc 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1385,7 +1385,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \tensure_not_expanded stash &&\n \tensure_not_expanded stash list &&\n \tensure_not_expanded stash show stash@{0} &&\n-\t! ensure_not_expanded stash apply stash@{0} &&\n+\tensure_not_expanded stash apply stash@{0} &&\n \tensure_not_expanded stash drop stash@{0} &&\n \n \techo >>sparse-index/deep/new &&\n@@ -1399,7 +1399,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \toid=$(git -C sparse-index stash create) &&\n \tensure_not_expanded stash store -m \"test\" $oid &&\n \tensure_not_expanded reset --hard &&\n-\t! ensure_not_expanded stash pop\n+\tensure_not_expanded stash pop\n '\n \n test_expect_success 'sparse index is not expanded: diff' '\n-- \ngitgitgadget\n\n"},{"id":"455138","messageId":"4b4c38fcc03515c113dc1c15210be6b42baf8029.1652225552.git.gitgitgadget@gmail.com","threadId":"57792","inReplyTo":"pull.1171.v3.git.1652225552.gitgitgadget@gmail.com","subject":"[PATCH v3 6/6] unpack-trees: preserve index sparsity","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-10T23:32:32Z","receivedAt":"2022-05-10T23:33:12Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen unpacking trees, set the default sparsity of the resultant index based\non repo settings and 'is_sparse_index_allowed()'.\n\nNormally, when executing 'unpack_trees', the output index is marked sparse\nwhen (and only when) it unpacks a sparse directory. However, an index may be\n\"sparse\" even if it contains no sparse directories - when all files fall\ninside the sparse-checkout definition or otherwise have SKIP_WORKTREE\ndisabled. Therefore, the output index may be marked \"full\" even when it is\n\"sparse\", resulting in unnecessary 'ensure_full_index' calls when writing to\ndisk. Avoid this by setting the \"default\" index sparsity to match what is\nexpected for the repository.\n\nAs a consequence of this fix, the (non-merge) 'read-tree' performed when\napplying a stash with untracked files no longer expands the index. Update\nthe corresponding test in 't1092'.\n\nSigned-off-by: Victoria Dye \n---\n t/t1092-sparse-checkout-compatibility.sh | 2 +-\n unpack-trees.c | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex aaf4d880dbc..19221c14225 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1392,7 +1392,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n \tensure_not_expanded stash -u &&\n \t(\n \t\tWITHOUT_UNTRACKED_TXT=1 &&\n-\t\t! ensure_not_expanded stash pop\n+\t\tensure_not_expanded stash pop\n \t) &&\n \n \tensure_not_expanded stash create &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7f528d35cc2..a1d0ff3a4d3 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -11,6 +11,7 @@\n #include \"refs.h\"\n #include \"attr.h\"\n #include \"split-index.h\"\n+#include \"sparse-index.h\"\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n #include \"fsmonitor.h\"\n@@ -1839,6 +1840,11 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->result.fsmonitor_last_update =\n \t\txstrdup_or_null(o->src_index->fsmonitor_last_update);\n \n+\tif (!o->src_index->initialized &&\n+\t !repo->settings.command_requires_full_index &&\n+\t is_sparse_index_allowed(&o->result, 0))\n+\t\to->result.sparse_index = 1;\n+\n \t/*\n \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n \t */\n-- \ngitgitgadget\n"},{"id":"455141","messageId":"xmqqsfpg52yk.fsf@gitster.g","threadId":"57792","inReplyTo":"e733c2fd9f497a8d80555126ec2e166e182ab8db.1652225552.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/6] stash: apply stash using 'merge_ort_nonrecursive()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-11T00:26:43Z","receivedAt":"2022-05-11T00:26:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 1bfba532044..3fe549f7d3c 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -492,13 +493,13 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)\n> static int do_apply_stash(const char *prefix, struct stash_info *info,\n> \t\t\t int index, int quiet)\n> {\n> -\tint ret;\n> +\tint clean, ret;\n> \tint has_index = index;\n> \tstruct merge_options o;\n> \tstruct object_id c_tree;\n> \tstruct object_id index_tree;\n> -\tstruct commit *result;\n> -\tconst struct object_id *bases[1];\n> +\tstruct tree *head, *merge, *merge_base;\n> +\tstruct lock_file lock = LOCK_INIT;\n> \n> \tread_cache_preload(NULL);\n> \tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> @@ -541,6 +542,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> \n> \to.branch1 = \"Updated upstream\";\n> \to.branch2 = \"Stashed changes\";\n> +\to.ancestor = \"Stash base\";\n> \n> \tif (oideq(&info->b_tree, &c_tree))\n> \t\to.branch1 = \"Version stash was based on\";\n> @@ -551,10 +553,26 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> \tif (o.verbosity >= 3)\n> \t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n> \n> -\tbases[0] = &info->b_tree;\n> +\thead = lookup_tree(o.repo, &c_tree);\n> +\tmerge = lookup_tree(o.repo, &info->w_tree);\n> +\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n> +\n> +\trepo_hold_locked_index(o.repo, &lock, LOCK_DIE_ON_ERROR);\n> +\tclean = merge_ort_nonrecursive(&o, head, merge, merge_base);\n> +\n> +\t/*\n> +\t * If 'clean' >= 0, reverse the value for 'ret' so 'ret' is 0 when the\n> +\t * merge was clean, and nonzero if the merge was unclean or encountered\n> +\t * an error.\n> +\t */\n> +\tret = clean >= 0 ? !clean : clean;\n> +\n> +\tif (ret < 0)\n> +\t\trollback_lock_file(&lock);\n> +\telse if (write_locked_index(o.repo->index, &lock,\n> +\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> +\t\tret = error(_(\"could not write index\"));\n> \n> -\tret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,\n> -\t\t\t\t &result);\n\nNice. We need a bit more boilerplate code, but all we need is a three-way\nmerge without any recursive magic.\n\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 85c6a56f1b7..aaf4d880dbc 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1385,7 +1385,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n> \tensure_not_expanded stash &&\n> \tensure_not_expanded stash list &&\n> \tensure_not_expanded stash show stash@{0} &&\n> -\t! ensure_not_expanded stash apply stash@{0} &&\n> +\tensure_not_expanded stash apply stash@{0} &&\n> \tensure_not_expanded stash drop stash@{0} &&\n> \n> \techo >>sparse-index/deep/new &&\n> @@ -1399,7 +1399,7 @@ test_expect_success 'sparse-index is not expanded: stash' '\n> \toid=$(git -C sparse-index stash create) &&\n> \tensure_not_expanded stash store -m \"test\" $oid &&\n> \tensure_not_expanded reset --hard &&\n> -\t! ensure_not_expanded stash pop\n> +\tensure_not_expanded stash pop\n> '\n> \n> test_expect_success 'sparse index is not expanded: diff' '\n"},{"id":"455188","messageId":"20220512010124.1800540-1-jonathantanmy@google.com","threadId":"57792","inReplyTo":"e733c2fd9f497a8d80555126ec2e166e182ab8db.1652225552.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/6] stash: apply stash using 'merge_ort_nonrecursive()'","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-05-12T01:01:23Z","receivedAt":"2022-05-12T01:01:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"\"Victoria Dye via GitGitGadget\" writes:\n> @@ -551,10 +553,26 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> \tif (o.verbosity >= 3)\n> \t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n> \n> -\tbases[0] = &info->b_tree;\n> +\thead = lookup_tree(o.repo, &c_tree);\n> +\tmerge = lookup_tree(o.repo, &info->w_tree);\n> +\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n> +\n> +\trepo_hold_locked_index(o.repo, &lock, LOCK_DIE_ON_ERROR);\n\nWhat's the reason for locking the index? I would think that\nmerge_ort_nonrecursive() does not modify the index (in any case, I\nremoved the locking code and the tests still pass). And as for changes\nin the worktree, I ran \"strace\" on the \"stash apply\" in the test and I\ndidn't see any changes in the worktree from here until the lock is\nreleased.\n\nOther than that, this patch set looks good to me.\n"},{"id":"455199","messageId":"CABPp-BGf35aOYb-nua-i16MOL_14w9c=QETjsGnKzE6D2rr27A@mail.gmail.com","threadId":"57792","inReplyTo":"e733c2fd9f497a8d80555126ec2e166e182ab8db.1652225552.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/6] stash: apply stash using 'merge_ort_nonrecursive()'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-12T14:51:35Z","receivedAt":"2022-05-12T14:51:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 10, 2022 at 4:32 PM Victoria Dye via GitGitGadget\n wrote:\n>\n> From: Victoria Dye \n>\n> Update 'stash' to use 'merge_ort_nonrecursive()' to apply a stash to the\n> current working tree. When 'git stash apply' was converted from its shell\n> script implementation to a builtin in 8a0fc8d19d (stash: convert apply to\n> builtin, 2019-02-25), 'merge_recursive_generic()' was used to merge a stash\n> into the working tree as part of 'git stash (apply|pop)'. However, with the\n> single merge base used in 'do_apply_stash()', the commit wrapping done by\n> 'merge_recursive_generic()' is not only unnecessary, but misleading (the\n> *real* merge base is labeled \"constructed merge base\"). Therefore, a\n> non-recursive merge of the working tree, stashed tree, and stash base tree\n> is more appropriate.\n>\n> There are two options for a non-recursive merge-then-update-worktree\n> function: 'merge_trees()' and 'merge_ort_nonrecursive()'. Use\n> 'merge_ort_nonrecursive()' to align with the default merge strategy used by\n> 'git merge' (6a5fb96672 (Change default merge backend from recursive to ort,\n> 2021-08-04)) and, because merge-ort does not operate in-place on the index,\n> avoid unnecessary index expansion. Update tests in 't1092' verifying index\n> expansion for 'git stash' accordingly.\n>\n> Signed-off-by: Victoria Dye \n> ---\n> builtin/stash.c | 30 +++++++++++++++++++-----\n> t/t1092-sparse-checkout-compatibility.sh | 4 ++--\n> 2 files changed, 26 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 1bfba532044..3fe549f7d3c 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -7,6 +7,7 @@\n> #include \"cache-tree.h\"\n> #include \"unpack-trees.h\"\n> #include \"merge-recursive.h\"\n> +#include \"merge-ort-wrappers.h\"\n> #include \"strvec.h\"\n> #include \"run-command.h\"\n> #include \"dir.h\"\n> @@ -492,13 +493,13 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)\n> static int do_apply_stash(const char *prefix, struct stash_info *info,\n> int index, int quiet)\n> {\n> - int ret;\n> + int clean, ret;\n> int has_index = index;\n> struct merge_options o;\n> struct object_id c_tree;\n> struct object_id index_tree;\n> - struct commit *result;\n> - const struct object_id *bases[1];\n> + struct tree *head, *merge, *merge_base;\n> + struct lock_file lock = LOCK_INIT;\n>\n> read_cache_preload(NULL);\n> if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> @@ -541,6 +542,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>\n> o.branch1 = \"Updated upstream\";\n> o.branch2 = \"Stashed changes\";\n> + o.ancestor = \"Stash base\";\n>\n> if (oideq(&info->b_tree, &c_tree))\n> o.branch1 = \"Version stash was based on\";\n> @@ -551,10 +553,26 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> if (o.verbosity >= 3)\n> printf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n>\n> - bases[0] = &info->b_tree;\n> + head = lookup_tree(o.repo, &c_tree);\n> + merge = lookup_tree(o.repo, &info->w_tree);\n> + merge_base = lookup_tree(o.repo, &info->b_tree);\n> +\n> + repo_hold_locked_index(o.repo, &lock, LOCK_DIE_ON_ERROR);\n> + clean = merge_ort_nonrecursive(&o, head, merge, merge_base);\n\nA very minor nit: I actually have a minor dislike for the\nmerge-ort-wrappers, but I included them in case people objected to the\nslightly more verbose real API. I assumed it'd only be used to\nconvert existing calls to merge_trees() and merge_recursive(); in this\ncase you were converting a call to merge_recursive_generic(), so I\nwould have preferred using merge_incore_nonrecursive(). That might\nhave answered Jonathan's question better too when he saw the explicit\nmerge_switch_to_result() call. However, it's a minor point; I'm not\nsure it's worth a re-roll.\n\n\nThis series looks good to me:\nReviewed-by: Elijah Newren \n"},{"id":"455200","messageId":"CABPp-BFjPtzYcysjkr+q-FYJm3AJTti-YniVYnx6E8aQAkkWAw@mail.gmail.com","threadId":"57792","inReplyTo":"20220512010124.1800540-1-jonathantanmy@google.com","subject":"Re: [PATCH v3 5/6] stash: apply stash using 'merge_ort_nonrecursive()'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-12T14:52:09Z","receivedAt":"2022-05-12T14:52:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, May 11, 2022 at 6:01 PM Jonathan Tan wrote:\n>\n> \"Victoria Dye via GitGitGadget\" writes:\n> > @@ -551,10 +553,26 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> > if (o.verbosity >= 3)\n> > printf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n> >\n> > - bases[0] = &info->b_tree;\n> > + head = lookup_tree(o.repo, &c_tree);\n> > + merge = lookup_tree(o.repo, &info->w_tree);\n> > + merge_base = lookup_tree(o.repo, &info->b_tree);\n> > +\n> > + repo_hold_locked_index(o.repo, &lock, LOCK_DIE_ON_ERROR);\n>\n> What's the reason for locking the index? I would think that\n> merge_ort_nonrecursive() does not modify the index (in any case, I\n> removed the locking code and the tests still pass). And as for changes\n> in the worktree, I ran \"strace\" on the \"stash apply\" in the test and I\n> didn't see any changes in the worktree from here until the lock is\n> released.\n\nmerge_incore_nonrecursive() doesn't modify the index or working tree,\nbut merge_ort_nonrecursive() certainly modifies both in general (via\nits call to merge_switch_to_result()). I'm surprised you didn't see\nworking tree changes in a strace; was the stash being applied on top\nof some commit that had equivalent changes already in its history, so\nthat the stash application involved a merge where the working tree was\nalready up-to-date?\n\nMore generally, I think it'd be nice if stash locked the index early\nin the beginning of its process, did all the modifications, and then\nreleased the lock. But that's somewhat orthogonal to these changes,\nand it'd require getting rid of the forking-subprocesses design of\ngit-stash. Since stash is just a transliteration of a shell script,\nit locks and unlocks after each individual subcomponent instead.\n"},{"id":"455212","messageId":"20220512165547.116747-1-jonathantanmy@google.com","threadId":"57792","inReplyTo":"CABPp-BFjPtzYcysjkr+q-FYJm3AJTti-YniVYnx6E8aQAkkWAw@mail.gmail.com","subject":"Re: [PATCH v3 5/6] stash: apply stash using 'merge_ort_nonrecursive()'","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-05-12T16:55:46Z","receivedAt":"2022-05-12T16:56:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Elijah Newren writes:\n> On Wed, May 11, 2022 at 6:01 PM Jonathan Tan wrote:\n> >\n> > \"Victoria Dye via GitGitGadget\" writes:\n> > > @@ -551,10 +553,26 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> > > if (o.verbosity >= 3)\n> > > printf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n> > >\n> > > - bases[0] = &info->b_tree;\n> > > + head = lookup_tree(o.repo, &c_tree);\n> > > + merge = lookup_tree(o.repo, &info->w_tree);\n> > > + merge_base = lookup_tree(o.repo, &info->b_tree);\n> > > +\n> > > + repo_hold_locked_index(o.repo, &lock, LOCK_DIE_ON_ERROR);\n> >\n> > What's the reason for locking the index? I would think that\n> > merge_ort_nonrecursive() does not modify the index (in any case, I\n> > removed the locking code and the tests still pass). And as for changes\n> > in the worktree, I ran \"strace\" on the \"stash apply\" in the test and I\n> > didn't see any changes in the worktree from here until the lock is\n> > released.\n> \n> merge_incore_nonrecursive() doesn't modify the index or working tree,\n> but merge_ort_nonrecursive() certainly modifies both in general (via\n> its call to merge_switch_to_result()). I'm surprised you didn't see\n> working tree changes in a strace; was the stash being applied on top\n> of some commit that had equivalent changes already in its history, so\n> that the stash application involved a merge where the working tree was\n> already up-to-date?\n> \n> More generally, I think it'd be nice if stash locked the index early\n> in the beginning of its process, did all the modifications, and then\n> released the lock. But that's somewhat orthogonal to these changes,\n> and it'd require getting rid of the forking-subprocesses design of\n> git-stash. Since stash is just a transliteration of a shell script,\n> it locks and unlocks after each individual subcomponent instead.\n\nThanks for the note. Yeah..I'm not sure what happened.\n"}]}