v2, 12 partsImprove git-pull test coverage

> 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 fetched from a specific remote, and a refspec was given, but it
>     ended up not fetching anything. This is usually because the user
>     provided a wildcard refspec which had no matches on the remote end.
>
> 2. We fetched from a non-default remote, but didn't specify a branch to
>     merge. We can't use the configured one because it applies to the
>     default remote, and thus the user must specify the branches to merge.
>
> 3. We fetched from the branch's or repo's default remote, but:
>
>     a. We are not on a branch, so there will never be a configured branch
>        to merge with.
>
>     b. We are on a branch, but there is no configured branch to merge
>        with.
>
> 4. We fetched from the branch's or repo's default remote, but the
>     configured branch to merge didn't get fetched (either it doesn't
>     exist, or wasn't part of the configured fetch refspec)
>
> Implement tests for the above 5 cases to ensure that the correct code
> paths are triggered for each of these cases.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>      * Re-worded commit message to match the logic used in git-pull.sh's
>        error_on_no_merge_candidates().
>      
>      * The tests have thus also been reordered to match the commit message.
>      
>      * Non-hackish solution for case 3a.
>      
>      * Add more checks to ensure that git-pull does not touch any files it
>        should not be touching on failure.
>
>   t/t5520-pull.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 7efd45b..5add900 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -109,6 +109,61 @@ test_expect_success 'the default remote . should not break explicit pull' '
>   	test `cat file` = modified
>   '
>   
> +test_expect_success 'fail if wildcard spec does not match any refs' '
> +	git checkout -b test copy^ &&
> +	test_when_finished "git checkout -f copy && git branch -D test" &&
> +	test `cat file` = file &&

> Torsten Bögershausen <tboegi@web.de> writes:
>
>> In other words:
>> test $(cat file) = file &&
>
> Is there a guarantee that file has a single word?  Can it be empty?
> Can it contain "foo bar\n"?

> Hi Junio,
>
> On Thu, May 7, 2015 at 10:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>>
>>> In other words:
>>> test $(cat file) = file &&
>>
>> Is there a guarantee that file has a single word?  Can it be empty?
>> Can it contain "foo bar\n"?
>
> It can, but it should not ;-). But yes, this will need to be quoted as
> well to be safe. Whoops.

> Torsten Bögershausen <tboegi@web.de> writes:
>
>> In other words:
>> test $(cat file) = file &&
> Is there a guarantee that file has a single word?  Can it be empty?
> Can it contain "foo bar\n"?

> 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 &&

> +       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

> 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

>> 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?

> Since commit b10ac50f (Fix pulling into the same branch), git-pull,
> upon detecting that git-fetch updated the current head, will
> fast-forward the working tree to the updated head commit.
>
> Implement tests to ensure that the fast-forward occurs in such a case,
> as well as to ensure that the user-friendly advice is printed upon
> failure.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>     * Ensure that on fast-forward failure, if there is a conflict, the work
>       tree should not be touched.
>
>  t/t5520-pull.sh | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 37ff45f..99b6f67 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -185,6 +185,28 @@ test_expect_success 'fail if the index has unresolved entries' '
>         test_cmp expected file
>  '
>
> +test_expect_success 'fast-forwards working tree if branch head is updated' '
> +       git checkout -b third master^ &&
> +       test_when_finished "git checkout -f copy && git branch -D third" &&
> +       echo file >expected &&
> +       test_cmp expected file &&
> +       git pull . second:third 2>out &&
> +       test_i18ngrep "fetch updated the current branch head" out &&
> +       echo modified >expected &&
> +       test_cmp expected file
> +'
> +
> +test_expect_success 'fast-forward fails with conflicting work tree' '
> +       git checkout -b third master^ &&
> +       test_when_finished "git checkout -f copy && git branch -D third" &&
> +       echo file >expected &&
> +       test_cmp expected file &&
> +       echo conflict >file &&
> +       test_must_fail git pull . second:third 2>out &&
> +       test_i18ngrep "Cannot fast-forward your working tree" out &&
> +       test `cat file` = conflict

> +'
> +
>  test_expect_success '--rebase' '
>         git branch to-rebase &&
>         echo modified again > file &&
> --
> 2.1.4
>

