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

Explicit "mark as merged" option in pull request UI #2

Open
isaacs opened this Issue Jun 4, 2013 · 40 comments

Comments

Projects
None yet
@isaacs
Owner

isaacs commented Jun 4, 2013

Pull requests are automatically marked as merged when the commits are not modified before pulling them into the main branch.

However, for repos that receive a lot of pull requests, the option is to either (a) have an excessive number of merge commits, or (b) force pull requestors to continually rebase on the lastest upstream, or (c) rebase/cherry-pick/am the patches on my repo, and forego the "merged" data point.

In many cases, (a) is unacceptable. Merge commits are used for specific cases, and "fixed a typo in a doc" is not worthy of the added noise.

If you are merging, let's say, 10 pull requests in a single day, then (b) is also unacceptable. I can tell all 10 people to pull, rebase, and force-push, however: 1) this is very tedious, as I have to wait for users to do this, and often explain how, etc. and 2) it creates a race condition if all 10 people do this, because they will have to do it again after I've merged one of their commits.

So, we fall back on (c) and, due to the impossibility of automatically determining whether a pull request was merged, it looks like Node doesn't accept patches.

It'd be great if I could explicitly indicate in the UI that a pull request was merged, so that the data could be more accurate.

@aredridel

This comment has been minimized.

aredridel commented Jun 4, 2013

👍

@glasser

This comment has been minimized.

glasser commented Jun 4, 2013

Additionally, allowing you to rebase (or even rebase-and-squash) in the UI would be great, rather than needing to go into a local copy to do so. It would be perfectly fine if this only worked in non-conflicting cases.

@silverjam

This comment has been minimized.

silverjam commented Jun 7, 2013

👍

5 similar comments
@piedar

This comment has been minimized.

piedar commented Jul 14, 2013

👍

@tonybruess

This comment has been minimized.

tonybruess commented Jul 15, 2013

👍

@germanop

This comment has been minimized.

germanop commented Jul 17, 2013

👍

@tkiran

This comment has been minimized.

tkiran commented Aug 30, 2013

+1

@tkiran

This comment has been minimized.

tkiran commented Aug 30, 2013

👍

@patcon

This comment has been minimized.

patcon commented Aug 30, 2013

Great idea @glasser! Created a new issue: #88

@gnarf

This comment has been minimized.

gnarf commented Sep 27, 2013

👍

@tonybruess

This comment has been minimized.

tonybruess commented Sep 27, 2013

How do we get this to the attention of GitHubers?

@gnarf

This comment has been minimized.

gnarf commented Sep 27, 2013

¯\_(ツ)_/¯ - I just used twitter: https://twitter.com/gnarf/status/378211055932936192

@gnarf

This comment has been minimized.

gnarf commented Sep 27, 2013

I remember at least one github talk talking about "TDD: Twitter Driven Development"

@intgr

This comment has been minimized.

intgr commented Aug 18, 2014

Edit: D'oh removed silly text, thanks to @rlidwka for pointing out that this isn't GitHub's official tracker.

+1

@rlidwka

This comment has been minimized.

rlidwka commented Aug 18, 2014

Maybe if we annoy GitHub developers enough with +1s they will implement it just to get some peace.

This isn't an official github issue tracker, so there are no gh developers here to annoy. Sorry. :)

@cvrebert

This comment has been minimized.

cvrebert commented Jan 19, 2015

👍

@davecgh

This comment has been minimized.

davecgh commented Feb 7, 2015

👍

1 similar comment
@carlosmmelo

This comment has been minimized.

carlosmmelo commented Apr 2, 2015

👍

@bel2125

This comment has been minimized.

bel2125 commented May 21, 2015

+1

5 similar comments
@mcshaman

This comment has been minimized.

mcshaman commented Jun 15, 2015

+1

@mslipper

This comment has been minimized.

mslipper commented Jun 16, 2015

👍

@anubiann00b

This comment has been minimized.

anubiann00b commented Jul 14, 2015

👍

@jmarshall

This comment has been minimized.

jmarshall commented Aug 6, 2015

👍

@pdilyard

This comment has been minimized.

pdilyard commented Aug 12, 2015

👍

@FilipZawada

This comment has been minimized.

FilipZawada commented Aug 26, 2015

