Add option to merge two branches (and choosing whether one of the branches disappears or not)

[jbsarrodie]

The story for the merge could be, from the branch view, an expandable action “Merge branch into -> [master, branch_1, branch_2]” (in more architecture terms “Merge architecture work set into…”). The option cannot be available for master and since the action will trigger checkouts, checks for need of commits and pushes must be implemented. A more restrictive first implementation is just allowing merges to master.

@jbsarrodie Can we plan the merge workflow? Should we allow merging only into master? Can we be clear about what the exact process is? For example:

• Does the user need to be "in" the target branch before merging another branch into it?
• Can we merge from any branch to any other branch?
• What do we need to check first before the merge? Outstanding commits? Do a Pull first? More?
• When a merge is attempted there may be conflicts. Do we treat this as we do now?
• Anything else?

Originally posted by @Phillipus in https://github.com/archimatetool/archi-modelrepository-plugin/issues/67#issuecomment-435660072

[jbsarrodie]

@jbsarrodie Can we plan the merge workflow? Should we allow merging only into master? Can we be clear about what the exact process is?

Before answering, I'm copying some interesting part of our global discussion about MVP...

[jbsarrodie]

Have you any thoughts of the user interaction for a merge function (to master)?

Well, I thought about it a bit...

• I wonder if we should allow merge locally? This could be useful in some cases, but there is a high risk that someone add other commits on remote branches, leading to unwanted/unexpected situation. Maybe we should (at least for some time) only allow merge when remote is accessible.
• I can see another potential issue: when doing a merge, we in fact have multiple merges to manage. the first ones are related to the publication of new local commits to the remote tracked branch (as we don't want to merge two branches without including all potential new commits on remote). The last one is the merge of one branch into another. I could imagine the following steps:
  • User wants to merge its current branch into another branch and runs an action (through menu or toolbar)
  • We check if remote is available. If not, an error dialog is shown to explain that merge is only possible when "connected". If yes, a dialog is opened for the user to select to branch into which we'll merge the current branch.
  • If needed user is prompted to save its model
  • if needed user is prompted to commit its changes
  • We publish local changes to the remote (tracked) branch. This can raise the conflict dialog if needed.
  • We switch to the target branch (the one into which we'll merge)
  • As we might have some unpublished commits or there might exist some new commits on remote, we start a "publish" action. This can raise the conflict dialog if needed.
  • We can now run the merge itself. This can raise the conflict dialog if needed.
  • The merge been (hopefully) successful, we can now remove the branch locally and remotely

Originally posted by @jbsarrodie in https://github.com/archimatetool/archi-modelrepository-plugin/issues/67#issuecomment-431142184

[jbsarrodie]

I’ve thought about merging a little bit too. There are some decisions that must be made.

At first there has to be a choice made regarding branching strategy (i.e. the Git workflow). There are several pre-defined workflows for different purposes. A possible, quite simple, workflow is feature branches where branches are branched off from master.

           +--9--10--12--13   Architecture Asset 2
          /         /      \
1--2--5--8--------11--------14   Accepted Architecture Landscape
    \     \       /
     +--3--4--6--7   Architecture Asset 1

For merges there are other choices. The first choice that has to be made is whether commits should come along into the master branch or a single merge commit should represent the sum of the work for a certain feature. These two different approaches are made in separate ways in git and I’m not sure of how the merge view will support the rebase function that adds all the commits made in one branch to another. I say, go for the one merge commit approach, might be most straight forward.

There are also different schools about in which order the various operations are to be performed - just merge into master or first merge master branch changes into feature branch before merge into master.

From an Archi user perspective both directions of merging probably have to be available. An architect that is working on an architecture asset during a long time has to be up to date with the accepted architecture landscape from start to finish, so Git functionality like ‘update current branch from master’ must be available. From the other perspective of an architecture workflow, there has to be a function for accepting a new architecture asset to the accepted architecture landscape, in Git terms, merge branch into master.

The sum of the above is two new actions (aside the existing Refresh):

• Update branch from master
• Merge branch into master

Maybe we should (at least for some time) only allow merge when remote is accessible.

Yes, a common flow of activities of merging includes pulls of the included branches before merging as

git checkout source_branch
git pull
git checkout target_branch
git pull

Not including the pull in a merge-flow will probably trigger a new merge when pushing to a remote.

Originally posted by @mjardemalm in https://github.com/archimatetool/archi-modelrepository-plugin/issues/67#issuecomment-431323272

[jbsarrodie]

A possible, quite simple, workflow is feature branches where branches are branched off from master.

I agree that in practice this might be the simplest workflow for users, but I don't think we should put some constraint in the plugin's code, unless this really simplify things at this level too.

For merges there are other choices. The first choice that has to be made is whether commits should come along into the master branch or a single merge commit should represent the sum of the work for a certain feature.

My view on this is to use the same strategy that the one currently in place when merging local work into remote master: don't force a rebase nor a squash, use a simple (potentially fast-forward) merge.

There are also different schools about in which order the various operations are to be performed - just merge into master or first merge master branch changes into feature branch before merge into master.

Good remark. The sole purpose of this being to avoid solving conflicts during a merge that involves two different branches (and not the same local/remote branch), we could add an option to enforce this (ie. automatically abort a branch merge if there are conflicts).

From an Archi user perspective both directions of merging probably have to be available

I agree for the same reasons: Architecture work can be long :-)

The sum of the above is two new actions

Well, we could also decide to simply add options to current "Refresh" and "Publish" (so we could refresh from, and publish to, another branch.

Originally posted by @jbsarrodie in https://github.com/archimatetool/archi-modelrepository-plugin/issues/67#issuecomment-431746440

[Phillipus]

Lots of information to digest. I'm going to need some help to break this down into a sequence of events. :-)

Perhaps it would be easier (for me at least) if we could create one (test) Action and then use that as a starting point and adjust it as needed?

Scenario:

User is either in "master" branch or some other branch. User wants to merge a branch into the current branch.

Workflow:

1. Ask user to save if needed.
2. Export to Grafico.
3. Ask user to commit if needed.
4. Pull from remote?
5. ...?

Remember that we cannot assume that the user is online or not online, only whether an action such as pull or push is successful or not.

[jbsarrodie]

Lots of information to digest. I'm going to need some help to break this down into a sequence of events. :-)

I'll make it digest later today ;-)

[jbsarrodie]

So let's try to clarify....

Scenario: User is either in "master" branch or some other branch. User wants to merge a branch into the current branch (the merge commit will be created in the current branch). User will stay in the current branch after the merge. The other branch is not impacted (ie. not deleted).

I wonder if we should allow merge locally? This could be useful in some cases, but there is a high risk that someone add other commits on remote branches, leading to unwanted/unexpected situation. Maybe we should (at least for some time) only allow merge when remote is accessible.

Having thought about it, I finally think we should start (small) with offline merge and later manage online merge.

Workflow for "offline" merge:

1. User wants to merge a branch (OtherBranch) into the current branch (CurrentBranch)
2. If needed user is prompted to save its model
3. Model is exported to Grafico
4. If needed user is prompted to commit its changes
5. We can now run the merge itself.
   1. We run the equivalent of a git merge OtherBranch
   2. If conflicts exist user solves them through the conflict dialog

FWIW, here is Workflow for "online" merge:

1. User wants to merge a branch (OtherBranch) into the current branch (CurrentBranch)
2. We run a Publish action:
   1. If needed user is prompted to save its model
   2. Model is exported to Grafico
   3. If needed user is prompted to commit its changes
   4. Publish is done (this can raise the conflict dialog if needed)
3. We switch to the other branch
4. We run a Publish action
5. We switch back to the "current" branch
6. We can now run the merge itself.
   1. We run the equivalent of a git merge OtherBranch
   2. If conflicts exist user solves them through the conflict dialog

[Phillipus]

I've just committed a basic merge Action which is the "Workflow for "offline" merge" above. We need to test this now to see if this is correct or for side-effects.

[jbsarrodie]

I've just committed a basic merge Action which is the "Workflow for "offline" merge" above. We need to test this now to see if this is correct or for side-effects.

I've just tested. This seems mostly correct but I noticed that in the conflict UI, "Theirs" shows bad information (it seems that "theirs" model is not extracted from the right commit). You can easily reproduce this by creating a branch and then doing some conflicting edits on both branches and then trying to merge them.

I also noticed some weird behavior: after having created a branch I did some changes and commited them. When switching to another branch, I'm asked to commit while I didn't do any new changes. Looking to the resulting commits in SmartGit shows that the first commit doesn't contain my changes, only the second does. I don't know how to reproduce it, I might check previous actions (involving several merges tests).

[Phillipus]

I've just tested. This seems mostly correct but I noticed that in the conflict UI, "Theirs" shows bad information (it seems that "theirs" model is not extracted from the right commit).

I've just committed a change would should hopefully fix this. The MergeConflictHandler was hard coded to use a remote Ref for "theirs" so this now uses the Ref for the merged branch.

I also noticed some weird behavior...

That's strange. I was going to suggest possible EOL problems, but seems different.

[jbsarrodie]

That's strange. I was going to suggest possible EOL problems, but seems different.

Yes, that's really different.

It is like if some changes made during a merge to solve some conflicts had not been commited as part of the merge commit. So triggering the Commit action didn't asked me to save the model, but instead went directly to the commit of the pre-recorded changes (it seems like if model was not exported to grafico at this time but instead previous export had been used). And then, when switching to another branch, this time I was asked to save my model, which certainly triggered a grafico export and then the "right" commit content.

As said, this happened after a merge in which I had some conflicts, but as the conflict solver had some issues with identifying the right "Theirs" commit, it might be the root cause. I'll test again tonight to see if I can reproduce the issue or not.

[Phillipus]

We need to review the sequence of events in MergeBranchAction to check whether we've made the right assumptions about what needs to happen.

[mjardemalm]

We need to review the sequence of events in MergeBranchAction to check whether we've made the right assumptions about what needs to happen.

What does a git.merge() defaults to? I see that both setFastForward and setSquash is commented out, but it looks like JGit defaults to a fast forward when there are no conflicts and maybe a rebase when there are conflicting changes. Is this right? (Personally, I prefer a squash since I prefer the feature branch strategy which also keeps the target branch clean of work-in-progress commits.) Might that affect the behavior?

[jbsarrodie]

I've just committed a change would should hopefully fix this.

I've just tested and it now works as expected.

As said, this happened after a merge in which I had some conflicts, but as the conflict solver had some issues with identifying the right "Theirs" commit, it might be the root cause. I'll test again tonight to see if I can reproduce the issue or not.

I try to reproduce the issue but can't. So I assume it has been fixed with the other issue.

We need to review the sequence of events in MergeBranchAction to check whether we've made the right assumptions about what needs to happen.

I checked it and this is aligned with my proposition and seems the right one (at least IMHO).

What does a git.merge() defaults to? I see that both setFastForward and setSquash is commented out, but it looks like JGit defaults to a fast forward when there are no conflicts and maybe a rebase when there are conflicting changes. Is this right?

From my test the result is similar to the one of the usual Publish action: a fast forward when possible and a merge commit (not a rebase nor a squash commit) in other cases.

Personally, I prefer a squash since I prefer the feature branch strategy which also keeps the target branch clean of work-in-progress commits.

Well, I personally prefer keeping the whole commit history, I find it more intuitive for user (squash commit is really a git thing).I suggest we keep it this way for the MVP. Maybe at some point we could offer user the choice to "group" all changes from the merged branch inside one (might be a better description for a squash commit), in this case the commit message could be auto-generated as the concatenation of all commit messages.

[mjardemalm]

I’ve done some testing and I must say that it feels really good testing merge in Archi. =)

My reflections:

• A successful merge, when there’s no changes on the merge target, the git.merge() will do a fast forward and does not seem to care about the setCommit(true). The commits are merged as expected into the target with a straightforward traceability.

• If there are changes in both target and source branch the commits are merged as expected but succeed of an empty commit with the message “Merged Branch ‘branch_name’”. For some reason the setCommit(true) then applies, I’ll guess it’s why it’s a merge and not a fast forward. What merge-strategy that JGit uses I’m not able to analyze but resolve or recursive seems most likely, probably depending on how changes are analyzed before merge.

From a user point of view, these mergers are pretty much the same, even though JGit handles it in different ways, but one is made with an empty merge commit where the other isn’t. It does not feel like a consistent behavior.

• On a merge, with changes in both target and source, and conflicting changes the merge handles compatible commits as expected and with a “Resolve merge”-commit for the conflicting changes.

This seems like an expected behavior. The one thing is the automatical commit. If the commit should be made automatically the message could be clearer. The other possible way is to not do the automatic commit and let the user commit the changes manually.

[Phillipus]

Are there any options on the MergeCommand that we need to set or unset?

http://download.eclipse.org/jgit/site/4.5.0.201609210915-r/apidocs/org/eclipse/jgit/api/MergeCommand.html

What merge-strategy that JGit uses

Looking at the source code of MergeCommand it seems to be MergeStrategy.RECURSIVE

If JGit is doing inconsistent things then we need to look at the available options, and see if we've made the right assumptions. It may be a JGit quirk.

If the commit should be made automatically the message could be clearer.

Any suggestions? I think it can be set easily.

The other possible way is to not do the automatic commit and let the user commit the changes manually.

Not sure if leaving the state as "unmerged" is safe?

[Phillipus]

I've looked at the source code for MergeCommand and seen that if an option is not set explicitly JGit will use settings in the config file for the branch. To be safe, I set them as:

MergeResult mergeResult = git.merge()
    .include(mergeBase)
    .setCommit(true)
    .setFastForward(FastForwardMode.FF)
    .setSquash(false)
    .setMessage(message)
    .call();

[mjardemalm]

To be consistent, either the fast-forward should add an merge-commit (setFastForward(FastForwardMode.NO_FF)) or the recursive merge should remove the merge-commit (setCommit(false)). I think either way will work but I think consistency is important.

About the merge strategy, I think it’s a good choice to use recursive. If JGit don’t try something else in other situations, default values with fast forward or r_ecursive merge_ will work fine.

[Phillipus]

Regarding the next stage for branch merging, how should we achieve this?

Is it the case that the user needs to do some actions first before a merge?

• Pull / Fetch
• Commit all outstanding commits
• Push all outstanding commits

Do we want these actions to be automatically done before a Merge Branch action? If so, I think that things could go wrong and the user might not know what to do. How do we determine a "success" or "fail"?

[jbsarrodie]

Regarding the next stage for branch merging, how should we achieve this? Do we want these actions to be automatically done before a Merge Branch action? If so, I think that things could go wrong and the user might not know what to do. How do we determine a "success" or "fail"?

My feeling is that we should decide what is the "normal" what to merge, ie. online or offline. I would argue that, for a merge to be really a success, it should be done online, so I would go for an online merge by default, unless the user decides the opposite.

In addition, as merge is a multi-step action, I suggest we display some progress information.

So this merge action could become something like:

• User decide to merge
• A dialog appears explaining the basic steps:
  1. Any unsaved work should be committed ad published. This might raise some conflicts to solve and cancelling at this step will abort the whole operation.
  2. Other branch should be synced (ie. refreshed or published if some changes are still only local). This might raise some conflicts to solve and cancelling at this step will abort the whole operation.
  3. Merge will be done. This might raise some conflicts to solve and cancelling at this step will abort the whole operation.
• In previous dialog, There should be a message explaining that it is possible to do an offline merge, but this is not recommended. Then there should be 3 options:
  • Continue. This is the default
  • Continue with an offline merge.
  • Cancel
• Depending on user choice, do an offline or online merge. I would suggest to show a kind of dialog or progress note between each steps (sort of wizard), so that the user always know what has been done and what remains.

So merge workflows become:

Workflow for "offline" merge:

1. User wants to merge a branch (OtherBranch) into the current branch (CurrentBranch)
2. The previously detailed dialog appears. User choose the non default option "Continue Offline"
3. If model is dirty, user is asked to save its model, and...
4. Model is exported to Grafico
5. If needed user is prompted to commit its changes
6. A dialog or progress note explains that the merge will now take place.
7. We can now run the merge itself.
   1. We run the equivalent of a git merge OtherBranch
   2. If conflicts exist user solves them through the conflict dialog
8. A dialog or progress note explains that the merge has been successful (or not if conflicts were detected and not resolved)

Workflow for "online" merge:

1. User wants to merge a branch (OtherBranch) into the current branch (CurrentBranch)
2. The previously detailed dialog appears. User choose the default option "Continue"
3. A dialog or progress note explains that we'll now publish any local changes
4. We run a Publish action:
   1. If needed user is prompted to save its model
   2. Model is exported to Grafico
   3. If needed user is prompted to commit its changes
   4. Publish is done (this can raise the conflict dialog if needed)
5. A dialog or progress note explains that we will now refresh the other branch and publish any local changes
6. We switch to the other branch
7. We run a Publish action
8. If conflicts appear at this step and are not resolved, then after cancelling we have to switch back to the "current" branch
9. (no conflicts or conflicts resolved) We switch back to the "current" branch
10. A dialog or progress note explains that the merge will now take place.
11. We can now run the merge itself.
    1. We run the equivalent of a git merge OtherBranch
    2. If conflicts exist user solves them through the conflict dialog
12. A dialog or progress note explains that the merge has been successful (or not if conflicts were detected and not resolved)

[Phillipus]

My fear is that's a lot of dialog boxes and stages that need to be strung together (and where a "fail" could happen at any stage).

• Choice dialog
• Save model dialog
• Commit dialog
• (Possible Credentials dialog)
• Refresh / Publish dialog
• Merge Conflict dialog
• Switch branch dialog
• Possible error dialogs

It's probably not a good idea to have several dialog boxes one after the other, but assembling these stages into a single UI wizard would mean re-writing these actions in existing code and dialogs into one UI and handling things differently.

Are we ideally thinking of a multi-stage wizard? Or are we asking the user to do these operations manually?

[Phillipus]

Boiling this down, it amounts to doing these Archi code Actions:

Current Branch - PushModelAction Switch Branch - SwitchBranchAction Other Branch - PushModelAction Switch Branch - SwitchBranchAction Current Branch - MergeBranchAction

PushModelAction

1. Save model
2. Export to Grafico
3. Commit any changes
4. Get user credentials
5. Pull from remote
6. Resolve merge conflicts (if any)
7. Load model from Grafico
8. Do an auto-commit in case any missing objects were restored from merge conflict
9. Push to remote

(Stages 1 - 8 are the RefreshAction)

SwitchBranchAction

1. Save model
2. Export to Grafico
3. Commit any changes
4. Track and create branch if needed
5. Switch branch
6. Load model from Grafico
7. Save checksum

MergeBranchAction

1. Ask for confirmation to do this.
2. Save model
3. Export to Grafico
4. Commit any changes
5. Merge branch
6. Resolve merge conflicts (if any)
7. Load model from Grafico
8. Do an auto-commit in case any missing objects were restored from merge conflict

Thinking aloud...

• It seems there's some duplication of stages.
• I'm trying to avoid duplication of code.
• These actions are written in such a way because they are single actions.
• Creating a wizard would require refactoring of existing code for re-use in both scenarios (single action and sequenced actions)
• I don't like modal dialog boxes spawned from other modal dialog boxes or wizards.
• Really what we're saying to the user is "update both branches before attempting a merge". In SmartGit I would do this manually, but here we're trying to to automate it.

[jbsarrodie]

What about keeping only the first (and main) dialog that explains the whole process and ask user to continue (online), continue (offline) or abort. Once the user has decided he will know what happens, so no need for additional dialogs along the way.

[Phillipus]

I'll add an experimental POC second action, something like "Merge Branch (exp)" (which won't have the explanatory dialog for now) to see if it's possible to sequence the existing actions together and if it works...

