search

archimatetool / archi-modelrepository-plugin

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

Add an way to cleanup "Deleted" branches (ie. branches that exist locally but track a non existent remote branch). #147

Closed jbsarrodie closed 3 years ago

jbsarrodie commented 3 years ago

Make sure to warn a user if he's still working on one of these branches.

Edit: some discussions occured on #140 so I'm moving them here

jbsarrodie commented 3 years ago

from https://github.com/archimatetool/archi-modelrepository-plugin/issues/140#issuecomment-688827202

Add an way to cleanup "Deleted" branches (ie. branches that exist locally but track a non existent remote branch). Make sure to warn a user if he's still working on one of these branches.

I've seen your comment (I also played around with a "Remove all Deleted branches" action but it turned out to be a bit complicated (lots of additional checks). It works but now that we can see the status of a "Deleted" branch more clearly in the table I personally prefer to use the existing "Delete Branch" action to delete these branches individually.). Maybe we could find a simple way to do it: ~we in fact only speak about branches which have been checked out locally, so we have to see if local commits are already published (ie. remote should be ahead of local). If this is the case, then the branch can be safely removed. If not, then the user should be prompted with "Branch XXX is deleted on the remote server but you have unpublished work locally. Do you want to keep it or deleted it anyway ?".~

Edit: Not doable this way because obviously, if remote branch has been deleted we can't check if remote is ahead of local. But what we can do is check if the latest commit we have locally in this branch is accessible from another branch (i.e. it has been merged into another branch before deleting the remote branch). With git this can be done through git branch --contains <commit> (here's a potential way to do it in JGit)

jbsarrodie commented 3 years ago

Original comment from @Phillipus from https://github.com/archimatetool/archi-modelrepository-plugin/issues/140#issuecomment-691048151

But what we can do is check if the latest commit we have locally in this branch is accessible from another branch (i.e. it has been merged into another branch before deleting the remote branch). With git this can be done through git branch --contains (here's a potential way to do it in JGit)

It might be that two branches share the same commit and neither is merged.

@jbsarrodie I've thought of a simpler way to do this but maybe there is a flaw in this idea?

jbsarrodie commented 3 years ago

From https://github.com/archimatetool/archi-modelrepository-plugin/issues/140#issuecomment-691282125

@jbsarrodie I've thought of a simpler way to do this but maybe there is a flaw in this idea?

Yes there is :-)

  • If the latest commit of a branch that is not "master" has one parent then it has not been merged

No, it simply isn't a merge commit, but you don't know if this commit itself has been merged somewhere.

  • If it has more than one parent (usually 2) then it has been merged into another branch

No, it is simply a merge commit, but you still don't know if this commit itself has been merged.

To know if a commit A has been merged elsewhere, you need to find a commit B which has A has one of its parents... But this is not enough in our case because there might not be a merge commit if the merge has been done fast-forward. So this only solution (and easier IMHO) is to check if the latest commit is included in another branch on remote.

jbsarrodie commented 3 years ago

Original comment from @Phillipus from https://github.com/archimatetool/archi-modelrepository-plugin/issues/140#issuecomment-691451144

Perhaps this - https://stackoverflow.com/questions/26644919/how-to-determine-with-jgit-which-branches-have-been-merged-to-master

But checking all branches not just master.

jbsarrodie commented 3 years ago

From https://github.com/archimatetool/archi-modelrepository-plugin/issues/140#issuecomment-691533679

But checking all branches not just master.

You mean this (the question is about a variation, but the sample code provided at the begining does exactly what we want.

Phillipus commented 3 years ago

The merge status is done now. After finding a way to do it, I discovered that there is a JGIt function:

RevWalkUtils.findBranchesReachableFrom()

jbsarrodie commented 3 years ago

From recent discussions with Guillaume (who should join this thread at some point):

Some users in the team (because of their role) have most branches locally, which means several dozens of branches. In this (extreme) case, I can imagine that deleting each branch in “deleted xxx” state is still a time consuming operation. Maybe in this case a good compromise to a real “auto-delete” could be to enable multi-selection so that user simply has to pick (e.g.) 34 branches and then delete them in one action.

Phil's comment on this is:

multi-select and delete would be easier.

For reference here is the current Delete Branch operation sequence for deleting one branch:

  1. Branch should not be "master" and should not be currently checked out
  2. If the branch is a local branch delete it locally
  3. If this local branch also exists online ask for user details (or get user details from local storage file) and delete it online
  4. If the branch is a remote branch (no local corresponding branch) ask for user details (or get user details from local storage file) and delete it online So bear in mind that user details are required to delete online branches.

We need to decide what branches can be deleted (perhaps with warnings?)>

Local / Remote branches? Commit status - has to be "up to date"? No pending commits or commits available online? Merge status - "Not merged" allowed?

Guillaume's comment:

I imagined a multiple selection dialog box allowing to choose the branches to delete locally. So JB’s proposal is close, except that we have about 100 branches today and selecting the deleted ones among them is not convenient. I think there could be warnings if a branch has uncommitted changes or is unmerged. A confirmation box should be raised.

jbsarrodie commented 3 years ago

The merge status is done now. After finding a way to do it, I discovered that there is a JGIt function: RevWalkUtils.findBranchesReachableFrom()

Never too late ;-)

