Re: [PATCH 12/16] test-lib: modernize test_create_repo() function
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Apr 16, 2021, 21:22 UTC
- Message-ID
- <20210416212251.GQ2947267@szeder.dev>
- In-Reply-To
- <20210415211013.GM2947267@szeder.dev>
On Thu, Apr 15, 2021 at 11:10:13PM +0200, SZEDER Gábor wrote:
Show 33 quoted lines
> > 3. We won't ever hit that "Cannot setup test environment" > > error. > > ENOSPC? Some rogue background process on Windows still desperately > clinging to an open file descriptor to some file in the same > directory, preventing 'rm -rf "$TRASH_DIRECTORY"' near the beginning > of 'test-lib.sh' and interfering with 'git init'? > > > Checking the test environment sanity when doing "git init" dates > > back to eea420693be (t0000: catch trivial pilot errors., > > 2005-12-10) and 2ccd2027b01 (trivial: check, if t/trash directory > > was successfully created, 2006-01-05). > > > > We can also see it in another form a bit later in my own > > 0d314ce834d (test-lib: use subshell instead of cd $new && .. && cd > > $old, 2010-08-30). > > > > But since 2006f0adaee (t/test-lib: make sure Git has already been > > built, 2012-09-17) we already check if we have a built git > > earlier. > > > > The one thing this was testing after that 2012 change was that > > we'd just built "git", but not "git-init", but since > > 3af4c7156c4 (tests: respect GIT_TEST_INSTALLED when initializing > > repositories, 2018-11-12) we invoke "git", not "git-init". > > > > So all of that's been checked already, and we don't need to > > re-check it here. > > I agree that if we already have a 'git' binary that can run 'git > version', then we can safely assume that it will be able to run 'git > init' as well. It might be that 'git init' is buggy and segfaults, > but that is not a "have you built things yet?" kind of error.
Show 23 quoted lines
> > - mkdir -p "$repo"
> > - (
> > - cd "$repo" || error "Cannot setup test environment"
> > - "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" \
> > - init \
> > - "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> > - error "cannot run git init -- have you built things yet?"
> > - mv .git/hooks .git/hooks-disabled
> > - ) || exit
>
> This patch removes this '|| exit', which is...
>
> - good: if 'test_create_repo' is invoked in a test case (and not in
> a subshell) and if it were to fail for some reason, then it won't
> abort the whole test script, but will fail only that test case.
>
> - bad: 'test_create_repo' is responsible for creating the repository
> in the trash directory as well; if that were to fail for any
> reason, then the test script will not be aborted early.
>
> I think the 'exit' on error should be removed from 'test_create_repo',
> but the callsite in 'test-lib.sh' should become 'test_create_repo ||
> exit 1'.Case in point: the bug I just reported in
https://public-inbox.org/git/20210416211451.GP2947267@szeder.dev/
does break Git in a way that in one of our CI jobs 'git init' is unable to create the repository in the trash directory.