[Phillipus]

I've pushed a new branch - "online-merge". This is experimental work.

I've added a new context menu item to Branches View - "Experimental Merge", It does a sequence of actions as follows:

Current Branch - PushModelAction Switch Branch - SwitchBranchAction Other Branch - PushModelAction Switch Branch - SwitchBranchAction Current Branch - MergeBranchAction

This allows us to re-use code from the existing Actions. You may see more than one ProgressMonitor because of this.

Please test!

[Phillipus]

I've done some testing and it seems to work, but we need to really try different scenarios. If the sequence works, I can then improve the UI with a unified dialog as suggested.

[Phillipus]

There's a problem with the way that JGit merges after resolving a merge conflict. Here's an image taken after merging in Archi:

The "origin/master" branch is not on the timeline, so the next time you pull it will merge this back into local "master."

Here's an image taken after doing a manual merge in SmartGit:

The "origin/master" branch is on the timeline so it does not create a problem.

I've tried setting different merge strategies in JGit but still have this problem.

Edit - FIXED! See below.

[jbsarrodie]

There's a problem with the way that JGit merges after resolving a merge conflict.

I'm not sure to understand the issue, can you explain it a bit more (its consequences)?

Please test!

Will do soon.

[Phillipus]

Look at the second image above. That's how it should look after a merge conflict. "origin/master" is behind "master" in the timeline.

However, the first image shows that "origin/master" is not on the timeline after a merge commit. Thus, you cannot push until you pull to merge origin into master.

I have found the problem and submitted a fix:

When a merge conflict is resolved a commit is necessary to commit the merge result, It was being committed with "amend last commit" set to true. Setting it to "false" solves the problem.

[Phillipus]

Here is the sequence of actions performed in the "Experimental Merge" menu action:

Init

1. Ask to save model, if needed
2. Export to Grafico
3. Offer to commit any changes, if needed
4. Get user credentials (dialog or stored)

Pull 1

5. Pull from remote
6. If Merge conflict: Handle merge
7. Load model from Grafico and restore any missing objects
8. Commit any changes, if needed

Push 1

9. Push to remote

Switch

10. Switch to other branch

Pull 2

11. Pull from remote
12. If Merge conflict: Handle merge
13. Load model from Grafico and restore any missing objects
14. Commit any changes, if needed

Push 2

15. Push to remote

Switch

16. Switch back to first branch

Merge

17. Merge branches
18. If Merge conflict: Handle merge
19. Load model from Grafico and restore any missing objects
20. Commit any changes, if needed

Push 3

21. Push to remote

That's a lot of stuff to do, so it can take a while.