From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 10 Dec 2007 03:42:45 GMT
Subject: Re: [PATCH] Interactive editor tests for commit-msg hook
Message-ID: <7vwsrnmczu.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1197204731-19553-1-git-send-email-win@wincent.com>

Wincent Colaiuta <win@wincent.com> writes:

> I didn't add similar tests for the pre-commit hook because I don't
> think that's an interesting code path; we don't care about the commit
> message in that case, only whether the commit is allowed to proceed or
> not.

Sensible.

> +# set up fake editor for interactive editing
> +cat > fake-editor <<'EOF'
> +#!/bin/sh
> +cp FAKE_MSG "$1"
> +exit 0
> +EOF
> +chmod +x fake-editor
> +FAKE_EDITOR="$(pwd)/fake-editor"
> +export FAKE_EDITOR

Hmm.  Why "export"?

> +test_expect_success "with no hook (editor)" \
> +	"echo 'more foo' >> file &&
> +	 git add file &&
> +	 echo 'more foo' > FAKE_MSG &&
> +	 GIT_EDITOR="$FAKE_EDITOR" git commit"
> +

I initially was going to say "this is just a style thing", but I really
want our tests to read uniformly like this:

        test_expect_success 'without hook (editor)' '

                echo more foo >>file &&
                git add file &&
                echo more foo >FAKE_MSG &&
                GIT_EDITOR="$FAKE_EDITOR" git commit

        '

 * Use single quotes around test unless there is a compelling reason not
   to;

 * The opening single quote of the test body on the same line as
   expect-success; the end quote at column 1 on a line on its own.

Notice a bug in your version?  Dq around $FAKE_EDITOR is stripped away
and it is harder to spot it because the script does not follow that
style.