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

101–110 of 115 posts

Re: Merge Pull Request Considered Harmful

#101

Post by rjbwork »

al2o3cr wrote: post #35
rjbwork wrote: post #34

Earlier quoted context omitted.

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…

Translation: "My co-worker wrote a clusterfuck of indirection spaghetti and the open-source volunteer refused to maintain it!" Every PR that adds a feature adds future support and maintenance effort. If you and your buddy are unwilling to spend the time to get it right, why are you expecting the maintainer to spend the time to support it?

In short, it was a perfect implementation of adding support for dynamically updating scripts via NuGet for a C# port of HuBot by typing " update-scripts". This is the standard way of managing dependencies, and would mean not having to manually upload scripts to a folder restart the bot every time you need to add or change the scripts.

The maintainer shortly thereafter attempted to do it himself his way, ended up with a bug-ridden and memory-leaking implementation that barely worked, and sits languishing in a lonely branch.

Don't make assumptions about the code you didn't see and the attitudes involved coders might have. We would have been (and still would be) more than happy to help maintain it had the bot turned out useful for us, but because it was a new project and the structure was changing constantly, attempting to fork and integrate upstream changes would have been a nightmare, so we decided to go a different direction.

Re: Merge Pull Request Considered Harmful

#102

Post by jessaustin »

gknoy wrote: post #80
jessaustin wrote: post #54

Earlier quoted context omitted.

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…

Good point; thanks for the info! Under the hypothesis that Bob is lazy, however, he still might not perform that merge...

Re: Merge Pull Request Considered Harmful

#103

Post by [deleted] »

tytso wrote: post #39
ulisesrmzroche wrote: post #33

Earlier quoted context omitted.

That's cuz Linus Torvalds doesn't have much of a bedside manner. I agree with ya'll on the quality commit message stuff, but that last third was all conjecture from your part, homey.

The last third is based on my extensive opinion having worked on open source software for my entire career, including being the ext4 subsystem maintainer and the e2fsprogs author and maintainer.

[deleted]

Re: Merge Pull Request Considered Harmful

#104

Post by IanCal »

npsimons wrote: post #86
ForHackernews wrote: post #58

Earlier 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 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 wan…

> I want each commit to compile and pass tests at a minimum, so that I can do automated git bisects.

What stops you from doing automated git bisects when some commits shouldn't be tested? Returning a 125 from your script if it doesn't compile will skip that commit. When it comes to GH you can skip any commit where the message doesn't start with "Merge pull request #" for example.

Re: Merge Pull Request Considered Harmful

#105

Post by IanCal »

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").

Why does it make it harder?

> The commit you end up with or N prior commits which don't build (and which is incidentally usually full of "noise").

The alternative, where things are squashed together, would leave you with just one commit but it'd be those N combined together (at least, it could well be more).

Re: Merge Pull Request Considered Harmful

#106

Post by IanCal »

npsimons wrote: post #88
Alupis wrote: post #77

Earlier quoted context omitted.

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.

I don't understand the bisect argument, maybe I'm missing something but can't you just skip everything that doesn't build and pass the tests? Returning a 125 from your script automatically skips the commit.

Re: Merge Pull Request Considered Harmful

#107

Post by IanCal »

owenversteeg wrote: post #90

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…

> This whole process would've been a lot simpler if Github allowed people to merge to another branch.

Is this a problem that comes from forking? I raise PRs on my projects onto various different branches all the time. Or maybe I've misunderstood the issue.

Re: Merge Pull Request Considered Harmful

#108

Post by fidlefodl »

dr4g0n wrote: post #7
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…

You can create a new branch containing the pull request fairly easily.[1] It's just: git fetch origin pull/ID/head:BRANCHNAME git checkout BRANCHNAME Edit: I see that MaikuMori[2] posted the same information just before me; ah well. [1]: https://help.github.com/articles/checking-out-pull-requests-... [2]: https://news.ycombinator.com/item?id=7949107

I appreciate it! I had no idea that this was possible.

Re: Merge Pull Request Considered Harmful

#109

Post by npsimons »

IanCal wrote: post #106
npsimons wrote: post #88

Earlier quoted context omitted.

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.

I don't understand the bisect argument, maybe I'm missing something but can't you just skip everything that doesn't build and pass the tests? Returning a 125 from your script automatically skips the commit.

And how do you know that wasn't the commit you were looking for? The whole point of bisect is that you don't have to do a manual bug hunt; you can create a script that tests for the thing you are looking for. If it's something static sure, that's easy. But if it's something more complicated and you don't know what's causing it (a prime candidate for bisect), you will need to build and analyze the behavior of the built executable. Non-building commit = more manual bughunting.

Re: Merge Pull Request Considered Harmful

#110

Post by npsimons »

IanCal wrote: post #104
npsimons wrote: post #86

Earlier quoted context omitted.

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 wan…

> I want each commit to compile and pass tests at a minimum, so that I can do automated git bisects. What stops you from doing automated git bisects when some commits shouldn't be tested? Returning a 125 from your script if it doesn't compile will skip that commit. When it comes to GH you can skip any commit where the message doesn't start with "Merge pull request #" for example.

And how do you know that wasn't the commit where the bug was introduced? You check it by hand? Great, you've now defeated the purpose of git bisect.
Post reply on HN