On Wed, Apr 22, 2015 at 10:41:04AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > Right, I am suggesting that latter: that stash should abort if the index
> > has modified entries. The abort for modified working tree files is done
> > by git-merge, which can be selective about which entries will be changed
> > (since it knows which ones need written). I haven't thought hard enough
> > to say whether it should be doing the same for the index (i.e., whether
> > this is a "merge" problem or a "stash" problem).
>
> This is a stash problem. I've always thought that it insisted on
> having a clean index and a clean working tree, but apparently it
> doesn't, as shown in Dmitry's example sequence.
It did check the working tree manually, until my e0e2a9c (stash: drop
dirty worktree check on apply, 2011-04-05). But I don't think we ever
checked that the index was clean with respect to HEAD.
> Generally speaking, any mergy operation should be done with a clean
> index (i.e. matches HEAD) so that any difference between the index
> and the HEAD after it stops in the middle is purely the result of
> that mergy operation, and the commands should stop when the index is
> not clean as a safety measure (e.g. "git am -3", "git merge") [*1*].
I guess this was the heart of my question. Should a mergy operation
start with a clean index (which the caller can enforce), or does it only
need the index entries that it is going to touch to be clean (which is
known only to the merging code)? The latter is more permissive, as we
know that we will not create conflict entries that overwrite what is
staged in the index. But I don't think we have good tool support for
operating on only those entries afterwards (e.g., "git reset --keep"
would hose your staged changes along with undoing the parts modified by
the merge). So probably asking for a completely clean index is the only
sane thing.
> An especially tricky command may also insist on a clean working tree
> if it is not easy to guarantee that it will not clobber changes by
> the user when it stops in the middle (e.g. "git rebase"). But this
> is an escape hatch for lazy implementations ;-) It would be ideal if
> a command stops without doing anything when the set of paths it
> needs to touch would overlap the set of paths in which user has
> changes before the command is run (e.g. "git merge" works that way).
Right, and I think we do that appropriately in stash since e0e2a9c.
> I think that "stash apply" requires a clean working tree for the
> latter reason, and does not require a clean index because it just
> forgot that it must do so.
Ironically, the message before e0e2a9c actually recommends staging
changes before applying the stash, which would lead to this exact
situation! So I think the most trivial patch is:
diff --git a/git-stash.sh b/git-stash.sh
index d4cf818..f1865c9 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -442,6 +442,7 @@ apply_stash () {
assert_stash_like "$@"
git update-index -q --refresh || die "$(gettext "unable to refresh index")"
+ git diff-index --cached HEAD || die "dirty index; cannot apply stash"
# current index state
c_tree=$(git write-tree) ||
but it makes me wonder if somebody would find it annoying that they
cannot apply a stash into their work-in-progress (i.e., it _might_ cause
annoyance, but most of the time it will be convenient to do so).
We also have require_clean_work_tree() in git-sh-setup.sh. We definitely
don't want the first half of that, but we do want the diff-index check.
So probably we'd want to refactor that into two separate functions, and
only call the require_clean_index part.
-Peff
--
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