[INFRA] Add clarification on merge methods to DECISION_MAKING

[sappelhoff]

As @franklin-feingold has detected, several of the recently merged PRs to bids-specification have not triggered our automatic changelog generator. This is because the current code for that changelog generator (see circle.yml file) only picks up merge commits as a merge method (not rebase, not squash).

In this PR I thus add to the DECISION MAKING, that merges should always be made through the "make a merge commit" method.

This PR is accompanied with some changes to the GitHub repository, where I changed the settings to only allow merge commits as the method for merging (see image):

An alternative could be to improve the changelog generator script to also pick up rebases and squashes as methods. Feel free to take over if you want to give that a shot.

Note: This does not resolve the issue that our changelog is now missing a few entries. So we should deal with that as well.

[franklin-feingold]

LGTM

one note - I just went through the latest changelog and did not find discrepancies between the merged PRs and the changelog. Was there some you found that were not listed?

[sappelhoff]

Mmmh interesting, I was acting on your comment:

One thing I'm looking into is our commit log and for some reason recent merges are not being represented in the git log. This affects downstream processing of the changelog

and then investigated the line in the changelog generator:

bids-specification/.circleci/config.yml

Line 42 in c782b7a

if (git log -1 --pretty=%s | grep Merge*) && (! git log -1 --pretty=%b | grep REL:) ; then

To me this line seems as if the conditional is only hit if

• there is a sentence starting with the word Merge* in the commit message
• the body (description) of the commit does not contain Rel:

Since commit messages in most cases only start with Merge when the Github "Merge Pull Request" option has been used, I thought this needed to be fixed.

Now that you said it however, I also went back to change the current changelog - and everything works fine. 🤔 I have no idea why/how. *EDIT: I am wondering, because all merges that I did in the last weeks were done through the "rebase and merge" method (i.e., NOT creating a merge commit that starts with Merge ..... )

In addition: Why do we have the REL: conditional in the changelog?

[effigies]

In addition: Why do we have the REL: conditional in the changelog?

REL: pull requests are just final testing and tidying up prior to a release. The idea is that when the REL 1.3.0 branch is merged, that merge commit can be tagged 1.3.0 without any further action required. REL: 1.3.0 wouldn't be a useful thing to have in the changelog and we wouldn't want to trigger a new changelog commit after the commit to be tagged.

[franklin-feingold]

Yup! That's all right! The reason it looks all right now is because the changelog is generated after each merge PR. This is not a heavy job to do. There is flexibility with the changelog generator that in the future to limit how much is built (i.e. from a particular tag) if this does become heavy. It works well.

I am wondering, because all merges that I did in the last weeks were done through the "rebase and merge" method (i.e., NOT creating a merge commit that starts with Merge ..... )

That's right too! It didn't run because it didn't see the signal from Merge in the commit message. When it saw the Merge message signal (i.e. when Yarik merged) it was able to rebuild and the latest specification has the changelog with all the needed entries. It was able to pick up the merge PRs and cite them because that action is different than my conditional. That action is more on the inner workings of the changelog generator

[sappelhoff]

It didn't run because it didn't see the signal from Merge in the commit message. When it saw the Merge message signal (i.e. when Yarik merged) it was able to rebuild and the latest specification has the changelog with all the needed entries.

Ahh I see, so it will build also those that were missed previously. That's great, so we can also "squash and merge" and "rebase and merge", as long as occasionally there is a "merge commit" method for merging.

What do you think, Should I close this PR and re-allow the other merge methods? Or should we stick with it and try to always do "merge commits"? (rebasing and squashing needs to be done by the user on their fork/branch then) @effigies @franklin-feingold

[franklin-feingold]

I think it is better to stick with this PR (it is fine to document the merge process so in the future it is there). It is probably best practices to do the "merge commits" because of the downstream changelog generation. We want latest to be the most up to date for those that want to see the exact state we are in. I think that is useful for us as well to have a most up to date version in case we want to check something we know it will be there. I do not have a strong opinion for rebasing and squashing, from my understanding it doesn't do much except condense the git log. My concern is that it is a more advanced feature that can be difficult for some users to do.

[sappelhoff]

Okay, I agree with your points. Let's hear @effigies opinion

[effigies]

Sorry for the delay. This got buried in a storm of notifications and I never got back to it.

I'd actually be inclined, instead of banning squashes and rebases, to explicitly note that PRs merged by squash or rebase will not appear in the changelog, so should only be used if both the submitter and reviewer agree that it is appropriate not to include it. So default to include unless all parties agree to exclude. If a specific use case is desired, I would say that a lot of [INFRA] things are not really relevant to the changelog in the spec document (similar to [REL], actually), and so a squash or rebase would be desirable.

WDYT?

[sappelhoff]

but: The rebases and squashes WILL get included upon the next "merge commit" ... it will handle those commits that were missed - if I understood this correct. See #217 (comment)

[effigies]

Oh, weird. Well, if it's not a way out of the changelog, then I don't have a strong argument for allowing them, but I also don't see a great argument for banning them, either.

[franklin-feingold]

The argument for banning them would be to allow the changelog to be the most up to date. If we only allow the merge with commit option, this would keep us up to date (and have to wait until that signal is given to update the changelog)

[effigies]

Okay.