Something further down the line makes this work properly on Drupal.org, but with the current implementation in versioncontrol_gitlab, the parts of the label name after the initial / missing is causing issues.

CommentFileSizeAuthor
#6 3008378-6-test-should-pass.patch43.99 KBmarvil07
#6 3008378-6-test-should-fail.patch42.9 KBmarvil07
#5 3008378-tests.patch39.8 KBmarvil07
#2 3008378.patch739 bytesdrumm

Comments

drumm created an issue. See original summary.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Active » Needs review
StatusFileSize
new 3008378.patch739 bytes

This patch limits explode() to 3 items, so $ref contains the remainder of the ref path.

drumm’s picture

drumm
he/him
Location NY, US
commented

This tested well on Drupal.org staging. New tags and branches containing / are still correctly parsed and put into the versioncontrol_labels table.

drumm’s picture

drumm
he/him
Location NY, US
commented
Parent issue: » #3002827: [GitLab] Scheduling Phase 1 of our migration to GitLab
marvil07’s picture

marvil07 commented
StatusFileSize
new 3008378-tests.patch39.8 KB

I see why this could be a problem.

I have been trying to reproduce the problem locally using the test repository.
I am attaching a patch with a couple of changes: (i) make git base test class check also for branch/tag names, not only the number of them; and (ii) renaming one of the branches to include a slash, from feature to feature/one-topic.

I could not reproduce the problem locally even in the case that (ii) is included.
Let us see what test bot thinks.

@drumm, I will probably add the change since it looks good; but I am wondering if you happen to have some extra information I can use for debug/add-to-the-test, e.g. the branch name which you are getting the problem with.

marvil07’s picture

marvil07 commented
StatusFileSize
new 3008378-6-test-should-fail.patch42.9 KB
new 3008378-6-test-should-pass.patch43.99 KB

I was testing the wrong place, on normal sync.
This error seems to be happening only on code arrival and not on full sync.

I have added a related test, and I could reproduce the problem.
Adding a couple of patches.
I will add the code if it goes as expected.

The last submitted patch, 6: 3008378-6-test-should-fail.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

  • marvil07 committed 0748719 on 7.x-1.x 
    Issue #3008378: Add a test checking for a pushed branch with a slash on...
  • marvil07 committed 371b326 on 7.x-1.x
    Issue #3008378: Check for both branch and tag names during test...
  • marvil07 committed 8a717cb on 7.x-1.x authored by drumm
    Issue #3008378 by drumm: Parse tag and branch names which contain...
  • marvil07 committed e22e268 on 7.x-1.x
    Issue #3008378: Re-order add-branch push test to check first number of...
  • marvil07 committed fc01504 on 7.x-1.x
    Issue #3008378: Rename one test repository branch to include a slash.
marvil07’s picture

marvil07 commented
Status: Needs review » Fixed

testbot results went as expected, I have added the code to mainline, hashes changed b/c I rebased the branch.

@drumm, thanks for the report, and the fix!

drumm’s picture

drumm
he/him
Location NY, US
commented

Thanks! I never saw this problem manifest when running on Drupal.org’s production stack. Whatever path a label takes to getting in the DB managed to avoid this. It surfaced when implementing https://www.drupal.org/project/versioncontrol_gitlab, which subclasses a lot of this module.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.