Isn't this sort of solved by rebasing first instead of just merging? (well, and some rules in place which tells people 'we only accept properly rebased branches for review). Then the conflicts are resolved in the branch and there's a clear view of what actually changed in the diff, before anything is effectively merged into the main branch. Or maybe I'm missing something here.

It's absolutely resolved that way.

Rebase should be the weapon of choice in all cases, except those where it would be prohibitively expensive.

People are too enamored with the SVN ways though, or have even made up rationalizations about fictional concepts such as "historical purity". So you have a ton of people merging instead, producing code repositories that look worse than the worst perl code i've touched.

> People have even made up rationalizations about fictional concepts such as "historical purity".

The idea behind not rewriting history is not to gain some kind of academic pleasure from purity. It's to make sure that the commits being committed are actually the same ones that you actually wrote and (hopefully) found to be working, so that when a bug makes you refer to the commit later, you (a) don't start scratching your head as to why it didn't do what you thought you saw it do yesterday, and (b) can actually refer to it to figure out what the correct code should've looked like.

If you don't see what I mean, here's an example. Say you have

  foo():
      foo1()
      foo2()
      foo4()
      foo5()

and you turned it into this:

  foo():
      foo1()
      foo2()
      foo4()
      foo5()
      foo6()

Now now if someone has in the meantime done this:

  foo():
      foo1()
      foo2()
      if (!foo3()): return 0
      foo4()
      foo5()

then after a rebase you will end up with

  foo():
      foo1()
      foo2()
      if (!foo3()): return 0
      foo4()
      foo5()
      foo6()

which is not code that you ever had, and which is likely to fail because your assumption that foo6() will always be hit will no longer be valid.

With a rebase, not only will this be confusing when you look back and make you blame the wrong person or lead you in the wrong direction, but it will also remove the option of rolling back to the latest of either of the two branches that you combined. You'll only be able to revert to the "known-good" branch that was rebased onto, not the one that was rebased.

Oh, and did I mention you very likely just lost the working (pre-rebase) version of your code entirely? Have fun digging it out of your reflog if you notice it quickly enough, and good luck finding a copy if you do so too late. You'll have probably deleted the branch by now, and chances are nobody else will have a copy lying around for no reason either. After some period of time you'll have no idea what the working code looked like, because the only version you have is the one that already incorporated the branch you rebased onto.

Life sucks sometimes but it doesn't matter whether you merge or rebase in the scenario you describe, you need to test the combined version of the code. You can't just assume it is still working and whoever comes second needs to shoulder that responsibility.

> Life sucks sometimes but it doesn't matter whether you merge or rebase in the scenario you describe, you need to test the combined version of the code. You can't just assume it is still working and whoever comes second needs to shoulder that responsibility.

Sure you do. But you're missing the fact that testing is in no way even remotely guaranteed to catch every error. If it did then projects that have tests would never have bugs, and that doesn't seem to even approximate the world we live in. (Why did I need to point this out though? Is this not obvious?)

Sorry for being a bit vague but I didn't mean to imply there were tests. I meant test in the sense of "do whatever you did to check correctness" in the first place. You change the code, you should do that process again or at least some approximation of it. Automated tests make it pretty easy, manual testing makes it doable, stepping through with a debugger or dumping printing statements makes it painful and error prone.

Many codebases don't have tests at all, most have terrible tests, and under no circumstances do I think they magically prevent all bugs. But I do think obnoxious coders who assume they can just merge and move on without checking the correctness have a special place in hell.

> I meant test in the sense of "do whatever you did to check correctness" in the first place.

Sure. I already responded to this:

>> But you're missing the fact that testing is in no way even remotely guaranteed to catch every error.

Obviously you have to test the combination but the merging allows an easier rollback.

Easier? How so? If a rebase of a branch isn't successful you just reset back to the commit you rebased onto, and continue work on your old pre-rebase branch.

Your example is in fact a good example why you should always rebase before merge. Only then would the reviewer have a chance to see the (potential) problem before it is merged.

I'm not sure I understand why rebasing would confuse blame here. Have you tried it?

If you rebase your "foo6" change on top of the "!foo3" change, the state of your branch will be exactly the state of your code base going forward. And that is the useful history to keep, not the order in which the actual keys were pressed.

But if the idea is that I want `foo6()` etc to always be executed, wouldn't I just handle the rebase from master by resolving the conflict as follows, instead?

  foo():
      foo1()
      foo2()
      foo4()
      foo5()
      foo6()
      if (!foo3()): return 0

> wouldn't I just handle the rebase from master by resolving the conflict

(a) No, because there is no conflict. It gets merged "cleanly" without you realizing it. (Let me emphasize this: merge is NOT guaranteed give a correct result if it happens cleanly. This is just one example.)

(b) No, obviously in general doing foo3() at the end will not give the same result as doing it in the middle.

It would be nice if git kept both versions of the history around. One with work in progress commits, corrections from review, and merges. The other a clean rebased history for usual history browsing.

