[JENKINS-26587]

[toyPoodle]

https://issues.jenkins-ci.org/browse/JENKINS-26587

• extended CommitHookCause to contain repo URL to be able to check it on checkout in GitSCM
• added test for the bug and CommitHookCause
• extended TestGitRepo to return commit SHA1 for the committed file

[jenkinsadmin]

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

[KostyaSha]

Wrote short description in first commit description line

[KostyaSha]

👎 injected RPA in ghprb-plugin is a ghprb-plugin regression and this not a git-plugin issue.

[toyPoodle]

Sorry for delay in my answer, KostyaSha. Multiple scms plugin fails on the line https://github.com/jenkinsci/multiple-scms-plugin/blob/master/src/main/java/org/jenkinsci/plugins/multiplescms/MultiSCM.java#L118. I also created a test to show it in this PR. Multiple scms plugin looks SCM agnostic at line 118 and I am not sure if introducing a dependency to git specific CommitHookCause would be a good idea. Additionally I found no way of removing RPA from build actions if I don't want to propagate it to the underlying GitSCM. Could you point me in the right direction?

[KostyaSha]

RPA is a design and it internal object, don't inject it to build, btw ghprb-plugin finally removed it one release before.

[toyPoodle]

rebased the branch on top of jenkinsci master (2be13e1)

[toyPoodle]

I think that there is a misunderstanding. I have found a problem when using Multiple SCMs Plugin. When searching for solutions, I found that users of ghprb-plugin had same issue. My changes are inspired only by Multiple SCMs Plugin.
When debugging, I found out that GitSCM fails to check out the code in one of the repositories because the call to GitSCM#checkout() fails when a commit doesn't exist in the corresponding repository.
At the moment of a checkout Multiple SCMs Plugin has no chance to know what repository triggered the commit, because this information doesn't leave GitStatus#onNotifyCommit().
The approach was to extend CommitHookCause to contain the information, so GitSCM doesn't try to check out a commit on a repository the notification doesn't come from.

[KostyaSha]

Do you mean that all SCMs are trying to checkout the same revision? Was RPA injected by CommitHook?

[KostyaSha]

How i can reproduce this error?

[toyPoodle]

RPA is injected at https://github.com/toyPoodle/git-plugin/blob/master/src/main/java/hudson/plugins/git/GitStatus.java#L255. Multiple SCMs Plugin loops over all SCMs at https://github.com/jenkinsci/multiple-scms-plugin/blob/master/src/main/java/org/jenkinsci/plugins/multiplescms/MultiSCM.java#L116 and tries to check out few lines later. Since GitSCM checks for RPA at https://github.com/toyPoodle/git-plugin/blob/master/src/main/java/hudson/plugins/git/GitSCM.java#L900 and I found no solution to suppress this, rev-parse fails on unknown commit and fails the build.

You can reproduce the issue if you checkout the branch JENKINS-26587 in this repo, comment out code added by me at https://github.com/toyPoodle/git-plugin/blob/JENKINS-26587/src/main/java/hudson/plugins/git/GitSCM.java#L904 906, 907, 908 and run GitStatusMultipleSCMTest

[toyPoodle]

Yes, all SCMs try to check out same revision

[toyPoodle]

• updated with new commits from jenkinsci master
• fixed a case where commit notification log message may contain "null"
• updated dependency to multiple-scms-plugin

[toyPoodle]

@KostyaSha : do you have time / are you interested in looking further at this PR? Or should I ask other devs to look at it?

[KostyaSha]

Ghprb expects hardcoded refspec, so such GItSCM configuration can be used only for building PRs. Injected RPA was already removed from ghprb. Not sure why GIT scm should be patched because of multiple SCMs.

[KostyaSha]

Why it has "Cause" class if you are getting GitStatus.CommitHookCause.class ?

[toyPoodle]

Agree, makes no sense

[KostyaSha]

Current logic is obvious: if RevisionParameterAction exists, then use it as candidate for build. Adding hacks with Cause looks a bad idea for me, because there are other plugins that may use Causes and it may affect somehow.

