| Message ID | 20251201163100.143476-1-naftaly.ralamboarivony@smile.fr |
|---|---|
| State | Accepted, archived |
| Commit | 98ecba96fb3ade50a686d390ec6139de421be9a5 |
| Headers | show |
| Series | [v2,1/6] patchtest: fix failure when oe-core repo is in detached HEAD | expand |
On 2025-12-01 11:30, naftaly.ralamboarivony via lists.openembedded.org wrote: > From: Naftaly RALAMBOARIVONY <naftaly.ralamboarivony@smile.fr> > > Patchtest fails when oe-core git repo is in a "detached HEAD" state: > > Error log: > >> File "/usr/lib/python3/dist-packages/git/repo/base.py", line 881, in > active_branch return self.head.reference ^^^^^^^^^^^^^^^^^^^ > >> File "/usr/lib/python3/dist-packages/git/refs/symbolic.py", line 311, in > _get_reference raise TypeError("%s is a detached symbolic reference as it > points to %r" % (self, sha)) TypeError: HEAD is a detached symbolic reference > as it points to '3dd31d3b29730fa1130645d76bb71914ac036335' None > > In this case, no current branch is available for the clean operation. > > To fix this, updates the checkout logic: > - if a current branch is available, use it, > - otherwise, fall back to the commit pointed to by HEAD. > > This ensures that the script works correctly even when HEAD is detached. Hi, This is a really nice improvement to the patchtest code, thank you for submitting it. I have two general comments: 1. There are a few places where the new printouts are confusing, particularly in test_head_detached, e.g.: |Test '%s' failed with git in detach HEAD repository state changed after detached HEAD test I think this could be simpler, like |Test '%s' failed with git in detached HEAD mode: state changed after test 2. The bigger issue, IMO is the way the test output has changed. Here's a result from my local selftest with your patches applied: |(venv) tgamblin@megalith ~/workspace/yocto/openembedded-core (master)$ ./meta/lib/patchtest/selftest/selftest |XFAIL: TestMbox.test_author_valid (file: TestMbox.test_author_valid.1.fail) |XPASS: TestMbox.test_author_valid (file: TestMbox.test_author_valid.1.pass) |... | |XFAIL: TestMbox.test_author_valid (file: TestMbox.test_author_valid.1.fail) |Test 'TestMbox.test_author_valid' passed, with git in detach HEAD repository state remained identical after detached HEAD test. |============================================================================ |Testsuite summary for patchtest |============================================================================ |# TOTAL: 37 |# XPASS: 17 |# XFAIL: 18 |# XSKIP: 2 |# PASS: 0 |# FAIL: 0 |# SKIP: 0 |# ERROR: 0 |============================================================================ My thought is that the selftests shouldn't have the else: part of test_head_detached(), since it's not tracking that pass in the totals. If we're going to include it, then maybe the code could be adjusted so that the output looks like: |(venv) tgamblin@megalith ~/workspace/yocto/openembedded-core (master)$ ./meta/lib/patchtest/selftest/selftest |XFAIL: TestMbox.test_author_valid (file: TestMbox.test_author_valid.1.fail) |XPASS: TestMbox.test_author_valid (file: TestMbox.test_author_valid.1.pass) |... | |XFAIL: TestMbox.test_author_valid (file: TestMbox.test_author_valid.1.fail) |XPASS: selftest.test_head_detached And the xpass part of counts could be incremented. What do you think? > > Signed-off-by: Naftaly RALAMBOARIVONY <naftaly.ralamboarivony@smile.fr> > --- > meta/lib/patchtest/repo.py | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/meta/lib/patchtest/repo.py b/meta/lib/patchtest/repo.py > index 2cdd6736e4..6a7d7d2d3b 100644 > --- a/meta/lib/patchtest/repo.py > +++ b/meta/lib/patchtest/repo.py > @@ -21,7 +21,12 @@ class PatchTestRepo(object): > self.repodir = repodir > self.repo = git.Repo.init(repodir) > self.patch = mbox.PatchSeries(patch) > - self.current_branch = self.repo.active_branch.name > + > + if self.repo.head.is_detached: > + self.current_commit = self.repo.head.commit.hexsha > + self.current_branch = None > + else: > + self.current_branch = self.repo.active_branch.name > > # targeted branch defined on the patch may be invalid, so make sure there > # is a corresponding remote branch > @@ -80,6 +85,6 @@ class PatchTestRepo(object): > self._patchmerged = True > > def clean(self): > - self.repo.git.execute(['git', 'checkout', self.current_branch]) > + self.repo.git.execute(['git', 'checkout', self.current_branch if self.current_branch else self.current_commit]) > self.repo.git.execute(['git', 'branch', '-D', self._workingbranch]) > self._patchmerged = False > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#227129): https://lists.openembedded.org/g/openembedded-core/message/227129 > Mute This Topic: https://lists.openembedded.org/mt/116559758/7611679 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [tgamblin@baylibre.com] > -=-=-=-=-=-=-=-=-=-=-=- >
diff --git a/meta/lib/patchtest/repo.py b/meta/lib/patchtest/repo.py index 2cdd6736e4..6a7d7d2d3b 100644 --- a/meta/lib/patchtest/repo.py +++ b/meta/lib/patchtest/repo.py @@ -21,7 +21,12 @@ class PatchTestRepo(object): self.repodir = repodir self.repo = git.Repo.init(repodir) self.patch = mbox.PatchSeries(patch) - self.current_branch = self.repo.active_branch.name + + if self.repo.head.is_detached: + self.current_commit = self.repo.head.commit.hexsha + self.current_branch = None + else: + self.current_branch = self.repo.active_branch.name # targeted branch defined on the patch may be invalid, so make sure there # is a corresponding remote branch @@ -80,6 +85,6 @@ class PatchTestRepo(object): self._patchmerged = True def clean(self): - self.repo.git.execute(['git', 'checkout', self.current_branch]) + self.repo.git.execute(['git', 'checkout', self.current_branch if self.current_branch else self.current_commit]) self.repo.git.execute(['git', 'branch', '-D', self._workingbranch]) self._patchmerged = False