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, 05:23 UTC
- Message-ID
- <AANLkTikA0_Og4bzB8AHo3s2cLCvf6pc9=wC4w_8emuxU@mail.gmail.com>
- In-Reply-To
- <7vwrs6djie.fsf@alter.siamese.dyndns.org>
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:
Show 46 quoted lines
> 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?
>
>