From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 22 Mar 2024 02:01:25 GMT
Subject: Re: [PATCH] t5601: exercise clones with "includeIf.*.onbranch"
Message-ID: <ZfzmdcBT5MUqoxrk@tanuki>
In-Reply-To: <xmqqo7bjjid9.fsf@gitster.g>

On Tue, Mar 12, 2024 at 01:38:26PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +test_expect_success 'clone with includeIf' '
> > +	test_when_finished "rm -rf repo \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\"" &&
> > +	git clone --bare --no-local src "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> > +
> > +	test_when_finished "rm \"$HOME\"/.gitconfig" &&
> > +	cat >"$HOME"/.gitconfig <<-EOF &&
> > +	[includeIf "onbranch:something"]
> > +		path = /does/not/exist.inc
> > +	EOF
> > +	git clone $HTTPD_URL/smart/repo.git repo
> > +'
> 
> Hmph, isn't the end-user expectation more like if you clone with
> "git clone -b something" then the configuration stored in the named
> file to take effect, while "git clone" that would never place you on
> that something branch would ignore that missing file?  Is this only
> the latter half of the pair?

It probably is, but I'm not really sure to be honest. That's why I
punted on it and just assert that it doesn't die.

In any case I would claim that the current behaviour is really quite
broken in Git v2.43:

```
$ cat >$HOME/.gitconfig <<EOF
[includeIf "onbranch:master"]
    path = $HOME/include
EOF

$ cat >$HOME/include <<EOF
garbage
EOF

# Init source with a different branch name than our condition.
$ git init source --initial-branch=main
$ git -C source commit --allow-empty -m initial

$ git clone source target
Cloning into 'target'...
error: key does not contain a section: garbage
error: key does not contain a section: garbage
error: key does not contain a section: garbage
error: key does not contain a section: garbage
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Total 2 (delta 0), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (2/2), done.
error: key does not contain a section: garbage

$ git clone -b main source target
Cloning into 'target'...
error: key does not contain a section: garbage
error: key does not contain a section: garbage
error: key does not contain a section: garbage
error: key does not contain a section: garbage
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Total 2 (delta 0), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (2/2), done.
error: key does not contain a section: garbage

$ git clone -b other source target
Cloning into 'target'...
error: key does not contain a section: garbage
error: key does not contain a section: garbage
error: key does not contain a section: garbage
error: key does not contain a section: garbage
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Total 2 (delta 0), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (2/2), done.
error: key does not contain a section: garbage

$ cat >$HOME/.gitconfig <<EOF
[includeIf "onbranch:other"]
    path = $HOME/include
EOF
$ git -C source branch other

$ git clone -b other source target
Cloning into 'target'...
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Total 2 (delta 0), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (2/2), done.
error: key does not contain a section: garbage
```

So the behaviour is inconsistent already and does not make a lot of
sense. Part of the problem is likely that we pre-initialize "HEAD" to
"refs/heads/master", which is why you can see some of the includes being
triggered.

With Git v2.44 this behaviour did indeed change, and arguably for the
better. This is because we now pre-init "HEAD" to "refs/heads/.invalid"
instead of using the default branch name. Thus, we do not match "master"
anymore, which is likely the correct thing to do.

```
$ cat >$HOME/.gitconfig <<EOF
[includeIf "onbranch:other"]
    path = $HOME/include
EOF

$ git clone -b main source target
Cloning into 'target'...
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Total 2 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
Receiving objects: 100% (2/2), done.

$ git clone -b other source target
Cloning into 'target'...
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Total 2 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
Receiving objects: 100% (2/2), done.
error: key does not contain a section: garbage
```

So arguably, the ".invalid" hack actually fixed this case somewhat and
made it behave saner. But I'm just not sure whether I feel comfortable
enough with all of this incidental behaviour to actually put it into a
test and cast it into stone.

Thus the current version of my test that simply asserts that this does
something successfully instead of crashing. In my opinion, we need to
put more thought into how this is supposed to work before adding more
tests.

Patrick