[PATCH 0/3] Fix stash apply in sparse checkouts (and a submodule test)

[PATCH 0/3] Fix stash apply in sparse checkouts (and a submodule test)

[Elijah Newren via GitGitGadget]

Heavier usage of sparse-checkouts at $DAYJOB is commencing. And an issue
with git stash apply was found.

git stash's implementation as a pipeline of forked commands presents some
problems, especially when implemented atop of three commands that all behave
differently in the presence of sparse checkouts. Add a testcase
demonstrating some issues with git stash apply in a repository with a
different set of sparse-checkout patterns at apply vs create time, clean up
the relevant section of git stash code, and incidentally fix a submodule
testcase unrelated to sparse checkouts. Provide some detailed commit
messages explaining the issues along the way.

NOTE: I found a couple minor issues with other commands in sparse checkouts
while debugging this issue, but I don't yet have fixes for them and I can
submit them separately.

Elijah Newren (3):
  t7012: add a testcase demonstrating stash apply bugs in sparse
    checkouts
  stash: remove unnecessary process forking
  stash: fix stash application in sparse-checkouts

 builtin/stash.c                  | 130 +++++++++++++++++++++----------
 t/lib-submodule-update.sh        |  16 ++--
 t/t7012-skip-worktree-writing.sh |  88 +++++++++++++++++++++
 3 files changed, 184 insertions(+), 50 deletions(-)


base-commit: faefdd61ec7c7f6f3c8c9907891465ac9a2a1475
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-919%2Fnewren%2Fsparse-checkout-fixups-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-919/newren/sparse-checkout-fixups-v1
Pull-Request: https://github.com/git/git/pull/919
-- 
gitgitgadget

[PATCH 1/3] t7012: add a testcase demonstrating stash apply bugs in sparse checkouts

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Applying stashes in sparse-checkouts, particularly when the patterns
used to define the present paths have changed between when the stash was
created and when it is applied, has a number of bugs.  The primary
problem is perhaps that stashes can be only partially applied (sometimes
without any warning or error being displayed and with 0 exit status).
In addition, there are cases where partial stash application comes with
non-translated error messages and an early abort.  The first is when
there are files present despite the SKIP_WORKTREE bit being set, in
which case the error message shown is:

    error: Entry 'PATHNAME' not uptodate. Cannot merge.

The other situation is when a stash contains new files to add to the
working tree; in this case, the code aborts early (but after at least
partial stash application) with the following error message being shown:

    error: NEWFILE: does not exist and --remove not passed
    fatal: Unable to process path NEWFILE

Add a test that can trigger all three of these problems that carefully
checks that the working copy and SKIP_WORKTREE bits are as expected
after the stash application...but currently marked as expected to fail.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7012-skip-worktree-writing.sh | 88 ++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index 7476781979..a184ee97fb 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -149,6 +149,94 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
 		--diff-filter=D -- keep-me.t
 '
 
+test_expect_failure 'stash restore in sparse checkout' '
+	test_create_repo stash-restore &&
+	(
+		cd stash-restore &&
+
+		mkdir subdir &&
+		echo A >subdir/A &&
+		echo untouched >untouched &&
+		echo removeme >removeme &&
+		echo modified >modified &&
+		git add . &&
+		git commit -m Initial &&
+
+		echo AA >>subdir/A &&
+		echo addme >addme &&
+		echo tweaked >>modified &&
+		rm removeme &&
+		git add addme &&
+
+		git stash push &&
+
+		git sparse-checkout set subdir &&
+
+		# Ensure after sparse-checkout we only have expected files
+		cat >expect <<-EOF &&
+		S modified
+		S removeme
+		H subdir/A
+		S untouched
+		EOF
+		git ls-files -t >actual &&
+		test_cmp expect actual &&
+
+		test_path_is_missing addme &&
+		test_path_is_missing modified &&
+		test_path_is_missing removeme &&
+		test_path_is_file    subdir/A &&
+		test_path_is_missing untouched &&
+
+		# Put a file in the working directory in the way
+		echo in the way >modified &&
+		git stash apply &&
+
+		# Ensure stash vivifies modifies paths...
+		cat >expect <<-EOF &&
+		H addme
+		H modified
+		H removeme
+		H subdir/A
+		S untouched
+		EOF
+		git ls-files -t >actual &&
+		test_cmp expect actual &&
+
+		# ...and that the paths show up in status as changed...
+		cat >expect <<-EOF &&
+		A  addme
+		 M modified
+		 D removeme
+		 M subdir/A
+		?? actual
+		?? expect
+		?? modified.stash.XXXXXX
+		EOF
+		git status --porcelain | \
+			sed -e s/stash......./stash.XXXXXX/ >actual &&
+		test_cmp expect actual &&
+
+		# ...and that working directory reflects the files correctly
+		test_path_is_file    addme &&
+		test_path_is_file    modified &&
+		test_path_is_missing removeme &&
+		test_path_is_file    subdir/A &&
+		test_path_is_missing untouched &&
+
+		# ...including that we have the expected "modified" file...
+		cat >expect <<-EOF &&
+		modified
+		tweaked
+		EOF
+		test_cmp expect modified &&
+
+		# ...and that the other "modified" file is still present...
+		echo in the way >expect &&
+		test_cmp expect modified.stash.*
+	)
+'
+
 #TODO test_expect_failure 'git-apply adds file' false
 #TODO test_expect_failure 'git-apply updates file' false
 #TODO test_expect_failure 'git-apply removes file' false
-- 
gitgitgadget

[PATCH 2/3] stash: remove unnecessary process forking

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

When stash was converted from shell to a builtin, it merely
transliterated the forking of various git commands from shell to a C
program that would fork the same commands.  Some of those were converted
over to actual library calls, but much of the pipeline-of-commands
design still remains.  Fix some of this by replacing the portion
corresponding to

    git diff-index --cached --name-only --diff-filter=A $CTREE >"$a"
    git read-tree --reset $CTREE
    git update-index --add --stdin <"$a"
    rm -f "$a"

into a library function that does the same thing.  (The read-tree
--reset was already partially converted over to a library call, but as
an independent piece.)  Note here that this came after a merge operation
was performed.  The merge machinery always staged anything that cleanly
merges, and the above code only runs if there were no conflicts.  Its
purpose is to make it so that when there are no conflicts, all the
changes from the stash are unstaged.  However, that causes brand new
files from the stash to become untracked, so the code above first saves
those files off and then re-adds them afterwards.

We replace the whole series of commands with a simple function that will
unstage files that are not newly added.  This doesn't fix any bugs in
the usage of these commands, it simply matches the existing behavior but
making it an actual builtin that we can then operate on as a whole.  A
subsequent commit will take advantage of this to fix issues with these
commands in sparse-checkouts.

This conversion incidentally fixes t3906.1, because the separate
update-index process would die with the following error messages:
    error: uninitialized_sub: is a directory - add files inside instead
    fatal: Unable to process path uninitialized_sub
The unstaging of the directory as a submodule meant it was no longer
tracked, and thus as an uninitialized directory it could not be added
back using `git update-index --add`, thus resulting in this error and
early abort.  Most of the submodule tests in 3906 continue to fail after
this change, this change was just enough to push the first of those
tests to success.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/stash.c           | 96 ++++++++++++++++++++++-----------------
 t/lib-submodule-update.sh | 16 +++----
 2 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 24ddb1bffa..8117d7647d 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -325,33 +325,64 @@ static void add_diff_to_buf(struct diff_queue_struct *q,
 	}
 }
 