> +test_expect_success 'fast-forward fails with conflicting work tree' '
> +	git checkout -b third master^ &&
> +	test_when_finished "git checkout -f copy && git branch -D third" &&
> +	echo file >expected &&
> +	test_cmp expected file &&
> +	echo conflict >file &&
> +	test_must_fail git pull . second:third 2>out &&
> +	test_i18ngrep "Cannot fast-forward your working tree" out &&
> +	test `cat file` = conflict

> +'
> +
>  test_expect_success '--rebase' '
>  	git branch to-rebase &&
>  	echo modified again > file &&

> Paul Tan <pyokagan@gmail.com> writes:
>
>> +test_expect_success 'fast-forward fails with conflicting work tree' '
>> +     git checkout -b third master^ &&
>> +     test_when_finished "git checkout -f copy && git branch -D third" &&
>> +     echo file >expected &&
>> +     test_cmp expected file &&
>> +     echo conflict >file &&
>> +     test_must_fail git pull . second:third 2>out &&
>> +     test_i18ngrep "Cannot fast-forward your working tree" out &&
>> +     test `cat file` = conflict
>
> At this point, HEAD would match either master^ (as initially checked
> out) or second (as fetch fast-forwarded), but I cannot read what this
> test is expecting to happen.
>
> Should the HEAD move or stay?

