Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.
- From
Jon Seymour <jon.seymour@gmail.com>
- Date
- Aug 5, 2010, 07:50 UTC
- Message-ID
- <AANLkTi=FbF3X83uLjBDoCYUCuig3udTzZCXuE=1_w-Ld@mail.gmail.com>
- In-Reply-To
- <AANLkTikA0_Og4bzB8AHo3s2cLCvf6pc9=wC4w_8emuxU@mail.gmail.com>
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 <jon.seymour@gmail.com> wrote:
Show 56 quoted lines
> 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 <gitster@pobox.com> wrote:
>> Jon Seymour <jon.seymour@gmail.com> writes:
>>
>>> 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 <jon.seymour@gmail.com>
>>
>> Please wrap very long lines.
>>
>>> 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?
>>
>>
>