Merge Pull Request Considered Harmful • Hacker News Board

Re: Merge Pull Request Considered Harmful

#111

Post by IanCal » Fri, Jun 27, 2014, 8:34 PM UTC

npsimons wrote: post #109

IanCal wrote: post #106

Earlier quoted context omitted.

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

But aren't we talking about squashing those commits? You're not going to lose anything by skipping commits between PRs instead of squashing everything between PRs into one commit, you can only narrow it down more.

> you can create a script that tests for the thing you are looking for.

Yes, which you can tell to skip anything that doesn't compile/pass tests. Return 125 from your script and it'll be skipped.

> Non-building commit = more manual bughunting.

But you can just do a loop of "make || git skip" and skip everything that doesn't commit, surely. Or if you're using a script, just return 125 for things you want to skip.

Re: Merge Pull Request Considered Harmful

#112

Post by lmm » Sat, Jun 28, 2014, 7:52 AM UTC

josephlord wrote: post #84

lomnakkus wrote: post #83

Earlier quoted context omitted.

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.

> At worst the intermediate commits gives you the same problem you would have if you had squashed to start with.

Well, if you're going to be bisecting a lot or compilation takes a while, it saves you time to do the squash now (when you know which commits are supposed to compile and which aren't) rather than when you come back to it.

Re: Merge Pull Request Considered Harmful

#113

Post by IanCal » Sat, Jun 28, 2014, 12:01 PM UTC

npsimons wrote: post #110

IanCal wrote: post #104

Earlier quoted context omitted.

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

If you're squashing commits together and only leaving chunks that compile & pass the tests, what are you losing by skipping commits that don't compile & pass the tests?

Can you come up with an concrete example of where this would be a problem? I might be missing something but I can't picture a case where there'd be an issue.

Re: Merge Pull Request Considered Harmful

#114

Post by IanCal » Sat, Jun 28, 2014, 12:03 PM UTC

npsimons wrote: post #85

ntalbott wrote: post #72

Earlier quoted context omitted.

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

> otherwise how do you do automated bisects?

Return a 125 from your script if the tests fail.

Re: Merge Pull Request Considered Harmful

#115

Post by nirvdrum » Sat, Jun 28, 2014, 4:08 PM UTC

ntalbott wrote: post #48

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.

I'm not concerned with who's ultimately responsible for the commit. I'm concerned with being called the author on something I didn't write. These distinctions are not made or enforced by git. At the end of the day, it's how humans interpret two pieces of metadata.

The first hit on Google and Bing for "git committer vs author" brings up this SO entry:

http://stackoverflow.com/questions/18750808/difference-betwe...

So, I don't think I'm taking some fringe view here. For my part, I don't think I want someone else writing code and putting my name on it.