Re: [PATCH 3/6] t5516 (fetch-push): introduce mk_test_with_name()

Jonathan Nieder wrote:

Ramkumar Ramachandra wrote:

quoted

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.