1. Assuming that ghprb finally removed enforced rpa, your issue must disappear - have you tried to reproduce with latest versions?
2. From design view point of git plugin i think it makes more sense to add additional information in RPA if it needed. RPA already can contain:

• hash
• Revision object, that i see may contain branch names

It maybe possible to add a repository field with additional information and then use it as clarification in some sensitive parts. Also such additional clarification maybe optionally in this sensitive parts. I also see periodic requests to support triggering where initial url doesn't match configured because of infrastructure nuances. So such changes should be careful.

[toyPoodle]

As I already said above, my only issue is multiple-scms-plugin and not ghprb. If I knew that it would cause so many misunderstandings, I would've never mentioned it. Just forget I ever said it.

I will look at the approach with RPA you mentioned.

[toyPoodle]

• remote URL is now part of RPA
• rebased on top of master

There are following cases now:

1. not a commit notification -> no remote URL is supplied -> RPA#canOriginateFrom() returns true -> RPA resolves like before
2. commit notification for normal git configuration -> remote URL is supplied -> RPA#canOriginateFrom() returns true since the remote from repo matches -> RPA resolves like before
3. commit notification with multiple-scms-plugin with multiple repos -> remote URL is supplied -> RPA#canOriginateFrom() returns true only for repo which triggered the notification -> RPA resolution only triggers for correct repo, otherwise its ignored
4. bogus commit notification for correct repo -> RPA#canOriginateFrom() returns true -> RPA resolves like before (and breaks the build, because commit cannot be resolved)

There are two points where I am unsure:

• I have increased serialVersionUID in RPA. I don't think that the change would break serialization, I cannot verify that the change is backward compatible.
• RevisionParameterAction#shouldSchedule() wasn't changed by me, since the risk of breaking other things was too high. In a very theoretical case it can happen that two different remotes have a different commit with same sha1 and two builds are triggered for this two equal sha1. Then the second build wouldn't be scheduled, because remotes are not considered and sha1 are same.

[markya0616]

Is this fix merged into master release?
We also encounter this issue, and hope to find a solution.
If yes, what is the git plugin version?
Many thanks!

[toyPoodle]

@markya0616, this pull request is still in review. Git plugin version is 2.3.6-SNAPSHOT at the moment. I am periodically rebasing it on top of latest master commit from jenkinsci repo.

[toyPoodle]

@KostyaSha the pull request is ready to review again

[jmdcal]

I am having this problem mentioned as well. It seems to occur on polling but not on manual builds.

[muffl0n]

Would also love to see this fix included in a next release.

[sgargan]

I've checked out this PR and verified that it works well with multiple scms. It's reporting merge conflicts, but there were none when fetched. Thanks for the fix.

[toyPoodle]

I will try to rebase the branch this week to get rid of conflict warning. Hope the guys merge it then or give me feedback on how to improve it.

[josh-burton]

Any progress on this issue?

[toyPoodle]

@athornz: updated.

[stanchan]

Any progress on this merge?

[MarkEWaite]

I hope to evaluate this proposal in the next week.

[toyPoodle]

Will recheck commit messages. Dropping latest commit is not a problem.

[toyPoodle]

Changed commit messages to have an empty line after summary, also added a short description after issue key in every commit. Most entries are written now in imperative.
Empty commit is now gone.
Changed indentation.

[toyPoodle]

@MarkEWaite: your comment seems to be gone after rebase. Yes the change is to reduce the scope of envVars. It was used only in constructor.

[MarkEWaite]

I would rather not have the CommitCauseHook changes inserted in one commit, then removed in a subsequent commit.

Would you be willing to consider my changes which switched it from 5 commits to a single commit?

[toyPoodle]

Sure. What is the procedure? Should I take over the commit you created or would you create a pull request for your changes?

[MarkEWaite]

I'll handle the merge machinery after I've finished some further tests. I'll probably create a pull request, merge it, and close this pull request.

[MarkEWaite]

Rebased and merged with 4979ace