Can't you have an anteroom (alternatively pullrequests) branch, only for pull requests? You can then cherry-pick commits you're interested in to your develop branch from there. This allows you to:

  • Use inviting green merge button and have fun of it
  • It's clearly visible which PR were merged, which not
  • You can accept a PR early, while spending time on cherry-pick and squashing later (once you got time). This way you give feedback to PR creator asap.

anteroom would be just a tmp branch which could be force cleaned on a regular basis.

@ramnes

This comment has been minimized.

ramnes commented Aug 29, 2015

👍

@gaboesquivel

This comment has been minimized.

gaboesquivel commented Jan 11, 2016

👍

@bhack

This comment has been minimized.

bhack commented Jan 21, 2016

👍

@bhack

This comment has been minimized.

bhack commented Jan 21, 2016

/cc @github

@Siilwyn

This comment has been minimized.

Siilwyn commented Feb 15, 2016

This is a much needed capability for everybody who rebases commits.

@TheNotary

This comment has been minimized.

TheNotary commented Feb 25, 2016

+1

The red 'no' sign on PRs that were actually accepted seems pretty unwelcoming. I think at the very least that symbol should just be replaced with this GIF which is a bit more neutral

giphy

@UdjinM6

This comment has been minimized.

UdjinM6 commented Apr 2, 2016

+1 for this and #548 please!

@purpleidea

This comment has been minimized.

purpleidea commented Jun 7, 2016

+1 and for maintainers who want to automate the rebase+merge, I've created git cherryfetch

https://ttboj.wordpress.com/2015/03/16/fancy-git-aliases-and-git-cherryfetch/

Hope this helps, even if @github isn't showing it.

@leachdaniel

This comment has been minimized.

leachdaniel commented Aug 24, 2016

+1 when doing manual via command line I cannot close as merged, without it merging again in the UI

@bfred-it

This comment has been minimized.

bfred-it commented Mar 9, 2018

If I understand it correctly, I don't think this is necessary any longer. GitHub now allows you to rebase or squash directly from the UI, so you can "merge" the PRs without having having the useless merge commit. 👍

@purpleidea

This comment has been minimized.

purpleidea commented Mar 9, 2018

@bfred-it unfortunately when you click via the webui it also modifies the commits! try it yourself, you'll see github signs them itself, in fact, quite different from if you use the cli. It's just junk.

@bfred-it

This comment has been minimized.

bfred-it commented Mar 9, 2018

How can it not modify the commit? The only way to get a commit onto a branch without modifying it is to have the same base. If you open two PRs from master and then merge one (ok, without modifying it) the second one will be modified. You can't merge without rebasing it... right?

@purpleidea

This comment has been minimized.

purpleidea commented Mar 9, 2018

@bfred-it A simple fast forward should occur in many situations but doesn't. Try it yourself.

@bfred-it

This comment has been minimized.

bfred-it commented Mar 9, 2018

As I said, that only works for the first merge, but not for consecutive ones:

❯ git checkout -b test
❯ git commit --allow-empty -m "test"
[test 3573e328] test

❯ git checkout master

❯ git checkout -b test2
❯ git commit --allow-empty -m "test2"
[test2 6ce85395] test2

❯ git checkout master

❯ git merge test
Updating b3859c62..3573e328
Fast-forward

❯ git merge test2
Already up-to-date!
Merge made by the 'recursive' strategy.

The first one is fast forward, the second one isn't:

❯ git log -3 -U
commit 28163afe030afe2b5b8d156af52e5ab71e881884 (HEAD -> master)
Merge: 3573e328 6ce85395
Author: Federico Brigante <github@bfred.it>
Date:   Fri Mar 9 15:43:00 2018 +0700

    Merge branch 'test2'

commit 6ce85395d2b1e77cc765419ac232254c2058ce84 (test2)
Author: Federico Brigante <github@bfred.it>
Date:   Fri Mar 9 15:42:37 2018 +0700

    test2

commit 3573e32877e3be7255054d17259add9df2bcfda4 (test)
Author: Federico Brigante <github@bfred.it>
Date:   Fri Mar 9 15:42:21 2018 +0700

    test
@jhurwitz

This comment has been minimized.

jhurwitz commented May 19, 2018

@bfred-it #95 is why I prefer to merge via the CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment