On Thu, Aug 15, 2013 at 10:52:39AM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html