> Commit 19a7fcbf (allow pull --rebase on branch yet to be born) special
> cases git-pull on an unborn branch in a different code path such that
> git-pull --rebase is still valid even though there is no HEAD yet.
>
> This code path still ensures that there is no index in order not to lose
> any staged changes. Implement a test to ensure that this check is
> triggered.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>     * Test for error message.
>
>     * Ensure that when git-pull does not modify the index.
>
>     * Moved test_when_finished
>
>  t/t5520-pull.sh | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 05a92a2..9107991 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -415,6 +415,21 @@ test_expect_success 'pull --rebase works on branch yet to be born' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
> +       test_when_finished "rm -rf empty_repo2" &&
> +       git init empty_repo2 &&
> +       (
> +               cd empty_repo2 &&
> +               echo staged-file >staged-file &&
> +               git add staged-file &&
> +               test "$(git ls-files)" = staged-file &&

> +               test_must_fail git pull --rebase .. master 2>../out &&
> +               test "$(git ls-files)" = staged-file &&
> +               test "$(git show :staged-file)" = staged-file
> +       ) &&
> +       test_i18ngrep "unborn branch with changes added to the index" out
> +'
> +
>  test_expect_success 'setup for detecting upstreamed changes' '
>         mkdir src &&
>         (cd src &&
> --
> 2.1.4
>

> On Thu, May 7, 2015 at 1:44 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> +test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
>> +       test_when_finished "rm -rf empty_repo2" &&
>> +       git init empty_repo2 &&
>> +       (
>> +               cd empty_repo2 &&
>> +               echo staged-file >staged-file &&
>> +               git add staged-file &&
>> +               test "$(git ls-files)" = staged-file &&
>
> I think usually people use
>
>     git ls-files >actual
>     echo staged-file >expected && # you have this already in your 2nd
>     # line in the paragraph
>     test_cmp staged-file actual
>
> to make debugging easier as you can inspect the files (actual, expected)
> after the test has failed.
>
> Personally I don't mind the difference as when it comes to debugging
> using the test suite I haven't found the silver bullet yet.

> On Fri, May 8, 2015 at 12:32 AM, Stefan Beller <sbeller@google.com> wrote:
>> On Thu, May 7, 2015 at 1:44 AM, Paul Tan <pyokagan@gmail.com> wrote:
>>> +test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
>>> +       test_when_finished "rm -rf empty_repo2" &&
>>> +       git init empty_repo2 &&
>>> +       (
>>> +               cd empty_repo2 &&
>>> +               echo staged-file >staged-file &&
>>> +               git add staged-file &&
>>> +               test "$(git ls-files)" = staged-file &&
>>
>> I think usually people use
>>
>>     git ls-files >actual
>>     echo staged-file >expected && # you have this already in your 2nd
>>     # line in the paragraph
>>     test_cmp staged-file actual
>>
>> to make debugging easier as you can inspect the files (actual, expected)
>> after the test has failed.
>>
>> Personally I don't mind the difference as when it comes to debugging
>> using the test suite I haven't found the silver bullet yet.
>
> Ehh, but using test_cmp will litter the test with lots of "echo X
>>expected" lines which I find quite distracting.
>
> Just thinking aloud, but it would be great if there was a function to
> compare a string and a file, or a string and a string.
>
> But yeah, I guess if the patches are verified to be correct, then I
> should change these comparisons to use test_cmp.

> Hi Dscho,
> 
> On Fri, May 8, 2015 at 12:26 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> Both this patch and 9/12 change `git pull` invocations to equivalent non-pull ones, but I wonder whether it would not be a better idea to leave them as-are, so that we can make sure that scripts out there that might use similar `git pull` invocations would be unaffected by the rewrite?
> 
> In the current state[1], I'm aiming for the git-pull rewrite patch
> series to break all git-pull tests in the first patch, and then
> subsequently make them pass again in later smaller patches by
> implementing back the old features. This will make reviewing the code
> much easier, as opposed to dumping a huge patch every single re-roll
> ;-).
> 
> For both patches and test suites, if the "setup" tests fail, the whole
> test suite fails. Given that the test suites are about testing the
> diff formatting options and submodules update implementation, which is
> mostly irrelevant to git-pull, I think it would be better if the test
> suite was not affected by the rewrite, especially since it only
> requires changing one line.

> Both this patch and 9/12 change `git pull` invocations to equivalent
> non-pull ones, but I wonder whether it would not be a better idea to
> leave them as-are, so that we can make sure that scripts out there
> that might use similar `git pull` invocations would be unaffected by
> the rewrite?

> At this point, the HEAD of super/submodule/ is a direct descendent of
> submodule/ and thus a fast-forward merge can occur. There is no need to
> rebase.
>
> Call "git pull" instead of "git pull --rebase" in order to reduce
> dependence on git-pull's functionality, and thus prevent the whole test suite
> from failing when git-pull is rewritten to C.

>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>     * This is a new patch in the patch series.
>
>  t/t7406-submodule-update.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index dda3929..e38d830 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -44,7 +44,7 @@ test_expect_success 'setup a submodule tree' '
>  	) &&
>  	(cd super &&
>  	 (cd submodule &&
> -	  git pull --rebase origin
> +	  git pull origin
>  	 ) &&
>  	 git add submodule &&
>  	 git commit -m "submodule update"

> Unless
> the operating assumption is "it is OK to break 'git pull --rebase',
> as long as we do not break 'git pull', while rewriting it", I am not
> sure the value of the change in this patch.  We'd need to keep both
> form working, no?

> Hi,
>
> On Fri, May 8, 2015 at 1:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Paul Tan <pyokagan@gmail.com> writes:
>> If 'git pull' gets broken, it will break this test _anyway_.
>
> You have a point, perhaps this should be changed to git-fetch + git-merge?
>
> Well my reasoning is that it's because git-pull --rebase requires more
> code to be implemented than git-pull. So I'm thinking that git-pull
> --rebase is more likely to break than git-pull.
>
>> Unless
>> the operating assumption is "it is OK to break 'git pull --rebase',
>> as long as we do not break 'git pull', while rewriting it", I am not
>> sure the value of the change in this patch.  We'd need to keep both
>> form working, no?
>
> Yes, ultimately the git-pull rewrite must re-implement everything, but
> if this test suite is affected by any git-pull (--rebase) breakage,
> then there will be lots of patch noise as this test suite's tests get
> disabled/re-enabled in the git-pull rewrite patches.
>
> But if the patch noise is okay, then I'm fine with dropping this patch
> and 07/12.

> I do not think touching this test which does not have anything to do
> with "git pull" in your series is sensible at all, and you shouldn't
> flip test_expect_success temporarily to _expect_failure, if that is
> what you have in mind.
>
> Just don't run unrelated tests while your series is in flux.

> error_on_no_merge_candidates() does not consider the case where "$#"
> includes command-line flags that are passed to git-fetch.
>
> As such, when the current branch has no configured upstream, and there
> are no merge candidates because of that, git-pull --all erroneously reports
> that we are pulling from "--all", as it believes that the first argument
> is the remote name.

>
> Add a failing test that shows this case.
>
> Reported-by: Stephen Robin <stephen.robin@gmail.com>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>     * Added this test to the patch series.
>
>  t/t5520-pull.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index d97a575..b93b735 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -153,6 +153,18 @@ test_expect_success 'fail if no configuration for current branch' '
>  	test `cat file` = file
>  '
>  
> +test_expect_failure 'pull --all: fail if no configuration for current branch' '
> +	git remote add test_remote . &&
> +	test_when_finished "git remote remove test_remote" &&
> +	git checkout -b test copy^ &&
> +	test_when_finished "git checkout -f copy && git branch -D test" &&
> +	test_config branch.test.remote test_remote &&
> +	test `cat file` = file &&
> +	test_must_fail git pull --all 2>out &&
> +	test_i18ngrep "There is no tracking information" out &&
> +	test `cat file` = file
> +'
> +
>  test_expect_success 'fail if upstream branch does not exist' '
>  	git checkout -b test copy^ &&
>  	test_when_finished "git checkout -f copy && git branch -D test" &&

> Paul Tan <pyokagan@gmail.com> writes:
>
>> error_on_no_merge_candidates() does not consider the case where "$#"
>> includes command-line flags that are passed to git-fetch.
>>
>> As such, when the current branch has no configured upstream, and there
>> are no merge candidates because of that, git-pull --all erroneously reports
>> that we are pulling from "--all", as it believes that the first argument
>> is the remote name.
>
> Interesting.
>
> I do not think "pull [origin] --all" makes much sense for the same
> reason why we error out when you say "pull [origin] --tag", so I am
> not sure "There is no tracking information" is the right diag we
> would want to give the user, but I agree that "--all" is not a
> remote name.
>
> Does the same comment as 11/12 applies to this as well?

> Paul Tan <pyokagan@gmail.com> writes:
>
>> .... This can be
>> done with the current git-pull.sh, but would be easier with the
>> superior C parse-options API.
>
> Just this part, but I thought parse-options API is available to
> scripts via "rev-parse --parseopt".

> @@ -32,4 +35,18 @@ test_expect_success pull '
>  )
>  '
>  
> +test_expect_failure '--log=1 limits shortlog length' '
> +(
> +	cd cloned &&
> +	git reset --hard HEAD^ &&
> +	test `cat afile` = original &&
> +	test `cat bfile` = added &&
> +	git pull --log &&
> +	git log -3 &&
> +	git cat-file commit HEAD >result &&
> +	grep Dollar result &&
> +	! grep "second commit" result
> +)

> Hi Paul,
>
> On 2015-05-07 10:44, Paul Tan wrote:
>> @@ -32,4 +35,18 @@ test_expect_success pull '
>>  )
>>  '
>>
>> +test_expect_failure '--log=1 limits shortlog length' '
>> +(
>> +     cd cloned &&
>> +     git reset --hard HEAD^ &&
>> +     test `cat afile` = original &&
>> +     test `cat bfile` = added &&
>> +     git pull --log &&
>> +     git log -3 &&
>> +     git cat-file commit HEAD >result &&
>> +     grep Dollar result &&
>> +     ! grep "second commit" result
>> +)
>
> I think it might be better to use `test_must_fail` here, just for consistency (the `!` operator would also pass if `grep` itself could not be executed correctly, quite academic, I know, given that `grep` is exercised plenty of times by the test suite, but still...)
>
> What do you think?

