Skip to content
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

Making mgit close really useful #84

Open
Reinmar opened this issue Sep 13, 2018 · 9 comments
Open

Making mgit close really useful #84

Reinmar opened this issue Sep 13, 2018 · 9 comments

Comments

@Reinmar
Copy link
Member

Reinmar commented Sep 13, 2018

The discussion started in #82 (comment) and continued for a couple next comments.

Initially, I assumed that mgit close should work like git merge on steroids – you call it when on the branch to which you want to merge the given branch. But, in addition to calling git merge, mgit close should also remove the branch from the remote and push the current branch to the remote.

We started discussing whether mgit close should ask whether you want to remove the merged branch from the remote and whether to push the current branch to the remote too. I insisted on this because that:

  • ensures that all branches are closed consistently (you don't forget about anything),
  • it lets you check whether everything worked fine before making the decision (e.g. you can run the tests).

However, then I started realising that there's more about mgit close that we might improve. That it can be far more useful than a simple shorthand to git merge:

  • It can open a text editor so you can easily paste the merge message. This is very important because I still can't use backticks in the -m "" param, despite googling how to do that. Plus, multiline messages are very hard to type in the CLI.

  • The workflow for closing branches is actually different than:

    1. be on master,
    2. merge branch xxx to master.

    When merging, you're usually on the setup of branches you want to close. There may be more branches called xxx in other repos, but you may not be using them at the moment. You are interested in closing exactly what you have there and just tested. Therefore, I want to propose a different workflow.

Workflow

  1. Switch your repository to the setup of branches that you're testing. Usually – you'll change the branch in the main repo and call mgit sync so to use the setup from mgit.json.
  2. Call mgit close (if executed without params it will merge to master).
    1. The command checks which repositories are not on master.
    2. It lists them and lets you unselect the ones you don't want to merge. This is important because often times you want to specify different messages for particular repos.
    3. It opens the OS's text editor so you can type the merge message.
    4. It then performs the merge.
    5. It asks you whether you want to remove the merged branches from the remote. That's a time
    6. It asks you whether you want to push master.
  3. Then you repeat the above for the remaining branches.
  4. Finally, if you didn't decide to push master before merging everything you call mgit push.

That's how my workflow with dealing with branch setups usually looks (but of course I do all those things manually). If mgit close worked this way I feel that it'd be most useful.

Also, please bear in mind that we may still have mgit merge for more git merge-like behaviour. But we should see first if we need it.

WDYT? cc @pjasiun @jodator @oleq

@jodator
Copy link
Contributor

jodator commented Sep 13, 2018

OK - I need to learn more about mgit since I'm using mostly mgit update (it's a bit faster then PHPStorm's equivalent) and sometimes mgit st. Other things I do either with PHPStorm (commit, checkout branches) or on github (closing PRs).

That being said here are some thoughts on this.

  1. Removing obsolete branches from local repo - this thing is mostly leftover after working on some tasks as remote branches are almost always removed: I like to have tidy locally and I do use PHPStorm to update all repos since its calling:

    12:25:45.673: [packages/ckeditor5-engine] git -c core.quotepath=false \
     -c log.showSignature=false fetch origin --progress --prune
    From github.com:ckeditor/ckeditor5-engine
     - [deleted]           (none)     -> origin/t/1521
     - [deleted]           (none)     -> origin/t/1537
    

    the --prune command could be used in mgit update

  2. The mgit close I might start using this with above specifications - looks useful especially for merging PRs in 2+ repos with message like "Align code to changes in bla bla bla...". 👏 for this.

  3. As for:

    It opens the OS's text editor so you can type the merge message.

    I suppose that this could work as git uses system text editor (in CLI - no GUI) - just to be sure how you guys thinking about this.

@pjasiun
Copy link

pjasiun commented Sep 13, 2018

I think that this command should be very similar to closing PRs on GitHub. It should let me edit the message, merge the branch and delete the branch. I agree that it should let me define the message it a nice way (opening the editor would be nice). Since we will use it often to merge all branches related to the PR it should also have some default message already prepared.

I also agree that we need a confirmation question with all what mgit will do. I would not use it if I would not be sure if it will do what I expect it to do (especially if it will merge, delete branches and push).

@oleq
Copy link
Member

oleq commented Sep 13, 2018

It opens the OS's text editor so you can type the merge message.

Like for each merge in the constellation? Because usually, the messages are different across the packages in branch constellations.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 13, 2018

Like for each merge in the constellation? Because usually, the messages are different across the packages in branch constellations.

Many times, if you have N branches to merge, the message will repeat in N-1 or N-2 because there's one main change that gets a special message and most other repos get "Aligned to changes in ...".

That's why I proposed that you can pick in which repos you want merge a change.

@pjasiun
Copy link

pjasiun commented Sep 13, 2018

there's one main change that gets a special message and most other repos get "Aligned to changes in ...".

I think that mgit close should be focused on these "other repos". I agree that all of them have the same message (Aligned to changes in ..."), this is why I propose there should be a default message. There is no reason to use mgit for the main repo, you can use git for it and usually main repo has PR on GH and you can merge it using GH.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 13, 2018

I think that mgit close should be focused on these "other repos". I agree that all of them have the same message (Aligned to changes in ..."), this is why I propose there should be a default message. There is no reason to use mgit for the main repo, you can use git for it and usually main repo has PR on GH and you can merge it using GH.

If it lets you to choose which repos you want to touch then it's up to you. You can deselect that main repo and close it the way you want.

But personally, I usually close all from the console first and then push. Why? Because I can then make a quick check that I didn't screw something up.

So, I'd do this:

mgit close
# assuming that the main PR is in the engine – deselect it
# pick "no" when asking about pushing so I can close engine first

mgit close
# at this stage mgit will only try to merge the engine because all other branches were merged
# when ask whether to push I can now check if all worked fine, I can check `mgit st` in another
# tab and let it push

And done.

In fact, that'd be even better if mgit would work like this:

  1. Switch your repository to the setup of branches that you're testing. Usually – you'll change the branch in the main repo and call mgit sync so to use the setup from mgit.json.
  2. Call mgit close (if executed without params it will merge to master).
    1. The command checks which repositories are not on master.
    2. It lists them and lets you unselect the ones you don't want to merge. This is important because often times you want to specify different messages for particular repos.
    3. It opens the OS's text editor so you can type the merge message.
    4. It then performs the merge.
    5. It asks whether you want to get back to step "1" again to merge more branches.
    6. It asks you whether you want to remove the merged branches from the remote. That's a time
    7. It asks you whether you want to push master.
  3. Then you repeat the above for the remaining branches.
  4. Finally, if you didn't decide to push master before merging everything you call mgit push.

By letting me iteratively merge branches before steps 6 and 7, if anything gets bad I can easily revert everything. Perhaps even mgit could do that.

@pjasiun
Copy link

pjasiun commented Sep 14, 2018

This tool should also fetch all changes from the remote. It happens sometimes that someone else pushes some changes to the branch, but when I am closing this branch locally I forget about these changes. It is the most often when I am closing multiple branches.

@pjasiun pjasiun closed this as completed Sep 14, 2018
@pjasiun pjasiun reopened this Sep 14, 2018
@Reinmar
Copy link
Member Author

Reinmar commented Sep 14, 2018

This tool should also fetch all changes from the remote. It happens sometimes that someone else pushes some changes to the branch, but when I am closing this branch locally I forget about these changes. It is the most often when I am closing multiple branches.

I agree with pulling changes to master. But I think that if you want to merge an outdated branch mgit close should abort with an error. Because that means that you've been testing the wrong thing. Or at least it should ask whether you want to continue or something so you know that you forgot to pull and you need to perform any testing you want again.

@Reinmar
Copy link
Member Author

Reinmar commented Nov 22, 2018

BTW, so I don't forget – one more reason for mgit close's special powers – it sometimes happens that I'm not closing a setup of branches having the same name. Sometimes, one of them will have a different name. Also, sometimes more branches can be pushed (i.e. in more repositories) than what's finally included in a constellation. Therefore, I'd like to merge exactly what I have at the moment.

@Reinmar Reinmar added this to the next milestone Jan 11, 2019
@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants