Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Sep 12, 2019, 11:06 UTC
Message-ID: <20190912110620.GA3729@szeder.dev>
In-Reply-To: <20190912020748.GA76228@syl.lan>

On Wed, Sep 11, 2019 at 10:07:48PM -0400, Taylor Blau wrote:

> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> > index 8210f63d41..7601664b74 100755
> > --- a/t/t5500-fetch-pack.sh
> > +++ b/t/t5500-fetch-pack.sh

> > +#  - we must use protocol v2, because it handles the "have" negotiation before
> > +#    processing the shallow direectives

s/ee/e/

Show 21 quoted lines

> > +#
> > +test_expect_success 'shallow since with commit graph and already-seen commit' '
> > +	test_create_repo shallow-since-graph &&
> > +	(
> 
> I'm not sure if this same-level indentation is common, or you're missing
> an extra tab here. Either way.
> 
> > +	cd shallow-since-graph &&
> > +	test_commit base &&
> > +	test_commit master &&
> > +	git checkout -b other HEAD^ &&
> > +	test_commit other &&
> > +	git commit-graph write --reachable &&
> > +	git config core.commitGraph true &&
> 
> Another nit, but do you have any thoughts about using 'test_config' here
> instead of a pure 'git config'? I don't think that it really would
> matter much (since none of the other tests hopefully have anything to do
> with commit-graph, and doubly so if it is enabled by default, _and_
> since you're using your own repository), but anyway.

We can't simply replace that 'git config' command with 'test_config', because it is implemented using 'test_when_finished', which doesn't work in a subshell [1]. What we could do is:

  test_create_repo shallow-since-graph &&
  test_config -C shallow-since-graph core.commitGraph true &&
  (
     cd shallow-since-graph &&
     ....

Or we could entirely avoid the subshell by passing '-C shallow-since-graph' to every single command... [2]

However, since this repo was specifically created for this test, it doesn't really matter in what state it's left behind, so I don't think it's worth it.

[1] 0968f12a99 (test-lib-functions: detect test_when_finished in
    subshell, 2015-09-05)
[2] Or bite the bullet, and declare that every test case shall start
    in $TRASH_DIRECTORY.