threads / patch / 33533

patchstash: tighten IS_STASH_LIKE logic

Subject: [PATCH] stash: tighten IS_STASH_LIKE logic

## tl;dr

6 messages between Apr 18, 2013 and Apr 19, 2013. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Ramkumar Ramachandra· Apr 18, 2013, 07:38 UTC · lore

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(-)

Show changes to 2 files +13 −1

git-stash.sh, t/t3903-stash.sh

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

Junio C Hamano· Apr 18, 2013, 22:20 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 5 quoted lines

> 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?

Ramkumar Ramachandra· Apr 19, 2013, 05:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

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:

Show 6 quoted lines

> 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?

Junio C Hamano· Apr 19, 2013, 17:50 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

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.

Ramkumar Ramachandra· Apr 19, 2013, 18:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

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.

Junio C Hamano· Apr 19, 2013, 18:54 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] stash: tighten IS_STASH_LIKE logic

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 8 quoted lines

> 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.