Stream: general

Topic: merge commit chat


centril (Mar 17 2019 at 15:07, on Zulip):

what I don't understand is why we think merge commits are so bad

Jake Goulding (Mar 17 2019 at 15:11, on Zulip):

is why we think merge commits are so bad

There are many blog posts comparing the two strategies. Which of them apply to the Rust project I'm not sure.

davidtwco (Mar 17 2019 at 15:11, on Zulip):

what I don't understand is why we think merge commits are so bad

I don't think merge commits are bad. bors does merging even when the base commit of a branch isn't the same as master and so we have merge commits in tree. But when that fails, isn't a rebase the only option? If we decided to instead merge your changes with the new master, then I don't think it could then be automatically merged into master when it changes again (but doesn't cause a conflict this time), or could it? I'm struggling to work it out in my head.

davidtwco (Mar 17 2019 at 15:12, on Zulip):

Or does it always do fast-forward merging? It doesn't.

davidtwco (Mar 17 2019 at 15:12, on Zulip):

I think I've thoroughly confused myself.

centril (Mar 17 2019 at 15:12, on Zulip):

Let's have eternaleye write up a document wrt. why we do the things we do ^^

centril (Mar 17 2019 at 15:13, on Zulip):

I do know that merge commits exists in the history which are not due to bors but in the PRs themselves

Jake Goulding (Mar 17 2019 at 15:16, on Zulip):

For my own projects, I prefer rebasing for a cleaner (more linear) git history. I also tend to perform an interactive rebase and then execute the linter / tests for every commit, so the fact that I've rewritten history is moot.

centril (Mar 17 2019 at 15:17, on Zulip):

@Jake Goulding does that mean that all your commits can be built?

Jake Goulding (Mar 17 2019 at 15:17, on Zulip):

In some technical sense, merge commits are better because they preserve the original history, as most people don't ensure that tests pass (or even that code compiles) for every commit, much less after rebasing

centril (Mar 17 2019 at 15:18, on Zulip):

(I find that encourages large commits myself)

Jake Goulding (Mar 17 2019 at 15:18, on Zulip):

Yes, in the vast majority of cases. This greatly aids with the ability to bisect.

Jake Goulding (Mar 17 2019 at 15:19, on Zulip):

I do not, but do think it takes effort. It's like writing; editing is as equally important as the content generation.

centril (Mar 17 2019 at 15:19, on Zulip):

Ah; true. @Jake Goulding but one could ostensibly mark commits with "this is test-worthy"

Jake Goulding (Mar 17 2019 at 15:19, on Zulip):

I don't think such is applicable because you have to have knowledge of the future to do the marking.

davidtwco (Mar 17 2019 at 15:20, on Zulip):

That's essentially what we do now with the merge commits from bors.

Jake Goulding (Mar 17 2019 at 15:21, on Zulip):

Yep; and again that's a valid position to take. But, that's effectively the same as "giant commits" as now instead of "it's in this 1000 line commit" you have "it's in this 1000 line merge with 3 commits, none of which compile/pass tests except the last"

centril (Mar 17 2019 at 15:21, on Zulip):

Yea, was thinking of that...

@Jake Goulding hmm... why not... you do a few commits that are small and then eventually you run the tests and it works, and then you do a special bisectable commit

davidtwco (Mar 17 2019 at 15:22, on Zulip):

Yep; and again that's a valid position to take. But, that's effectively the same as "giant commits" as now instead of "it's in this 1000 line commit" you have "it's in this 1000 line merge with 3 commits, none of which compile"

Those three commits can make it easier to review though. You can logically segment a 1000 line diff into three changes, each with their own messages and rationale.

Jake Goulding (Mar 17 2019 at 15:23, on Zulip):

easier to review

Except that you often have to have some knowledge of the three commits forward and back because they don't stand on their own.

centril (Mar 17 2019 at 15:24, on Zulip):

Another thing I like about it is that taking "snapshots" becomes easier with many commits; less need for big amounts of ctrl+z

Jake Goulding (Mar 17 2019 at 15:24, on Zulip):

few commits that are small and then eventually you run the tests and it works, and then you do a special bisectable commit

And then squash them to have a single commit ;-)

centril (Mar 17 2019 at 15:25, on Zulip):

^^

Jake Goulding (Mar 17 2019 at 15:25, on Zulip):

Again, this fits into the editing analogy. To be clear, I do 10s of commits all the time for local development

centril (Mar 17 2019 at 15:26, on Zulip):

@Jake Goulding I should do this more often, it could seriously improve my rust workflow

Jake Goulding (Mar 17 2019 at 15:26, on Zulip):

I reorder, tweak, fix, etc. When I'm "done", I organize, squash, split into multiple PRs, etc. I switch from a mindset of good for me to "good for the reviewer". I'll even identify a potential reviewer and show them the unedited branch and ask how they might like to review it.

centril (Mar 17 2019 at 15:27, on Zulip):

@Jake Goulding sshhh :P you make me look too bad :D

Jake Goulding (Mar 17 2019 at 15:27, on Zulip):

