From: Johannes Sixt <j.sixt@viscovery.net>
Date: Wed, 09 Apr 2008 06:55:31 GMT
Subject: Re: [PATCH 1/8] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Message-ID: <47FC6863.8070704@viscovery.net>
In-Reply-To: <1207704604-30393-1-git-send-email-bdonlan@fushizen.net>

Bryan Donlan schrieb:
> -dotest=$GIT_DIR/.dotest-merge
> +dotest="$GIT_DIR/.dotest-merge"

This is not strictly necessary: The RHS expression of an assignment does
not undergo IFS splitting; but better safe than sorry. (But note that
'export foo=$bar', which is not POSIX, is *not* an assignment, and
different shells that support this construct treat it differently.)

> +### Test that we handle strange characters properly
> +work_dir="$(pwd)/test \" ' \$ \\ dir"

In effect, you modify only this test to stress-test strange characters,
but other tests in the test suite still run in a "sane" environment. IOW,
I don't think you should go to this extreme for this one test only. The
better approach would be to rename 'trash' in test-lib.sh to this strange
name so that all tests suffer from a challenging environment.

> -		git reset --hard pre-rebase
> -		test_must_fail git rebase'"$type"' master &&
> -		test -d '$dotest' &&
> +		git reset --hard pre-rebase &&
> +		test_must_fail git rebase$type master &&
> +		test -d \"\$dotest\" &&

I could imagine that the missing && after the git reset is deliberate. Mike?

> +		git reset --hard pre-rebase &&

Ditto.

Apart from that, this looks good.

-- Hannes