On Sun, Jan 8, 2017 at 6:33 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Stefan Beller <sbeller@xxxxxxxxxx> writes: > >> This provides an easier way to have submodules in tests, by just setting >> TEST_CREATE_SUBMODULE to a non empty string, similar to >> TEST_NO_CREATE_REPO. > > Yuck. > > I find it doubtful that it is a good idea to create two submodule > repositories by merely dot-including the test-lib.sh; I find it > doubly doubtful that it is a good idea to make test_create_repo > pay attention to the special variable to implement that. > > I am OK with a solution where callers that set TEST_CREATE_SUBMODULE > variable in this patch to instead have an explicit call > > test_create_repo --submodule pretzel > > That would be a lot more obvious. agreed. > > The primary reason why I hate the implementation in this patch is > that it is very easy for a test that says TEST_CREATE_SUBMODULE > upfront, only to get the initial test repository (which everybody > else gets) with two test submodules, to later gain a test that wants > to use a separate repository and call "test_create_repo". It will > always get the pretzel submodules, which may or may not match what > the test writer who adds a new test needs. I agree. At the time of writing the patch series, I was anticipating writing way more submodule tests, but now I have these future tests integrated into lib-submodule-update.sh. > >> Make use of it in those tests that add a submodule from ./. except for >> the occurrence in create_lib_submodule_repo as there it seems we craft >> a repository deliberately for both inside as well as outside use. > > But isn't the point of this change that use of ./. cannot be > mimicking any real-world use, hence pointless for the purpose of > really testing the components of the system? If "we craft > deliberately for both inside and outside use" indeed _IS_ a good > thing, then perhaps use of ./. has practical real-world use---if > not, wouldn't we want to fix the scripts that include the > lib-submodule-repo helper not to test such an unrealistic layout? > Makes sense; I tried to fix it up to avoid the ./. clone in create_lib_submodule_repo, but the issue is there are too many implicit assumption of these two repos, such that a faithful conversion would just duplicate code for the submodule, (e.g. create the same amount of commits, containing the same diffs, etc.), which then can be argued to just slow down the test suite as the clone from ./. is actually reducing the needed work by a factor of 2.