I'll test it soon

Phillipus commented 3 years ago

I'll test it soon

We have a class BranchInfo that holds the known state of a branch. BranchInfo#isMerged() uses the new method. It does this:

"Find the list of branches a given commit is reachable from when following parents"

This is shown as either "OK" or "Not Merged" in the Branches View table column "Merge Status".

If the latest commit of a given branch is reachable from at least one other branch then it is "OK". Otherwise it means that it has not been merged to any other branch.

Phillipus commented 3 years ago

Perhaps a candidate for deletion is:

jbsarrodie commented 3 years ago

JB’s proposal is close, except that we have about 100 branches today and selecting the deleted ones among them is not convenient.

@Phillipus If I summarize, here are our options:

  1. Add an option to the "Branches" view's icon bar to Cleanup deleted branches. This action would first ask for a global confirmation, then delete all branches which are in deleted state and have been previously merged.
  2. Let people select multiple branches and run a global "delete branches" action
  3. Same as 2. but let people sort branches based on their status so that it gets easier to select them in one go (select the first one, press shift, and then select the last one)
Phillipus commented 3 years ago

If the latest commit of a given branch is reachable from at least one other branch then it is "OK". Otherwise it means that it has not been merged to any other branch.

Note - this does mean that Branch "A" might be reachable from Branch "B" (and shows as "OK" or merged) but neither branch has been merged to "master". This is OK if you delete one of these branches as the status is then "Not merged" but if you were to delete both branches then that would be bad.

So maybe we should check if a branch is reachable from "master"?

jbsarrodie commented 3 years ago

So maybe we should check if a branch is reachable from "master"?

Not wanted because there are some setups in which master only contains some common stuff that you merge into other branches, but in this case you never merge into master.

Not needed anyway...

Note - this does mean that Branch "A" might be reachable from Branch "B" (and shows as "OK" or merged) but neither branch has been merged to "master". This is OK if you delete one of these branches as the status is then "Not merged" but if you were to delete both branches then that would be bad.

In this case, if B is not reachable from another branch, then it won't be shown as "OK" and won't be deleted.

The only edge case I can see is if you created B off of A and never worked on it. In this case B contains A and A contains B so both are "OK" and could be deleted. If we want to avoid this, then we have to refresh the status of a branch just before trying to delete it. In this case, while A would be deleted first, B wouldn't as it would switch from "OK" to "Not merged".

Phillipus commented 3 years ago

The only edge case I can see is if you created B off of A and never worked on it. In this case B contains A and A contains B so both are "OK" and could be deleted. If we want to avoid this, then we have to refresh the status of a branch just before trying to delete it. In this case, while A would be deleted first, B wouldn't as it would switch from "OK" to "Not merged".

Yes.

  1. Create a new branch "A" and make a commit.
  2. "A" is "not merged"
  3. Create a new branch "B" at the point of "A"
  4. Now "A" is merged and "B" is merged
  5. Delete "A" or "B" and the remaining branch is now "not merged"

But if you delete A and B together in a bulk operation then we have a problem. So for a bulk delete each merge status has to be re-checked (as you said above)

Phillipus commented 3 years ago

I've added an experimental action to the Branches View, available from right-click and named "Delete Stale Branches" (we can rename this to something better).

These are the conditions for deleting a branch:

  1. Is a Local branch
  2. Is not the current branch
  3. Is not the "master" branch
  4. Is being tracked to remote but there is no remote ref ("Deleted" status)
  5. Is merged (accessible from another branch)
  6. Has no unpushed commits

I also added a safety check in case of the edge case described above. After a branch is deleted the branch status is recalculated to determine if (5) merge status is valid. If not it will not be deleted.

@jbsarrodie When you get a moment ( 😉 ) is it possible for you to test this?

jbsarrodie commented 3 years ago

Just an update for those who follow this issue.

I did test some weeks ago and everything is fine. Only minor adjustments in wording were needed and have been implemented since.

Closing this issue now ;-)

jbsarrodie commented 3 years ago

Reopening this issue has a bug has been found (and fixed).

While checking the isMerged, all branches should be tested (including remote branches). I've commited a fix.