"If the contribution is a single commit, I’ll do a git commit --amend and just smoosh my changes right into the original commit."
that is right, editing history is the way to go, or is it? :)
Merge Pull Request Considered Harmful
71–80 of 115 posts
Re: Merge Pull Request Considered Harmful
#72Earlier quoted context omitted.
So to be clear, every commit in the ActiveMerchant history also points back at the initiating PR via the commit message including the PR number (which gets linked up). So there's no real loss of information in that sense And I hear your RE adding another tool, as I'm very cautious about that myself. That said, `git am` is "git cannon", and all hub is really adding is the ability for it to slurp Github urls as well as…
> 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 that in multiple, logical commits, and it may even be me that ends up doing the splitting.
I'm sure a lot of it just has to do with what types of contributions a given OSS project receives, so YMMV.
Re: Merge Pull Request Considered Harmful
#73I 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…
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 state, Linus’s answer is actually ignore it.
Re: Merge Pull Request Considered Harmful
#74I 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…
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. ;)
Well, quoting from that message, "If it's not ready, you send patches around". I think the big difference is that I'm willing to take patches that aren't ready and get them ready, whereas I'm sure Linus usually pushes back on the contributor to clean up their stuff. And I do think merges are super useful in a lot of cases, but Linus added `git am` too, and I like using all the tools at my disposal when they fit.
Re: Merge Pull Request Considered Harmful
#75I 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.
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.
Re: Merge Pull Request Considered Harmful
#76I wish github would either detect when a branch is merged back into master and automatically close the PR, or allow for some more complex merge operation. Squash merge would be particularly helpful.
Re: Merge Pull Request Considered Harmful
#77Earlier quoted context omitted.
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.
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.
Re: Merge Pull Request Considered Harmful
#78Earlier quoted context omitted.
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 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.
Re: Merge Pull Request Considered Harmful
#79Earlier quoted context omitted.
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 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?
[deleted]
Re: Merge Pull Request Considered Harmful
#80Earlier quoted context omitted.
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?
You totally could do that, but it doesn't really help with the lack of interest problem he described. If the contributor can't be arsed to fix their patch, can we expect them to be arsed to merge a patch to their patch and then re-PR it?
Since PRs on Github auto-update when the branch they are based on is updated, they would not need to create a new pull request:
Bob requests that Alice merge bob/bob-feature into alice/master.
Alice requests that Bob merge alice/fix-bob-to-conform-to-pep8 into bob/bob-feature.
Bob merges that into bob/bob-feature, and Bob's PR into Alice's repo is now auto-updated to reflect the changes that he merged into his branch.
Alice is now satisfied, and merges bob/bob-feature with alice/master.