doc: Explain squashing with merge commits #25165

MarcoFalke commented at 7:14 AM on May 18, 2022: member

This avoids having to explain it in each thread

MarcoFalke commented at 7:15 AM on May 18, 2022: member

:rotating_light: Warning, this is untested, but tests and review are welcome.

david-bakin commented at 7:27 AM on May 18, 2022: contributor

What's wrong with a git merge --squash which I see is not recommended earlier in this document or in your update? (I really don't know, BTW, which is why I'm asking.)

jonatack commented at 7:29 AM on May 18, 2022: member

-0 on adding further git documentation to the repo, there are other and better resources

jonatack commented at 7:31 AM on May 18, 2022: member

This avoids having to explain it in each thread

AFAICS you use pre-prepared maintainer responses and could add an appropriate link to an external resource where needed.

in CONTRIBUTING.md:220 in faa280a9d8 outdated

 221 | -interface and ask for help in the pull request.
 222 | +If your change contain a merge commit, the above workflow may not work. You
 223 | +have the option to squash everything into *one* commit, or *keep* the commits
 224 | +in a non-interactive rebase.
 225 | +
 226 | +To squash everything into one commit.

laanwj commented at 7:42 AM on May 18, 2022:

I think keeping this part is enough. By far and wide, a new contributor will want to squash everything into one commit. Providing more options may just confuse them.

MarcoFalke commented at 12:31 PM on May 18, 2022:

Thx, fixed.

laanwj commented at 7:44 AM on May 18, 2022: member

Concept ACK. I think having some easy to follow steps here is useful. I do agree with @jonatack that going into too much details about git usage is out of scope.

DrahtBot added the label Docs on May 18, 2022

vincenzopalazzo approved

vincenzopalazzo commented at 8:40 AM on May 18, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/25165/commits/faa280a9d8084786e1fd5f82b3b6a5a45652ade7

LGTM

MarcoFalke commented at 12:24 PM on May 18, 2022: member

What's wrong with a git merge --squash which I see is not recommended earlier in this document or in your update? (I really don't know, BTW, which is why I'm asking.)

For me, this does not work:

$ git merge --squash bitcoin-core/master 
Already up to date. (nothing to squash)

MarcoFalke commented at 12:25 PM on May 18, 2022: member

-0 on adding further git documentation to the repo, there are other and better resources

I agree. Mind sharing a link to the "other and better resources" that can be used?

MarcoFalke force-pushed on May 18, 2022

MarcoFalke commented at 12:30 PM on May 18, 2022: member

Recall that we are probably the only open source project that doesn't use the "squash on merge" GitHub feature, so I don't think there is documentation about this out there, but I am more than happy to be proven wrong.

jonatack commented at 1:19 PM on May 18, 2022: member

I agree. Mind sharing a link to the "other and better resources" that can be used?

A prepared response may be a better place than the repo for an external link, but here are a couple that are similar to your method (rather than, say, the more flexible git rebase -i).

MarcoFalke commented at 1:37 PM on May 18, 2022: member

here are a couple that are similar to your method

Yes, this is where I got the idea from. However, they rely on commit-counting, which is non-trivial with merge commits.

MarcoFalke force-pushed on May 18, 2022

MarcoFalke commented at 2:17 PM on May 18, 2022: member

Changed to a more general approach (that may involve fixing conflicts again)

suhailsaqan commented at 6:19 PM on May 18, 2022: contributor

Changed to a more general approach (that may involve fixing conflicts again)

That didn't work for my case in #25161 unfortunately.

MarcoFalke commented at 7:47 PM on May 18, 2022: member

Can you elaborate a bit on this? I tried it on #25161 and it worked for me.

suhailsaqan commented at 8:18 PM on May 18, 2022: contributor

I must have done something wrong then. I can't remember exactly what happened but I think I actually accidentally pulled and pushed after running git rebase FETCH_HEAD which messed it all up again so I had to do something longer to fix it. Sorry for the confusion.

in CONTRIBUTING.md:223 in 888890302e outdated

 221 | -interface and ask for help in the pull request.
 222 | +If your change contains a merge commit, the above workflow may not work and you
 223 | +will need to remove the merge commit first.
 224 | +
 225 | +    git fetch https://github.com/bitcoin/bitcoin  # Fetch the latest upstream commit
 226 | +    git rebase FETCH_HEAD

laanwj commented at 9:28 AM on May 20, 2022:

Does a plain rebase work to remove merge commits? I always assumed it was much more hassle.

MarcoFalke commented at 9:33 AM on May 20, 2022:

Yeah, it should remove them. Obviously you'll have to solve the conflicts again (if there were any solved in the merge commit).

Though I think this general approach should work in all possible cases regardless of how messed up the commit history is. Any other approach requires a bit more git knowledge, but there were complaints that we don't want to dive too deep in this document.

MarcoFalke commented at 6:39 AM on May 24, 2022:

Resolving as fixed?

theStack commented at 10:11 AM on May 23, 2022: member

If your change contains a merge commit...

Very likely I'm missing something fundamental here, but why would a change in a PR ever contain a merge commit? Is this for the scenario of "PR foo is based on PR bar, PR bar has been merged, now PR foo needs to rebase?"

MarcoFalke commented at 5:06 PM on May 23, 2022: member

It usually happens when the author merges with current Bitcoin Core master instead of rebase.

theStack approved

theStack commented at 12:01 AM on May 24, 2022: member

ACK 888890302edacf3392957ef9e682cd6cb17a5f5c

Alternatively, could move the two git lines to the rebase section below (where currently concrete git instructions are missing, it only says that "git rebase" can be used, but without example) and link to there, e.g. "you will need to remove the merge commit first by rebasing on the target branch".

doc: Explain squashing with merge commits fa2d226ac9

MarcoFalke force-pushed on May 24, 2022

MarcoFalke commented at 6:41 AM on May 24, 2022: member

Thx, done.

jonatack commented at 12:11 PM on May 24, 2022: member

Aside, just came across https://github.com/lsilva01/operating-bitcoin-core-v1/blob/main/git-tutorial.md ... maybe reply to people with https://github.com/lsilva01/operating-bitcoin-core-v1/blob/main/git-tutorial.md#2-squashing-and-rebasing when useful. (I'm not sure how sustainable it is for a contributor to not become familiar with git rebase -i, and most of the better tutorials and blog posts when searching for "how to squash your commits" seem to cover that approach, so suggesting it may be helpful to people, I don't know.)

MarcoFalke commented at 9:09 AM on May 25, 2022: member

Yeah, I am happy to review a pull request that removes the sections in our readme and replaces them with a link. Or should I do it here?

laanwj commented at 6:38 AM on June 1, 2022: member

ACK fa2d226ac950d8b4f7e430732f13ad408c504745 IMO, this is an improvement. The high-level longer term discussion on the scope of our documentation can of course go on.

laanwj merged this on Jun 1, 2022

laanwj closed this on Jun 1, 2022

MarcoFalke deleted the branch on Jun 1, 2022

sidhujag referenced this in commit c0af7ba9ca on Jun 1, 2022

DrahtBot locked this on Jun 1, 2023