| Message ID | 0bede59a53862585c49bc635f82e44e983144a7f.1710246859.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0eab85b90f55e4bf9dfb97abd8e7d9b0cdac73b9 |
| Headers | show |
| Series | t5601: exercise clones with "includeIf.*.onbranch" | expand |
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? Thanks.
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
Patrick Steinhardt <ps@pks.im> writes: > 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. ;-) > 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. OK. That is a sensible position to take. Thanks.
On Fri, Mar 22, 2024 at 03:01:25AM +0100, Patrick Steinhardt wrote: > On Tue, Mar 12, 2024 at 01:38:26PM -0700, Junio C Hamano wrote: > > Patrick Steinhardt <ps@pks.im> writes: [snip] > 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. This is wrong by the way. I didn't mean to say Git v2.44, but the upcoming Git v2.45 starting with 199f44cb2e (builtin/clone: allow remote helpers to detect repo, 2024-02-27). Patrick
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index fb1b9c686d..ca43185681 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -776,6 +776,18 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd +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 +' + test_expect_success 'partial clone using HTTP' ' partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server" '
It was reported that git-clone(1) started to fail in Git v2.44 when cloning via HTTPS when the config contains an "includeIf.*.onbranch" condition: $ git clone https://example.com/repo.git Cloning into 'repo'... BUG: refs.c:2083: reference backend is unknown error: git-remote-https died of signal 6 This regression was bisected to 0fcc285c5e (refs: refactor logic to look up storage backends, 2023-12-29). This commit tightens the logic to look up ref backends such that we now die when the backend has not yet been detected by reading the gitconfig. Now on its own, this commit wouldn't have caused the failure. But in 18c9cb7524 (builtin/clone: create the refdb with the correct object format, 2023-12-12) we have also changed how git-clone(1) initializes the refdb such that it happens after the remote helper is spawned, which is required so that we can first learn about the object format used by the remote repository before initializing the refdb. Starting with this change, the remote helper will be unable to detect the repository right from the start and thus have an unconfigured ref backend. Consequently, when we try to resolve the "includeIf.*.onbranch" condition, we will now fail to look up the refdb and die. This regression has already been fixed via 199f44cb2e (builtin/clone: allow remote helpers to detect repo, 2024-02-27), where we now pre-initialize a partial refdb so that the remote helper can detect the repository right from the start. But it's clear that we're lacking test coverage of this functionality. Add a test to avoid regressing in the future. Note that this test stops short of defining the desired behaviour for the "onbranch" condition during a clone. It's not quite clear how exactly it should behave, so this is a leftover bit for the future. Reported-by: Angelo Dureghello <angelo@kernel-space.org> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t5601-clone.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)