From: Peter Kästle Date: Thu, 03 Dec 2020 07:54:47 GMT Subject: Re: BUG in fetching non-checked out submodule Message-ID: <04968f5c-c8bd-c57e-d646-7c9f7691e1a8@nokia.com> In-Reply-To: Hi, On 03.12.20 00:06, Junio C Hamano wrote: > Philippe Blain writes: > >> Thanks for bisecting it. That commit wanted to fix a different bug >> related to nested submodules, and the route taken was simply >> reverting an earlier commit (a62387b (submodule.c: fetch in >> submodules git directory instead of in worktree, 2018-11-28). >> >> As you discovered, it breaks other scenarios. >> >>> >>> $ git version >>> git version 2.29.2.435.g72ffeb997e >>> >>> $ git config --get submodule.recurse >>> true > > I think the current situation is probably worse. > > As a short-term fix, we should revert 1b7ac4e6d4 until we can come > up with a real fix, probably. Junio: This is why I originally intended to commit the test case for the testsuite separated from the revert and wanted to start a discussion about the actual real fix for the issue: https://public-inbox.org/git/1604413399-63090-1-git-send-email-peter.kaestle@nokia.com/ My proposal would be to revert 1b7ac4e6d4 and isolate the test case "test_expect_success 'setup nested submodule fetch test' '" make it "test_expect_failure" and apply it instead, until we come up with a real solution. >> Yeah, I think the test suite could make more efforts >> to run more tests with that setting turned 'on', but >> it would require significants efforts since it changes >> the behaviour of several commands. > > I am not sure if the question is about amount of efforts. > > A configuration variable is there to change the behaviour of > commands, so a test of a command that has been running happily and > producing a set of expected outcome with a configuration unset > should break the expectation when the configuration is set --- > otherwise there is no point in having a configuration variable. > >> Meta question: is there an easy way to run the whole test >> suite with specific config options turned on ? > > Hence, I do not think it even makes sense to have such an "easy > way". If the "fetch" command, for example, is expected to change > behaviour depending on the value of submodule.recurse, a test > written for the case where the variable is not set should produce > different outcome when the variable is set. > > What we need may be a better test coverage. submodule.recurse is a > later addition, and all tests written earlier do test how the commands > behave without the configuration being set. If one wants to change > the behaviour of these commands when the configuration is set, new > tests to specify what the expected behaviour need to be added. Can we start a new test suite for tests on which this variable is set? The case of Ralf can be used as first test case. Ralf: could you please send a command sequence, which recreates the required repository setup from scratch and is able to trigger your observation? Then we can convert this into a test case. Thanks. -- best regards --peter;