Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> On Tue, May 10, 2016 at 3:12 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Tue, May 10, 2016 at 2:39 PM, Jeff King <peff@xxxxxxxx> wrote:
>>> On Tue, May 10, 2016 at 01:20:54AM -0400, Eric Sunshine wrote:
>>>> while :
>>>> do
>>>> case "$1" in
>>>> -C) dir="-C $2"; shift; shift ;;
>>>> -b) bare="$2"; shift; shift ;;
>>>> + -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
>>>> @@ -36,6 +38,8 @@ test_rev_parse () {
>>>> do
>>>> expect="$1"
>>>> test_expect_success "$name: $o" '
>>>> + test_when_finished "sane_unset GIT_DIR" &&
>>>> + eval $env &&
>>>
>>> This will set up the sane_unset regardless of whether $env does
>>> anything. Would it make more sense to stick the test_when_finished
>>> inside $env? You could use regular unset then, too, since you know the
>>> variable would be set.
>>
>> I didn't worry about it too much because the end result is effectively
>> the same and, with all the 'case' arms being short one-liners, I think
>> the code is a bit easier to read as-is; bundling 'test_when_finished'
>> into the 'env' assignment line would probably require wrapping the
>> line. I do like the improved encapsulation of your suggestion but
>> don't otherwise feel strongly about it.
>
> Actually, I think we can have improved encapsulation and maintain
> readability like this:
>
> case "$1" in
> ...
> -g) env="$2"; shift; shift ;;
> ...
> esac
>
> ...
> test_expect_success "..." '
> if test -n "$env"
> do
> test_when_finished "unset GIT_DIR"
> GIT_DIR="$env"
> export GIT_DIR
> fi
> ...
> '
That's even better. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html