Re: git stash takes excessively long when many untracked files present

On Thu, Aug 15, 2013 at 10:52:39AM -0700, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

In any case, this is a regression introduced in 'master' since the
last release, and the attempted fix was for an issue that has long
been with us, so I'll revert a7365313 (git stash: avoid data loss
when "git stash save" kills a directory, 2013-06-28) soon.  For
today's -rc3, I'm already deep into the integration cycle, so it is
too late to do the revert it and then redo everything.

Then we will plan to re-apply the patch once "ls-files --killed"
gets fixed not to waste too much cycles needlessly, after the coming
release.

I've already reverted the problematic patch to "git stash" and it
will not be part of the upcoming release.

Thanks!

Here is a quick attempt to see if we can do better in "ls-files -k".

We have an existing test t3010.3 that tries all the combinations of
directory turning into a regular file, symlink, etc. and vice versa,
and it seems to pass.  The test has a directory path6 in the working
tree without any paths in it in the index, and the added bypass code
seems to correctly trigger and prevents us from digging into that
directory, so this patch may be sufficient to improve "ls-files -k".

By the way, regarding the reverted commit, I do not think it is
enough to ask "ls-files -k" to see if the state recorded in the
current index is sufficient.  Imagine your HEAD records "path" as a
file and then you did this:

    $ git reset --hard ;# "path" is now a regular file
    $ mv path path.bak
    $ mkdir path
    $ mv path.bak path/file
    $ git add -A ;# "path/file" in the index and in the working tree
    $ >path/cruft ;# "path/cruft" in the working tree

Then call "save_stash" without saving untracked.  The resulting
stash will save the contents of "path/file" but "path/cruft" is not
recorded anywhere, and then we would need to bring the state in the
working tree and the index back to the state recorded in HEAD, hence
"path" needs to be turned back to a directory.

But "ls-files -k" is asked to check with the index, which has the
path as a directory, so this case is missed.

Since git stash resets to the state in HEAD, whatever --killed check it
does needs to check against HEAD, yes.  It still doesn't need to check
any path that doesn't exist in HEAD, though; it makes more sense to
drive this from the list of files in HEAD rather than from the list of
files in the working directory, even with a filter applied to the latter
to prune bits not in HEAD.

So instead of

	test -n "$(git ls-files --killed | head -n 1)"

in Pasky's patch, which probably is a right thing to do if you are
running "git stash save --keep-index", you would need something like
this if you are not running with "--keep-index":

	test -n "$(
        	GIT_INDEX_FILE=tmp_index
                export GIT_INDEX_FILE
                git read-tree HEAD
                git ls-files -k
	)"

in order to make sure that the result of going back to the state in
the HEAD will not clobber leftover "path/cruft".

Sure, that works.  However, wouldn't it make sense to just directly let
git ls-files output to the screen, then test its return value (after
adding some ls-files option to set the return value)?  Since ls-files
--killed will have no output if git stash can proceed, and since git
stash should show the list of files that'd be killed before it fails,
using the output directly makes sense.

- Josh Triplett