Re: [PATCH 2/7] t5520: implement tests for no merge candidates cases

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: May 4, 2015, 08:04 UTC
Message-ID: <vpqvbg8u5sq.fsf@anie.imag.fr>
In-Reply-To: <1430581035-29464-3-git-send-email-pyokagan@gmail.com>

Thanks for the patch. On overall, it looks good to me. Some minor comments below.

Paul Tan <pyokagan@gmail.com> writes:

Show 6 quoted lines

> Commit a8c9bef4 fully established the current advices given by git-pull
> for the different cases where git-fetch will not have anything marked
> for merge:
>
> 1. We're not on a branch, so there is no branch
>    configuration.

Nit: you seem to be wrapping your lines with a rather short length. I
would prefer reading this as a single line:

1. We're not on a branch, so there is no branch configuration.

(62 columns)

Show 6 quoted lines

> ---
>
> I'm having trouble hitting the 1st case without resorting to the hack below. A
> detached HEAD will always have no remote configured, and the code flow would
> make it such that case (4) is hit in the detached HEAD case instead of case
> (1).

This should appear in comments in the test 'fail if not on a branch'. People reading your [branch ""] in the future won't look for below-triple-dash comments in the mailing-list archives ...

And actually, it would be more user-friendly to trigger this error message in the normal senario, i.e. check for 1. before 4. in the code. This was most likely the intension of the programmer who wrote this error message. You may want to fix this now, or add a test_expect_failure which will become a test_expect_success when you replace git-pull.sh with builtin/pull.c.

Show 23 quoted lines

>  t/t5520-pull.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 01ae1bf..635ec02 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -122,6 +122,58 @@ test_expect_success 'the default remote . should not break explicit pull' '
>  	test `cat file` = modified
>  '
>  
> +test_expect_success 'fail if not on a branch' '
> +	cp .git/config .git/config.bak &&
> +	test_when_finished "cp .git/config.bak .git/config" &&
> +	git remote add test_remote . &&
> +	git checkout HEAD^{} &&
> +	test_when_finished "git checkout -f copy" &&
> +	cat >>.git/config <<-\EOF &&
> +	[branch ""]
> +	remote = test_remote
> +	EOF
> +	test_must_fail git pull test_remote 2>out &&
> +	test_i18ngrep "You are not currently on a branch" out

It may make sense to grep only a subset of the string, to be less sensitive to rewords of error message. For example, just:

test_i18ngrep "not currently on a branch"

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/