Merge Pull Request Considered Harmful • Hacker News Board

Re: Merge Pull Request Considered Harmful

#41

Post by ntalbott » Thu, Jun 26, 2014, 1:39 PM UTC

jakub_g wrote: post #17

MaikuMori wrote: post #5

Is this exactly situation that https://help.github.com/articles/checking-out-pull-requests-... documents? So basically: git fetch origin pull/ID/head:BRANCHNAME git checkout BRANCHNAME And now you can edit that pull request and/or make new pull request based on the previous one. Or just simply merge in master.

It's possible to configure it to have all PRs automatically fetched when you use `git fetch origin`: https://gist.github.com/piscisaureus/3342247 and then you'll have each PR available under sth like git checkout pr/123

Cool, I'll have to try that! Does it also pull PR's that live on feature branches in forks of the root repo?

Re: Merge Pull Request Considered Harmful

#42

Post by nirvdrum » Thu, Jun 26, 2014, 1:39 PM UTC

I'm not sure how I feel about someone writing something and making me the author. I guess attribution makes sense in the form of paraphrasing. But I tend to think of commits as literal quotations.

Re: Merge Pull Request Considered Harmful

#43

Post by nirvdrum » Thu, Jun 26, 2014, 1:39 PM UTC

I'm not sure how I feel about someone writing something and making me the author. I guess attribution makes sense in the form of paraphrasing. But I tend to think of commits as literal quotations.

Re: Merge Pull Request Considered Harmful

#44

Post by rlpb » Thu, Jun 26, 2014, 1:41 PM UTC

rjbwork wrote: post #34

thinkt4nk wrote: post #23

harmful?

It's a play on the classic Dijkstra letter, "Goto Considered Harmful." Also yes, I think it's harmful. A co-worker and I wanted to use a new up and coming FOSS project that's hosted on GitHub, but we needed a certain killer feature. Said co-worker worked for about 4 weekends in a row, and fully implemented it, with nice abstraction and separation of concerns. The code was then turned down because it was "too seperate…

The right way to do this is to talk to the maintainer about how he wants the feature implemented first. Especially if it involves four weekends of work.

Re: Merge Pull Request Considered Harmful

#45

Post by eevilspock » Thu, Jun 26, 2014, 1:42 PM UTC

nirvdrum wrote: post #42

I'm not sure how I feel about someone writing something and making me the author. I guess attribution makes sense in the form of paraphrasing. But I tend to think of commits as literal quotations.

see ntalbott's comment: https://news.ycombinator.com/item?id=7949456

Re: Merge Pull Request Considered Harmful

#46

Post by peterwwillis » Thu, Jun 26, 2014, 1:42 PM UTC

peff wrote: post #21

ntalbott wrote: post #19

Earlier quoted context omitted.

OP author here... The result is the same, but I've found the `git am` workflow to be much smoother vs. mucking around with remotes. Some of that could be due to the nature of the OSS project I work on - ActiveMerchant - since just in the last month we've had 30+ unique contributors, and most of them have contributed a single change. My general recommendation is to make sure you try out the `git am` flow for a bit, bu…

Keep in mind that `git am` is not identical to pulling the commits. You might be applying the commits on a different base, and certainly your committer field will be different (not to mention any substantive changes you make). As a result, the sha1 of the commits you create will be different than those of the original submitter. This makes life harder for the submitter, because they cannot ask a simple question: were…

Well the commits may be changed during pulling if the maintainer deems it necessary (or a merge just ain't happening cleanly) so it seems like the solution would be to just include the SHAs that were merged in the 'git am' commits. Then the submitter knows what they intended to get pulled got in, which should resolve patch verification, right?

Re: Merge Pull Request Considered Harmful

#47

Post by ntalbott » Thu, Jun 26, 2014, 1:45 PM UTC

watson wrote: post #6

I just tried this on an open pull request we had. Pretty ok experience. For some reason though, there where a merge conflict when running the "hub am -3 " command. In my case was easy to fix, but Github reported the PR to be mergeable, so maybe Github uses a different merge strategy for PR's than "hub am" out of the box?

As I understand it `git am` is actually replaying the series of commits on top of the branch you run it in, whereas a merge commit flattens the commits out and applies them in one go (while keeping full history of the commits begin merged). So it's definitely a different strategy, and you can get conflicts where you wouldn't with a merge commit. As you said, though, they're super easy to fix when they happen. Often when I see a conflict like this it's because there are 52 (OK, exaggerating, a little) commits in the contribution and using the `git am` flow is a huge win since I'm now in a good spot to squash the down to one or two commits.

Re: Merge Pull Request Considered Harmful

#48

Post by ntalbott » Thu, Jun 26, 2014, 1:48 PM UTC

nirvdrum wrote: post #42

I'm not sure how I feel about someone writing something and making me the author. I guess attribution makes sense in the form of paraphrasing. But I tend to think of commits as literal quotations.

You may think of commits as literal quotations of the "Author", but it's important to realize that git doesn't. If you want to know who's ultimately responsible for a commit, you always want to look at the "Committer" since that's who actually signed on the dotted line so to speak.

Re: Merge Pull Request Considered Harmful

#49

Post by eevilspock » Thu, Jun 26, 2014, 1:49 PM UTC

ntalbott wrote: post #40

shadowmint wrote: post #2

Hm... this seems like a very complicated way of saying that github should have a way to merge pull requests into a new branch. ...but it doesn't so you have to: - checkout a local copy - add a remote to the PR - checkout a new branch - merge the PR into your local branch - fix code, merge to master Which is entirely true; it is annoying. The simple solution, though, is to require pull requests to come in a feature br…

Using git remotes and distinct branches per contribution is totally legit, and I still do it sometimes. But, I'm often/usually dealing with contributions that have already been reviewed and don't need a whole feature branch/review within the root repository before inclusion. And for that setup - where it's almost ready - it's so much easier to just `git am` it into master, make necessary tweaks, and push. In general…

What about submitting a PR with your cleanup to the original PR's branch? That person reviews and accepts, then submits a new PR back to you.

I haven't learned git yet so maybe my assumption that the original PR is on a branch/repo clone to which you can submit a PR is incorrect?

Re: Merge Pull Request Considered Harmful

#50

Post by ntalbott » Thu, Jun 26, 2014, 1:52 PM UTC

davedx wrote: post #10

watson wrote: post #6

I just tried this on an open pull request we had. Pretty ok experience. For some reason though, there where a merge conflict when running the "hub am -3 " command. In my case was easy to fix, but Github reported the PR to be mergeable, so maybe Github uses a different merge strategy for PR's than "hub am" out of the box?

Yeah, I don't get this hub thing. When I want to manually merge on a Github project I just follow the GH instructions on the PR page to manually checkout the fork, do my work there, and merge it locally then push. Why do you need another tool to do this?

FWIW hub is even handy for the "add a remote for the fork, check out the fork branch" flow, since it lets you just `git remote add ntalbott` instead of having to find or derive the full url for the remote.