Add rebase with merge commit merge style (#3844)

[apricote]

This PR adds the Rebase and Merge (--no-ff) merge style as requested in #3844.

---

Known Bugs:

• As with the Rebase and Merge style, if the rebase can not be done cleanly, the merge will result in a 500 error page Tracked in Error while rebasing PR with conflicts #4078

[codecov-io]

Codecov Report

Merging #4052 into master will decrease coverage by <.01%.
The diff coverage is 24.35%.

@@            Coverage Diff             @@
##           master    #4052      +/-   ##
==========================================
- Coverage   37.56%   37.55%   -0.01%     
==========================================
  Files         321      322       +1     
  Lines       47206    47283      +77     
==========================================
+ Hits        17731    17756      +25     
- Misses      26929    26979      +50     
- Partials     2546     2548       +2

Impacted Files	Coverage Δ
models/migrations/migrations.go	2.61% <ø> (ø)	⬆️
modules/auth/repo_form.go	39% <ø> (ø)	⬆️
routers/repo/setting.go	11.18% <0%> (-0.03%)	⬇️
models/migrations/v76.go	0% <0%> (ø)
routers/repo/issue.go	36.81% <0%> (-0.07%)	⬇️
models/repo.go	42.78% <100%> (ø)	⬆️
models/repo_unit.go	61.4% <100%> (+0.68%)	⬆️
routers/repo/pull.go	33.8% <100%> (+0.21%)	⬆️
models/pull.go	50.65% <48.27%> (-0.08%)	⬇️
models/repo_list.go	63.29% <0%> (-1.27%)	⬇️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58bdff5...7da356b. Read the comment docs.

[apricote]

I have created a seperate issue to track the error during rebase #4078.

This PR is ready to be reviewed now.

[InExtremaRes]

Any change this could be merged into master? This would be very useful to us. Thank you very much for the work!

[apricote]

@InExtremaRes Currently I am waiting for the review process. You too can leave suggestions/improvements to the code. ;)

Besides that you can try to convince some maintainers in the discord channel to review it. As the 1.5.0 Release has been cut off and is mostly done, there might be someone willing to do it.

Edit: @maintainers How should i proceed with conflicting migrations? As there are always some new ones merged in the one created in the PR is always out of date with the numbering. Should I try to keep up or will you just bump the numbers once you are merging?

[InExtremaRes]

@apricote I'd love to review/suggest/improve the code but I don't know Go. What is that discord channel you mentioned?

[techknowlogick]

@apricote you may find a maintainer that will resolve conflicts for you, however my personal approach is to only touch someones PR when they've asked for help resolving a conflict. My preference to start reviews is that all conflicts are resolved (so that I can trigger drone to build against the most recent commit in master).

Like you said, the best way to get someones attention is to bring it up in the #develop channel in discord.

[techknowlogick]

@InExtremaRes https://discord.gg/NsatcWJ is the discord server

[lafriks]

Commit should be removed from all variables/constants

[Claudisimo]

This seems like a nice feature. It would have been nice to use it a few times that i remember. How is this going?

[lunny]

file conflicted.

[InExtremaRes]

@apricote I was testing this branch in our server and I can notice a couple of issues:

1. The message for the merge commit is not customizable.
2. The text head_repo is appended to the branch's name at merging.

Looking at the changes it seem like 2. is deliberated (example). Why is that the default behaviour? I think this does not align very well with the default of git, that always preserve the name of the branch.

[lunny]

Before @InExtremaRes 's question resolved, don't merge.

[apricote]

@InExtremaRes

1. This behaviour was copied from the Rebase style, which did not use/need a commit message. This should be fixed.
2. This behaviour was also copied from the Rebase style.
   • Rebase:

     gitea/models/pull.go

     Line 391 in b387fc1

     "git", "checkout", "-b", "head_repo_"+pr.HeadBranch, "head_repo/"+pr.HeadBranch); err != nil {

   • RebaseMerge:

     gitea/models/pull.go

     Line 416 in b387fc1

     "git", "checkout", "-b", "head_repo_"+pr.HeadBranch, "head_repo/"+pr.HeadBranch); err != nil {

     
     I do not know why the branch is called like this.

[lunny]

maybe @lafriks could know that.

[InExtremaRes]

@apricote That's interesting. Well, when you rebase a branch (and merge --ff as in default rebase style) there is no commit merge that needs a message and then there's no evidence of that branch's existence anymore, in the commit logs at least. With this PR the message of the commit and by default the name of the branch are both kept in the history so that name could be helpful in some cases.

For the latest I just can guest. I think is for safety: if the rebase/pull/merge/something fails the branch can just be discarded without touching the original one and, as I said, in the Rebase and Squash styles the name of that branch is not seen anymore. But just my guess, and as @lunny said maybe @lafriks knows better and can explain.

[InExtremaRes]

There is another, small issue I didn't realize before. The author of the commit is set to the Gitea user instead of the user merging the PR, which is the behavior in the default merge strategy, and presumably the desire one. Is this deliberated for some reason?

[lafriks]

That is ok that message is not customizable and it is correct that head_repo is prepended to branch name for local checkout, that is just not to conflict with exiting branch.

Author for merge message should be user who is doing PR merge. So should probably be merge without commit and then separate commit with author if that is possible with merge style.

[InExtremaRes]

@lafriks

That is ok that message is not customizable and it is correct that head_repo is prepended to branch name for local checkout, that is just not to conflict with exiting branch.

I think I understand the rationales behind, but I would prefer that even if the name of the branch is prepended the commit message would not, since it is clearer in the log what branch was merged. Most of the time the usefulness of this merge style, as opposed to classic rebase style, is that it's straightforward to see what commits are coming from specific branch and are (probably) related or part of the same feature.

@apricote Is it possible to keep the merge commit with the name of the original branch and the author as the user doing the merge? BTW, thanks for this feature, it is working pretty well until now.

[apricote]

Build failure seems to be unrelated:

package github.com/jteeuwen/go-bindata/...: github.com/jteeuwen/go-bindata/...: invalid import path: malformed import path "github.com/jteeuwen/go-bindata/...": double dot
Makefile:95: recipe for target 'generate' failed

---

@InExtremaRes I added the option to set a custom commit title and message. Also the author of the merge commit is now the user doing the merge.

---

The default merge commit message is now the same as with a usual merge:

• Previous default message: Merge branch 'head_repo_feature-1'
• Current default message: Merge branch 'feature-1' of apricote/merge-test into master

[InExtremaRes]

Thank you @apricote. I'll be happy to keep testing and report back later. Since I already executed this when the migration version was 74, is there anything I need to do?

[apricote]

@InExtremaRes If you keep using this branch and eventually switch back to the master, you will skip the Migration v74, this migration adds a column (I think) and skipping it will result in a unexpected/invalid database schema.

The migration introduced in this PR is idempotent and can safely be re-run.

You will want to change the current database version to 73 (using standard sql): UPDATE version SET version = 73 WHERE id = 1;

On the next server start, this will retrigger all migrations > 73 and your database should be in the desired state.