Spell-checking job fails with "couldn't find remote ref refs/heads/11.x" when 11.x branch does not exist in fork

🐛 Spell-checking job fails with "Argument list too long" when too many files are changed Active was technically solved, as we shouldn't get that error anymore.

This one is now different and happens when 11.x is not present in the fork (ie: old forks).

I think there are two possible ways of approaching this:
1. Catch the fail and let the users know what's needed and how to do it, instead of getting a hard git fail.
2. Check if the desired target branch is there, and if it's not, fetch it from the remote to the job. We can do this before the git fetch -v... line. Note that this would not update the fork nor rebase, it would just do it inside the job.

Ah, now I see. I also hit that in 📌 InvocationMocker::withConsecutive() is deprecated in PHPUnit 9.6 and removed from PHPUnit 10 RTBC , MR!3795. I think it's critical, it also affects rebasing of RTBC MRs.

This is tricky to test as you need an issue fork that doesn't contain 11.x; you can't delete branches from issue fork repos. So I found an old issue and started experimenting.

From reading the docs I think $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is the correct variable to use, and shouldn't require fetching; because the MR merges into the target repo for the purposes of the build, then this SHA should be present already in the local repo.

https://git.drupalcode.org/project/drupal/-/jobs/731750 is 11.x but rolled back a few commits (so HEAD is not the same as origin/11.x), and with a change to .gitlab-ci.yml to diff against $CI_MERGE_REQUEST_TARGET_BRANCH_SHA - which should be the tip of the target branch, in this case 11.x. This seemed to work, only one file was present in the diff:

$ git diff $CI_MERGE_REQUEST_TARGET_BRANCH_SHA --name-only
.gitlab-ci.yml
$ git diff $CI_MERGE_REQUEST_TARGET_BRANCH_SHA --name-only | sed "s_^_../_" | yarn --cwd=./core run -s spellcheck:core --no-must-find-files --file-list stdin
1/1 ./.gitlab-ci.yml
 379.06ms
CSpell: Files checked: 1, Issues found: 0 in 0 files

https://git.drupalcode.org/project/drupal/-/jobs/731793 is the same but cleaned up and with a deliberate spelling error:

 00:02
$ git diff $CI_MERGE_REQUEST_TARGET_BRANCH_SHA --name-only | sed "s_^_../_" | yarn --cwd=./core run -s spellcheck:core --no-must-find-files --file-list stdin
1/1 ./.gitlab-ci.yml 286.71ms
2/2 ./core/lib/Drupal.php
 143.87ms X
./core/lib/Drupal.php:9:23 - Unknown word (Durpal)
CSpell: Files checked: 2, Issues found: 1 in 1 files

So this looks to work for the MR case at least; however not sure what we need to do (if anything) about other job types here?

Do we also need the same changes in the test-only.sh script?

Thanks for the changes and the testing!!

Purely based on the code and your testing, it looks good and it it could potentially go into RTBC, but I don't know if somebody else wants to double check.

The reason why we needed to increase the depth value was due to old forks, where there were many commits between the target (11.x) and the branch within the repo. Not sure if this simplified "git diff" command takes care of that.

From: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predef...

The HEAD SHA of the target branch of the merge request. The variable is empty in merge request pipelines. The SHA is present only in merged results pipelines.

Sounds good. Seems like this will work as long as we're using the correct type of merge pipeline. That also explains why we can get rid of the fetch so great news!

As this is affecting multiple other issues let's get this in and either revert or iterate from here. Backported to 10.2.x to keep things in sync there as well.

Committed and pushed cdf03eab78 to 11.x and b320fc757d to 10.2.x. Thanks!

Reopening, because this is only apparently working in MRs created by core committers.

I think this is because the target SHA variable is only available in "merged results pipelines" which only maintainers can use, issue forks work a bit differently. Somehow we have to take both these into account in the CI script.

This is all documented at https://docs.gitlab.com/ee/ci/pipelines/merge_request_pipelines.html#typ...

Merge request pipelines, which run on the changes in the merge request’s source branch, ignoring the target branch. These pipelines display a merge request label in pipeline lists.

This type of pipeline is created by non-committers.

