Merge instead of rebase

[mdevlamynck]

Would it be possible to add an option to use a git merge instead of a git rebase ?

[jcpetruzza]

It should be possible, but I'd need to understand how this would work.

So, assuming you set the "Merge request settings" for your project in gitlab to "Merge commit" (instead of "Fast-forward merge" or "Merge commit with semi-linear history"), what do you expect to happen when a request is assigned to the marge user?

[mdevlamynck]

I may miss some details but isn't the result of a merge fast-forwardable?

Considering we want to merge the MR feature1 into master, if marge does a git checkout feature1 && git merge master then master can be fast-forwarded to the resulting merge commit (unless commits are added to master in the meantime but having the bot check for those things is kind of the point). So I'd still use "Fast-forward merge".

[jcpetruzza]

Just to be sure I understand correctly, what would be your main goal here?

1. Have marge press "merge" once the CI pipeline passes? (e.g., because it takes long)
2. Ensure that the exact merge commit that becomes the new head of master has already passed CI?

[mdevlamynck]

The goal is the 2nd. To answer your first question, I'd expect marge to just use git merge instead of git rebase. Am I missing something? I don't really see the difference it makes.

I'll test the bot against some MR with "rebase" replaced with "merge" in the Repo.rebase() function at

marge-bot/marge/git.py

Line 62 in d5e588c

def rebase(self, branch, new_base, source_repo_url=None):

As I said, merging the MR should be a fast-forward so I don't really see why it wouldn't work.

[jcpetruzza]

Assuming git considers this a fast-forward operation (I didn't try it, but sounds sensible), afaics it should work. A small issue is the following scenario:

1. You create a MR and eventually assign to marge
2. Marge checks out the branch, creates a merge-commit with the current head of master (call it m0) and pushes it back to gitlab
3. CI is flakey and fails; the issue is reassigned to marge. In the meantime, the head of master moves to m1.
4. Marge checks out the branch again and creates a merge commit, this time against m1. But the merge-commit against m0 is still there as well.

This means that by the time the branch is merged to master, you will have a merge-commit for each failed merge attempt, and this is probably not what you want? One could have marge undo the push if the request can't be merged, I guess.

Does this make sense?

[mdevlamynck]

Yes it totally does. I don't mind the failed merge commits and I don't think we can really avoid them.

What if merging m1 introduce conflicts? A developer would have to resolve them but we don't know if he has the last commit on the MR branch or not, so undoing the failed merge commit is a bit dangerous, he may not be able to simply git pull / git push if we rewrite the history.

I'm not used to rebase-based workflow in git so I don't know how to handle these history rewriting cleanly.

[jcpetruzza]

As far as I can see, your proposal looks sensible. I'm more used to a rebase-based workflow so I don't have good intuitions on the ergonomics of this, but from what you say, it should be fine. The part that probably won't work is adding commit trailers like "Tested by:", but it would be a matter of rejecting that combination of flags.

Do you want to give it a go at adding a flag to use "merge" instead of "rebase"?

[mdevlamynck]

Yes I'll try to submit a PR implementing this. I've already spent the day at work running a patched marge-bot using merge instead of rebase in the Repo.rebase() function and so far it worked.

I don't get your point about commit trailers though. Since we create a new commit when doing git merge we can put what we want in the commit message.

Do you have any recommendations regarding the actual implementation? I'd like to avoid too much duplication between the rebase and merge code paths.

[jcpetruzza]

Re the trailers: the idea of the "Tested-by" trailer is that you can know that the particular commit that contains this trailer was run against CI and passed. You could attach the "Tested-by" trailer to the merge-commit you produce, but it would be meaningless since you don't know at that point whether CI will pass. For the rebase-workflow, we add tentatively the "Tested-by" trailer to the head commit and remove it from all other commits in the branch, so effectively, by the time the branch makes it to master, only the commit that passed CI contains the Tested-by. It is probably fine to have a Reviewed-by or Part-of on the merge-commit, I guess.

Re implementation, I'd try to keep it simple, at least initially. In marge.job.MergeJob we should probably rename rebase_and_accept() to something that doesn't say "rebase", and check self._options to decide whether to call push_rebased_and_rewritten_version() or a new function called push_merged_version() or something along that lines. Would that make sense?

[mdevlamynck]

Re the trailers: Oh that makes sense. Since as we previously said we keep the previous potentially failed merge commits, we can't use the same technique you describe (optimistically setting the Tested-by and removing it if it fails). I'll need to think about it some more to see if keeping those failed merge commit makes sense (and if not how to avoid them).

Re implementation: I agree on the direction you're suggesting especially if we end up having really different behavior. The thing is push_rebased_and_rewritten_version() and push_merged_version() will probably end up with a lot of common code and I'd like to avoid duplication there while not tying too much the two strategy.

With that, I'll start hacking on the PR. Thank you for all this help!

[jcpetruzza]

I've merged this change, I think it is a first step. I still suspect for this to be truly useful, we'd need to remove merge commits to recover from attempted merge attempts that failed to pass CI, etc. I will open a separate issue for that and people actually trying this feature can comment on it.