archimatetool / archi-modelrepository-plugin

coArchi - a plug-in to share and collaborate on Archi models.
153 stars 52 forks source link

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

Closed jbsarrodie closed 1 year ago

jbsarrodie commented 5 years ago

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:

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

jbsarrodie commented 5 years ago

@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 commented 5 years ago

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

Well, I thought about it a bit...

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

jbsarrodie commented 5 years ago

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):

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

My reflections:

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.

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 recursive merge will work fine.

Phillipus commented 5 years ago

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?

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 commented 5 years ago

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:

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 commented 5 years ago

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

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 commented 5 years ago

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

jbsarrodie commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

archimerge

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:

smartgitmerge

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

  1. Pull from remote
  2. If Merge conflict: Handle merge
  3. Load model from Grafico and restore any missing objects
  4. Commit any changes, if needed

Push 1

  1. Push to remote

Switch

  1. Switch to other branch

Pull 2

  1. Pull from remote
  2. If Merge conflict: Handle merge
  3. Load model from Grafico and restore any missing objects
  4. Commit any changes, if needed

Push 2

  1. Push to remote

Switch

  1. Switch back to first branch

Merge

  1. Merge branches
  2. If Merge conflict: Handle merge
  3. Load model from Grafico and restore any missing objects
  4. Commit any changes, if needed

Push 3

  1. Push to remote

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

jbsarrodie commented 5 years ago

Ok, I've just done some tests and while this works as expected, I have some remarks:

I can then improve the UI with a unified dialog as suggested.

I guess we can move on with this. BTW, is the text of Merge Conflict dialog easily customizable depending on the stage at which it is called? If yes we could tune the text a bit so that user know at which step he is and which branches are compared (local branch X versus remote branch X, local branch Y versus remote branch Y or local branch X versus local branch Y).

Phillipus commented 5 years ago

When switching branch while I did some (uncommitted) changes, I am asked to save but not to commit.Not being forced to commit makes sens because I could want to commit these new changes in another branch, but being offered the choice to commit or not would be even better.

Yes, agreed. Now the logic is this:

  1. If the current branch and the target branch are at the same Ref (commit) then you may not want to commit when switching.
  2. If this is the case the user is asked whether they want to commit before switching.
  3. If yes, the commit dialog is shown and the branch is switched. If no, the switch is made.
Phillipus commented 5 years ago

Update - the GitHub branch "online-merge" is now gone and we are back to the main development branch "branching"

There is now only one "Merge into Current Branch" Action. This shows a dialog with the choices of "online", "local" or "cancel".

The final Merge should be followed by a push to remote (we're online after all).

Done.

We should decide how to manage the deletion of a branch after a merge. Maybe a dialog at the end: "You've successfully merged branch X into Y. You can now safely delete branch X." with two options: "Delete" and "Don't Delete" ?

Done. The options are "Yes" and "No" to the question "Do you want to delete branch x?"

BTW, is the text of Merge Conflict dialog easily customizable depending on the stage at which it is called? If yes we could tune the text a bit so that user know at which step he is and which branches are compared (local branch X versus remote branch X, local branch Y versus remote branch Y or local branch X versus local branch Y).

There are two places where the Merge Conflict dialog can appear:

I've added some text depending on whether we are pulling and merging from the remote or merging from local branch to local branch.

Please test!

jbsarrodie commented 5 years ago

Please test!

Ok, I've just tested. It works but I noticed some small bugs:

I've added some text depending on whether we are pulling and merging from the remote or merging from local branch to local branch.

I'd suggest to check if both branches have the same name: if yes we can assume that we are publishing, if no we are merging 2 branches. So the message could be either:

Phillipus commented 5 years ago

We should not offer to remove the merged branch if it is master (deleting master is never a good idea)

Fixed

It seems that the Change History view only lists branches which exist locally while Branches view also lists remote branches.

This was intentional because I'm not sure if user actions were suitable for remote branches that have not been checked out. However, I've now enabled remote branches here. Let's test that.

Very strange: a publish action with conflict seems to amend previous commit

This is actually happening on the "Refresh" (pull) action prior to "Publish". I seem to remember a case where we wanted this behaviour. However, I've now set amend to false. Let's test that.