hotfix/feature merge: push on successful merge

[danielbeardsley]

We've historically done this by hand, but forgetting to push one or
both branches has been a source of frustration and confusion.

Automate all the things!

Git allows pushing as many branches as you want:
git push {remote_name} {branch1} {branch2} ...

[timothyasp]

Awesome - CR . 👍

[marczych]

Nice! CR . 🍖

[MutantFreak]

I'm not veto-ing this pull, but I will add that on at least a few occasions that I know of in the last few weeks, having the merge take place without an automatic push has actually been very helpful. I've seen someone do a merge then realize they made a mistake that has to be rectified. If the push automatically happened, we would have had a few messes on our hands reverting commits in master and stable.

I don't personally merge things very often, so I leave it to you guys to decide if this change is something you collectively want (I'd like to see more opinions from other devs, and especially QA's since you guys merge more often.) Tagging to get more people's attention and opinions: @sctice @adam000 @cdcline @chpatton013 @sterlinghirsh @BaseInfinity @scotttherobot @joshterrell805 @bleventh @davidrans

I do think if we're going to do this change, we should make sure there's a confirmation when the user tries to merge, along with a prominent warning that the push into stable and / or master will be automatic.

[adam000]

Confirmation sounds good to me.

[danielbeardsley]

A y/n confirmation is kinda useless, I might be ok with asking the user to type the name of the branch at the beginning of the process... so it's very obvious what they are doing.

@MutantFreak -- can you elaborate on the mistakes? Is there something else we can change to reduce the frequency of those mistakes? I've seen at least 2 or 3 unpushed merge commits cause issues this week, which is what prompted me to make this change.

[MutantFreak]

The issues I've seen have been where someone does the merge on their local branch, then, when prompted to push will stop and think, and realize that they shouldn't have merged. Having any break / pause in the action seems to allow time to catch that potential mistake before the push happens, which is why I suggested a prompt.

[sctice]

I don't merge very often myself, but if a delay really has helped people avoid pushing something that they ought not to have, then it seems like a confirmation might not be useless. Perhaps a confirmation plus some relevant information? It would help to know exactly what kinds of mistakes are causing people to regret having pushed. I suspect that displaying a confirmation would be useless if the user couldn't have received any new information after executing the merge command (or isn't likely to have).

[adam000]

For me, I like reading the numstat diff to make sure that the right thing is going out (I've aborted more than once because I forgot to switch branches). My $0.02

[xiongchiamiov]

+1 to diffstat. I just pushed some extra commits to server-templates the other day because I forgot I had merged things into master I didn't actually want there. But of course, I never check that myself :), so the current two-step process didn't prevent it at all.

[danielbeardsley]

Ok, sounds good.. how about this:

$ feature merge
... usual stuff
git merge
...
----- About to push these changes ------
numstat
---
commit-list (git log master@{u}..master --first-parent --oneline)
Are you sure you want to push this?

For hotfix branches we could still just show the devbranch (and ignore stable).

[adam000]

Sounds good, but what do you do if you say 'no' to that dialogue? It seem like that would place you in limbo.

[danielbeardsley]

@adam000 -- It'd be pretty easy to just git reset --hard origin/#{dev_branch} Since you just did a merge, you for sure don't have any modified files.

[adam000]

poke on diffstat

[kwiens]

Can we please either merge this or close it?

[marczych]

I vote for closing this because it's really not a big deal to push the branch after running hotfix/feature merge. On the other hand, I've very rarely stopped in between merging and pushing to verify that I'm good with it. In fact, I more often forget to push before deploying.

Anybody else have any other thoughts? I'll probably close this in a few days if there isn't an argument compelling enough to merge it.

[adam000]

Let's close it. I like automating this, but we can combine these steps as a separate tool.

[mlahargou]

Think we should get some new CR on this. The last ones were over 5 years ago.

[addison-grant]

#120 (comment) sounds like a good idea.. I cancelled the old CRs with periods.

[djmetzle]

CR

[hackalot805]

CR 💀

[addison-grant]

I am in favor of closing this without merging. I have experienced very few problems with forgetting to git push, and had several times where I have undid the feature merge after realizing I had feature merged the wrong branch by seeing the diff stats that appear in between.

[addison-grant]

Suggestion: If people want to, they can make an alias like alias fmpush="feature merge && git push".

[k0rvusk0r4x]

I have to agree with @addison-grant's suggestion here and here. Personally, I have never been in a situation where I would type feature merge when I wasn't deploying, so git push naturally came after. However, the more we hear from everyone else, the more I see it does happen and could very well be something I might do in the future. I like the idea of making an alias, and will probably do so for myself, but seeing this has been a problem for people (and recently), I would also prefer having the commands separate under normal circumstances.

If we were combining the entire deploy process into a command no one has ever used before (while still maintaining the stages we have now), that'd be great. With this as it is, we are combining two of the more dangerous parts into a command we use now. Seems a good way for people to make even more accidental mistakes than before.

[addison-grant]

We are going to hold off on signing off on this until we have some more conclusive discussion.

[danielbeardsley]

I'll conclusively decide to abandon this since it doesn't seem helpful. Let's move on :-)