> Hi Dscho,
>
> On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> Hi Paul,
>>
>> On 2015-05-07 10:44, Paul Tan wrote:
>>> @@ -32,4 +35,18 @@ test_expect_success pull '
>>>   )
>>>   '
>>>
>>> +test_expect_failure '--log=1 limits shortlog length' '
>>> +(
>>> +     cd cloned &&
>>> +     git reset --hard HEAD^ &&
>>> +     test `cat afile` = original &&
>>> +     test `cat bfile` = added &&
>>> +     git pull --log &&
>>> +     git log -3 &&
>>> +     git cat-file commit HEAD >result &&
>>> +     grep Dollar result &&
>>> +     ! grep "second commit" result
>>> +)
>>
>> I think it might be better to use `test_must_fail` here, just for
>> consistency (the `!` operator would also pass if `grep` itself could not
>> be executed correctly, quite academic, I know, given that `grep` is
>> exercised plenty of times by the test suite, but still...)
>>
>> What do you think?
>
> Yep, it's definitely better. Sometimes I forget about the existence of
> some test utility functions :-/.

> Am 07.05.2015 um 19:06 schrieb Paul Tan:
>
>> On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>>
>>> On 2015-05-07 10:44, Paul Tan wrote:
>>>> @@ -32,4 +35,18 @@ test_expect_success pull '
>>>>   )
>>>>   '
>>>>
>>>> +test_expect_failure '--log=1 limits shortlog length' '
>>>> +(
>>>> +     cd cloned &&
>>>> +     git reset --hard HEAD^ &&
>>>> +     test `cat afile` = original &&
>>>> +     test `cat bfile` = added &&
>>>> +     git pull --log &&
>>>> +     git log -3 &&
>>>> +     git cat-file commit HEAD >result &&
>>>> +     grep Dollar result &&
>>>> +     ! grep "second commit" result
>>>> +)
>>>
>>> I think it might be better to use `test_must_fail` here, just for
>>> consistency (the `!` operator would also pass if `grep` itself could not
>>> be executed correctly, quite academic, I know, given that `grep` is
>>> exercised plenty of times by the test suite, but still...)
>>>
>>> What do you think?
>>
>> Yep, it's definitely better. Sometimes I forget about the existence of
>> some test utility functions :-/.
> 
> Nope, it's not better. test_must_fail is explicitly only for git
> invocations. We do not expect 'grep' to segfault or something.
> 
> Cf. eg.
> http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258752

