[PATCH] stash: tighten IS_STASH_LIKE logic

[PATCH] stash: tighten IS_STASH_LIKE logic

[Ramkumar Ramachandra]

Currently, 'git stash show' and 'git stash apply' can show/apply any
merge commit, as the test for setting IS_STASH_LIKE simply asserts if
the commit is a merge.  Improve the situation by asserting if the
index_commit and the worktree_commit are based off the same commit, by
checking that $REV^1 is equal to $REV^2^1.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-stash.sh     |  3 ++-
 t/t3903-stash.sh | 11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index bbefdf6..d0428a8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -366,13 +366,14 @@ parse_flags_and_rev()
 	}
 
 	i_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&
-	set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&
+	set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: $REV^2^1 2>/dev/null) &&
 	s=$1 &&
 	w_commit=$1 &&
 	b_commit=$2 &&
 	w_tree=$3 &&
 	b_tree=$4 &&
 	i_tree=$5 &&
+	test $b_commit = $6 &&
 	IS_STASH_LIKE=t &&
 	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" &&
 	IS_STASH_REF=t
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5dfbda7..11bcd72 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -637,4 +637,15 @@ test_expect_success 'stash where working directory contains "HEAD" file' '
 	test_cmp output expect
 '
 
+test_expect_success 'show refuses to show any random merge commit' '
+	git stash clear &&
+	git reset --hard &&
+	git checkout -b quux &&
+	test_commit bar &&
+	git checkout - &&
+	test_commit foo &&
+	git merge quux &&
+	test_must_fail git stash show HEAD
+'
+
 test_done
-- 
1.8.2.1.423.g4fb5c0a.dirty

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Currently, 'git stash show' and 'git stash apply' can show/apply any
> merge commit, as the test for setting IS_STASH_LIKE simply asserts if
> the commit is a merge.  Improve the situation by asserting if the
> index_commit and the worktree_commit are based off the same commit, by
> checking that $REV^1 is equal to $REV^2^1.

When was the last time you tried to run "git stash apply next"?

The current code parses the ancestors and trees because the real
work needs them, and the 'does this look like a stash?' is a side
effect, but reading it^2^1 and comparing with it^1 is a pure
overhead for the sake of checking.  It looks like a futile mental
masturbation to solve theoretical non-issue to me.

Is it worth it?

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> [...]

I'm curious.  Why are you going back on what you said just one day
ago?  What changed?

In a previous email, you wrote:
> You are free to try to think of a way to tighten the implemention to
> reject a random two-or-three parent merge commit that is not a
> product of "stash create".  People already have looked at this code
> since it was written, and didn't find a reasonable way to tighten it
> without triggering false negatives, so I wouldn't be surprised if
> anybody tried it again today and failed.

So, my patch is not a "reasonable" way to achieve this?

> When was the last time you tried to run "git stash apply next"?

My patch is not solving an end-user problem.  Think of it as a source
code comment: to answer the question "what kind of commit does stash
create make?", the reader simple has to look at when IS_STASH_LIKE is
set.  It's helping make the source code clearer.  Previously,
IS_STASH_LIKE might as well have been named IS_MERGE_COMMIT, and
nothing would've changed.  The reader will wonder what IS_MERGE_COMMIT
has to do with stashes, so we named it IS_STASH_LIKE.  This is another
minor improvement in the same spirit.

> Is it worth it?

Is it worth what?  What are we losing?

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> My patch is not solving an end-user problem.  Think of it as a source
> code comment: to answer the question "what kind of commit does stash
> create make?",

There already is sufficient comment that explains how a stash commit
is constructed, isn't it?

I may have forgotten to say this, but we were helped by the logic
that makes sure we can read what we need to carry out the operation
and nothing more in the then-current (which is the same as current)
code that was written before we added --include-untracked.  If the
check were enforcing that a stash-like must be a two parent merge,
which may have seemed reasonable back when the stash-like logic was
written, it would have been more painful to add three parent
possibility while still allowing people to use different vintage of
Git in the same repository.

Insisting on the i-commit and w-commit to have exactly the same
parent would mean that somebody who wants to improve create-stash
has one less option---even when the easiest and/or cleanest way to
improve create-stash for the particular goal were to record these
two commits based on a different parents, the requirement the patch
is trying to add here will prevent her from doing so and force her
to work around the pointless (read: not necessary when the current
code shows or applies the stash) check.

>> Is it worth it?
>
> Is it worth what?  What are we losing?

The risk of actually closing the door for future developers.  That
is a downside of this patch, if we were to apply it.

And having to spend braincycles to worry about what door we may be
unintentionally closing.  That's a downside of even discussing this
patch.

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> The risk of actually closing the door for future developers.  That
> is a downside of this patch, if we were to apply it.

Okay, no issues.  Drop it.

Consider documenting the fact that IS_STASH_LIKE is merely a
contextual synonym for IS_MERGE_COMMIT somewhere though.

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> The risk of actually closing the door for future developers.  That
>> is a downside of this patch, if we were to apply it.
>
> Okay, no issues.  Drop it.
>
> Consider documenting the fact that IS_STASH_LIKE is merely a
> contextual synonym for IS_MERGE_COMMIT somewhere though.

Documenting what IS_STASH_LIKE means and what it is meant to be used
for would add a lot of value, if it is not done already, but I doubt
it adds much value to document that it _happens to be implemented_
as "is it a merge commit?" in the current version.