-static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)
+static void unstage_changes_unless_new(struct object_id *cache_tree)
 {
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *c_tree_hex = oid_to_hex(c_tree);
-
 	/*
-	 * diff-index is very similar to diff-tree above, and should be
-	 * converted together with update_index.
+	 * When we enter this function, there has been a clean merge of
+	 * relevant trees, and the merge logic always stages whatever merges
+	 * cleanly.  We want to unstage those changes, unless it corresponds
+	 * to a file that didn't exist as of cache_tree.
 	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "diff-index", "--cached", "--name-only",
-		     "--diff-filter=A", NULL);
-	strvec_push(&cp.args, c_tree_hex);
-	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
-}
 
-static int update_index(struct strbuf *out)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
+	struct diff_options diff_opts;
+	struct lock_file lock = LOCK_INIT;
+	int i;
 
-	/*
-	 * Update-index is very complicated and may need to have a public
-	 * function exposed in order to remove this forking.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "update-index", "--add", "--stdin", NULL);
-	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
+	diff_setup(&diff_opts);
+	diff_opts.flags.recursive = 1;
+	diff_opts.detect_rename = 0;
+	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	diff_setup_done(&diff_opts);
+
+	do_diff_cache(cache_tree, &diff_opts);
+	diffcore_std(&diff_opts);
+
+	for (i = 0; i < diff_queued_diff.nr; i++) {
+		struct diff_filepair *p;
+		struct cache_entry *ce;
+		int pos;
+
+		p = diff_queued_diff.queue[i];
+		pos = index_name_pos(&the_index, p->two->path,
+				     strlen(p->two->path));
+		if (pos < 0) {
+			assert(p->one->oid_valid);
+			assert(!p->two->oid_valid);
+			ce = make_cache_entry(&the_index,
+					      p->one->mode,
+					      &p->one->oid,
+					      p->one->path,
+					      0, 0);
+			add_index_entry(&the_index, ce, ADD_CACHE_OK_TO_ADD);
+			continue;
+		}
+		ce = active_cache[pos];
+		if (p->one->oid_valid) {
+			ce = make_cache_entry(&the_index,
+					      p->one->mode,
+					      &p->one->oid,
+					      p->one->path,
+					      0, 0);
+			add_index_entry(&the_index, ce,
+					ADD_CACHE_OK_TO_REPLACE);
+		}
+	}
+	diff_flush(&diff_opts);
+
+	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
+	if (write_locked_index(&the_index, &lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+		die(_("Unable to write index."));
 }
 
 static int restore_untracked(struct object_id *u_tree)
@@ -467,26 +498,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 		if (reset_tree(&index_tree, 0, 0))
 			return -1;
 	} else {
-		struct strbuf out = STRBUF_INIT;
-
-		if (get_newly_staged(&out, &c_tree)) {
-			strbuf_release(&out);
-			return -1;
-		}
-
-		if (reset_tree(&c_tree, 0, 1)) {
-			strbuf_release(&out);
-			return -1;
-		}
-
-		ret = update_index(&out);
-		strbuf_release(&out);
-		if (ret)
-			return -1;
-
-		/* read back the result of update_index() back from the disk */
-		discard_cache();
-		read_cache();
+		unstage_changes_unless_new(&c_tree);
 	}
 
 	if (!quiet) {
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index bd3fa3c6da..4b714e9308 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -316,14 +316,7 @@ test_submodule_switch_common () {
 	command="$1"
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
-	if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1
-	then
-		# Restoring stash fails to restore submodule index entry
-		RESULT="failure"
-	else
-		RESULT="success"
-	fi
-	test_expect_$RESULT "$command: added submodule creates empty directory" '
+	test_expect_success "$command: added submodule creates empty directory" '
 		prolog &&
 		reset_work_tree_to no_submodule &&
 		(
@@ -337,6 +330,13 @@ test_submodule_switch_common () {
 		)
 	'
 	# ... and doesn't care if it already exists.
+	if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1
+	then
+		# Restoring stash fails to restore submodule index entry
+		RESULT="failure"
+	else
+		RESULT="success"
+	fi
 	test_expect_$RESULT "$command: added submodule leaves existing empty directory alone" '
 		prolog &&
 		reset_work_tree_to no_submodule &&
-- 
gitgitgadget

[PATCH 3/3] stash: fix stash application in sparse-checkouts

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

sparse-checkouts are built on the patterns in the
$GIT_DIR/info/sparse-checkout file, where commands have modified
behavior for paths that do not match those patterns.  The differences in
behavior, as far as the bugs concerned here, fall into three different
categories (with git subcommands that fall into each category listed):

  * commands that only look at files matching the patterns:
      * status
      * diff
      * clean
      * update-index
  * commands that remove files from the working tree that do not match
    the patterns, and restore files that do match them:
      * read-tree
      * switch
      * checkout
      * reset (--hard)
  * commands that omit writing files to the working tree that do not
    match the patterns, unless those files are not clean:
      * merge
      * rebase
      * cherry-pick
      * revert

There are some caveats above, e.g. a plain `git diff` ignores files
outside the sparsity patterns but will show diffs for paths outside the
sparsity patterns when revision arguments are passed.  (Technically,
diff is treating the sparse paths as matching HEAD.)  So, there is some
internal inconsistency among these commands.  There are also additional
commands that should behave differently in the face of sparse-checkouts,
as the sparse-checkout documentation alludes to, but the above is
sufficient for me to explain how `git stash` is affected.

What is relevant here is that logically 'stash' should behave like a
merge; it three-way merges the changes the user had in progress at stash
creation time, the HEAD at the time the stash was created, and the
current HEAD, in order to get the stashed changes applied to the current
branch.  However, this simplistic view doesn't quite work in practice,
because stash tweaks it a bit due to two factors: (1) flags like
--keep-index and --include-untracked (why we used two different verbs,
'keep' and 'include', is a rant for another day) modify what should be
staged at the end and include more things that should be quasi-merged,
(2) stash generally wants changes to NOT be staged.  It only provides
exceptions when (a) some of the changes had conflicts and thus we want
to use staged to denote the clean merges and higher order stages to
mark the conflicts, or (b) if there is a brand new file we don't want
it to become untracked.

stash has traditionally gotten this special behavior by first doing a
merge, and then when it's clean, applying a pipeline of commands to
modify the result.  This series of commands for
unstaging-non-newly-added-files came from the following commands:

    git diff-index --cached --name-only --diff-filter=A $CTREE >"$a"
    git read-tree --reset $CTREE
    git update-index --add --stdin <"$a"
    rm -f "$a"

You might that between the merge that proceeded these commands and these
different commands here, that we have commands from each of the
different types of special sparsity handling listed at the beginning of
this message, and in fact this precisely led to the following buggy
behaviors:

(1) If a path merged cleanly and it didn't match the sparsity patterns,
the merge backend would know to avoid writing it to the working tree and
keep the SKIP_WORKTREE bit, simply only updating it in the index.
Unfortunately, the subsequent commands would essentially undo the
changes in the index and thus simply toss the changes altogether since
there was nothing left in the working tree.  This means the stash is
only partially applied.

(2) If a path existed in the worktree before `git stash apply` despite
having the SKIP_WORKTREE bit set, then the `git read-tree --reset` would
print an error message of the form
      error: Entry 'modified' not uptodate. Cannot merge.
and cause stash to abort early.

(3) If there was a brand new file added by the stash, then the
diff-index command would save that pathname to the temporary file, the
read-tree --reset would remove it from the index, and the update-index
command would barf due to no such file being present in the working
copy; it would print a message of the form:
      error: NEWFILE: does not exist and --remove not passed
      fatal: Unable to process path NEWFILE
and then cause stash to abort early.

Basically, the whole idea of unstage-unless-brand-new requires special
care when you are dealing with a sparse-checkout.  Fix these problems
by applying the following simple rule:

  When we unstage files, if they have the SKIP_WORKTREE bit set,
  clear that bit and write the file out to the working directory.

  (*) If there's already a file present in the way, rename it first.

This fixes all three problems in t7012.13 and allows us to mark it as
passing.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/stash.c                  | 36 +++++++++++++++++++++++++++++++-
 t/t7012-skip-worktree-writing.sh |  2 +-
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 8117d7647d..0f7e78d315 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -331,13 +331,23 @@ static void unstage_changes_unless_new(struct object_id *cache_tree)
 	 * When we enter this function, there has been a clean merge of
 	 * relevant trees, and the merge logic always stages whatever merges
 	 * cleanly.  We want to unstage those changes, unless it corresponds
-	 * to a file that didn't exist as of cache_tree.
+	 * to a file that didn't exist as of cache_tree.  However, if any
+	 * SKIP_WORKTREE path is modified relative to cache_tree, then we
+	 * want to clear the SKIP_WORKTREE bit and write it to the worktree
+	 * before unstaging.
 	 */
 
+	struct checkout state = CHECKOUT_INIT;
 	struct diff_options diff_opts;
 	struct lock_file lock = LOCK_INIT;
 	int i;
 
+	/* If any entries have skip_worktree set, we'll have to check 'em out */
+	state.force = 1;
+	state.quiet = 1;
+	state.refresh_cache = 1;
+	state.istate = &the_index;
+
 	diff_setup(&diff_opts);
 	diff_opts.flags.recursive = 1;
 	diff_opts.detect_rename = 0;
@@ -367,6 +377,30 @@ static void unstage_changes_unless_new(struct object_id *cache_tree)
 			continue;
 		}
 		ce = active_cache[pos];
+		if (ce_skip_worktree(ce)) {
+			struct stat st;
+			if (!lstat(ce->name, &st)) {
+				struct strbuf new_path = STRBUF_INIT;
+				int fd;
+
+				strbuf_addf(&new_path,
+					    "%s.stash.XXXXXX", ce->name);
+				fd = xmkstemp(new_path.buf);
+				close(fd);
+				printf(_("WARNING: Untracked file in way of "
+					 "tracked file!  Renaming\n "
+					 "           %s -> %s\n"
+					 "         to make room.\n"),
+				       ce->name, new_path.buf);
+				if (rename(ce->name, new_path.buf))
+					die("Failed to move %s to %s\n",
+					    ce->name, new_path.buf);
+				strbuf_release(&new_path);
+			}
+			checkout_entry(ce, &state, NULL, NULL);
+		}
+
+		ce->ce_flags &= ~CE_SKIP_WORKTREE;
 		if (p->one->oid_valid) {
 			ce = make_cache_entry(&the_index,
 					      p->one->mode,
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index a184ee97fb..e5c6a038fb 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -149,7 +149,7 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
 		--diff-filter=D -- keep-me.t
 '
 
-test_expect_failure 'stash restore in sparse checkout' '
+test_expect_success 'stash restore in sparse checkout' '
 	test_create_repo stash-restore &&
 	(
 		cd stash-restore &&
-- 
gitgitgadget

Re: [PATCH 3/3] stash: fix stash application in sparse-checkouts

[Chris Torek]

On Fri, Nov 20, 2020 at 8:56 AM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> branch.  However, this simplistic view doesn't quite work in practice,
> because stash tweaks it a bit due to two factors: (1) flags like
> --keep-index and --include-untracked (why we used two different verbs,
> 'keep' and 'include', is a rant for another day)

:-)

Not that this should affect any of these changes, but I'd also note
that the fact that using `-u` or  `-a` makes a third commit that cannot
be ignored later is a problem as well.  (`git stash list` should probably
annotate the listed stashes as to whether they are two- or three-commit
stashes.)

> stash has traditionally gotten this special behavior by first doing a
> merge, and then when it's clean, applying a pipeline of commands to
> modify the result.  This series of commands for
> unstaging-non-newly-added-files came from the following commands:
>
>     git diff-index --cached --name-only --diff-filter=A $CTREE >"$a"
>     git read-tree --reset $CTREE
>     git update-index --add --stdin <"$a"
>     rm -f "$a"
>
> You might that between the merge that proceeded these commands and these

s/might/might think/, perhaps?  And, s/proceeded/preceded/, probably.

(I didn't look closely at the rest of this but the idea seems sound.)

Chris

Re: [PATCH 3/3] stash: fix stash application in sparse-checkouts

[Elijah Newren]

On Sat, Nov 21, 2020 at 4:47 AM Chris Torek <chris.torek@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 8:56 AM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > branch.  However, this simplistic view doesn't quite work in practice,
> > because stash tweaks it a bit due to two factors: (1) flags like
> > --keep-index and --include-untracked (why we used two different verbs,
> > 'keep' and 'include', is a rant for another day)
>
> :-)
>
> Not that this should affect any of these changes, but I'd also note
> that the fact that using `-u` or  `-a` makes a third commit that cannot
> be ignored later is a problem as well.  (`git stash list` should probably
> annotate the listed stashes as to whether they are two- or three-commit
> stashes.)

Yeah, it seems a bit odd when you compare to saving and applying
cached changes; for that, stash requires you to specify BOTH
--keep-index to to push/save, AND --index to pop/apply.

However, for saving and applying untracked or ignored changes, you
merely need to record them at push/save time and they automatically
always apply.

Not very consistent.  (But, again, unrelated to these patches, so this
is mostly just recreational complaining at this point.  I'm not
planning to tackle these bits.)

> > stash has traditionally gotten this special behavior by first doing a
> > merge, and then when it's clean, applying a pipeline of commands to
> > modify the result.  This series of commands for
> > unstaging-non-newly-added-files came from the following commands:
> >
> >     git diff-index --cached --name-only --diff-filter=A $CTREE >"$a"
> >     git read-tree --reset $CTREE
> >     git update-index --add --stdin <"$a"
> >     rm -f "$a"
> >
> > You might that between the merge that proceeded these commands and these
>
> s/might/might think/, perhaps?  And, s/proceeded/preceded/, probably.

Indeed; thanks for catching those.  I will fix them up.

> (I didn't look closely at the rest of this but the idea seems sound.)
>
> Chris

Re: [PATCH 0/3] Fix stash apply in sparse checkouts (and a submodule test)

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Heavier usage of sparse-checkouts at $DAYJOB is commencing. And an issue
> with git stash apply was found.
>
> git stash's implementation as a pipeline of forked commands presents some
> problems, especially when implemented atop of three commands that all behave
> differently in the presence of sparse checkouts. Add a testcase
> demonstrating some issues with git stash apply in a repository with a
> different set of sparse-checkout patterns at apply vs create time, clean up
> the relevant section of git stash code, and incidentally fix a submodule
> testcase unrelated to sparse checkouts. Provide some detailed commit
> messages explaining the issues along the way.
>
> NOTE: I found a couple minor issues with other commands in sparse checkouts
> while debugging this issue, but I don't yet have fixes for them and I can
> submit them separately.

Any comments on this from reviewers?  The second patch is a but too
busy looking and I am having a bit of trouble convincing myself that
it is doing the right thing.

Thanks.

Re: [PATCH 0/3] Fix stash apply in sparse checkouts (and a submodule test)

[Elijah Newren]

On Wed, Nov 25, 2020 at 2:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Heavier usage of sparse-checkouts at $DAYJOB is commencing. And an issue
> > with git stash apply was found.
> >
> > git stash's implementation as a pipeline of forked commands presents some
> > problems, especially when implemented atop of three commands that all behave
> > differently in the presence of sparse checkouts. Add a testcase
> > demonstrating some issues with git stash apply in a repository with a
> > different set of sparse-checkout patterns at apply vs create time, clean up
> > the relevant section of git stash code, and incidentally fix a submodule
> > testcase unrelated to sparse checkouts. Provide some detailed commit
> > messages explaining the issues along the way.
> >
> > NOTE: I found a couple minor issues with other commands in sparse checkouts
> > while debugging this issue, but I don't yet have fixes for them and I can
> > submit them separately.
>
> Any comments on this from reviewers?  The second patch is a but too
> busy looking and I am having a bit of trouble convincing myself that
> it is doing the right thing.

Hmm, that diff is a little hard to read.  It's a removal of two
functions, and an addition of a new one, but the way the diff reads it
looks like I'm modifying the existing functions because it catches
some comment line markers and thinks they're similar.  Maybe it'd be
easier to read if I inserted that function elsewhere in the file?

I'll send a re-roll with that and add a comment or two to help explain
it (as well as fix up the small issues Chris highlighted).

[PATCH v2 0/3] Fix stash apply in sparse checkouts (and a submodule test)

[Elijah Newren via GitGitGadget]

This series fixes some issues with git stash apply in sparse checkouts, when
the sparsity patterns have changed between when the stash was created and
when the stash is being applied. It also incidentally fixes a submodule
testcase unrelated to sparse checkouts.

Changes since v1:

 * Commit message cleanup, including a couple wording issues highlighted by
   Chris
 * Cleaned up the code a bit and commented it better, to try to make it
   easier for Junio (and others) to follow.

Elijah Newren (3):
  t7012: add a testcase demonstrating stash apply bugs in sparse
    checkouts
  stash: remove unnecessary process forking
  stash: fix stash application in sparse-checkouts

 builtin/stash.c                  | 165 ++++++++++++++++++++++---------
 t/lib-submodule-update.sh        |  16 +--
 t/t7012-skip-worktree-writing.sh |  88 +++++++++++++++++
 3 files changed, 212 insertions(+), 57 deletions(-)


base-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-919%2Fnewren%2Fsparse-checkout-fixups-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-919/newren/sparse-checkout-fixups-v2
Pull-Request: https://github.com/git/git/pull/919

Range-diff vs v1:

 1:  de4b4d207b ! 1:  2155bbfe20 t7012: add a testcase demonstrating stash apply bugs in sparse checkouts
     @@ Commit message
          t7012: add a testcase demonstrating stash apply bugs in sparse checkouts
      
          Applying stashes in sparse-checkouts, particularly when the patterns
     -    used to define the present paths have changed between when the stash was
     +    used to define the sparseness have changed between when the stash was
          created and when it is applied, has a number of bugs.  The primary
     -    problem is perhaps that stashes can be only partially applied (sometimes
     -    without any warning or error being displayed and with 0 exit status).
     -    In addition, there are cases where partial stash application comes with
     -    non-translated error messages and an early abort.  The first is when
     -    there are files present despite the SKIP_WORKTREE bit being set, in
     -    which case the error message shown is:
     +    problem is that stashes are sometimes only partially applied.  In most
     +    such cases, it does so silently without any warning or error being
     +    displayed and with 0 exit status.
     +
     +    There are, however, a few cases when non-translated error messages are
     +    shown and the stash application aborts early.  The first is when there
     +    are files present despite the SKIP_WORKTREE bit being set, in which case
     +    the error message shown is:
      
              error: Entry 'PATHNAME' not uptodate. Cannot merge.
      
          The other situation is when a stash contains new files to add to the
     -    working tree; in this case, the code aborts early (but after at least
     -    partial stash application) with the following error message being shown:
     +    working tree; in this case, the code aborts early but still has the
     +    stash partially applied, and shows the following error message:
      
              error: NEWFILE: does not exist and --remove not passed
              fatal: Unable to process path NEWFILE
      
     -    Add a test that can trigger all three of these problems that carefully
     -    checks that the working copy and SKIP_WORKTREE bits are as expected
     -    after the stash application...but currently marked as expected to fail.
     +    Add a test that can trigger all three of these problems.  Have it
     +    carefully check that the working copy and SKIP_WORKTREE bits are as
     +    expected after the stash application.  The test is currently marked as
     +    expected to fail, but subsequent commits will implement the fixes and
     +    toggle the expectation.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
 2:  eb9ebcf0bd ! 2:  1fa263cf3c stash: remove unnecessary process forking
     @@ Commit message
          into a library function that does the same thing.  (The read-tree
          --reset was already partially converted over to a library call, but as
          an independent piece.)  Note here that this came after a merge operation
     -    was performed.  The merge machinery always staged anything that cleanly
     -    merges, and the above code only runs if there were no conflicts.  Its
     +    was performed.  The merge machinery always stages anything that cleanly
     +    merges, and the above code only runs if there are no conflicts.  Its
          purpose is to make it so that when there are no conflicts, all the
          changes from the stash are unstaged.  However, that causes brand new
          files from the stash to become untracked, so the code above first saves
     @@ Commit message
          We replace the whole series of commands with a simple function that will
          unstage files that are not newly added.  This doesn't fix any bugs in
          the usage of these commands, it simply matches the existing behavior but
     -    making it an actual builtin that we can then operate on as a whole.  A
     -    subsequent commit will take advantage of this to fix issues with these
     -    commands in sparse-checkouts.
     +    makes it into a single atomic operation that we can then operate on as a
     +    whole.  A subsequent commit will take advantage of this to fix issues
     +    with these commands in sparse-checkouts.
      
          This conversion incidentally fixes t3906.1, because the separate
          update-index process would die with the following error messages:
     @@ builtin/stash.c: static void add_diff_to_buf(struct diff_queue_struct *q,
       }
       
      -static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)
     -+static void unstage_changes_unless_new(struct object_id *cache_tree)
     - {
     +-{
      -	struct child_process cp = CHILD_PROCESS_INIT;
      -	const char *c_tree_hex = oid_to_hex(c_tree);
      -
     - 	/*
     +-	/*
      -	 * diff-index is very similar to diff-tree above, and should be
      -	 * converted together with update_index.
     -+	 * When we enter this function, there has been a clean merge of
     -+	 * relevant trees, and the merge logic always stages whatever merges
     -+	 * cleanly.  We want to unstage those changes, unless it corresponds
     -+	 * to a file that didn't exist as of cache_tree.
     - 	 */
     +-	 */
      -	cp.git_cmd = 1;
      -	strvec_pushl(&cp.args, "diff-index", "--cached", "--name-only",
      -		     "--diff-filter=A", NULL);
      -	strvec_push(&cp.args, c_tree_hex);
      -	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
      -}
     - 
     +-
      -static int update_index(struct strbuf *out)
      -{
      -	struct child_process cp = CHILD_PROCESS_INIT;
     -+	struct diff_options diff_opts;
     -+	struct lock_file lock = LOCK_INIT;
     -+	int i;
     - 
     +-
      -	/*
      -	 * Update-index is very complicated and may need to have a public
      -	 * function exposed in order to remove this forking.
     @@ builtin/stash.c: static void add_diff_to_buf(struct diff_queue_struct *q,
      -	cp.git_cmd = 1;
      -	strvec_pushl(&cp.args, "update-index", "--add", "--stdin", NULL);
      -	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
     +-}
     +-
     + static int restore_untracked(struct object_id *u_tree)
     + {
     + 	int res;
     +@@ builtin/stash.c: static int restore_untracked(struct object_id *u_tree)
     + 	return res;
     + }
     + 
     ++static void unstage_changes_unless_new(struct object_id *orig_tree)
     ++{
     ++	/*
     ++	 * When we enter this function, there has been a clean merge of
     ++	 * relevant trees, and the merge logic always stages whatever merges
     ++	 * cleanly.  We want to unstage those changes, unless it corresponds
     ++	 * to a file that didn't exist as of orig_tree.
     ++	 */
     ++
     ++	struct diff_options diff_opts;
     ++	struct lock_file lock = LOCK_INIT;
     ++	int i;
     ++
     ++	/*
     ++	 * Step 1: get a difference between orig_tree (which corresponding
     ++	 * to the index before a merge was run) and the current index
     ++	 * (reflecting the changes brought in by the merge).
     ++	 */
      +	diff_setup(&diff_opts);
      +	diff_opts.flags.recursive = 1;
      +	diff_opts.detect_rename = 0;
      +	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
      +	diff_setup_done(&diff_opts);
      +
     -+	do_diff_cache(cache_tree, &diff_opts);
     ++	do_diff_cache(orig_tree, &diff_opts);
      +	diffcore_std(&diff_opts);
      +
     ++	/* Iterate over the paths that changed due to the merge... */
      +	for (i = 0; i < diff_queued_diff.nr; i++) {
      +		struct diff_filepair *p;
      +		struct cache_entry *ce;
      +		int pos;
      +
     ++		/* Look up the path's position in the current index. */
      +		p = diff_queued_diff.queue[i];
      +		pos = index_name_pos(&the_index, p->two->path,
      +				     strlen(p->two->path));
     -+		if (pos < 0) {
     -+			assert(p->one->oid_valid);
     -+			assert(!p->two->oid_valid);
     -+			ce = make_cache_entry(&the_index,
     -+					      p->one->mode,
     -+					      &p->one->oid,
     -+					      p->one->path,
     -+					      0, 0);
     -+			add_index_entry(&the_index, ce, ADD_CACHE_OK_TO_ADD);
     -+			continue;
     -+		}
     -+		ce = active_cache[pos];
     ++
     ++		/*
     ++		 * Step 2: "unstage" changes, as long as they are still tracked
     ++		 */
      +		if (p->one->oid_valid) {
     ++			/*
     ++			 * Path existed in orig_tree; restore index entry
     ++			 * from that tree in order to "unstage" the changes.
     ++			 */
     ++			int option = ADD_CACHE_OK_TO_REPLACE;
     ++			if (pos < 0)
     ++				option = ADD_CACHE_OK_TO_ADD;
     ++
      +			ce = make_cache_entry(&the_index,
      +					      p->one->mode,
      +					      &p->one->oid,
      +					      p->one->path,
      +					      0, 0);
     -+			add_index_entry(&the_index, ce,
     -+					ADD_CACHE_OK_TO_REPLACE);
     ++			add_index_entry(&the_index, ce, option);
      +		}
      +	}
      +	diff_flush(&diff_opts);
      +
     ++	/*
     ++	 * Step 3: write the new index to disk
     ++	 */
      +	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
      +	if (write_locked_index(&the_index, &lock,
      +			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
      +		die(_("Unable to write index."));
     - }
     - 
     - static int restore_untracked(struct object_id *u_tree)
     ++}
     ++
     + static int do_apply_stash(const char *prefix, struct stash_info *info,
     + 			  int index, int quiet)
     + {
      @@ builtin/stash.c: static int do_apply_stash(const char *prefix, struct stash_info *info,
       		if (reset_tree(&index_tree, 0, 0))
       			return -1;
 3:  5143cba704 ! 3:  ccfedc7140 stash: fix stash application in sparse-checkouts
     @@ Commit message
          staged at the end and include more things that should be quasi-merged,
          (2) stash generally wants changes to NOT be staged.  It only provides
          exceptions when (a) some of the changes had conflicts and thus we want
     -    to use staged to denote the clean merges and higher order stages to
     +    to use stages to denote the clean merges and higher order stages to
          mark the conflicts, or (b) if there is a brand new file we don't want
          it to become untracked.
      
     @@ Commit message
              git update-index --add --stdin <"$a"
              rm -f "$a"
      
     -    You might that between the merge that proceeded these commands and these
     -    different commands here, that we have commands from each of the
     -    different types of special sparsity handling listed at the beginning of
     -    this message, and in fact this precisely led to the following buggy
     -    behaviors:
     +    Looking back at the different types of special sparsity handling listed
     +    at the beginning of this message, you may note that we have at least one
     +    of each type covered here: merge, diff-index, and read-tree.  The weird
     +    mix-and-match led to 3 different bugs:
      
          (1) If a path merged cleanly and it didn't match the sparsity patterns,
          the merge backend would know to avoid writing it to the working tree and
     @@ Commit message
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## builtin/stash.c ##
     -@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *cache_tree)
     - 	 * When we enter this function, there has been a clean merge of
     +@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)
       	 * relevant trees, and the merge logic always stages whatever merges
       	 * cleanly.  We want to unstage those changes, unless it corresponds
     --	 * to a file that didn't exist as of cache_tree.
     -+	 * to a file that didn't exist as of cache_tree.  However, if any
     -+	 * SKIP_WORKTREE path is modified relative to cache_tree, then we
     -+	 * want to clear the SKIP_WORKTREE bit and write it to the worktree
     -+	 * before unstaging.
     + 	 * to a file that didn't exist as of orig_tree.
     ++	 *
     ++	 * However, if any SKIP_WORKTREE path is modified relative to
     ++	 * orig_tree, then we want to clear the SKIP_WORKTREE bit and write
     ++	 * it to the worktree before unstaging.
       	 */
       
      +	struct checkout state = CHECKOUT_INIT;
     @@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *cache_
      +	state.refresh_cache = 1;
      +	state.istate = &the_index;
      +
     - 	diff_setup(&diff_opts);
     - 	diff_opts.flags.recursive = 1;
     - 	diff_opts.detect_rename = 0;
     -@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *cache_tree)
     - 			continue;
     - 		}
     - 		ce = active_cache[pos];
     -+		if (ce_skip_worktree(ce)) {
     + 	/*
     + 	 * Step 1: get a difference between orig_tree (which corresponding
     + 	 * to the index before a merge was run) and the current index
     +@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)
     + 				     strlen(p->two->path));
     + 
     + 		/*
     +-		 * Step 2: "unstage" changes, as long as they are still tracked
     ++		 * Step 2: Place changes in the working tree
     ++		 *
     ++		 * Stash is about restoring changes *to the working tree*.
     ++		 * So if the merge successfully got a new version of some
     ++		 * path, but left it out of the working tree, then clear the
     ++		 * SKIP_WORKTREE bit and write it to the working tree.
     ++		 */
     ++		if (pos >= 0 && ce_skip_worktree(active_cache[pos])) {
      +			struct stat st;
     ++
     ++			ce = active_cache[pos];
      +			if (!lstat(ce->name, &st)) {
     ++				/* Conflicting path present; relocate it */
      +				struct strbuf new_path = STRBUF_INIT;
      +				int fd;
      +
     @@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *cache_
      +				strbuf_release(&new_path);
      +			}
      +			checkout_entry(ce, &state, NULL, NULL);
     ++			ce->ce_flags &= ~CE_SKIP_WORKTREE;
      +		}
      +
     -+		ce->ce_flags &= ~CE_SKIP_WORKTREE;
     ++		/*
     ++		 * Step 3: "unstage" changes, as long as they are still tracked
     + 		 */
       		if (p->one->oid_valid) {
     - 			ce = make_cache_entry(&the_index,
     - 					      p->one->mode,
     + 			/*
     +@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)
     + 	diff_flush(&diff_opts);
     + 
     + 	/*
     +-	 * Step 3: write the new index to disk
     ++	 * Step 4: write the new index to disk
     + 	 */
     + 	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
     + 	if (write_locked_index(&the_index, &lock,
      
       ## t/t7012-skip-worktree-writing.sh ##
      @@ t/t7012-skip-worktree-writing.sh: test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '

-- 
gitgitgadget

[PATCH v2 1/3] t7012: add a testcase demonstrating stash apply bugs in sparse checkouts

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Applying stashes in sparse-checkouts, particularly when the patterns
used to define the sparseness have changed between when the stash was
created and when it is applied, has a number of bugs.  The primary
problem is that stashes are sometimes only partially applied.  In most
such cases, it does so silently without any warning or error being
displayed and with 0 exit status.

There are, however, a few cases when non-translated error messages are
shown and the stash application aborts early.  The first is when there
are files present despite the SKIP_WORKTREE bit being set, in which case
the error message shown is:

    error: Entry 'PATHNAME' not uptodate. Cannot merge.

The other situation is when a stash contains new files to add to the
working tree; in this case, the code aborts early but still has the
stash partially applied, and shows the following error message:

    error: NEWFILE: does not exist and --remove not passed
    fatal: Unable to process path NEWFILE

Add a test that can trigger all three of these problems.  Have it
carefully check that the working copy and SKIP_WORKTREE bits are as
expected after the stash application.  The test is currently marked as
expected to fail, but subsequent commits will implement the fixes and
toggle the expectation.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7012-skip-worktree-writing.sh | 88 ++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index 7476781979..a184ee97fb 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -149,6 +149,94 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
 		--diff-filter=D -- keep-me.t
 '
 
+test_expect_failure 'stash restore in sparse checkout' '
+	test_create_repo stash-restore &&
+	(
+		cd stash-restore &&
+
+		mkdir subdir &&
+		echo A >subdir/A &&
+		echo untouched >untouched &&
+		echo removeme >removeme &&
+		echo modified >modified &&
+		git add . &&
+		git commit -m Initial &&
+
+		echo AA >>subdir/A &&
+		echo addme >addme &&
+		echo tweaked >>modified &&
+		rm removeme &&
+		git add addme &&
+
+		git stash push &&
+
+		git sparse-checkout set subdir &&
+
+		# Ensure after sparse-checkout we only have expected files
+		cat >expect <<-EOF &&
+		S modified
+		S removeme
+		H subdir/A
+		S untouched
+		EOF
+		git ls-files -t >actual &&
+		test_cmp expect actual &&
+
+		test_path_is_missing addme &&
+		test_path_is_missing modified &&
+		test_path_is_missing removeme &&
+		test_path_is_file    subdir/A &&
+		test_path_is_missing untouched &&
+
+		# Put a file in the working directory in the way
+		echo in the way >modified &&
+		git stash apply &&
+
+		# Ensure stash vivifies modifies paths...
+		cat >expect <<-EOF &&
+		H addme
+		H modified
+		H removeme
+		H subdir/A
+		S untouched
+		EOF
+		git ls-files -t >actual &&
+		test_cmp expect actual &&
+
+		# ...and that the paths show up in status as changed...
+		cat >expect <<-EOF &&
+		A  addme
+		 M modified
+		 D removeme
+		 M subdir/A
+		?? actual
+		?? expect
+		?? modified.stash.XXXXXX
+		EOF
+		git status --porcelain | \
+			sed -e s/stash......./stash.XXXXXX/ >actual &&
+		test_cmp expect actual &&
+
+		# ...and that working directory reflects the files correctly
+		test_path_is_file    addme &&
+		test_path_is_file    modified &&
+		test_path_is_missing removeme &&
+		test_path_is_file    subdir/A &&
+		test_path_is_missing untouched &&
+
+		# ...including that we have the expected "modified" file...
+		cat >expect <<-EOF &&
+		modified
+		tweaked
+		EOF
+		test_cmp expect modified &&
+
+		# ...and that the other "modified" file is still present...
+		echo in the way >expect &&
+		test_cmp expect modified.stash.*
+	)
+'
+
 #TODO test_expect_failure 'git-apply adds file' false
 #TODO test_expect_failure 'git-apply updates file' false
 #TODO test_expect_failure 'git-apply removes file' false
-- 
gitgitgadget

[PATCH v2 2/3] stash: remove unnecessary process forking

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

When stash was converted from shell to a builtin, it merely
transliterated the forking of various git commands from shell to a C
program that would fork the same commands.  Some of those were converted
over to actual library calls, but much of the pipeline-of-commands
design still remains.  Fix some of this by replacing the portion
corresponding to

    git diff-index --cached --name-only --diff-filter=A $CTREE >"$a"
    git read-tree --reset $CTREE
    git update-index --add --stdin <"$a"
    rm -f "$a"

into a library function that does the same thing.  (The read-tree
--reset was already partially converted over to a library call, but as
an independent piece.)  Note here that this came after a merge operation
was performed.  The merge machinery always stages anything that cleanly
merges, and the above code only runs if there are no conflicts.  Its
purpose is to make it so that when there are no conflicts, all the
changes from the stash are unstaged.  However, that causes brand new
files from the stash to become untracked, so the code above first saves
those files off and then re-adds them afterwards.

We replace the whole series of commands with a simple function that will
unstage files that are not newly added.  This doesn't fix any bugs in
the usage of these commands, it simply matches the existing behavior but
makes it into a single atomic operation that we can then operate on as a
whole.  A subsequent commit will take advantage of this to fix issues
with these commands in sparse-checkouts.

This conversion incidentally fixes t3906.1, because the separate
update-index process would die with the following error messages:
    error: uninitialized_sub: is a directory - add files inside instead
    fatal: Unable to process path uninitialized_sub
The unstaging of the directory as a submodule meant it was no longer
tracked, and thus as an uninitialized directory it could not be added
back using `git update-index --add`, thus resulting in this error and
early abort.  Most of the submodule tests in 3906 continue to fail after
this change, this change was just enough to push the first of those
tests to success.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/stash.c           | 119 ++++++++++++++++++++++----------------
 t/lib-submodule-update.sh |  16 ++---
 2 files changed, 78 insertions(+), 57 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 3f811f3050..a5465b565a 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -325,35 +325,6 @@ static void add_diff_to_buf(struct diff_queue_struct *q,
 	}
 }
 
-static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *c_tree_hex = oid_to_hex(c_tree);
-
-	/*
-	 * diff-index is very similar to diff-tree above, and should be
-	 * converted together with update_index.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "diff-index", "--cached", "--name-only",
-		     "--diff-filter=A", NULL);
-	strvec_push(&cp.args, c_tree_hex);
-	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
-}
-
-static int update_index(struct strbuf *out)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Update-index is very complicated and may need to have a public
-	 * function exposed in order to remove this forking.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "update-index", "--add", "--stdin", NULL);
-	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
-}
-
 static int restore_untracked(struct object_id *u_tree)
 {
 	int res;
@@ -385,6 +356,75 @@ static int restore_untracked(struct object_id *u_tree)
 	return res;
 }
 
+static void unstage_changes_unless_new(struct object_id *orig_tree)
+{
+	/*
+	 * When we enter this function, there has been a clean merge of
+	 * relevant trees, and the merge logic always stages whatever merges
+	 * cleanly.  We want to unstage those changes, unless it corresponds
+	 * to a file that didn't exist as of orig_tree.
+	 */
+
+	struct diff_options diff_opts;
+	struct lock_file lock = LOCK_INIT;
+	int i;
+
+	/*
+	 * Step 1: get a difference between orig_tree (which corresponding
+	 * to the index before a merge was run) and the current index
+	 * (reflecting the changes brought in by the merge).
+	 */
+	diff_setup(&diff_opts);
+	diff_opts.flags.recursive = 1;
+	diff_opts.detect_rename = 0;
+	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+	diff_setup_done(&diff_opts);
+
+	do_diff_cache(orig_tree, &diff_opts);
+	diffcore_std(&diff_opts);
+
+	/* Iterate over the paths that changed due to the merge... */
+	for (i = 0; i < diff_queued_diff.nr; i++) {
+		struct diff_filepair *p;
+		struct cache_entry *ce;
+		int pos;
+
+		/* Look up the path's position in the current index. */
+		p = diff_queued_diff.queue[i];
+		pos = index_name_pos(&the_index, p->two->path,
+				     strlen(p->two->path));
+
+		/*
+		 * Step 2: "unstage" changes, as long as they are still tracked
+		 */
+		if (p->one->oid_valid) {
+			/*
+			 * Path existed in orig_tree; restore index entry
+			 * from that tree in order to "unstage" the changes.
+			 */
+			int option = ADD_CACHE_OK_TO_REPLACE;
+			if (pos < 0)
+				option = ADD_CACHE_OK_TO_ADD;
+
+			ce = make_cache_entry(&the_index,
+					      p->one->mode,
+					      &p->one->oid,
+					      p->one->path,
+					      0, 0);
+			add_index_entry(&the_index, ce, option);
+		}
+	}
+	diff_flush(&diff_opts);
+
+	/*
+	 * Step 3: write the new index to disk
+	 */
+	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
+	if (write_locked_index(&the_index, &lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+		die(_("Unable to write index."));
+}
+
 static int do_apply_stash(const char *prefix, struct stash_info *info,
 			  int index, int quiet)
 {
@@ -467,26 +507,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 		if (reset_tree(&index_tree, 0, 0))
 			return -1;
 	} else {
-		struct strbuf out = STRBUF_INIT;
-
-		if (get_newly_staged(&out, &c_tree)) {
-			strbuf_release(&out);
-			return -1;
-		}
-
-		if (reset_tree(&c_tree, 0, 1)) {
-			strbuf_release(&out);
-			return -1;
-		}
-
-		ret = update_index(&out);
-		strbuf_release(&out);
-		if (ret)
-			return -1;
-
-		/* read back the result of update_index() back from the disk */
-		discard_cache();
-		read_cache();
+		unstage_changes_unless_new(&c_tree);
 	}
 
 	if (!quiet) {
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 87a759149f..c6d7aab6df 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -316,14 +316,7 @@ test_submodule_switch_common () {
 	command="$1"
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
-	if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1
-	then
-		# Restoring stash fails to restore submodule index entry
-		RESULT="failure"
-	else
-		RESULT="success"
-	fi
-	test_expect_$RESULT "$command: added submodule creates empty directory" '
+	test_expect_success "$command: added submodule creates empty directory" '
 		prolog &&
 		reset_work_tree_to no_submodule &&
 		(
@@ -337,6 +330,13 @@ test_submodule_switch_common () {
 		)
 	'
 	# ... and doesn't care if it already exists.
+	if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1
+	then
+		# Restoring stash fails to restore submodule index entry
+		RESULT="failure"
+	else
+		RESULT="success"
+	fi
 	test_expect_$RESULT "$command: added submodule leaves existing empty directory alone" '
 		prolog &&
 		reset_work_tree_to no_submodule &&
-- 
gitgitgadget

[PATCH v2 3/3] stash: fix stash application in sparse-checkouts

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

sparse-checkouts are built on the patterns in the
$GIT_DIR/info/sparse-checkout file, where commands have modified
behavior for paths that do not match those patterns.  The differences in
behavior, as far as the bugs concerned here, fall into three different
categories (with git subcommands that fall into each category listed):

  * commands that only look at files matching the patterns:
      * status
      * diff
      * clean
      * update-index
  * commands that remove files from the working tree that do not match
    the patterns, and restore files that do match them:
      * read-tree
      * switch
      * checkout
      * reset (--hard)
  * commands that omit writing files to the working tree that do not
    match the patterns, unless those files are not clean:
      * merge
      * rebase
      * cherry-pick
      * revert

There are some caveats above, e.g. a plain `git diff` ignores files
outside the sparsity patterns but will show diffs for paths outside the
sparsity patterns when revision arguments are passed.  (Technically,
diff is treating the sparse paths as matching HEAD.)  So, there is some
internal inconsistency among these commands.  There are also additional
commands that should behave differently in the face of sparse-checkouts,
as the sparse-checkout documentation alludes to, but the above is
sufficient for me to explain how `git stash` is affected.

What is relevant here is that logically 'stash' should behave like a
merge; it three-way merges the changes the user had in progress at stash
creation time, the HEAD at the time the stash was created, and the
current HEAD, in order to get the stashed changes applied to the current
branch.  However, this simplistic view doesn't quite work in practice,
because stash tweaks it a bit due to two factors: (1) flags like
--keep-index and --include-untracked (why we used two different verbs,
'keep' and 'include', is a rant for another day) modify what should be
staged at the end and include more things that should be quasi-merged,
(2) stash generally wants changes to NOT be staged.  It only provides
exceptions when (a) some of the changes had conflicts and thus we want
to use stages to denote the clean merges and higher order stages to
mark the conflicts, or (b) if there is a brand new file we don't want
it to become untracked.

stash has traditionally gotten this special behavior by first doing a
merge, and then when it's clean, applying a pipeline of commands to
modify the result.  This series of commands for
unstaging-non-newly-added-files came from the following commands:

    git diff-index --cached --name-only --diff-filter=A $CTREE >"$a"
    git read-tree --reset $CTREE
    git update-index --add --stdin <"$a"
    rm -f "$a"

Looking back at the different types of special sparsity handling listed
at the beginning of this message, you may note that we have at least one
of each type covered here: merge, diff-index, and read-tree.  The weird
mix-and-match led to 3 different bugs:

(1) If a path merged cleanly and it didn't match the sparsity patterns,
the merge backend would know to avoid writing it to the working tree and
keep the SKIP_WORKTREE bit, simply only updating it in the index.
Unfortunately, the subsequent commands would essentially undo the
changes in the index and thus simply toss the changes altogether since
there was nothing left in the working tree.  This means the stash is
only partially applied.

(2) If a path existed in the worktree before `git stash apply` despite
having the SKIP_WORKTREE bit set, then the `git read-tree --reset` would
print an error message of the form
      error: Entry 'modified' not uptodate. Cannot merge.
and cause stash to abort early.

(3) If there was a brand new file added by the stash, then the
diff-index command would save that pathname to the temporary file, the
read-tree --reset would remove it from the index, and the update-index
command would barf due to no such file being present in the working
copy; it would print a message of the form:
      error: NEWFILE: does not exist and --remove not passed
      fatal: Unable to process path NEWFILE
and then cause stash to abort early.

Basically, the whole idea of unstage-unless-brand-new requires special
care when you are dealing with a sparse-checkout.  Fix these problems
by applying the following simple rule:

  When we unstage files, if they have the SKIP_WORKTREE bit set,
  clear that bit and write the file out to the working directory.

  (*) If there's already a file present in the way, rename it first.

This fixes all three problems in t7012.13 and allows us to mark it as
passing.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/stash.c                  | 50 ++++++++++++++++++++++++++++++--
 t/t7012-skip-worktree-writing.sh |  2 +-
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index a5465b565a..84886e84e0 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -363,12 +363,23 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)
 	 * relevant trees, and the merge logic always stages whatever merges
 	 * cleanly.  We want to unstage those changes, unless it corresponds
 	 * to a file that didn't exist as of orig_tree.
+	 *
+	 * However, if any SKIP_WORKTREE path is modified relative to
+	 * orig_tree, then we want to clear the SKIP_WORKTREE bit and write
+	 * it to the worktree before unstaging.
 	 */
 
+	struct checkout state = CHECKOUT_INIT;
 	struct diff_options diff_opts;
 	struct lock_file lock = LOCK_INIT;
 	int i;
 
+	/* If any entries have skip_worktree set, we'll have to check 'em out */
+	state.force = 1;
+	state.quiet = 1;
+	state.refresh_cache = 1;
+	state.istate = &the_index;
+
 	/*
 	 * Step 1: get a difference between orig_tree (which corresponding
 	 * to the index before a merge was run) and the current index
@@ -395,7 +406,42 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)
 				     strlen(p->two->path));
 
 		/*
-		 * Step 2: "unstage" changes, as long as they are still tracked
+		 * Step 2: Place changes in the working tree
+		 *
+		 * Stash is about restoring changes *to the working tree*.
+		 * So if the merge successfully got a new version of some
+		 * path, but left it out of the working tree, then clear the
+		 * SKIP_WORKTREE bit and write it to the working tree.
+		 */
+		if (pos >= 0 && ce_skip_worktree(active_cache[pos])) {
+			struct stat st;
+
+			ce = active_cache[pos];
+			if (!lstat(ce->name, &st)) {
+				/* Conflicting path present; relocate it */
+				struct strbuf new_path = STRBUF_INIT;
+				int fd;
+
+				strbuf_addf(&new_path,
+					    "%s.stash.XXXXXX", ce->name);
+				fd = xmkstemp(new_path.buf);
+				close(fd);
+				printf(_("WARNING: Untracked file in way of "
+					 "tracked file!  Renaming\n "
+					 "           %s -> %s\n"
+					 "         to make room.\n"),
+				       ce->name, new_path.buf);
+				if (rename(ce->name, new_path.buf))
+					die("Failed to move %s to %s\n",
+					    ce->name, new_path.buf);
+				strbuf_release(&new_path);
+			}
+			checkout_entry(ce, &state, NULL, NULL);
+			ce->ce_flags &= ~CE_SKIP_WORKTREE;
+		}
+
+		/*
+		 * Step 3: "unstage" changes, as long as they are still tracked
 		 */
 		if (p->one->oid_valid) {
 			/*
@@ -417,7 +463,7 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)
 	diff_flush(&diff_opts);
 
 	/*
-	 * Step 3: write the new index to disk
+	 * Step 4: write the new index to disk
 	 */
 	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
 	if (write_locked_index(&the_index, &lock,
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index a184ee97fb..e5c6a038fb 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -149,7 +149,7 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
 		--diff-filter=D -- keep-me.t
 '
 
-test_expect_failure 'stash restore in sparse checkout' '
+test_expect_success 'stash restore in sparse checkout' '
 	test_create_repo stash-restore &&
 	(
 		cd stash-restore &&
-- 
gitgitgadget

Re: [PATCH v2 2/3] stash: remove unnecessary process forking

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     git diff-index --cached --name-only --diff-filter=A $CTREE >"$a"
>     git read-tree --reset $CTREE
>     git update-index --add --stdin <"$a"
>     rm -f "$a"

This is orthogonal to what this patch does, as this is supposed to
be just bug-for-bug compatible rewrite.

But I wonder if the above sequence, whether it is done as a series
of plumbing invocations or subroutine calls, is a relic dating back
in the days before i-t-a existed.  If we want to revert the changes
to the index for working tree files for removed or modified ones, I
do not offhand see a good reason why we would want to keep the
contents to new paths---if i-t-a were available when the sequence
was designed, I suspect we would just have added the path as i-t-a
in order to keep track of the presence of the path but not
necessarily the contents in it.

Re: [PATCH v2 2/3] stash: remove unnecessary process forking

[Elijah Newren]

On Tue, Dec 1, 2020 at 3:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >     git diff-index --cached --name-only --diff-filter=A $CTREE >"$a"
> >     git read-tree --reset $CTREE
> >     git update-index --add --stdin <"$a"
> >     rm -f "$a"
>
> This is orthogonal to what this patch does, as this is supposed to
> be just bug-for-bug compatible rewrite.
>
> But I wonder if the above sequence, whether it is done as a series
> of plumbing invocations or subroutine calls, is a relic dating back
> in the days before i-t-a existed.  If we want to revert the changes
> to the index for working tree files for removed or modified ones, I
> do not offhand see a good reason why we would want to keep the
> contents to new paths---if i-t-a were available when the sequence
> was designed, I suspect we would just have added the path as i-t-a
> in order to keep track of the presence of the path but not
> necessarily the contents in it.

Yeah, I thought a little bit about the same thing, but wasn't sure if
there was some other reason for the current behavior or if there was
some workflow that might be relying upon it.  Rather than investigate
and try to switch it over to i-t-a (which could still be done later),
I more narrowly focused this series at just doing the "make the
changes be available in the working tree, but remove them from the
index unless it'd make it untracked" more carefully.