> Hi Hannes,
>
> On 2015-05-07 21:12, Johannes Sixt wrote:
>> Am 07.05.2015 um 19:06 schrieb Paul Tan:
>>
>>> On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
>>> <johannes.schindelin@gmx.de> wrote:
>>>
>>>> On 2015-05-07 10:44, Paul Tan wrote:
>>>>> @@ -32,4 +35,18 @@ test_expect_success pull '
>>>>>   )
>>>>>   '
>>>>>
>>>>> +test_expect_failure '--log=1 limits shortlog length' '
>>>>> +(
>>>>> +     cd cloned &&
>>>>> +     git reset --hard HEAD^ &&
>>>>> +     test `cat afile` = original &&
>>>>> +     test `cat bfile` = added &&
>>>>> +     git pull --log &&
>>>>> +     git log -3 &&
>>>>> +     git cat-file commit HEAD >result &&
>>>>> +     grep Dollar result &&
>>>>> +     ! grep "second commit" result
>>>>> +)
>>>>
>>>> I think it might be better to use `test_must_fail` here, just for
>>>> consistency (the `!` operator would also pass if `grep` itself could not
>>>> be executed correctly, quite academic, I know, given that `grep` is
>>>> exercised plenty of times by the test suite, but still...)
>>>>
>>>> What do you think?
>>>
>>> Yep, it's definitely better. Sometimes I forget about the existence of
>>> some test utility functions :-/.
>>
>> Nope, it's not better. test_must_fail is explicitly only for git
>> invocations. We do not expect 'grep' to segfault or something.
>>
>> Cf. eg.
>> http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258752
>
> That link leads to a patch that changes `! grep` to a `test_must_fail grep` and is not contested, at least not in the thread visible on GMane. Would you have a link with a more convincing argument for me?

> Thank you,
> Johannes
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

> t/README states:
> 
>    On the other hand, don't use test_must_fail for running regular
>    platform commands; just use '! cmd'.  We are not in the business
>    of verifying that the world given to us sanely works.

> On 2015-05-07 21:12, Johannes Sixt wrote:
>> Nope, it's not better. test_must_fail is explicitly only for git
>> invocations. We do not expect 'grep' to segfault or something.
>>
>> Cf. eg.
>> http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258752
>
> That link leads to a patch that changes `! grep` to a `test_must_fail
> grep` and is not contested, at least not in the thread visible on
> GMane. Would you have a link with a more convincing argument for me?

