Planned change to git clone logic

With those criteria I couldn’t come up with an alternative. My original proposal creates both a detached head (since we don’t know the source branch) and a merge commit. So I stick with what’s there already. Thanks.

It’s very minimal feedback Brad, but “next” is not picking up the fact we’re pulling from GitLab and changing the ref path. (Drone : 0.8.2). I assume that’s because the $DRONE_COMMIT_REF isn’t being evaluated correctly - but I’m not sure how to make that obvious and get that information for you. Let me know if I can be of any help.

Update: Here’s what I get from my build (and I don’t know why it doesn’t pick it up in “next”)

echo $DRONE_COMMIT_REF
refs/merge-requests/1/head

Update: Seems it’s down to the version of /bin/sh. I’ve submitted a PR with a workaround.

I tried using git:next and it seems that cd to $DRONE_WORKSPACE fails since the directory does not exist.
On the other hand, it seems that code in master branch creates the workspace directory if it does not exist.
It would be helpful if you could add the same procedure (create the workspace directory) to next branch.

Here is a data point - the GitHub refspecs have been particularly flaky today, meaning that Drone was giving very confusing errors for hours. I ended up switching that repo to plugins/git:next, and the problems disappeared immediately. We’re going to merge it into master, as I don’t see any regression or bug in that version :slight_smile:

That’s great to here. I’d like to also confirm that I have no issues since I switched to :next few months ago.

We’ve been using it and it has been great. I would love to see it merged because it is a pain to have to add it. When I forget it we have issues on new projects. :smiley:

The newer version of this plugin is default in 0.9-alpha

@bradrydzewski Thanks for working on improving the cloning process! We are running into issues with drone building non-current versions of PRs on a weekly basis.

Having said that, I would like to suggest a) using git merge 26923... instead of git rebase 26923... since this more closely resembles what happens on Github when you merge a PR. Also, there can be merge conflicts for a rebase that do not exist for a merge.

And b) the whole idea of “we merge ourselves because Github is too slow” is not really implemented currently, since with git fetch origin pull/14596/head: we still depend on the Github pull ref to be up-to-date. And because I see now way of avoiding to use the Github pull ref for inter-repository PRs, I would suggest to simply check that the fetched ref matches the DRONE_COMMIT_SHA and retry if not (yet).

If these ideas are welcome, I would volunteer to work on a PR for drone-git:next for this!?
Or should it be a PR for drone-git:master for only my idea b)?
Were there other reasons for next except outdated Github pull refs? (Yes, I have seen the listed benefits at the top post, but… 1) is true but maybe not so relevant since github itself tells you something about merge failures already, 2) is also in my idea b), 3) is not possible for inter-repo PRs and 4) I do not really understand - since the head sha will be changed either way (true for rebase, reset and merge)

Thanks for the feedback. The window for making changes has expired. The current implementation is being shipped in 0.9 and we have a number of teams using the new version successfully.

I suggest creating a custom git plugin and sharing with the community. This gives the community a chance to test your plugin (without the risk of breaking changes) and then suggest upstream improvements after successful testing.

I also think there might be some flaws in your design / assumption. I started writing a critique but it was taking a while, and I do not have the free time to allocate to this right now – but I do think that creating your own custom plugin and using it for a few weeks, and getting others to use it, would be a helpful exercise and would likely surface some issues and allow you to tweak the design.

2 Likes

The current implementation is being shipped in 0.9

That means we can think about changes for 0.10 or 1.0 :grin:

I started writing a critique but it was taking a while, and I do not have the free time to allocate to this right now

I totally get that! It is a complicated topic and I am sure I am also missing some corner cases! If you want, have a quick look at the following argument to support my suggestion a):


This shows a branch fourth conflicting with a commit (“third branch conflicting with second”) inside the master branch but not with the HEAD of master:

Thus, github says

This branch has no conflicts with the base branch

but the new drone git plugin fails:

Initialized empty Git repository in /drone/src/github.com/ploh/dronetest/.git/
+ git fetch origin +refs/heads/master:
From https://github.com/ploh/dronetest
 * branch            master     -> FETCH_HEAD
 * [new branch]      master     -> origin/master
+ git checkout master
Branch master set up to track remote branch master from origin.
Already on 'master'
+ git fetch origin pull/3/head:
From https://github.com/ploh/dronetest
 * branch            refs/pull/3/head -> FETCH_HEAD
+ git rebase be6ba555aabaaa422cdbe2a62a083b36ae28419e
First, rewinding head to replay your work on top of it...
Applying: third branch conflicting with second
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M   .drone.yml
Falling back to patching base and 3-way merge...
Auto-merging .drone.yml
CONFLICT (content): Merge conflict in .drone.yml
Patch failed at 0001 third branch conflicting with second
The copy of the patch that failed is found in: .git/rebase-apply/patch

On the other hand, I have not heard an argument for using rebase (and even in the “wrong” direction, since we are rebasing master onto PR branch with git:next) instead of merge. I will create a custom plugin with this but in the meantime I just wanted to make clear why this matters :stuck_out_tongue_winking_eye:

On the other hand, I have not heard an argument for using rebase

It is mentioned in my initial post, but rebase preserves the head sha, which is important for coveralls, codecov, etc. Without a rebase it creates a new merge commit, which breaks these tools, and could also cause problems when we eventually try to support the github checks api.

But rebase will also create new commits. It will rebase master on top of the PR branch’s HEAD. Whereas merge will create a merge commit on top of the PR branch’s (as well as master’s) HEADs. Either way we change the commit sha of the pushed PR branch. That is unavoidable imo.

btw: Thanks for taking some time to try to enlighten me - even if unssuccesful until now :+1:

The unit tests demonstrate the git rev-parse HEAD returns the sha that matches the pull request commit sha (as provided by the github API) and git rev-parse --abbrev-ref HEAD returns the branch that matches the pull request head branch. You can see these unit tests here: https://github.com/drone/drone-git/blob/master/posix/posix_test.go#L149

The pull request clone behavior therefore meets the requirements that I presented in my initial post, and ensure compatibility with a number of tools (coveralls, etc) that use git rev-parse.

If you can change the commands without changing the unit tests and without breaking the unit tests I am open to improvements. But any changes that break or change the existing unit tests would not satisfy our current requirements.

The current unit test is working because it only checks correct behaviour for a PR where the src branch is a descendant of the dest branch, i.e. in github terminology the PR is “up to date before merging”.

I have created another test PR, https://github.com/octocat/Spoon-Knife/pull/16667, where this is not the case.

Then I added another unit test to test cloning this PR. This exposes that rebase is not always working even if merge is.

I created https://github.com/drone-plugins/drone-git/pull/76 to be merged into next.

PS: If you do not want my unit test extension (because I was not supposed to change it), you can revert that and keep the merge instead of rebase - it will not break the existing unit tests.

I moved the PR to the new repository: https://github.com/drone/drone-git/pull/2

thanks, I will look into getting this merged

@bradrydzewski

(currently running drone v1.3.1 which is the default version set up on the stable helm chart)

I’ve been using plugins/git:next for a while now but have started receiving an error lately:

+ git rebase 2a5f629ee8d1d34c1aa6c27d693899f2e642bdc3
15 First, rewinding head to replay your work on top of it…
16 fatal: empty ident name (for <>) not allowed

If you are using 1.x you should no longer be using plugins/git:next

The default clone login in 1.x resolves the issues discussed in this thread, and the plugins/git repository has been deprecated.

2 Likes