Merged results pipelines, which run on the result of combining the source branch’s changes with the target branch. These pipelines display a merged results label in pipeline lists.

This type of pipeline is created by committers.

But, if possible, we want the target branch to be combined with the source branch in an MR in all cases - it's possible that an MR contains old code and interacts badly with a change made since in the target branch, and we won't know that if the changes are not combined?

Maybe we just need to (automatically) check the "Enable merged results pipelines" setting in issue forks? https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipelines.html

GitLab uses pull mirroring to keep forked repositories up to date: https://gitlab.com/gitlab-community/meta#configure-pull-mirroring

Unsure if we can enable the same thing here, one for @drumm @fjgarlin I guess? This might also become easier once we have main instead of version branches.

Maybe we just need to (automatically) check the "Enable merged results pipelines" setting in issue forks?

I think that's the default for all projects. You can see it in https://git.drupalcode.org/project/PROJECT-NAME/-/settings/merge_requests

That pull mirroring seems to be for community-forks, but I'm not sure we are using those at all.

"as long as we're using the correct type of merge pipeline." welp...

Now enabling "mirroring" by default might be an option: https://about.gitlab.com/blog/2016/12/01/how-to-keep-your-fork-up-to-dat...

https://docs.gitlab.com/ee/user/project/repository/forking_workflow.html

Repository mirroring keeps your fork synced with the original repository. This method updates your fork once per hour, with no manual git pull required.

We might be able to do that at forks creation.
Will need to double check it with @drumm.

I found some code over in GitLab's own repo that appears to do something kinda similar: https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/ci/rails.gitl...

    - if [ -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" ]; then
        echo "HEAD is $(git rev-parse HEAD). \$CI_MERGE_REQUEST_TARGET_BRANCH_SHA is ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA}";
      else
        echo "HEAD is $(git rev-parse HEAD). \$CI_MERGE_REQUEST_DIFF_BASE_SHA is ${CI_MERGE_REQUEST_DIFF_BASE_SHA}";
      fi;
    - UNDERCOVERAGE_COMPARE="${CI_MERGE_REQUEST_TARGET_BRANCH_SHA:-$CI_MERGE_REQUEST_DIFF_BASE_SHA}"
    - git diff ${UNDERCOVERAGE_COMPARE} --stat

In other words if CI_MERGE_REQUEST_TARGET_BRANCH_SHA is set then use that, otherwise use CI_MERGE_REQUEST_DIFF_BASE_SHA. I think as a stopgap we should try that here and see if it works on both project MRs and fork MRs?

https://git.drupalcode.org/project/drupal/-/jobs/739167 created by a committer:

HEAD is 9e9f8c3009e1f7576e5835864cd3097dc1663628. $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is b8a5017e194e854b783534bfe83d65ce8fdc4662

1/1 ./.gitlab-ci.yml 294.23ms
CSpell: Files checked: 1, Issues found: 0 in 0 files

https://git.drupalcode.org/issue/drupal-3418207/-/jobs/739378 created by a non-committer:

HEAD is 7d235d2930783eead171d7f86fa4d272d7ec2b80. $CI_MERGE_REQUEST_DIFF_BASE_SHA is b8a5017e194e854b783534bfe83d65ce8fdc4662

1/1 ./.gitlab-ci.yml
 372.02ms
CSpell: Files checked: 1, Issues found: 0 in 0 files

This is a GREAT find! Even tho the "echo" lines aren't needed, they might be useful for debugging.

Based on the link shown in #20 and the tests done in #23, and the fact that the code looks ok and it runs as it should, I'm marking this issue RTBC. Both MRs have the same code.

Once deployed, we should follow up with the test-only code (https://git.drupalcode.org/project/drupal/-/blob/11.x/.gitlab-ci/scripts...). Or maybe alter the scope of this issue to include that too (in that case it'd need to be moved to "Needs work"). For now, I'll leave this issue as is.

Let's commit this here as spell checking is not currently working at all in non-committer MRs, and then handle test-only in a followup which I will create shortly.

Committed and pushed e1312a3d8b to 11.x and 993fc38bf1 to 10.2.x. Thanks!

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