Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.

One question about test patches. Are you ok with test_expect_failure
tests that document the expected failure of a feature yet to be
developed, followed by the feature, followed by the patch that makes
the tests into test_expect_success tests, or would you prefer to see
the pre- and post- test patches rolled into a single test that is
delivered after the feature patch?

On Thu, Aug 5, 2010 at 3:23 PM, Jon Seymour [off-list ref] wrote:

Junio,

Thanks for the feedback. I'll rework along the lines you suggest. If
it makes sense to make the other stash commands tolerant of non-stash
entry references I'll add tests, support and documentation for that.

jon.

On Thu, August 2010 at 9:51 AM, Junio C Hamano [off-list ref] wrote:

quoted

Jon  Seymour [off-list ref] writes:

quoted

This patch allows git stash branch to work with stash-like commits created by git stash create.

Two changes were required:

* relax the pre-condition so that a stash stack is required if and only if a stash argument is not specified
* don't attempt to drop a stash argument that doesn't look like a stash reference.


Signed-off-by: Jon Seymour <redacted>

Please wrap very long lines.

quoted

diff --git a/git-stash.sh b/git-stash.sh
index 1d95447..432ddae 100755
--- a/git-stash.sh
+++ b/git-stash.sh

@@ -225,6 +225,12 @@ show_stash () {

      git diff $flags $b_commit $w_commit
 }

+if_stash_ref() {
+     ref="$1"
+     shift
+     test "${ref#stash}" = "${ref}" -a "${ref#$ref_stash}" = "${ref}" || "$@"
+}

The interface to this function looks a rather bad taste to me; wouldn't it
look more natural if the callers can say:

       if stash_ref $it
       then
               do this
       fi

Your criteria used here is that the given parameter does not begin with
"stash" nor "refs/stash".  If it begins with either of these two strings,
the "test" fails and "$@" is run.  Wouldn't this produce a false hit if
you kept a handcrafted stash-looking commit with a tag "stash-42" or
something?

It may make more sense to give "stash drop" an option to be silent if
the given parameter is not on the list to begin with, perhaps?