You can do something like that easily by keeping multiple branches around. I do that sometimes when I'm doing some major rebase/cleanup of commits spanning more than a couple of days. Just so afterwards I can compare what I end up with with what I had originally.

Conversely, people get too enamored of rebasing as a way to solve the problem of merging in lots of junky commits.

There are lots of ways to rewrite history and clean it up. Rebasing is just one of them, and it's one that can force you to spend more time resolving conflicts than you otherwise might have to (since you have to replay all of your commits and resolve any conflicts caused by each one individually).

There's no reason why rebasing is a better way to clean up your history than squash merging (or even -- carefully -- using git reset).

I already said that it can be too expensive, but thanks for playing. <3

Also, a squash merge is not a merge. It's just another form of rebase.

And if you mention reset as an alternative to cherry pick and squash you should also mention --soft.

My point was that many git users don't understand git all that well, and I think that encouraging everyone to rebase is a bad idea.

If you're going to promote a particular workflow as the the one that "should be the weapon of choice," I think the workflow of merge-master-into-feature-branch, squash-merge-feature-branch-into-master is a more user friendly alternative, since then the cases where rebasing is expensive never even come up.

And yes, you're right that I was remiss not to mention the --soft part. The git reset workflow is one that I would definitely not encourage people to do if they don't understand git well.

> Rebase should be the weapon of choice [....] People are too enamored with the SVN ways though

I'd expect SVN users to be quite comfortable with rebasing. `git pull --rebase` is effectively the same thing as `svn update`. You just couldn't commit unfinished work when using SVN, so the changes were loose in the working tree, rather than stored in local commits.

I don't think you need to use rebase solve this issue at all. All you need to do is to make sure is that every commit on master, is on your feature branch, which you can still do with merging. Something like:

    git checkout my-branch
    git merge master

Whether you use rebase or merge is a completely orthogonal issue.

Maybe I'm misunderstanding here. Who is going to run `git merge master`? If it's not you, the maintainer, then you need to trust that the submitter didn't pull any funny business with that command. With rebasing, you can just confirm that there are no merge commits.

Too many experiences like that I make merge commits a required part of a code review. If a branch needs conflict resolution in order to merge, it needs that merge in that branch, and as part of the code review of that branch. (Some of the best conflict resolution is sometimes new code in light of the changes in the other branch, and so of course should be reviewed.)

If a merge commit diff is showing too much information, that's more often than not a tooling problem. Most tools provide ways to filter out the "automated stuff" and focus only on the conflict resolutions. If they don't, they are deficient (and if there are too many conflict resolutions to easily review, that may be a sign of a larger process problem). (GitHub has gotten much better at merge commit views in the last couple of years, as one anecdotal bullet point.)

> Too many experiences like that I make merge commits a required part of a code review.

How can you do this practically though? In the turnaround time to do the code review and make the changes, a new conflict might have been introduced into the branch being targeted by the merge.

I think merging e.g. the master branch into your branch before code review is a good idea though as when the review is complete there's going to be less or no conflicts merging into master.

That's also a tooling issue I believe. GitHub, again, as easy example, has gotten pretty good at "these are the commits since your last review" incremental reviews, so if you complete a code review and need conflicts to be fixed before it can merge, you expect a merge commit to be made in that branch and should just need to review that merge commit.

I've not recently seen an issue with a PR cycling through a lot of merge commits to keep up with the branch it is targeting, but in almost every case where I've seen that there was some other process decision in the way (two people working on the exact same feature simultaneously) and at that point it was an architectural, "political", or "social" problem to fix first, outside of the code review process. Your mileage may vary, of course.

What I personally do is commit the merge as is, with all the <<< === >>> conflict markers.

Then, in the next couple commits, fix each conflict via a normal commit that mixes and matches "mine" and "theirs" as needed + deletes the markers.

Each resolution commit is independent of the giant merge commit, and can be separately diff'ed and code reviewed via the github PR or any other tool.

Also easier IMO to make the complicated resolution you want, or reset back to a previous resolution attempt without messing up the entire merge.

The drawback is that some commits won't compile, but compilable mid-branch commits are not something that I view as a goal.

Non-compiling commits make bisecting harder, when the goal is to find the commit that broke the build or a specific test. IMHO having to skip over non-compiling merge commits only makes bisecting take more time, and it may also "pollute" the test pipelines if all commits are going to be built. Additionally, merging and fixing the merge in separate commits makes it much hard to revert a merge later, in case it turned out that the merge introduced something unwanted).

So I will always recommend doing the merge and fixing the merge in the same commit. Ideally this produces a squash commit that can easily be reverted later when needed. This makes the merge process more time-consuming, but it can help to keep the builds more stable, and to more easily track which commit (merge) introduced a particular problem.

> makes it much hard to revert a merge later

I don't think this would apply, as I'd be merging from `dev` > `feature`, resolving conflicts, and then doing a single clean no-conflict merge back from `feature` > `dev`. That's the only commit you'd need to revert to undo the merge.

