{"thread":{"id":"54681","subject":"[PATCH 0/3] Fix stash apply in sparse checkouts (and a submodule test)","startedAt":"2020-11-20T16:54:07Z","lastAt":"2020-12-02T16:11:07Z","messageCount":14,"participants":["Elijah Newren via GitGitGadget","Chris Torek","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"410426","messageId":"pull.919.git.git.1605891222.gitgitgadget@gmail.com","threadId":"54681","inReplyTo":null,"subject":"[PATCH 0/3] Fix stash apply in sparse checkouts (and a submodule test)","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-20T16:53:39Z","receivedAt":"2020-11-20T16:54:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Heavier usage of sparse-checkouts at $DAYJOB is commencing. And an issue\nwith git stash apply was found.\n\ngit stash's implementation as a pipeline of forked commands presents some\nproblems, especially when implemented atop of three commands that all behave\ndifferently in the presence of sparse checkouts. Add a testcase\ndemonstrating some issues with git stash apply in a repository with a\ndifferent set of sparse-checkout patterns at apply vs create time, clean up\nthe relevant section of git stash code, and incidentally fix a submodule\ntestcase unrelated to sparse checkouts. Provide some detailed commit\nmessages explaining the issues along the way.\n\nNOTE: I found a couple minor issues with other commands in sparse checkouts\nwhile debugging this issue, but I don't yet have fixes for them and I can\nsubmit them separately.\n\nElijah Newren (3):\n  t7012: add a testcase demonstrating stash apply bugs in sparse\n    checkouts\n  stash: remove unnecessary process forking\n  stash: fix stash application in sparse-checkouts\n\n builtin/stash.c                  | 130 +++++++++++++++++++++----------\n t/lib-submodule-update.sh        |  16 ++--\n t/t7012-skip-worktree-writing.sh |  88 +++++++++++++++++++++\n 3 files changed, 184 insertions(+), 50 deletions(-)\n\n\nbase-commit: faefdd61ec7c7f6f3c8c9907891465ac9a2a1475\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-919%2Fnewren%2Fsparse-checkout-fixups-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-919/newren/sparse-checkout-fixups-v1\nPull-Request: https://github.com/git/git/pull/919\n-- \ngitgitgadget\n"},{"id":"410427","messageId":"de4b4d207b41c82718896f8e65dae663abd8e0bb.1605891222.git.gitgitgadget@gmail.com","threadId":"54681","inReplyTo":"pull.919.git.git.1605891222.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t7012: add a testcase demonstrating stash apply bugs in sparse checkouts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-20T16:53:40Z","receivedAt":"2020-11-20T16:54:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nApplying stashes in sparse-checkouts, particularly when the patterns\nused to define the present paths have changed between when the stash was\ncreated and when it is applied, has a number of bugs.  The primary\nproblem is perhaps that stashes can be only partially applied (sometimes\nwithout any warning or error being displayed and with 0 exit status).\nIn addition, there are cases where partial stash application comes with\nnon-translated error messages and an early abort.  The first is when\nthere are files present despite the SKIP_WORKTREE bit being set, in\nwhich case the error message shown is:\n\n    error: Entry 'PATHNAME' not uptodate. Cannot merge.\n\nThe other situation is when a stash contains new files to add to the\nworking tree; in this case, the code aborts early (but after at least\npartial stash application) with the following error message being shown:\n\n    error: NEWFILE: does not exist and --remove not passed\n    fatal: Unable to process path NEWFILE\n\nAdd a test that can trigger all three of these problems that carefully\nchecks that the working copy and SKIP_WORKTREE bits are as expected\nafter the stash application...but currently marked as expected to fail.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7012-skip-worktree-writing.sh | 88 ++++++++++++++++++++++++++++++++\n 1 file changed, 88 insertions(+)\n\ndiff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh\nindex 7476781979..a184ee97fb 100755\n--- a/t/t7012-skip-worktree-writing.sh\n+++ b/t/t7012-skip-worktree-writing.sh\n@@ -149,6 +149,94 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '\n \t\t--diff-filter=D -- keep-me.t\n '\n \n+test_expect_failure 'stash restore in sparse checkout' '\n+\ttest_create_repo stash-restore &&\n+\t(\n+\t\tcd stash-restore &&\n+\n+\t\tmkdir subdir &&\n+\t\techo A >subdir/A &&\n+\t\techo untouched >untouched &&\n+\t\techo removeme >removeme &&\n+\t\techo modified >modified &&\n+\t\tgit add . &&\n+\t\tgit commit -m Initial &&\n+\n+\t\techo AA >>subdir/A &&\n+\t\techo addme >addme &&\n+\t\techo tweaked >>modified &&\n+\t\trm removeme &&\n+\t\tgit add addme &&\n+\n+\t\tgit stash push &&\n+\n+\t\tgit sparse-checkout set subdir &&\n+\n+\t\t# Ensure after sparse-checkout we only have expected files\n+\t\tcat >expect <<-EOF &&\n+\t\tS modified\n+\t\tS removeme\n+\t\tH subdir/A\n+\t\tS untouched\n+\t\tEOF\n+\t\tgit ls-files -t >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_path_is_missing addme &&\n+\t\ttest_path_is_missing modified &&\n+\t\ttest_path_is_missing removeme &&\n+\t\ttest_path_is_file    subdir/A &&\n+\t\ttest_path_is_missing untouched &&\n+\n+\t\t# Put a file in the working directory in the way\n+\t\techo in the way >modified &&\n+\t\tgit stash apply &&\n+\n+\t\t# Ensure stash vivifies modifies paths...\n+\t\tcat >expect <<-EOF &&\n+\t\tH addme\n+\t\tH modified\n+\t\tH removeme\n+\t\tH subdir/A\n+\t\tS untouched\n+\t\tEOF\n+\t\tgit ls-files -t >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# ...and that the paths show up in status as changed...\n+\t\tcat >expect <<-EOF &&\n+\t\tA  addme\n+\t\t M modified\n+\t\t D removeme\n+\t\t M subdir/A\n+\t\t?? actual\n+\t\t?? expect\n+\t\t?? modified.stash.XXXXXX\n+\t\tEOF\n+\t\tgit status --porcelain | \\\n+\t\t\tsed -e s/stash......./stash.XXXXXX/ >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# ...and that working directory reflects the files correctly\n+\t\ttest_path_is_file    addme &&\n+\t\ttest_path_is_file    modified &&\n+\t\ttest_path_is_missing removeme &&\n+\t\ttest_path_is_file    subdir/A &&\n+\t\ttest_path_is_missing untouched &&\n+\n+\t\t# ...including that we have the expected \"modified\" file...\n+\t\tcat >expect <<-EOF &&\n+\t\tmodified\n+\t\ttweaked\n+\t\tEOF\n+\t\ttest_cmp expect modified &&\n+\n+\t\t# ...and that the other \"modified\" file is still present...\n+\t\techo in the way >expect &&\n+\t\ttest_cmp expect modified.stash.*\n+\t)\n+'\n+\n #TODO test_expect_failure 'git-apply adds file' false\n #TODO test_expect_failure 'git-apply updates file' false\n #TODO test_expect_failure 'git-apply removes file' false\n-- \ngitgitgadget\n\n"},{"id":"410428","messageId":"eb9ebcf0bd8ac946dab7bfd28a44587c5d9caf4f.1605891222.git.gitgitgadget@gmail.com","threadId":"54681","inReplyTo":"pull.919.git.git.1605891222.gitgitgadget@gmail.com","subject":"[PATCH 2/3] stash: remove unnecessary process forking","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-20T16:53:41Z","receivedAt":"2020-11-20T16:54:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nWhen stash was converted from shell to a builtin, it merely\ntransliterated the forking of various git commands from shell to a C\nprogram that would fork the same commands.  Some of those were converted\nover to actual library calls, but much of the pipeline-of-commands\ndesign still remains.  Fix some of this by replacing the portion\ncorresponding to\n\n    git diff-index --cached --name-only --diff-filter=A $CTREE >\"$a\"\n    git read-tree --reset $CTREE\n    git update-index --add --stdin <\"$a\"\n    rm -f \"$a\"\n\ninto a library function that does the same thing.  (The read-tree\n--reset was already partially converted over to a library call, but as\nan independent piece.)  Note here that this came after a merge operation\nwas performed.  The merge machinery always staged anything that cleanly\nmerges, and the above code only runs if there were no conflicts.  Its\npurpose is to make it so that when there are no conflicts, all the\nchanges from the stash are unstaged.  However, that causes brand new\nfiles from the stash to become untracked, so the code above first saves\nthose files off and then re-adds them afterwards.\n\nWe replace the whole series of commands with a simple function that will\nunstage files that are not newly added.  This doesn't fix any bugs in\nthe usage of these commands, it simply matches the existing behavior but\nmaking it an actual builtin that we can then operate on as a whole.  A\nsubsequent commit will take advantage of this to fix issues with these\ncommands in sparse-checkouts.\n\nThis conversion incidentally fixes t3906.1, because the separate\nupdate-index process would die with the following error messages:\n    error: uninitialized_sub: is a directory - add files inside instead\n    fatal: Unable to process path uninitialized_sub\nThe unstaging of the directory as a submodule meant it was no longer\ntracked, and thus as an uninitialized directory it could not be added\nback using `git update-index --add`, thus resulting in this error and\nearly abort.  Most of the submodule tests in 3906 continue to fail after\nthis change, this change was just enough to push the first of those\ntests to success.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/stash.c           | 96 ++++++++++++++++++++++-----------------\n t/lib-submodule-update.sh | 16 +++----\n 2 files changed, 62 insertions(+), 50 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 24ddb1bffa..8117d7647d 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -325,33 +325,64 @@ static void add_diff_to_buf(struct diff_queue_struct *q,\n \t}\n }\n \n-static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)\n+static void unstage_changes_unless_new(struct object_id *cache_tree)\n {\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *c_tree_hex = oid_to_hex(c_tree);\n-\n \t/*\n-\t * diff-index is very similar to diff-tree above, and should be\n-\t * converted together with update_index.\n+\t * When we enter this function, there has been a clean merge of\n+\t * relevant trees, and the merge logic always stages whatever merges\n+\t * cleanly.  We want to unstage those changes, unless it corresponds\n+\t * to a file that didn't exist as of cache_tree.\n \t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"diff-index\", \"--cached\", \"--name-only\",\n-\t\t     \"--diff-filter=A\", NULL);\n-\tstrvec_push(&cp.args, c_tree_hex);\n-\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n-}\n \n-static int update_index(struct strbuf *out)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct diff_options diff_opts;\n+\tstruct lock_file lock = LOCK_INIT;\n+\tint i;\n \n-\t/*\n-\t * Update-index is very complicated and may need to have a public\n-\t * function exposed in order to remove this forking.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"update-index\", \"--add\", \"--stdin\", NULL);\n-\treturn pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n+\tdiff_setup(&diff_opts);\n+\tdiff_opts.flags.recursive = 1;\n+\tdiff_opts.detect_rename = 0;\n+\tdiff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;\n+\tdiff_setup_done(&diff_opts);\n+\n+\tdo_diff_cache(cache_tree, &diff_opts);\n+\tdiffcore_std(&diff_opts);\n+\n+\tfor (i = 0; i < diff_queued_diff.nr; i++) {\n+\t\tstruct diff_filepair *p;\n+\t\tstruct cache_entry *ce;\n+\t\tint pos;\n+\n+\t\tp = diff_queued_diff.queue[i];\n+\t\tpos = index_name_pos(&the_index, p->two->path,\n+\t\t\t\t     strlen(p->two->path));\n+\t\tif (pos < 0) {\n+\t\t\tassert(p->one->oid_valid);\n+\t\t\tassert(!p->two->oid_valid);\n+\t\t\tce = make_cache_entry(&the_index,\n+\t\t\t\t\t      p->one->mode,\n+\t\t\t\t\t      &p->one->oid,\n+\t\t\t\t\t      p->one->path,\n+\t\t\t\t\t      0, 0);\n+\t\t\tadd_index_entry(&the_index, ce, ADD_CACHE_OK_TO_ADD);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tce = active_cache[pos];\n+\t\tif (p->one->oid_valid) {\n+\t\t\tce = make_cache_entry(&the_index,\n+\t\t\t\t\t      p->one->mode,\n+\t\t\t\t\t      &p->one->oid,\n+\t\t\t\t\t      p->one->path,\n+\t\t\t\t\t      0, 0);\n+\t\t\tadd_index_entry(&the_index, ce,\n+\t\t\t\t\tADD_CACHE_OK_TO_REPLACE);\n+\t\t}\n+\t}\n+\tdiff_flush(&diff_opts);\n+\n+\trepo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);\n+\tif (write_locked_index(&the_index, &lock,\n+\t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\tdie(_(\"Unable to write index.\"));\n }\n \n static int restore_untracked(struct object_id *u_tree)\n@@ -467,26 +498,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\tif (reset_tree(&index_tree, 0, 0))\n \t\t\treturn -1;\n \t} else {\n-\t\tstruct strbuf out = STRBUF_INIT;\n-\n-\t\tif (get_newly_staged(&out, &c_tree)) {\n-\t\t\tstrbuf_release(&out);\n-\t\t\treturn -1;\n-\t\t}\n-\n-\t\tif (reset_tree(&c_tree, 0, 1)) {\n-\t\t\tstrbuf_release(&out);\n-\t\t\treturn -1;\n-\t\t}\n-\n-\t\tret = update_index(&out);\n-\t\tstrbuf_release(&out);\n-\t\tif (ret)\n-\t\t\treturn -1;\n-\n-\t\t/* read back the result of update_index() back from the disk */\n-\t\tdiscard_cache();\n-\t\tread_cache();\n+\t\tunstage_changes_unless_new(&c_tree);\n \t}\n \n \tif (!quiet) {\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex bd3fa3c6da..4b714e9308 100644\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -316,14 +316,7 @@ test_submodule_switch_common () {\n \tcommand=\"$1\"\n \t######################### Appearing submodule #########################\n \t# Switching to a commit letting a submodule appear creates empty dir ...\n-\tif test \"$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES\" = 1\n-\tthen\n-\t\t# Restoring stash fails to restore submodule index entry\n-\t\tRESULT=\"failure\"\n-\telse\n-\t\tRESULT=\"success\"\n-\tfi\n-\ttest_expect_$RESULT \"$command: added submodule creates empty directory\" '\n+\ttest_expect_success \"$command: added submodule creates empty directory\" '\n \t\tprolog &&\n \t\treset_work_tree_to no_submodule &&\n \t\t(\n@@ -337,6 +330,13 @@ test_submodule_switch_common () {\n \t\t)\n \t'\n \t# ... and doesn't care if it already exists.\n+\tif test \"$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES\" = 1\n+\tthen\n+\t\t# Restoring stash fails to restore submodule index entry\n+\t\tRESULT=\"failure\"\n+\telse\n+\t\tRESULT=\"success\"\n+\tfi\n \ttest_expect_$RESULT \"$command: added submodule leaves existing empty directory alone\" '\n \t\tprolog &&\n \t\treset_work_tree_to no_submodule &&\n-- \ngitgitgadget\n\n"},{"id":"410429","messageId":"5143cba7047d25137b3d7f8c7811a875c1931aee.1605891222.git.gitgitgadget@gmail.com","threadId":"54681","inReplyTo":"pull.919.git.git.1605891222.gitgitgadget@gmail.com","subject":"[PATCH 3/3] stash: fix stash application in sparse-checkouts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-20T16:53:42Z","receivedAt":"2020-11-20T16:54:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nsparse-checkouts are built on the patterns in the\n$GIT_DIR/info/sparse-checkout file, where commands have modified\nbehavior for paths that do not match those patterns.  The differences in\nbehavior, as far as the bugs concerned here, fall into three different\ncategories (with git subcommands that fall into each category listed):\n\n  * commands that only look at files matching the patterns:\n      * status\n      * diff\n      * clean\n      * update-index\n  * commands that remove files from the working tree that do not match\n    the patterns, and restore files that do match them:\n      * read-tree\n      * switch\n      * checkout\n      * reset (--hard)\n  * commands that omit writing files to the working tree that do not\n    match the patterns, unless those files are not clean:\n      * merge\n      * rebase\n      * cherry-pick\n      * revert\n\nThere are some caveats above, e.g. a plain `git diff` ignores files\noutside the sparsity patterns but will show diffs for paths outside the\nsparsity patterns when revision arguments are passed.  (Technically,\ndiff is treating the sparse paths as matching HEAD.)  So, there is some\ninternal inconsistency among these commands.  There are also additional\ncommands that should behave differently in the face of sparse-checkouts,\nas the sparse-checkout documentation alludes to, but the above is\nsufficient for me to explain how `git stash` is affected.\n\nWhat is relevant here is that logically 'stash' should behave like a\nmerge; it three-way merges the changes the user had in progress at stash\ncreation time, the HEAD at the time the stash was created, and the\ncurrent HEAD, in order to get the stashed changes applied to the current\nbranch.  However, this simplistic view doesn't quite work in practice,\nbecause stash tweaks it a bit due to two factors: (1) flags like\n--keep-index and --include-untracked (why we used two different verbs,\n'keep' and 'include', is a rant for another day) modify what should be\nstaged at the end and include more things that should be quasi-merged,\n(2) stash generally wants changes to NOT be staged.  It only provides\nexceptions when (a) some of the changes had conflicts and thus we want\nto use staged to denote the clean merges and higher order stages to\nmark the conflicts, or (b) if there is a brand new file we don't want\nit to become untracked.\n\nstash has traditionally gotten this special behavior by first doing a\nmerge, and then when it's clean, applying a pipeline of commands to\nmodify the result.  This series of commands for\nunstaging-non-newly-added-files came from the following commands:\n\n    git diff-index --cached --name-only --diff-filter=A $CTREE >\"$a\"\n    git read-tree --reset $CTREE\n    git update-index --add --stdin <\"$a\"\n    rm -f \"$a\"\n\nYou might that between the merge that proceeded these commands and these\ndifferent commands here, that we have commands from each of the\ndifferent types of special sparsity handling listed at the beginning of\nthis message, and in fact this precisely led to the following buggy\nbehaviors:\n\n(1) If a path merged cleanly and it didn't match the sparsity patterns,\nthe merge backend would know to avoid writing it to the working tree and\nkeep the SKIP_WORKTREE bit, simply only updating it in the index.\nUnfortunately, the subsequent commands would essentially undo the\nchanges in the index and thus simply toss the changes altogether since\nthere was nothing left in the working tree.  This means the stash is\nonly partially applied.\n\n(2) If a path existed in the worktree before `git stash apply` despite\nhaving the SKIP_WORKTREE bit set, then the `git read-tree --reset` would\nprint an error message of the form\n      error: Entry 'modified' not uptodate. Cannot merge.\nand cause stash to abort early.\n\n(3) If there was a brand new file added by the stash, then the\ndiff-index command would save that pathname to the temporary file, the\nread-tree --reset would remove it from the index, and the update-index\ncommand would barf due to no such file being present in the working\ncopy; it would print a message of the form:\n      error: NEWFILE: does not exist and --remove not passed\n      fatal: Unable to process path NEWFILE\nand then cause stash to abort early.\n\nBasically, the whole idea of unstage-unless-brand-new requires special\ncare when you are dealing with a sparse-checkout.  Fix these problems\nby applying the following simple rule:\n\n  When we unstage files, if they have the SKIP_WORKTREE bit set,\n  clear that bit and write the file out to the working directory.\n\n  (*) If there's already a file present in the way, rename it first.\n\nThis fixes all three problems in t7012.13 and allows us to mark it as\npassing.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/stash.c                  | 36 +++++++++++++++++++++++++++++++-\n t/t7012-skip-worktree-writing.sh |  2 +-\n 2 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 8117d7647d..0f7e78d315 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -331,13 +331,23 @@ static void unstage_changes_unless_new(struct object_id *cache_tree)\n \t * When we enter this function, there has been a clean merge of\n \t * relevant trees, and the merge logic always stages whatever merges\n \t * cleanly.  We want to unstage those changes, unless it corresponds\n-\t * to a file that didn't exist as of cache_tree.\n+\t * to a file that didn't exist as of cache_tree.  However, if any\n+\t * SKIP_WORKTREE path is modified relative to cache_tree, then we\n+\t * want to clear the SKIP_WORKTREE bit and write it to the worktree\n+\t * before unstaging.\n \t */\n \n+\tstruct checkout state = CHECKOUT_INIT;\n \tstruct diff_options diff_opts;\n \tstruct lock_file lock = LOCK_INIT;\n \tint i;\n \n+\t/* If any entries have skip_worktree set, we'll have to check 'em out */\n+\tstate.force = 1;\n+\tstate.quiet = 1;\n+\tstate.refresh_cache = 1;\n+\tstate.istate = &the_index;\n+\n \tdiff_setup(&diff_opts);\n \tdiff_opts.flags.recursive = 1;\n \tdiff_opts.detect_rename = 0;\n@@ -367,6 +377,30 @@ static void unstage_changes_unless_new(struct object_id *cache_tree)\n \t\t\tcontinue;\n \t\t}\n \t\tce = active_cache[pos];\n+\t\tif (ce_skip_worktree(ce)) {\n+\t\t\tstruct stat st;\n+\t\t\tif (!lstat(ce->name, &st)) {\n+\t\t\t\tstruct strbuf new_path = STRBUF_INIT;\n+\t\t\t\tint fd;\n+\n+\t\t\t\tstrbuf_addf(&new_path,\n+\t\t\t\t\t    \"%s.stash.XXXXXX\", ce->name);\n+\t\t\t\tfd = xmkstemp(new_path.buf);\n+\t\t\t\tclose(fd);\n+\t\t\t\tprintf(_(\"WARNING: Untracked file in way of \"\n+\t\t\t\t\t \"tracked file!  Renaming\\n \"\n+\t\t\t\t\t \"           %s -> %s\\n\"\n+\t\t\t\t\t \"         to make room.\\n\"),\n+\t\t\t\t       ce->name, new_path.buf);\n+\t\t\t\tif (rename(ce->name, new_path.buf))\n+\t\t\t\t\tdie(\"Failed to move %s to %s\\n\",\n+\t\t\t\t\t    ce->name, new_path.buf);\n+\t\t\t\tstrbuf_release(&new_path);\n+\t\t\t}\n+\t\t\tcheckout_entry(ce, &state, NULL, NULL);\n+\t\t}\n+\n+\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n \t\tif (p->one->oid_valid) {\n \t\t\tce = make_cache_entry(&the_index,\n \t\t\t\t\t      p->one->mode,\ndiff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh\nindex a184ee97fb..e5c6a038fb 100755\n--- a/t/t7012-skip-worktree-writing.sh\n+++ b/t/t7012-skip-worktree-writing.sh\n@@ -149,7 +149,7 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '\n \t\t--diff-filter=D -- keep-me.t\n '\n \n-test_expect_failure 'stash restore in sparse checkout' '\n+test_expect_success 'stash restore in sparse checkout' '\n \ttest_create_repo stash-restore &&\n \t(\n \t\tcd stash-restore &&\n-- \ngitgitgadget\n"},{"id":"410457","messageId":"CAPx1GvfaZ-pRznnOQELBYQ7LCk-sr_-Rd0aPcJ6ghM5yt8LpnA@mail.gmail.com","threadId":"54681","inReplyTo":"5143cba7047d25137b3d7f8c7811a875c1931aee.1605891222.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] stash: fix stash application in sparse-checkouts","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-11-21T12:47:13Z","receivedAt":"2020-11-21T12:47:47Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Fri, Nov 20, 2020 at 8:56 AM Elijah Newren via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> branch.  However, this simplistic view doesn't quite work in practice,\n> because stash tweaks it a bit due to two factors: (1) flags like\n> --keep-index and --include-untracked (why we used two different verbs,\n> 'keep' and 'include', is a rant for another day)\n\n:-)\n\nNot that this should affect any of these changes, but I'd also note\nthat the fact that using `-u` or  `-a` makes a third commit that cannot\nbe ignored later is a problem as well.  (`git stash list` should probably\nannotate the listed stashes as to whether they are two- or three-commit\nstashes.)\n\n> stash has traditionally gotten this special behavior by first doing a\n> merge, and then when it's clean, applying a pipeline of commands to\n> modify the result.  This series of commands for\n> unstaging-non-newly-added-files came from the following commands:\n>\n>     git diff-index --cached --name-only --diff-filter=A $CTREE >\"$a\"\n>     git read-tree --reset $CTREE\n>     git update-index --add --stdin <\"$a\"\n>     rm -f \"$a\"\n>\n> You might that between the merge that proceeded these commands and these\n\ns/might/might think/, perhaps?  And, s/proceeded/preceded/, probably.\n\n(I didn't look closely at the rest of this but the idea seems sound.)\n\nChris\n"},{"id":"410483","messageId":"CABPp-BELMS9o2+cCsefY-p9fF1j4BiYFaeYpLOHNmYXH3YVrpw@mail.gmail.com","threadId":"54681","inReplyTo":"CAPx1GvfaZ-pRznnOQELBYQ7LCk-sr_-Rd0aPcJ6ghM5yt8LpnA@mail.gmail.com","subject":"Re: [PATCH 3/3] stash: fix stash application in sparse-checkouts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-11-22T03:47:43Z","receivedAt":"2020-11-22T03:48:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Nov 21, 2020 at 4:47 AM Chris Torek <chris.torek@gmail.com> wrote:\n>\n> On Fri, Nov 20, 2020 at 8:56 AM Elijah Newren via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> > branch.  However, this simplistic view doesn't quite work in practice,\n> > because stash tweaks it a bit due to two factors: (1) flags like\n> > --keep-index and --include-untracked (why we used two different verbs,\n> > 'keep' and 'include', is a rant for another day)\n>\n> :-)\n>\n> Not that this should affect any of these changes, but I'd also note\n> that the fact that using `-u` or  `-a` makes a third commit that cannot\n> be ignored later is a problem as well.  (`git stash list` should probably\n> annotate the listed stashes as to whether they are two- or three-commit\n> stashes.)\n\nYeah, it seems a bit odd when you compare to saving and applying\ncached changes; for that, stash requires you to specify BOTH\n--keep-index to to push/save, AND --index to pop/apply.\n\nHowever, for saving and applying untracked or ignored changes, you\nmerely need to record them at push/save time and they automatically\nalways apply.\n\nNot very consistent.  (But, again, unrelated to these patches, so this\nis mostly just recreational complaining at this point.  I'm not\nplanning to tackle these bits.)\n\n> > stash has traditionally gotten this special behavior by first doing a\n> > merge, and then when it's clean, applying a pipeline of commands to\n> > modify the result.  This series of commands for\n> > unstaging-non-newly-added-files came from the following commands:\n> >\n> >     git diff-index --cached --name-only --diff-filter=A $CTREE >\"$a\"\n> >     git read-tree --reset $CTREE\n> >     git update-index --add --stdin <\"$a\"\n> >     rm -f \"$a\"\n> >\n> > You might that between the merge that proceeded these commands and these\n>\n> s/might/might think/, perhaps?  And, s/proceeded/preceded/, probably.\n\nIndeed; thanks for catching those.  I will fix them up.\n\n> (I didn't look closely at the rest of this but the idea seems sound.)\n>\n> Chris\n"},{"id":"410820","messageId":"xmqqlfepcceq.fsf@gitster.c.googlers.com","threadId":"54681","inReplyTo":"pull.919.git.git.1605891222.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Fix stash apply in sparse checkouts (and a submodule test)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T22:14:05Z","receivedAt":"2020-11-25T22:14:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Heavier usage of sparse-checkouts at $DAYJOB is commencing. And an issue\n> with git stash apply was found.\n>\n> git stash's implementation as a pipeline of forked commands presents some\n> problems, especially when implemented atop of three commands that all behave\n> differently in the presence of sparse checkouts. Add a testcase\n> demonstrating some issues with git stash apply in a repository with a\n> different set of sparse-checkout patterns at apply vs create time, clean up\n> the relevant section of git stash code, and incidentally fix a submodule\n> testcase unrelated to sparse checkouts. Provide some detailed commit\n> messages explaining the issues along the way.\n>\n> NOTE: I found a couple minor issues with other commands in sparse checkouts\n> while debugging this issue, but I don't yet have fixes for them and I can\n> submit them separately.\n\nAny comments on this from reviewers?  The second patch is a but too\nbusy looking and I am having a bit of trouble convincing myself that\nit is doing the right thing.\n\nThanks.\n"},{"id":"410864","messageId":"CABPp-BF5GgpXdo+8TwBPPtxJWot+6h+9oLkWMTtiiqy_uNDKMw@mail.gmail.com","threadId":"54681","inReplyTo":"xmqqlfepcceq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/3] Fix stash apply in sparse checkouts (and a submodule test)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-11-26T05:31:56Z","receivedAt":"2020-11-26T05:32:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Nov 25, 2020 at 2:14 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > Heavier usage of sparse-checkouts at $DAYJOB is commencing. And an issue\n> > with git stash apply was found.\n> >\n> > git stash's implementation as a pipeline of forked commands presents some\n> > problems, especially when implemented atop of three commands that all behave\n> > differently in the presence of sparse checkouts. Add a testcase\n> > demonstrating some issues with git stash apply in a repository with a\n> > different set of sparse-checkout patterns at apply vs create time, clean up\n> > the relevant section of git stash code, and incidentally fix a submodule\n> > testcase unrelated to sparse checkouts. Provide some detailed commit\n> > messages explaining the issues along the way.\n> >\n> > NOTE: I found a couple minor issues with other commands in sparse checkouts\n> > while debugging this issue, but I don't yet have fixes for them and I can\n> > submit them separately.\n>\n> Any comments on this from reviewers?  The second patch is a but too\n> busy looking and I am having a bit of trouble convincing myself that\n> it is doing the right thing.\n\nHmm, that diff is a little hard to read.  It's a removal of two\nfunctions, and an addition of a new one, but the way the diff reads it\nlooks like I'm modifying the existing functions because it catches\nsome comment line markers and thinks they're similar.  Maybe it'd be\neasier to read if I inserted that function elsewhere in the file?\n\nI'll send a re-roll with that and add a comment or two to help explain\nit (as well as fix up the small issues Chris highlighted).\n"},{"id":"411110","messageId":"pull.919.v2.git.git.1606861519.gitgitgadget@gmail.com","threadId":"54681","inReplyTo":"pull.919.git.git.1605891222.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Fix stash apply in sparse checkouts (and a submodule test)","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-01T22:25:15Z","receivedAt":"2020-12-01T22:26:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series fixes some issues with git stash apply in sparse checkouts, when\nthe sparsity patterns have changed between when the stash was created and\nwhen the stash is being applied. It also incidentally fixes a submodule\ntestcase unrelated to sparse checkouts.\n\nChanges since v1:\n\n * Commit message cleanup, including a couple wording issues highlighted by\n   Chris\n * Cleaned up the code a bit and commented it better, to try to make it\n   easier for Junio (and others) to follow.\n\nElijah Newren (3):\n  t7012: add a testcase demonstrating stash apply bugs in sparse\n    checkouts\n  stash: remove unnecessary process forking\n  stash: fix stash application in sparse-checkouts\n\n builtin/stash.c                  | 165 ++++++++++++++++++++++---------\n t/lib-submodule-update.sh        |  16 +--\n t/t7012-skip-worktree-writing.sh |  88 +++++++++++++++++\n 3 files changed, 212 insertions(+), 57 deletions(-)\n\n\nbase-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-919%2Fnewren%2Fsparse-checkout-fixups-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-919/newren/sparse-checkout-fixups-v2\nPull-Request: https://github.com/git/git/pull/919\n\nRange-diff vs v1:\n\n 1:  de4b4d207b ! 1:  2155bbfe20 t7012: add a testcase demonstrating stash apply bugs in sparse checkouts\n     @@ Commit message\n          t7012: add a testcase demonstrating stash apply bugs in sparse checkouts\n      \n          Applying stashes in sparse-checkouts, particularly when the patterns\n     -    used to define the present paths have changed between when the stash was\n     +    used to define the sparseness have changed between when the stash was\n          created and when it is applied, has a number of bugs.  The primary\n     -    problem is perhaps that stashes can be only partially applied (sometimes\n     -    without any warning or error being displayed and with 0 exit status).\n     -    In addition, there are cases where partial stash application comes with\n     -    non-translated error messages and an early abort.  The first is when\n     -    there are files present despite the SKIP_WORKTREE bit being set, in\n     -    which case the error message shown is:\n     +    problem is that stashes are sometimes only partially applied.  In most\n     +    such cases, it does so silently without any warning or error being\n     +    displayed and with 0 exit status.\n     +\n     +    There are, however, a few cases when non-translated error messages are\n     +    shown and the stash application aborts early.  The first is when there\n     +    are files present despite the SKIP_WORKTREE bit being set, in which case\n     +    the error message shown is:\n      \n              error: Entry 'PATHNAME' not uptodate. Cannot merge.\n      \n          The other situation is when a stash contains new files to add to the\n     -    working tree; in this case, the code aborts early (but after at least\n     -    partial stash application) with the following error message being shown:\n     +    working tree; in this case, the code aborts early but still has the\n     +    stash partially applied, and shows the following error message:\n      \n              error: NEWFILE: does not exist and --remove not passed\n              fatal: Unable to process path NEWFILE\n      \n     -    Add a test that can trigger all three of these problems that carefully\n     -    checks that the working copy and SKIP_WORKTREE bits are as expected\n     -    after the stash application...but currently marked as expected to fail.\n     +    Add a test that can trigger all three of these problems.  Have it\n     +    carefully check that the working copy and SKIP_WORKTREE bits are as\n     +    expected after the stash application.  The test is currently marked as\n     +    expected to fail, but subsequent commits will implement the fixes and\n     +    toggle the expectation.\n      \n          Signed-off-by: Elijah Newren <newren@gmail.com>\n      \n 2:  eb9ebcf0bd ! 2:  1fa263cf3c stash: remove unnecessary process forking\n     @@ Commit message\n          into a library function that does the same thing.  (The read-tree\n          --reset was already partially converted over to a library call, but as\n          an independent piece.)  Note here that this came after a merge operation\n     -    was performed.  The merge machinery always staged anything that cleanly\n     -    merges, and the above code only runs if there were no conflicts.  Its\n     +    was performed.  The merge machinery always stages anything that cleanly\n     +    merges, and the above code only runs if there are no conflicts.  Its\n          purpose is to make it so that when there are no conflicts, all the\n          changes from the stash are unstaged.  However, that causes brand new\n          files from the stash to become untracked, so the code above first saves\n     @@ Commit message\n          We replace the whole series of commands with a simple function that will\n          unstage files that are not newly added.  This doesn't fix any bugs in\n          the usage of these commands, it simply matches the existing behavior but\n     -    making it an actual builtin that we can then operate on as a whole.  A\n     -    subsequent commit will take advantage of this to fix issues with these\n     -    commands in sparse-checkouts.\n     +    makes it into a single atomic operation that we can then operate on as a\n     +    whole.  A subsequent commit will take advantage of this to fix issues\n     +    with these commands in sparse-checkouts.\n      \n          This conversion incidentally fixes t3906.1, because the separate\n          update-index process would die with the following error messages:\n     @@ builtin/stash.c: static void add_diff_to_buf(struct diff_queue_struct *q,\n       }\n       \n      -static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)\n     -+static void unstage_changes_unless_new(struct object_id *cache_tree)\n     - {\n     +-{\n      -\tstruct child_process cp = CHILD_PROCESS_INIT;\n      -\tconst char *c_tree_hex = oid_to_hex(c_tree);\n      -\n     - \t/*\n     +-\t/*\n      -\t * diff-index is very similar to diff-tree above, and should be\n      -\t * converted together with update_index.\n     -+\t * When we enter this function, there has been a clean merge of\n     -+\t * relevant trees, and the merge logic always stages whatever merges\n     -+\t * cleanly.  We want to unstage those changes, unless it corresponds\n     -+\t * to a file that didn't exist as of cache_tree.\n     - \t */\n     +-\t */\n      -\tcp.git_cmd = 1;\n      -\tstrvec_pushl(&cp.args, \"diff-index\", \"--cached\", \"--name-only\",\n      -\t\t     \"--diff-filter=A\", NULL);\n      -\tstrvec_push(&cp.args, c_tree_hex);\n      -\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n      -}\n     - \n     +-\n      -static int update_index(struct strbuf *out)\n      -{\n      -\tstruct child_process cp = CHILD_PROCESS_INIT;\n     -+\tstruct diff_options diff_opts;\n     -+\tstruct lock_file lock = LOCK_INIT;\n     -+\tint i;\n     - \n     +-\n      -\t/*\n      -\t * Update-index is very complicated and may need to have a public\n      -\t * function exposed in order to remove this forking.\n     @@ builtin/stash.c: static void add_diff_to_buf(struct diff_queue_struct *q,\n      -\tcp.git_cmd = 1;\n      -\tstrvec_pushl(&cp.args, \"update-index\", \"--add\", \"--stdin\", NULL);\n      -\treturn pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n     +-}\n     +-\n     + static int restore_untracked(struct object_id *u_tree)\n     + {\n     + \tint res;\n     +@@ builtin/stash.c: static int restore_untracked(struct object_id *u_tree)\n     + \treturn res;\n     + }\n     + \n     ++static void unstage_changes_unless_new(struct object_id *orig_tree)\n     ++{\n     ++\t/*\n     ++\t * When we enter this function, there has been a clean merge of\n     ++\t * relevant trees, and the merge logic always stages whatever merges\n     ++\t * cleanly.  We want to unstage those changes, unless it corresponds\n     ++\t * to a file that didn't exist as of orig_tree.\n     ++\t */\n     ++\n     ++\tstruct diff_options diff_opts;\n     ++\tstruct lock_file lock = LOCK_INIT;\n     ++\tint i;\n     ++\n     ++\t/*\n     ++\t * Step 1: get a difference between orig_tree (which corresponding\n     ++\t * to the index before a merge was run) and the current index\n     ++\t * (reflecting the changes brought in by the merge).\n     ++\t */\n      +\tdiff_setup(&diff_opts);\n      +\tdiff_opts.flags.recursive = 1;\n      +\tdiff_opts.detect_rename = 0;\n      +\tdiff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;\n      +\tdiff_setup_done(&diff_opts);\n      +\n     -+\tdo_diff_cache(cache_tree, &diff_opts);\n     ++\tdo_diff_cache(orig_tree, &diff_opts);\n      +\tdiffcore_std(&diff_opts);\n      +\n     ++\t/* Iterate over the paths that changed due to the merge... */\n      +\tfor (i = 0; i < diff_queued_diff.nr; i++) {\n      +\t\tstruct diff_filepair *p;\n      +\t\tstruct cache_entry *ce;\n      +\t\tint pos;\n      +\n     ++\t\t/* Look up the path's position in the current index. */\n      +\t\tp = diff_queued_diff.queue[i];\n      +\t\tpos = index_name_pos(&the_index, p->two->path,\n      +\t\t\t\t     strlen(p->two->path));\n     -+\t\tif (pos < 0) {\n     -+\t\t\tassert(p->one->oid_valid);\n     -+\t\t\tassert(!p->two->oid_valid);\n     -+\t\t\tce = make_cache_entry(&the_index,\n     -+\t\t\t\t\t      p->one->mode,\n     -+\t\t\t\t\t      &p->one->oid,\n     -+\t\t\t\t\t      p->one->path,\n     -+\t\t\t\t\t      0, 0);\n     -+\t\t\tadd_index_entry(&the_index, ce, ADD_CACHE_OK_TO_ADD);\n     -+\t\t\tcontinue;\n     -+\t\t}\n     -+\t\tce = active_cache[pos];\n     ++\n     ++\t\t/*\n     ++\t\t * Step 2: \"unstage\" changes, as long as they are still tracked\n     ++\t\t */\n      +\t\tif (p->one->oid_valid) {\n     ++\t\t\t/*\n     ++\t\t\t * Path existed in orig_tree; restore index entry\n     ++\t\t\t * from that tree in order to \"unstage\" the changes.\n     ++\t\t\t */\n     ++\t\t\tint option = ADD_CACHE_OK_TO_REPLACE;\n     ++\t\t\tif (pos < 0)\n     ++\t\t\t\toption = ADD_CACHE_OK_TO_ADD;\n     ++\n      +\t\t\tce = make_cache_entry(&the_index,\n      +\t\t\t\t\t      p->one->mode,\n      +\t\t\t\t\t      &p->one->oid,\n      +\t\t\t\t\t      p->one->path,\n      +\t\t\t\t\t      0, 0);\n     -+\t\t\tadd_index_entry(&the_index, ce,\n     -+\t\t\t\t\tADD_CACHE_OK_TO_REPLACE);\n     ++\t\t\tadd_index_entry(&the_index, ce, option);\n      +\t\t}\n      +\t}\n      +\tdiff_flush(&diff_opts);\n      +\n     ++\t/*\n     ++\t * Step 3: write the new index to disk\n     ++\t */\n      +\trepo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);\n      +\tif (write_locked_index(&the_index, &lock,\n      +\t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n      +\t\tdie(_(\"Unable to write index.\"));\n     - }\n     - \n     - static int restore_untracked(struct object_id *u_tree)\n     ++}\n     ++\n     + static int do_apply_stash(const char *prefix, struct stash_info *info,\n     + \t\t\t  int index, int quiet)\n     + {\n      @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info *info,\n       \t\tif (reset_tree(&index_tree, 0, 0))\n       \t\t\treturn -1;\n 3:  5143cba704 ! 3:  ccfedc7140 stash: fix stash application in sparse-checkouts\n     @@ Commit message\n          staged at the end and include more things that should be quasi-merged,\n          (2) stash generally wants changes to NOT be staged.  It only provides\n          exceptions when (a) some of the changes had conflicts and thus we want\n     -    to use staged to denote the clean merges and higher order stages to\n     +    to use stages to denote the clean merges and higher order stages to\n          mark the conflicts, or (b) if there is a brand new file we don't want\n          it to become untracked.\n      \n     @@ Commit message\n              git update-index --add --stdin <\"$a\"\n              rm -f \"$a\"\n      \n     -    You might that between the merge that proceeded these commands and these\n     -    different commands here, that we have commands from each of the\n     -    different types of special sparsity handling listed at the beginning of\n     -    this message, and in fact this precisely led to the following buggy\n     -    behaviors:\n     +    Looking back at the different types of special sparsity handling listed\n     +    at the beginning of this message, you may note that we have at least one\n     +    of each type covered here: merge, diff-index, and read-tree.  The weird\n     +    mix-and-match led to 3 different bugs:\n      \n          (1) If a path merged cleanly and it didn't match the sparsity patterns,\n          the merge backend would know to avoid writing it to the working tree and\n     @@ Commit message\n          Signed-off-by: Elijah Newren <newren@gmail.com>\n      \n       ## builtin/stash.c ##\n     -@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *cache_tree)\n     - \t * When we enter this function, there has been a clean merge of\n     +@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)\n       \t * relevant trees, and the merge logic always stages whatever merges\n       \t * cleanly.  We want to unstage those changes, unless it corresponds\n     --\t * to a file that didn't exist as of cache_tree.\n     -+\t * to a file that didn't exist as of cache_tree.  However, if any\n     -+\t * SKIP_WORKTREE path is modified relative to cache_tree, then we\n     -+\t * want to clear the SKIP_WORKTREE bit and write it to the worktree\n     -+\t * before unstaging.\n     + \t * to a file that didn't exist as of orig_tree.\n     ++\t *\n     ++\t * However, if any SKIP_WORKTREE path is modified relative to\n     ++\t * orig_tree, then we want to clear the SKIP_WORKTREE bit and write\n     ++\t * it to the worktree before unstaging.\n       \t */\n       \n      +\tstruct checkout state = CHECKOUT_INIT;\n     @@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *cache_\n      +\tstate.refresh_cache = 1;\n      +\tstate.istate = &the_index;\n      +\n     - \tdiff_setup(&diff_opts);\n     - \tdiff_opts.flags.recursive = 1;\n     - \tdiff_opts.detect_rename = 0;\n     -@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *cache_tree)\n     - \t\t\tcontinue;\n     - \t\t}\n     - \t\tce = active_cache[pos];\n     -+\t\tif (ce_skip_worktree(ce)) {\n     + \t/*\n     + \t * Step 1: get a difference between orig_tree (which corresponding\n     + \t * to the index before a merge was run) and the current index\n     +@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)\n     + \t\t\t\t     strlen(p->two->path));\n     + \n     + \t\t/*\n     +-\t\t * Step 2: \"unstage\" changes, as long as they are still tracked\n     ++\t\t * Step 2: Place changes in the working tree\n     ++\t\t *\n     ++\t\t * Stash is about restoring changes *to the working tree*.\n     ++\t\t * So if the merge successfully got a new version of some\n     ++\t\t * path, but left it out of the working tree, then clear the\n     ++\t\t * SKIP_WORKTREE bit and write it to the working tree.\n     ++\t\t */\n     ++\t\tif (pos >= 0 && ce_skip_worktree(active_cache[pos])) {\n      +\t\t\tstruct stat st;\n     ++\n     ++\t\t\tce = active_cache[pos];\n      +\t\t\tif (!lstat(ce->name, &st)) {\n     ++\t\t\t\t/* Conflicting path present; relocate it */\n      +\t\t\t\tstruct strbuf new_path = STRBUF_INIT;\n      +\t\t\t\tint fd;\n      +\n     @@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *cache_\n      +\t\t\t\tstrbuf_release(&new_path);\n      +\t\t\t}\n      +\t\t\tcheckout_entry(ce, &state, NULL, NULL);\n     ++\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n      +\t\t}\n      +\n     -+\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n     ++\t\t/*\n     ++\t\t * Step 3: \"unstage\" changes, as long as they are still tracked\n     + \t\t */\n       \t\tif (p->one->oid_valid) {\n     - \t\t\tce = make_cache_entry(&the_index,\n     - \t\t\t\t\t      p->one->mode,\n     + \t\t\t/*\n     +@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)\n     + \tdiff_flush(&diff_opts);\n     + \n     + \t/*\n     +-\t * Step 3: write the new index to disk\n     ++\t * Step 4: write the new index to disk\n     + \t */\n     + \trepo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);\n     + \tif (write_locked_index(&the_index, &lock,\n      \n       ## t/t7012-skip-worktree-writing.sh ##\n      @@ t/t7012-skip-worktree-writing.sh: test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '\n\n-- \ngitgitgadget\n"},{"id":"411111","messageId":"1fa263cf3c3d1b0c20ad89e6454a7b903a07f193.1606861519.git.gitgitgadget@gmail.com","threadId":"54681","inReplyTo":"pull.919.v2.git.git.1606861519.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] stash: remove unnecessary process forking","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-01T22:25:17Z","receivedAt":"2020-12-01T22:26:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nWhen stash was converted from shell to a builtin, it merely\ntransliterated the forking of various git commands from shell to a C\nprogram that would fork the same commands.  Some of those were converted\nover to actual library calls, but much of the pipeline-of-commands\ndesign still remains.  Fix some of this by replacing the portion\ncorresponding to\n\n    git diff-index --cached --name-only --diff-filter=A $CTREE >\"$a\"\n    git read-tree --reset $CTREE\n    git update-index --add --stdin <\"$a\"\n    rm -f \"$a\"\n\ninto a library function that does the same thing.  (The read-tree\n--reset was already partially converted over to a library call, but as\nan independent piece.)  Note here that this came after a merge operation\nwas performed.  The merge machinery always stages anything that cleanly\nmerges, and the above code only runs if there are no conflicts.  Its\npurpose is to make it so that when there are no conflicts, all the\nchanges from the stash are unstaged.  However, that causes brand new\nfiles from the stash to become untracked, so the code above first saves\nthose files off and then re-adds them afterwards.\n\nWe replace the whole series of commands with a simple function that will\nunstage files that are not newly added.  This doesn't fix any bugs in\nthe usage of these commands, it simply matches the existing behavior but\nmakes it into a single atomic operation that we can then operate on as a\nwhole.  A subsequent commit will take advantage of this to fix issues\nwith these commands in sparse-checkouts.\n\nThis conversion incidentally fixes t3906.1, because the separate\nupdate-index process would die with the following error messages:\n    error: uninitialized_sub: is a directory - add files inside instead\n    fatal: Unable to process path uninitialized_sub\nThe unstaging of the directory as a submodule meant it was no longer\ntracked, and thus as an uninitialized directory it could not be added\nback using `git update-index --add`, thus resulting in this error and\nearly abort.  Most of the submodule tests in 3906 continue to fail after\nthis change, this change was just enough to push the first of those\ntests to success.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/stash.c           | 119 ++++++++++++++++++++++----------------\n t/lib-submodule-update.sh |  16 ++---\n 2 files changed, 78 insertions(+), 57 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 3f811f3050..a5465b565a 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -325,35 +325,6 @@ static void add_diff_to_buf(struct diff_queue_struct *q,\n \t}\n }\n \n-static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *c_tree_hex = oid_to_hex(c_tree);\n-\n-\t/*\n-\t * diff-index is very similar to diff-tree above, and should be\n-\t * converted together with update_index.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"diff-index\", \"--cached\", \"--name-only\",\n-\t\t     \"--diff-filter=A\", NULL);\n-\tstrvec_push(&cp.args, c_tree_hex);\n-\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n-}\n-\n-static int update_index(struct strbuf *out)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Update-index is very complicated and may need to have a public\n-\t * function exposed in order to remove this forking.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"update-index\", \"--add\", \"--stdin\", NULL);\n-\treturn pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n-}\n-\n static int restore_untracked(struct object_id *u_tree)\n {\n \tint res;\n@@ -385,6 +356,75 @@ static int restore_untracked(struct object_id *u_tree)\n \treturn res;\n }\n \n+static void unstage_changes_unless_new(struct object_id *orig_tree)\n+{\n+\t/*\n+\t * When we enter this function, there has been a clean merge of\n+\t * relevant trees, and the merge logic always stages whatever merges\n+\t * cleanly.  We want to unstage those changes, unless it corresponds\n+\t * to a file that didn't exist as of orig_tree.\n+\t */\n+\n+\tstruct diff_options diff_opts;\n+\tstruct lock_file lock = LOCK_INIT;\n+\tint i;\n+\n+\t/*\n+\t * Step 1: get a difference between orig_tree (which corresponding\n+\t * to the index before a merge was run) and the current index\n+\t * (reflecting the changes brought in by the merge).\n+\t */\n+\tdiff_setup(&diff_opts);\n+\tdiff_opts.flags.recursive = 1;\n+\tdiff_opts.detect_rename = 0;\n+\tdiff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;\n+\tdiff_setup_done(&diff_opts);\n+\n+\tdo_diff_cache(orig_tree, &diff_opts);\n+\tdiffcore_std(&diff_opts);\n+\n+\t/* Iterate over the paths that changed due to the merge... */\n+\tfor (i = 0; i < diff_queued_diff.nr; i++) {\n+\t\tstruct diff_filepair *p;\n+\t\tstruct cache_entry *ce;\n+\t\tint pos;\n+\n+\t\t/* Look up the path's position in the current index. */\n+\t\tp = diff_queued_diff.queue[i];\n+\t\tpos = index_name_pos(&the_index, p->two->path,\n+\t\t\t\t     strlen(p->two->path));\n+\n+\t\t/*\n+\t\t * Step 2: \"unstage\" changes, as long as they are still tracked\n+\t\t */\n+\t\tif (p->one->oid_valid) {\n+\t\t\t/*\n+\t\t\t * Path existed in orig_tree; restore index entry\n+\t\t\t * from that tree in order to \"unstage\" the changes.\n+\t\t\t */\n+\t\t\tint option = ADD_CACHE_OK_TO_REPLACE;\n+\t\t\tif (pos < 0)\n+\t\t\t\toption = ADD_CACHE_OK_TO_ADD;\n+\n+\t\t\tce = make_cache_entry(&the_index,\n+\t\t\t\t\t      p->one->mode,\n+\t\t\t\t\t      &p->one->oid,\n+\t\t\t\t\t      p->one->path,\n+\t\t\t\t\t      0, 0);\n+\t\t\tadd_index_entry(&the_index, ce, option);\n+\t\t}\n+\t}\n+\tdiff_flush(&diff_opts);\n+\n+\t/*\n+\t * Step 3: write the new index to disk\n+\t */\n+\trepo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);\n+\tif (write_locked_index(&the_index, &lock,\n+\t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\tdie(_(\"Unable to write index.\"));\n+}\n+\n static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\t\t  int index, int quiet)\n {\n@@ -467,26 +507,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\tif (reset_tree(&index_tree, 0, 0))\n \t\t\treturn -1;\n \t} else {\n-\t\tstruct strbuf out = STRBUF_INIT;\n-\n-\t\tif (get_newly_staged(&out, &c_tree)) {\n-\t\t\tstrbuf_release(&out);\n-\t\t\treturn -1;\n-\t\t}\n-\n-\t\tif (reset_tree(&c_tree, 0, 1)) {\n-\t\t\tstrbuf_release(&out);\n-\t\t\treturn -1;\n-\t\t}\n-\n-\t\tret = update_index(&out);\n-\t\tstrbuf_release(&out);\n-\t\tif (ret)\n-\t\t\treturn -1;\n-\n-\t\t/* read back the result of update_index() back from the disk */\n-\t\tdiscard_cache();\n-\t\tread_cache();\n+\t\tunstage_changes_unless_new(&c_tree);\n \t}\n \n \tif (!quiet) {\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 87a759149f..c6d7aab6df 100644\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -316,14 +316,7 @@ test_submodule_switch_common () {\n \tcommand=\"$1\"\n \t######################### Appearing submodule #########################\n \t# Switching to a commit letting a submodule appear creates empty dir ...\n-\tif test \"$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES\" = 1\n-\tthen\n-\t\t# Restoring stash fails to restore submodule index entry\n-\t\tRESULT=\"failure\"\n-\telse\n-\t\tRESULT=\"success\"\n-\tfi\n-\ttest_expect_$RESULT \"$command: added submodule creates empty directory\" '\n+\ttest_expect_success \"$command: added submodule creates empty directory\" '\n \t\tprolog &&\n \t\treset_work_tree_to no_submodule &&\n \t\t(\n@@ -337,6 +330,13 @@ test_submodule_switch_common () {\n \t\t)\n \t'\n \t# ... and doesn't care if it already exists.\n+\tif test \"$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES\" = 1\n+\tthen\n+\t\t# Restoring stash fails to restore submodule index entry\n+\t\tRESULT=\"failure\"\n+\telse\n+\t\tRESULT=\"success\"\n+\tfi\n \ttest_expect_$RESULT \"$command: added submodule leaves existing empty directory alone\" '\n \t\tprolog &&\n \t\treset_work_tree_to no_submodule &&\n-- \ngitgitgadget\n\n"},{"id":"411112","messageId":"ccfedc7140dbf63ba26a15f93bd3885180b26517.1606861519.git.gitgitgadget@gmail.com","threadId":"54681","inReplyTo":"pull.919.v2.git.git.1606861519.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] stash: fix stash application in sparse-checkouts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-01T22:25:18Z","receivedAt":"2020-12-01T22:26:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nsparse-checkouts are built on the patterns in the\n$GIT_DIR/info/sparse-checkout file, where commands have modified\nbehavior for paths that do not match those patterns.  The differences in\nbehavior, as far as the bugs concerned here, fall into three different\ncategories (with git subcommands that fall into each category listed):\n\n  * commands that only look at files matching the patterns:\n      * status\n      * diff\n      * clean\n      * update-index\n  * commands that remove files from the working tree that do not match\n    the patterns, and restore files that do match them:\n      * read-tree\n      * switch\n      * checkout\n      * reset (--hard)\n  * commands that omit writing files to the working tree that do not\n    match the patterns, unless those files are not clean:\n      * merge\n      * rebase\n      * cherry-pick\n      * revert\n\nThere are some caveats above, e.g. a plain `git diff` ignores files\noutside the sparsity patterns but will show diffs for paths outside the\nsparsity patterns when revision arguments are passed.  (Technically,\ndiff is treating the sparse paths as matching HEAD.)  So, there is some\ninternal inconsistency among these commands.  There are also additional\ncommands that should behave differently in the face of sparse-checkouts,\nas the sparse-checkout documentation alludes to, but the above is\nsufficient for me to explain how `git stash` is affected.\n\nWhat is relevant here is that logically 'stash' should behave like a\nmerge; it three-way merges the changes the user had in progress at stash\ncreation time, the HEAD at the time the stash was created, and the\ncurrent HEAD, in order to get the stashed changes applied to the current\nbranch.  However, this simplistic view doesn't quite work in practice,\nbecause stash tweaks it a bit due to two factors: (1) flags like\n--keep-index and --include-untracked (why we used two different verbs,\n'keep' and 'include', is a rant for another day) modify what should be\nstaged at the end and include more things that should be quasi-merged,\n(2) stash generally wants changes to NOT be staged.  It only provides\nexceptions when (a) some of the changes had conflicts and thus we want\nto use stages to denote the clean merges and higher order stages to\nmark the conflicts, or (b) if there is a brand new file we don't want\nit to become untracked.\n\nstash has traditionally gotten this special behavior by first doing a\nmerge, and then when it's clean, applying a pipeline of commands to\nmodify the result.  This series of commands for\nunstaging-non-newly-added-files came from the following commands:\n\n    git diff-index --cached --name-only --diff-filter=A $CTREE >\"$a\"\n    git read-tree --reset $CTREE\n    git update-index --add --stdin <\"$a\"\n    rm -f \"$a\"\n\nLooking back at the different types of special sparsity handling listed\nat the beginning of this message, you may note that we have at least one\nof each type covered here: merge, diff-index, and read-tree.  The weird\nmix-and-match led to 3 different bugs:\n\n(1) If a path merged cleanly and it didn't match the sparsity patterns,\nthe merge backend would know to avoid writing it to the working tree and\nkeep the SKIP_WORKTREE bit, simply only updating it in the index.\nUnfortunately, the subsequent commands would essentially undo the\nchanges in the index and thus simply toss the changes altogether since\nthere was nothing left in the working tree.  This means the stash is\nonly partially applied.\n\n(2) If a path existed in the worktree before `git stash apply` despite\nhaving the SKIP_WORKTREE bit set, then the `git read-tree --reset` would\nprint an error message of the form\n      error: Entry 'modified' not uptodate. Cannot merge.\nand cause stash to abort early.\n\n(3) If there was a brand new file added by the stash, then the\ndiff-index command would save that pathname to the temporary file, the\nread-tree --reset would remove it from the index, and the update-index\ncommand would barf due to no such file being present in the working\ncopy; it would print a message of the form:\n      error: NEWFILE: does not exist and --remove not passed\n      fatal: Unable to process path NEWFILE\nand then cause stash to abort early.\n\nBasically, the whole idea of unstage-unless-brand-new requires special\ncare when you are dealing with a sparse-checkout.  Fix these problems\nby applying the following simple rule:\n\n  When we unstage files, if they have the SKIP_WORKTREE bit set,\n  clear that bit and write the file out to the working directory.\n\n  (*) If there's already a file present in the way, rename it first.\n\nThis fixes all three problems in t7012.13 and allows us to mark it as\npassing.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/stash.c                  | 50 ++++++++++++++++++++++++++++++--\n t/t7012-skip-worktree-writing.sh |  2 +-\n 2 files changed, 49 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex a5465b565a..84886e84e0 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -363,12 +363,23 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)\n \t * relevant trees, and the merge logic always stages whatever merges\n \t * cleanly.  We want to unstage those changes, unless it corresponds\n \t * to a file that didn't exist as of orig_tree.\n+\t *\n+\t * However, if any SKIP_WORKTREE path is modified relative to\n+\t * orig_tree, then we want to clear the SKIP_WORKTREE bit and write\n+\t * it to the worktree before unstaging.\n \t */\n \n+\tstruct checkout state = CHECKOUT_INIT;\n \tstruct diff_options diff_opts;\n \tstruct lock_file lock = LOCK_INIT;\n \tint i;\n \n+\t/* If any entries have skip_worktree set, we'll have to check 'em out */\n+\tstate.force = 1;\n+\tstate.quiet = 1;\n+\tstate.refresh_cache = 1;\n+\tstate.istate = &the_index;\n+\n \t/*\n \t * Step 1: get a difference between orig_tree (which corresponding\n \t * to the index before a merge was run) and the current index\n@@ -395,7 +406,42 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)\n \t\t\t\t     strlen(p->two->path));\n \n \t\t/*\n-\t\t * Step 2: \"unstage\" changes, as long as they are still tracked\n+\t\t * Step 2: Place changes in the working tree\n+\t\t *\n+\t\t * Stash is about restoring changes *to the working tree*.\n+\t\t * So if the merge successfully got a new version of some\n+\t\t * path, but left it out of the working tree, then clear the\n+\t\t * SKIP_WORKTREE bit and write it to the working tree.\n+\t\t */\n+\t\tif (pos >= 0 && ce_skip_worktree(active_cache[pos])) {\n+\t\t\tstruct stat st;\n+\n+\t\t\tce = active_cache[pos];\n+\t\t\tif (!lstat(ce->name, &st)) {\n+\t\t\t\t/* Conflicting path present; relocate it */\n+\t\t\t\tstruct strbuf new_path = STRBUF_INIT;\n+\t\t\t\tint fd;\n+\n+\t\t\t\tstrbuf_addf(&new_path,\n+\t\t\t\t\t    \"%s.stash.XXXXXX\", ce->name);\n+\t\t\t\tfd = xmkstemp(new_path.buf);\n+\t\t\t\tclose(fd);\n+\t\t\t\tprintf(_(\"WARNING: Untracked file in way of \"\n+\t\t\t\t\t \"tracked file!  Renaming\\n \"\n+\t\t\t\t\t \"           %s -> %s\\n\"\n+\t\t\t\t\t \"         to make room.\\n\"),\n+\t\t\t\t       ce->name, new_path.buf);\n+\t\t\t\tif (rename(ce->name, new_path.buf))\n+\t\t\t\t\tdie(\"Failed to move %s to %s\\n\",\n+\t\t\t\t\t    ce->name, new_path.buf);\n+\t\t\t\tstrbuf_release(&new_path);\n+\t\t\t}\n+\t\t\tcheckout_entry(ce, &state, NULL, NULL);\n+\t\t\tce->ce_flags &= ~CE_SKIP_WORKTREE;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Step 3: \"unstage\" changes, as long as they are still tracked\n \t\t */\n \t\tif (p->one->oid_valid) {\n \t\t\t/*\n@@ -417,7 +463,7 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)\n \tdiff_flush(&diff_opts);\n \n \t/*\n-\t * Step 3: write the new index to disk\n+\t * Step 4: write the new index to disk\n \t */\n \trepo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);\n \tif (write_locked_index(&the_index, &lock,\ndiff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh\nindex a184ee97fb..e5c6a038fb 100755\n--- a/t/t7012-skip-worktree-writing.sh\n+++ b/t/t7012-skip-worktree-writing.sh\n@@ -149,7 +149,7 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '\n \t\t--diff-filter=D -- keep-me.t\n '\n \n-test_expect_failure 'stash restore in sparse checkout' '\n+test_expect_success 'stash restore in sparse checkout' '\n \ttest_create_repo stash-restore &&\n \t(\n \t\tcd stash-restore &&\n-- \ngitgitgadget\n"},{"id":"411113","messageId":"2155bbfe205588387208690abf8071d462c69d64.1606861519.git.gitgitgadget@gmail.com","threadId":"54681","inReplyTo":"pull.919.v2.git.git.1606861519.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t7012: add a testcase demonstrating stash apply bugs in sparse checkouts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-01T22:25:16Z","receivedAt":"2020-12-01T22:26:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nApplying stashes in sparse-checkouts, particularly when the patterns\nused to define the sparseness have changed between when the stash was\ncreated and when it is applied, has a number of bugs.  The primary\nproblem is that stashes are sometimes only partially applied.  In most\nsuch cases, it does so silently without any warning or error being\ndisplayed and with 0 exit status.\n\nThere are, however, a few cases when non-translated error messages are\nshown and the stash application aborts early.  The first is when there\nare files present despite the SKIP_WORKTREE bit being set, in which case\nthe error message shown is:\n\n    error: Entry 'PATHNAME' not uptodate. Cannot merge.\n\nThe other situation is when a stash contains new files to add to the\nworking tree; in this case, the code aborts early but still has the\nstash partially applied, and shows the following error message:\n\n    error: NEWFILE: does not exist and --remove not passed\n    fatal: Unable to process path NEWFILE\n\nAdd a test that can trigger all three of these problems.  Have it\ncarefully check that the working copy and SKIP_WORKTREE bits are as\nexpected after the stash application.  The test is currently marked as\nexpected to fail, but subsequent commits will implement the fixes and\ntoggle the expectation.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t7012-skip-worktree-writing.sh | 88 ++++++++++++++++++++++++++++++++\n 1 file changed, 88 insertions(+)\n\ndiff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh\nindex 7476781979..a184ee97fb 100755\n--- a/t/t7012-skip-worktree-writing.sh\n+++ b/t/t7012-skip-worktree-writing.sh\n@@ -149,6 +149,94 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '\n \t\t--diff-filter=D -- keep-me.t\n '\n \n+test_expect_failure 'stash restore in sparse checkout' '\n+\ttest_create_repo stash-restore &&\n+\t(\n+\t\tcd stash-restore &&\n+\n+\t\tmkdir subdir &&\n+\t\techo A >subdir/A &&\n+\t\techo untouched >untouched &&\n+\t\techo removeme >removeme &&\n+\t\techo modified >modified &&\n+\t\tgit add . &&\n+\t\tgit commit -m Initial &&\n+\n+\t\techo AA >>subdir/A &&\n+\t\techo addme >addme &&\n+\t\techo tweaked >>modified &&\n+\t\trm removeme &&\n+\t\tgit add addme &&\n+\n+\t\tgit stash push &&\n+\n+\t\tgit sparse-checkout set subdir &&\n+\n+\t\t# Ensure after sparse-checkout we only have expected files\n+\t\tcat >expect <<-EOF &&\n+\t\tS modified\n+\t\tS removeme\n+\t\tH subdir/A\n+\t\tS untouched\n+\t\tEOF\n+\t\tgit ls-files -t >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_path_is_missing addme &&\n+\t\ttest_path_is_missing modified &&\n+\t\ttest_path_is_missing removeme &&\n+\t\ttest_path_is_file    subdir/A &&\n+\t\ttest_path_is_missing untouched &&\n+\n+\t\t# Put a file in the working directory in the way\n+\t\techo in the way >modified &&\n+\t\tgit stash apply &&\n+\n+\t\t# Ensure stash vivifies modifies paths...\n+\t\tcat >expect <<-EOF &&\n+\t\tH addme\n+\t\tH modified\n+\t\tH removeme\n+\t\tH subdir/A\n+\t\tS untouched\n+\t\tEOF\n+\t\tgit ls-files -t >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# ...and that the paths show up in status as changed...\n+\t\tcat >expect <<-EOF &&\n+\t\tA  addme\n+\t\t M modified\n+\t\t D removeme\n+\t\t M subdir/A\n+\t\t?? actual\n+\t\t?? expect\n+\t\t?? modified.stash.XXXXXX\n+\t\tEOF\n+\t\tgit status --porcelain | \\\n+\t\t\tsed -e s/stash......./stash.XXXXXX/ >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# ...and that working directory reflects the files correctly\n+\t\ttest_path_is_file    addme &&\n+\t\ttest_path_is_file    modified &&\n+\t\ttest_path_is_missing removeme &&\n+\t\ttest_path_is_file    subdir/A &&\n+\t\ttest_path_is_missing untouched &&\n+\n+\t\t# ...including that we have the expected \"modified\" file...\n+\t\tcat >expect <<-EOF &&\n+\t\tmodified\n+\t\ttweaked\n+\t\tEOF\n+\t\ttest_cmp expect modified &&\n+\n+\t\t# ...and that the other \"modified\" file is still present...\n+\t\techo in the way >expect &&\n+\t\ttest_cmp expect modified.stash.*\n+\t)\n+'\n+\n #TODO test_expect_failure 'git-apply adds file' false\n #TODO test_expect_failure 'git-apply updates file' false\n #TODO test_expect_failure 'git-apply removes file' false\n-- \ngitgitgadget\n\n"},{"id":"411116","messageId":"xmqqr1o9m8on.fsf@gitster.c.googlers.com","threadId":"54681","inReplyTo":"1fa263cf3c3d1b0c20ad89e6454a7b903a07f193.1606861519.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] stash: remove unnecessary process forking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-01T23:02:32Z","receivedAt":"2020-12-01T23:03:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     git diff-index --cached --name-only --diff-filter=A $CTREE >\"$a\"\n>     git read-tree --reset $CTREE\n>     git update-index --add --stdin <\"$a\"\n>     rm -f \"$a\"\n\nThis is orthogonal to what this patch does, as this is supposed to\nbe just bug-for-bug compatible rewrite.\n\nBut I wonder if the above sequence, whether it is done as a series\nof plumbing invocations or subroutine calls, is a relic dating back\nin the days before i-t-a existed.  If we want to revert the changes\nto the index for working tree files for removed or modified ones, I\ndo not offhand see a good reason why we would want to keep the\ncontents to new paths---if i-t-a were available when the sequence\nwas designed, I suspect we would just have added the path as i-t-a\nin order to keep track of the presence of the path but not\nnecessarily the contents in it.\n"},{"id":"411168","messageId":"CABPp-BGTkGO+19_AUrHFF4P8J8g_RnfbL6r4jHkxYvdE5_h8mw@mail.gmail.com","threadId":"54681","inReplyTo":"xmqqr1o9m8on.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] stash: remove unnecessary process forking","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-12-02T16:09:52Z","receivedAt":"2020-12-02T16:11:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Dec 1, 2020 at 3:02 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> >     git diff-index --cached --name-only --diff-filter=A $CTREE >\"$a\"\n> >     git read-tree --reset $CTREE\n> >     git update-index --add --stdin <\"$a\"\n> >     rm -f \"$a\"\n>\n> This is orthogonal to what this patch does, as this is supposed to\n> be just bug-for-bug compatible rewrite.\n>\n> But I wonder if the above sequence, whether it is done as a series\n> of plumbing invocations or subroutine calls, is a relic dating back\n> in the days before i-t-a existed.  If we want to revert the changes\n> to the index for working tree files for removed or modified ones, I\n> do not offhand see a good reason why we would want to keep the\n> contents to new paths---if i-t-a were available when the sequence\n> was designed, I suspect we would just have added the path as i-t-a\n> in order to keep track of the presence of the path but not\n> necessarily the contents in it.\n\nYeah, I thought a little bit about the same thing, but wasn't sure if\nthere was some other reason for the current behavior or if there was\nsome workflow that might be relying upon it.  Rather than investigate\nand try to switch it over to i-t-a (which could still be done later),\nI more narrowly focused this series at just doing the \"make the\nchanges be available in the working tree, but remove them from the\nindex unless it'd make it untracked\" more carefully.\n"}]}