New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rebase with merge commit merge style (#3844) #4052

Merged
merged 16 commits into from Dec 27, 2018

Conversation

8 participants
@apricote
Copy link
Contributor

apricote commented May 26, 2018

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 #4078
add rebase with merge commit merge style (#3844)
Signed-off-by: Julian Tölle <julian.toelle97@gmail.com>
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 26, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

@bkcsoft bkcsoft added the lgtm/need 2 label May 26, 2018

@lunny lunny added the kind/feature label May 27, 2018

@lunny lunny added this to the 1.x.x milestone May 27, 2018

@apricote apricote referenced this pull request May 29, 2018

Open

Error while rebasing PR with conflicts #4078

2 of 7 tasks complete
@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented May 29, 2018

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

This PR is ready to be reviewed now.

@lunny lunny modified the milestones: 1.x.x, 1.6.0 May 30, 2018

@InExtremaRes

This comment has been minimized.

Copy link

InExtremaRes commented Jul 19, 2018

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

@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Jul 19, 2018

@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

This comment has been minimized.

Copy link

InExtremaRes commented Jul 19, 2018

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

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Jul 19, 2018

@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

This comment has been minimized.

Copy link
Member

techknowlogick commented Jul 19, 2018

@lafriks
Copy link
Member

lafriks left a comment

Commit should be removed from all variables/constants

Show resolved Hide resolved models/migrations/v67.go Outdated
Show resolved Hide resolved models/pull.go Outdated
Show resolved Hide resolved models/migrations/v67.go Outdated

@techknowlogick techknowlogick modified the milestones: 1.6.0, 1.7.0 Sep 3, 2018

@Claudisimo

This comment has been minimized.

Copy link

Claudisimo commented Dec 7, 2018

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

This comment has been minimized.

Copy link
Member

lunny commented Dec 8, 2018

file conflicted.

Show resolved Hide resolved integrations/pull_merge_test.go Outdated

@bkcsoft bkcsoft added lgtm/need 1 and removed lgtm/need 2 labels Dec 10, 2018

@InExtremaRes

This comment has been minimized.

Copy link

InExtremaRes commented Dec 10, 2018

@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

lunny approved these changes Dec 11, 2018

@bkcsoft bkcsoft added lgtm/done and removed lgtm/need 1 labels Dec 11, 2018

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Dec 11, 2018

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

@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Dec 11, 2018

@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:
      "git", "checkout", "-b", "head_repo_"+pr.HeadBranch, "head_repo/"+pr.HeadBranch); err != nil {
    • RebaseMerge:
      "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

This comment has been minimized.

Copy link
Member

lunny commented Dec 11, 2018

maybe @lafriks could know that.

@InExtremaRes

This comment has been minimized.

Copy link

InExtremaRes commented Dec 11, 2018

@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

This comment has been minimized.

Copy link

InExtremaRes commented Dec 12, 2018

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

This comment has been minimized.

Copy link
Member

lafriks commented Dec 12, 2018

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

This comment has been minimized.

Copy link

InExtremaRes commented Dec 12, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

apricote commented Dec 15, 2018

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

This comment has been minimized.

Copy link

InExtremaRes commented Dec 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

apricote commented Dec 17, 2018

@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.

apricote and others added some commits Dec 17, 2018

@lunny lunny removed the status/blocked label Dec 18, 2018

lafriks added some commits Dec 27, 2018

@lafriks lafriks added the changelog label Dec 27, 2018

@lafriks lafriks merged commit 4a685f8 into go-gitea:master Dec 27, 2018

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment