Use commit SHA on PR builds

[fsouza]

Fix harness/harness#1000.
Fix #41.

This is just #38 and #50 in a single PR.

[bradrydzewski]

@fsouza thanks, would you mind bumping the plugin version number to 1.4?
https://github.com/drone-plugins/drone-git/blob/master/.drone.yml#L28

[fsouza]

@bradrydzewski sure, just did! Let me know if anything doesn't look right :)

Thanks for reviewing!

[bradrydzewski]

thanks, can you remove the 1.3 tags? then we are good to merge :)

-tags: [ latest, 1.4.0, 1.4, 1.3.0, 1.3, 1 ]
+tags: [ latest, 1.4.0, 1.4, 1 ]

[fsouza]

Ohhhh, I see. Sorry, pushing it now.

[tonglil]

@bradrydzewski I don't agree with the change in behavior of this PR.

Effectively this changes all steps run for event: pull_request to the state of a repo being the push commit instead of the merge ref commit.

And it won't solve the issue where the merge ref can't be found on GitHub (like https://beta.drone.io/drone-plugins/drone-git/132):

fatal: Couldn't find remote ref refs/pull/60/merge

git fetch happens before git checkout, with p.Build.Ref still being the same, the string refs/pull/#/merge.

[bradrydzewski]

I didn't realize it wasn't using the merge commit. I will rollback until we have a better solution. I think we need a proper RFC for this because everyone seems to have a slightly different idea for how this plugin should work, and I feel like I'm in a tough spot.

[bradrydzewski]

I added a backoff to the plugin to retry when it cannot fetch the merge ref
dd60dae

@fsouza I do apologize for the confusion I've caused. I think we might need to make a change at the drone level before we can re-merge this PR. I think when using the merge ref, drone needs to capture the merge commit sha, instead of the base commit sha. We'll get this fixed. Sorry, and am appreciate of the PR. We'll get it figured out :)

[geekdave]

@bradrydzewski Is it correct to assume that the backoff strategy may not prevent the "Wrong git hash being built on PR synchronization" issue? Sounds like that issue can occur when github finds the merge ref, but it's stale and refers to the previous hash in the PR.

[bradrydzewski]

@geekdave correct.

The backoff solves a problem where GitHub merge refs may not be available when at runtime, which can be an issue if GitHub or GitHub enterprise is slow to process pull requests (like it is today).

The reason I rolled back this pull request is because it was brought to my attention that this would result in checking out the head sha, and not the merge commit sha. Unfortunately github does not provide the merge commit sha in the hook payload; it was deprecated in 2013 [1]. So basically we can checkout the head ref which does not reflected the merged code, or checkout the merge ref but risk checking out the wrong commit ....

[1] https://developer.github.com/changes/2013-04-25-deprecating-merge-commit-sha/

[tonglil]

Should we reopen #41 then?

[bradrydzewski]

I am not confident #41 can be implemented. GitHub does not send the merge commit sha, which means we would always checkout the unmerged, head commit. I am not sure this approach is any better or worse than our current approach. Both approaches have pros and cons. 😞

For prior art, it seems Travis does not checkout a specific commit sha either https://travis-ci.org/urfave/cli/jobs/295356305#L431