While #1785 is closed, this should actually also better address some of the issues that came up there. I wonder if it may have some impact on #2205 as well.

LGTM, but you need to pass the tests ;) remove action-required when fixed.

Please squash into a single commit before merge! Having individual commits that don't build is somewhere between annoying and painful if one ever needs to do "git bitsect".

@ezyang is that standard cabal practice here? I think i've seen branches that had unsquished bits.

(also, I guess you can remove the action-required tag -- I don't have the rights to do so)

Even if it isn't standard practice I'll argue that it should be done in this particular case, regardless. Basically: Having a commit like "whoops, ..." separate from the "main" commit adds nothing of any value to anyone who's going to be looking back through the history or using "git annotate". The commit history should be meaningful to humans.

Of course, if there are logically separate commits which each add value on their own, then they shouldn't just be blindly squashed.

What I'm basically saying is that the commits should be "curated" such that they form a logical progression for human reviewer, each one of them builds individually and they don't needlessly clutter up history with "wip" or "whoops" or "blah" commits. Believe me, you wouldn't ever want to see the un-curated commit history of any of my feature branches -- they usually go into the tens or hundres of "wip" commits :). This also means that you can commit willy-nilly to your topic branch while you're working on it and clean up one you're ready for final review+merge.

This has been standard practice on every single git-based project I've ever worked on and it works really well. (Especially since GitHub actually supports rebase+force-push to a pull request branch quite well.)

@gbaz

is that standard cabal practice here? I think i've seen branches that had unsquished bits.

Not in general case, but sometimes I ask people to squish/rebase to make code easier to review. And @BardurArantsson is right about git bisect.

@BardurArantsson

Having individual commits that don't build is somewhere between annoying and painful if one ever needs to do "git bitsect".

I agree, but unfortunately it's the reality that one has to live with. Use bisect skip or return 125 from your bisect run script.

This one should be squashed. There's ~no reason not to, it's really small.

With larger sets I'm more sympathetic to not squashing.

Sorry, didn't want to derail. I'll be quiet after this post.

@ezyang : Agreed about larger sets. My criterion is spelled out here

Of course, if there are logically separate commits which each add value on their own, then they shouldn't just be blindly squashed.

... and it's one that I think makes pretty good sense even though it usually requires a judgement call about "adds value" and "logically separate".

Ok, let's sort out this last issue and I'm happy to squash -- just wanted to know what the standard practice is here (I know some people run relatively squash-free repos on principle, etc).

So on this last issue, I'm trying to figure out what is being recommended. You're suggesting I don't step on the whole env, but only the PATH and CFLAGS variables I guess? Note that we still will have certain clunkiness here. We want to change the PATH and CFLAGS variables, not just step on them willy-nilly. With PATH there's a helper function used elsewhere, but CFLAGS we still have to fish it out of the env and alter it -- it is just the case that we won't clobber the parts of the env that stay the same -- so the net effect is maybe slightly more "precise" code but the cost will imho be further verbosity and less of a "wholemeal" character.

That said, I'll go ahead and refactor the commit as so. We can review it on this branch, and then I guess I'll need to make a new PR when I squash?

We can review it on this branch, and then I guess I'll need to make a new PR when I squash?

No, you can just do push -f and this PR will be updated in-place.

Ok, refactored and squashed. The refactor ended up reducing the linecount, so actually does feel cleaner overall :-)

LGTM.

Probably could be simplified still further by extending getEffectiveEnvironment with support for appending/prepending (right now it just replaces) and using that instead of getEnvironment/lookup/maybe, but that's not a requirement for merging.

Merged, thanks!