Re: [PATCH 3/6] t5516 (fetch-push): introduce mk_test_with_name()
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Mar 20, 2013, 18:38 UTC
- Message-ID
- <CALkWK0mCVb6N76QU+U3iTO_gmU4PmhrTOgU53DAMd5x1bCQEtA@mail.gmail.com>
- In-Reply-To
- <20130320182830.GJ3655@google.com>
Jonathan Nieder wrote:
Show 15 quoted lines
> Ramkumar Ramachandra wrote:
>
>> mk_test() creates a repository with the constant name "testrepo", and
>> this may be limiting for tests that need to create more than one
>> repository for testing. To fix this, create a new mk_test_with_name()
>> which accepts the repository name as $1. Reimplement mk_test() as a
>> special case of this function, making sure that no tests need to be
>> rewritten.
>
> Why not give mk_test an optional parameter?
>
> repo_name=${1:-testrepo}
>
> Oh, it is because mk_test already takes arguments naming refs to push.
> I suppose the change description could make this clearer.Isn't it obvious?
Show 8 quoted lines
> Why not use mk_test and then rename, like so? > > mk_test ...refs... && > mv testrepo testrepo-a && > > mk_test ...refs... && > mv testrepo testrepo-b && > ...
No. This is ugly. mk_test() should not hardcode "testrepo".
Show 5 quoted lines
> I dunno. The helper functions at the top of this test are already > intimidating, so I guess I am looking for a way to avoid making that > problem worse. One way would be to add an opening comment before > the function definition explaining how it is meant to be used. See > t/test-lib-functions.sh for examples, such as test_cmp.
My patch does not make the situation worse in any way: it just adds one line that passes $1 as a parameter to existing code. Yes, the functions and tests can be improved greatly, but I refrained from doing so because of your series. We can save it for later.