Re: [PATCH v2 02/12] t5520: test for failure if index has unresolved entries

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 7, 2015, 18:28 UTC
Message-ID: <CAPig+cRBrcC+Zud=mQoCco9E3DC66gZkX5C0wTWxQ4B-Oje3eQ@mail.gmail.com>
In-Reply-To: <1430988248-18285-3-git-send-email-pyokagan@gmail.com>

A couple very minor comments applying to the entire patch series...

On Thu, May 7, 2015 at 4:43 AM, Paul Tan <pyokagan@gmail.com> wrote:

> Commit d38a30df (Be more user-friendly when refusing to do something
> because of conflict) introduced code paths to git-pull which will error

Custom for citing a commit is also to include the date:

    d38a30df (Be more user-friendly...conflict, 2010-01-12)

Some people use this git alias to help automate:

    whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short

Show 32 quoted lines

> out with user-friendly advices if the user is in the middle of a merge
> or has unmerged files.
>
> Implement tests to ensure that git-pull will not run, and will print
> these advices, if the user is in the middle of a merge or has unmerged
> files in the index.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  t/t5520-pull.sh | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 5add900..37ff45f 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -164,6 +164,27 @@ test_expect_success 'fail if upstream branch does not exist' '
>         test `cat file` = file
>  '
>
> +test_expect_success 'fail if the index has unresolved entries' '
> +       git checkout -b third master^ &&
> +       test_when_finished "git checkout -f copy && git branch -D third" &&
> +       echo file >expected &&
> +       test_cmp expected file &&
> +       echo modified2 >file &&
> +       git commit -a -m modified2 &&
> +       test -z "$(git ls-files -u)" &&
> +       test_must_fail git pull . second &&
> +       test -n "$(git ls-files -u)" &&
> +       cp file expected &&
> +       test_must_fail git pull . second 2>out &&

Perhaps call this stderr capture file 'err' rather than 'out' to clarify its nature and to distinguish it from a stdout capture which someone might add in the future?

Show 14 quoted lines

> +       test_i18ngrep "Pull is not possible because you have unmerged files" out &&
> +       test_cmp expected file &&
> +       git add file &&
> +       test -z "$(git ls-files -u)" &&
> +       test_must_fail git pull . second 2>out &&
> +       test_i18ngrep "You have not concluded your merge" out &&
> +       test_cmp expected file
> +'
> +
>  test_expect_success '--rebase' '
>         git branch to-rebase &&
>         echo modified again > file &&
> --
> 2.1.4