I do find that starting with "too small" commits is easier than "too large", but I've learned to handle both cases.

Jake Goulding (Mar 17 2019 at 15:28, on Zulip):

Ah, nah. It's not like I do this all the time. A typo does not require such. ;-)

davidtwco (Mar 17 2019 at 15:28, on Zulip):

I like that approach. It's what I try to do, I'm mostly playing devil's advocate w/r/t merge commits.

davidtwco (Mar 17 2019 at 15:28, on Zulip):

I'm going to split this off into a topic or two since it's pretty unrelated to the original policy discussion.

Jake Goulding (Mar 17 2019 at 15:30, on Zulip):

I'm a big fan of editing {Stack Overflow, Zulip chats, git commit history} to help future people :wink:

davidtwco (Mar 17 2019 at 15:30, on Zulip):

I'd like it if Zulip said "this was forked from ..." so there's more context to the initial messages though.

centril (Mar 17 2019 at 15:31, on Zulip):

oh it doesn't? (PS: time for a new topic)

centril (Mar 17 2019 at 15:31, on Zulip):

(in zulip)

davidtwco (Mar 17 2019 at 15:32, on Zulip):

(in zulip)

Can't move between streams. This discussion about forking discussions does make sense to fork, but then it'll start with a message that says "I'm going to fork this", which is even more confusing.

centril (Mar 17 2019 at 15:33, on Zulip):

@davidtwco how many levels of meta is that?

davidtwco (Mar 17 2019 at 15:33, on Zulip):

Way too many.

Pietro Albini (Mar 17 2019 at 15:33, on Zulip):

btw backporting merge commits is way more painful than it needs to be

Jake Goulding (Mar 17 2019 at 15:35, on Zulip):

@Pietro Albini even the bors merge commits?

Pietro Albini (Mar 17 2019 at 15:35, on Zulip):

we don't usually backport those :P

Pietro Albini (Mar 17 2019 at 15:36, on Zulip):

most of the beta branch is just cherry-picked commits

Jake Goulding (Mar 17 2019 at 15:36, on Zulip):

It's just the original authors commit(s) then?

Pietro Albini (Mar 17 2019 at 15:36, on Zulip):

yep

Pietro Albini (Mar 17 2019 at 15:36, on Zulip):

for example https://github.com/rust-lang/rust/pull/59235

Pietro Albini (Mar 17 2019 at 15:37, on Zulip):

and sometimes a standalone commit made by me that fixes differences between master and beta

Pietro Albini (Mar 17 2019 at 15:38, on Zulip):

but if I cherry-pick a merge commit I might pull in unwanted changes

Vadim Petrochenkov (Mar 17 2019 at 18:22, on Zulip):

I worked with rebase-based workflows not only in rust, but also on all jobs I had.
So I'm not actually sure how you can live with merge alone.

Vadim Petrochenkov (Mar 17 2019 at 18:23, on Zulip):

Suppose you have some local work and master went ahead, and you need to sync, and there is some conflict, and you make merge and have to create a merge commit.

Vadim Petrochenkov (Mar 17 2019 at 18:24, on Zulip):

If you continue the work for some time you need multiple syncs (e.g. daily), and you have to have multiple merge commits in the history? That's awful!

Vadim Petrochenkov (Mar 17 2019 at 18:30, on Zulip):

So, the result of the work when you submit it is not an "incapsulated unit" (a sequence of commits representing the work), but something else.
I'm not actually sure how you can cherry-pick it (or parts of it) somewhere else if necessary in this case, or perform a similar operation.

Jake Goulding (Mar 17 2019 at 18:51, on Zulip):

@Vadim Petrochenkov I'm also a fan of rebase, but I can still point out that the merge-based result (yes, with all of those "extra" merge commits) is more technically accurate to what the developer did (see my point above about running the tests at each step of history after a rebase)

Jake Goulding (Mar 17 2019 at 18:59, on Zulip):

Cherry-picking should have more-or-less the same problems as a rebase strategy — in both cases you are trying to transplant code that assumes some state of the universe into a different universe.

Björn Steinbrink (Mar 19 2019 at 09:20, on Zulip):

Having a accurate history of what a developer did is often just not that useful if you, for example, try to figure out why a piece of code came to be, because blame and log will give you a bunch of commits that show you that the developer had intermediate bugs here and there, and typos and what not, but you'll still have to figure out the big picture yourself, which is something that the original developer (hopefully) already did back then, but didn't care to make available to others in form of a clean history with well written commit messages.

Björn Steinbrink (Mar 19 2019 at 09:22, on Zulip):

I still remember hunting down a bug (not I. Rust), where blame only led me to a commit that had nothing but a poop emoji as its commit message, and the changed line went from buggy to buggy there. The PR was littered with such commits, no fun at all.

pnkfelix (Mar 19 2019 at 10:38, on Zulip):

yes: history crafting is as much an documentation-art as comment authoring is

Björn Steinbrink (Mar 19 2019 at 11:33, on Zulip):

I still remember hunting down a bug (not I. Rust)

I meant "not in Rust" there.

Last update: Nov 20 2019 at 13:15UTC