Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Merge Pull Request Considered Harmful

blog.spreedly.com

Post reply on HN
Print view View original

81–90 of 115 posts

Re: Merge Pull Request Considered Harmful

#81

Post by livingparadox »

chrismorgan wrote: post #73
livingparadox wrote: post #68

Earlier quoted context omitted.

Since the author of the article decided to invoke Linus, I thought I'd see what Linus thought about the merge vs rebase debate. http://www.mail-archive.com/dri-devel@lists.sourceforge.net/... Turns out Linus also agrees with drunken_thor, that merge messages are useful. Suppose that's why Linus added merges in the first place. ;)

His answer there on this topic boils down to “tell the patch author that they made a mistake, and steadfastly refuse to do anything with it yourself until the patch author does fix it”. This is the whole point of this article—that is simply not pragmatic; you can have changes made that aren’t quite perfect but where the original user is not willing to make the changes you require. It being abandoned in imperfect stat…

Yeah, I agree Linus takes things too far at times. My response was mostly because the author's "this is the way Linus would have done it" stuff. I do agree that we should be willing to do a little cleanup ourselves when feasible. But I disagree with the author that we should edit /somebody else's commits/.

Re: Merge Pull Request Considered Harmful

#82

Post by lomnakkus »

warfangle wrote: post #78
lmm wrote: post #60

Earlier quoted context omitted.

There's an argument that bisect is only useful if every commit is broken. If, as is often the case with my history, most commits don't compile, maybe there's an argument for squashing?

It might make sense to squash so you only have compile-ready commits in your log. In a sense I work this way, without the squashing. When I write a test, the build fails and nothing has been done on the application codebase so I don't commit. When I fix the build by writing new code, I commit. Maybe I should be doing small intermediate commits and squashing the commits into one commit.

> Maybe I should be doing small intermediate commits and squashing the commits into one commit.

I would recommend trying that, or at least something similar: Try committing regularly (this is useful if you ever want to go back to a prior state while working or e.g. pick up where you left off on a different machine, etc), but reorganizing and cleaning up your topic branch (via rebase -i, etc.) into a few commits before merging (fast forward). What is "a few commits"? Well, it's a bit of a judgement call, but I just group stuff into logicially coherent units which make sense as single units of functionality. Of course you must make sure each individual commit at least builds sensibly and preferably that all tests run too.

Re: Merge Pull Request Considered Harmful

#83

Post by lomnakkus »

josephlord wrote: post #69
lmm wrote: post #60

Earlier quoted context omitted.

There's an argument that bisect is only useful if every commit is broken. If, as is often the case with my history, most commits don't compile, maybe there's an argument for squashing?

You can always `git bisect skip` when you hit a commit that doesn't build or you can't identify whether the bug of interest is present.

This works as long as there are only a few commits which don't build. It fails miserably if, say, 25% of all commits don't build (don't ask). In such a situation it becomes increasingly hard to tell what actually broke whatever functionality you're interested in: The commit you end up with or N prior commits which don't build (and which is incidentally usually full of "noise").

Re: Merge Pull Request Considered Harmful

#84

Post by josephlord »

lomnakkus wrote: post #83
josephlord wrote: post #69

Earlier quoted context omitted.

You can always `git bisect skip` when you hit a commit that doesn't build or you can't identify whether the bug of interest is present.

This works as long as there are only a few commits which don't build. It fails miserably if, say, 25% of all commits don't build (don't ask). In such a situation it becomes increasingly hard to tell what actually broke whatever functionality you're interested in: The commit you end up with or N prior commits which don't build (and which is incidentally usually full of "noise").

You can still diff between last good and first known bad. If necessary you can branch and squash the non builders to see what happened. At worst the intermediate commits gives you the same problem you would have if you had squashed to start with.

Re: Merge Pull Request Considered Harmful

#85

Post by npsimons »

ntalbott wrote: post #72
lelf wrote: post #59

Earlier quoted context omitted.

> So there's no real loss of information in that sense It is in reality. Try to git bisect a bug. (Yes, I read the part about the bisect in the post. But thanks, I'd rather search the bug in several 10-line patches than in one 1000+).

Most of the contributions I'm merging in are 10 lines, but a lot of them (half?) take two or more (sometimes many more) commits to converge on those 10 lines, and none of those intermediate commits has useful information in them. The other half of contributions are only a single commit already. If someone contributes a true 1000+ line contribution that I'm even willing to take (yikes!) then you better believe I want…

MMDV, and for my current project we have code reviews that don't pass and merge unless the automated CI (builds, tests, lints, etc) pass, so if your new feature takes 1000 lines of code, so be it. We do try to keep to the minimum necessary change, though, and focus on tight conceptual chunks. But a patch should never break the tests, especially not to keep it below a certain size, otherwise how do you do automated bisects?

Re: Merge Pull Request Considered Harmful

#86

Post by npsimons »

ForHackernews wrote: post #58
drunken_thor wrote: post #30

I really do not see the problem with the rails commit history. Having those merge messages that point back to a pull request leaves lots of documentation about what was done and why it was done. I really do not know why people are so particular about their git log either. It shows an accurate history of the repo, not a revised cleaned up history. However being able to edit pull request before merging was a good thing…

Yeah, I found the notion of a commit being "history worthy" kind of silly. If that's how it happened, then it's history! It's not a value judgement.

There's an issue here where people are conflating two definitions of "history". Sure, what happened in the real world can't be changed, but why should we be constrained by that in the worlds we construct in software? Just because someone calls a record of development "history" doesn't make it inviolate, and quite frankly, as a maintainer, I don't care about every little sneeze that a developer had on a project. I want each commit to compile and pass tests at a minimum, so that I can do automated git bisects. I would prefer to have a coherent history of well thought out conceptual chunks, so that I can figure out how things are supposed to be, not what some sleep-deprived programmer was thinking at 3AM on a Sunday.

Re: Merge Pull Request Considered Harmful

#88

Post by npsimons »

Alupis wrote: post #77
ntalbott wrote: post #75

Earlier quoted context omitted.

I think a lot of it depends on context. In a private repo I'm right there with you. But when I look at a public repo with a lot of contributors, I want to know what the "logical" history is a lot more than I want to know about the three steps it took to get a feature ready for public consumption.

one of the points of Git was short and small commits, often. it helps prevent a SVN/CVS-style workflow, which will negate many of the other benefits of Git IMHO.

And one of the major benefits of git is that you can do short, small commits often, to your private repo. When your fix or feature add is ready, you can squash them into a more coherent commit that builds and passes tests (so that it doesn't break bisect) and push it to the public repo.

Re: Merge Pull Request Considered Harmful

#90

Post by owenversteeg »

You know, when I started reading this I thought that I would disagree with you and write a grumpy comment to that effect. ("Considered Harmful" usually makes me grumpy.) However, you convinced me otherwise. Congratulations on a good "Considered Harmful" piece!

Here's another example: I recently made a PR to a project to fix a broken URL. I changed a wrong file and forgot about the PR. The maintainer had to close the PR, make the change himself, and explain why/what he did. This whole process would've been a lot simpler if Github allowed people to merge to another branch.

Post reply on HN