From: Ramkumar Ramachandra Date: Wed, 20 Mar 2013 18:38:54 GMT Subject: Re: [PATCH 3/6] t5516 (fetch-push): introduce mk_test_with_name() Message-ID: In-Reply-To: <20130320182830.GJ3655@google.com> Jonathan Nieder wrote: > 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? > 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". > 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.