> Am 08.05.2015 um 12:07 schrieb Johannes Schindelin:
>> On 2015-05-07 21:12, Johannes Sixt wrote:
>>> Nope, it's not better. test_must_fail is explicitly only for git
>>> invocations. We do not expect 'grep' to segfault or something.
>>>
>>> Cf. eg.
>>> http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258752
>>
>> That link leads to a patch that changes `! grep` to a `test_must_fail
>> grep` and is not contested, at least not in the thread visible on
>> GMane. Would you have a link with a more convincing argument for me?
>
> Gah! Sorry for sending you in circles. I see that others have brought
> forward sufficient arguments. Just to get my own argument straight,
> here is the message I wanted to direct you to:
>
> http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258792

> While git-pull supports --log and passes the switch to git-merge, it
> does not support --log=<n>, ignoring the value <n>.
>
> This is not only at odds with the documentation of git-pull, it's also a
> undesirable limitation as <n> could simply be passed to git-merge as
> well.

>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>     * Added this test to the patch series
>
>  t/t5524-pull-msg.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
> index 8cccecc..5b7af07 100755
> --- a/t/t5524-pull-msg.sh
> +++ b/t/t5524-pull-msg.sh
> @@ -17,6 +17,9 @@ test_expect_success setup '
>  		git commit -m "add bfile"
>  	) &&
>  	test_tick && test_tick &&
> +	echo "second" >afile &&
> +	git add afile &&
> +	git commit -m "second commit" &&
>  	echo "original $dollar" >afile &&
>  	git add afile &&
>  	git commit -m "do not clobber $dollar signs"
> @@ -32,4 +35,18 @@ test_expect_success pull '
>  )
>  '
>  
> +test_expect_failure '--log=1 limits shortlog length' '
> +(
> +	cd cloned &&
> +	git reset --hard HEAD^ &&
> +	test `cat afile` = original &&
> +	test `cat bfile` = added &&
> +	git pull --log &&
> +	git log -3 &&
> +	git cat-file commit HEAD >result &&
> +	grep Dollar result &&
> +	! grep "second commit" result
> +)
> +'
> +
>  test_done

> When testing a fast-forward merge with git-pull, check to see if the
> reflog action is "pull" with the arguments passed to git-pull.
>
> While we are in the vicinity, remove the empty line as well.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>     * Added this test to the patch series.
>
>  t/t5520-pull.sh | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index b93b735..6045491 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -86,7 +86,6 @@ test_expect_success 'pulling into void must not create an octopus' '
>  '
>
>  test_expect_success 'test . as a remote' '
> -
>         git branch copy master &&
>         git config branch.copy.remote . &&
>         git config branch.copy.merge refs/heads/master &&
> @@ -95,7 +94,11 @@ test_expect_success 'test . as a remote' '
>         git checkout copy &&
>         test `cat file` = file &&
>         git pull &&
> -       test `cat file` = updated
> +       test `cat file` = updated &&

> +       git reflog -1 >reflog.actual &&
> +       sed "s/$_x05[0-9a-f]*/OBJID/g" reflog.actual >reflog.fuzzy &&
> +       echo "OBJID HEAD@{0}: pull: Fast-forward" >reflog.expected &&
> +       test_cmp reflog.expected reflog.fuzzy
>  '
>
>  test_expect_success 'the default remote . should not break explicit pull' '
> @@ -106,7 +109,11 @@ test_expect_success 'the default remote . should not break explicit pull' '
>         git reset --hard HEAD^ &&
>         test `cat file` = file &&
>         git pull . second &&
> -       test `cat file` = modified
> +       test `cat file` = modified &&
> +       git reflog -1 >reflog.actual &&
> +       sed "s/$_x05[0-9a-f]*/OBJID/g" reflog.actual >reflog.fuzzy &&
> +       echo "OBJID HEAD@{0}: pull . second: Fast-forward" >reflog.expected &&
> +       test_cmp reflog.expected reflog.fuzzy
>  '
>
>  test_expect_success 'fail if wildcard spec does not match any refs' '
> --
> 2.1.4
>