(or on a new, third, `merge` branch, if you don't want to pollute the `feature` branch with commits from `dev`)

I admit the non-compiling commits might be a problem in some workflows, but `git bisect skip` is the solution to the only problem I've ever had with it.

It's kind of a problem if the compile/build step takes rather long. Then you want to avoid having any additional non-compiling revision to check when bisecting.

But I agree in general, if the compile/build turn-around times allow for it, then a single no-conflict squash merge back into devel is definitely a very good approach to have clean and compiling commits, and for identifying the commits/merges that break the builds.

I upvoted you for the sheer uniqueness of that workflow (almost a decade using git, never seen anything like that), but I think if I ever had to work on a git tree that did that I would throw a fit ...

There is so much wrong with that approach. Kills bisects, kills diffs, kills signed commits, kills merge automation.. Yikes.

I forgot to mention that the conflict resolution commits would be occurring on the feature branch (or a 3rd branch), not on the main dev branch, if that makes it easier to grok.

Anything merged into `dev` would be a single clean no-conflict merge commit that compiles. That eliminates any merge automation issues I've encountered, although other systems might require every commit to compile.

`git bisect skip` fixes bisect, just like any other non-compilable commit on a branch.

You say it "kills diffs", and that may be true for some aspects of a command-line diff. But I find the main place my team is "diffing" is via the web view of a Pull Request. This workflow makes it possible to see the diff via the PR. Each commit shows you exactly how the conflict was resolved. Without this workflow the conflict resolutions are essentially hidden from the web view of the PR and leads to the "hidden code" issue I mentioned in the OP. (Although some newer PR views may now make this possible.)

That sounds horrible, squash -> rebase -> fast-forward merge... Much more easier to reason about the commits/changes.

Git bisect is really important in some cases and that needs all/most commits to be compilable..., especially big merge commits.

Unless you have a special reason to preserve all commits as they were made (eg. signed commits), in that case go with the linux model of small single responsibility merges that preferably have no conflicts...

GitLab makes it really easy to see what changed between commits while reviewing a merge request. Parts of the diff that are identical to the upstream branch are automatically ignored, and you can view a diff between of last two pushes.

https://docs.gitlab.com/ce/user/project/merge_requests/versi...

Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity.

> Edit: GitHub has similar functionality, but it doesn't know how to diff force pushes, so you would end up having to review the full diff again to ensure integrity.

I realize this is a religious war I'm stepping into, but I fall on the side of "if you are doing a force push of a branch already in code review you are doing something wrong". Once commits are out in the open and in review between multiple developers, that is shared code history that encodes important information about the code review process itself. (I make obvious exceptions for things like accidental PII disclosure or similar bone-headed mistakes, but the longer a branch has been in code review, the less willing I am to allow it to be force pushed.)

Of course, to each their own and your mileage may vary, but that comment about diffing force pushes in a merge/pull request sent a shiver down my spine, regardless of whether or not the tool supports it.

We often have to rebase our feature branches before they can be merged, so changing the history is common and force pushes are inevitable. As a rule I don't base any work on top of unmerged feature branches, because there is no expectation of stability.

I like the commit history to form a narrative of meaningful changes. I don't think commit messages about fixing errors that were introduced in the feature branch's previous commits belong in the history.

I like the ability to drill down into the details of why a change was made, the narrative of a PR and the back and forth flow can be hugely useful. The git DAG can be a very powerful way to encode both the high level narrative and the low level details.

The `--first-parent` option to git log and git annotate lets you stick to that high level narrative of just PRs/features that changed. (More tools should offer that as an option, it's probably my biggest request for GitHub's commit view, personally.)

It's a DAG for a reason, and it's powerful to take advantage of that, spending a lot of work (and potentially building a lot of footguns) forcing it to be a straight line seems counter-productive to me.

Phabricator is really nice here. The history of the code review process is maintained by Phab itself, you can amend all you want against an inflight code review, and upon landing it all becomes a single commit in master with a link to the code review in the commit message.

You’d hate our team—we force push to master (not just feature branches) all the time. It seems like anarchy at first but it’s not really that hard to deal with. Our situation is a bit unique though, and I doubt it would scale well beyond a small team.

Yes, that sounds like you are trying to maximize footguns to me. I do hope for your sake that no one manages to trigger one. (…and I'm personally very glad at this point with GitHub supporting branch restrictions and the ability to disable force pushes for master.)

That's why I say "you have conflicts" and have them resolve the conflict before I do the review. GitHub, at least, correctly diffs against master after the conflicts are resolved so you don't see any of the merged code (if it's the same as on the master branch), only the actual changes.

Doesn't that solve the problem?

That is scary. I think it would be a nifty feature for a code review tool to show only the conflict resolution for merge commits. (Although I suppose it would need knowledge of exactly which automated merge method/options were used to create the pre-CR merge, for any given commit with multiple parents.)