CONTRIBUTING.md -- suggested change to working with Github process

[TerryE]

At the moment the advice is:

• to avoid intermediate merge commits
• to squash your commits against a dev rebaseline.

I feel that this is a one-size fits all approach an it doesn't, as a single commit can make it very difficult to see what is what if the change is anything but trivial.

1. In Tidying up the Library coding styles #1028, I discuss that we've often got dead code inline with large blocks of #if 0 or '//' commenting out, The layout and spacing can be poor and what would be very useful commenting is omitted. Also the most appropriate Lua argument checking API calls aren't used. We should have a standard template and practice for laying out out modules.
2. I have suggested that if any of us does non-trivial work on a module then it makes sense to do a (1) pass at the same PR.
3. After the first submission of the PR, we may get review comments that need incorporating.

My suggestion is that whilst we should expect contributors to squash working commits, it really makes it a lot easier to review changes if we split the PR into up to four commits, viz: (a) Formatting and dead code removal -- here in principle the binary output of the two versions would be unchanged. (b) Tidy up and move to template compliant error checking, etc. (c) The functional changes that triggered the issues / work. (d) The changes are a result of any issue and PR comments.

I would also suggest we adopt a naming convention for commits, E.g. PR title followed by code tidy / rework checking / main changes / Review.

Lastly I am not a Github expert but what worries me about fetching dev and rebaselining against it is that this might back out other changes made in parallel. My standard approach now is:

1. If it's a complex change do it against a recent change and commit it.
2. git fetch nodemcu dev:dev-<some-branch> where the branch name reflects the PR
3. git checkout -f :dev-<some-branch>
4. Either git cherry-pick commit at (1) or do a git diff on (1) to a /tmp/xxx.diff and patch the working branch with this. If it is a simple change just do it.
5. Devise a test to test the change; do a make; blow to ESP and test it.
6. Commit it.
7. git push terrye dev-<some-branch>:dev-<some-branch>
8. Use github interactively to issue the PR against nodemcu/nodemcu-firmware:dev
9. Update the raising issue and add any further b/g comment to the PR.
10. Roll up any agreed PR comments into an additional commit on my local instance and push this to terrye.

My aim here is to do (3) - (10) in one working session, say no more than a couple of hours.

If anyone has any suggestions on how to improve this then I'd be very interested. If not then why not just suggest this or something like this.

[marcelstoer]

Our CONTRIBUTING.md says:

This is just one way of doing things. If you're proficient in Git matters you're free to choose your own.

Therefore, we're fine IMO. Git is so versatile (some say complex) that how you use it very often comes down to "a matter of style".

a single commit can make it very difficult to see what is what if the change is anything but trivial

I think that really is a personal preference. IIRC when this discussion first came up whoever did the review didn't agree with you - might also be that it was the other way 'round, you reviewed and the committer disagreed.

I would also suggest we adopt a naming convention for commits

I agree but we've got that already, at least partially. In general I wouldn't want to be too strict on such matters. "Form follows function" is my mantra. We shouldn't forget what our focus is.

[robertfoss]

Forcing PRs to have all commits be squashed is problematic in two ways:

• Debugging during PR process will be harder.
• Instead of having commits reflect atomic changes to the codebase, the commit will likely reflect a much larger set of changes. Which has a very clear cost in terms of development and debugging time outside of the PR process.

[marcelstoer]

Robert, I agree but as I replied to Terry we do not enforce all commits be squashed #1117 (comment).

I don't see a necessity to change our description in CONTRIBUTING.md.

[robertfoss]

I don't particularly agree with un-enforced rules. Either a rule is deemed to be good and should be enforced or it isn't good enough and shouldn't be what people are asked to do.
Those are my 2 cents, I don't feel particularly strongly about this issue though.

[marcelstoer]

Terry, are you waiting for more feedback or has this stalled?

[TerryE]

Marcel, Sorry, I viewed your as the primary maintainer of CONTRIBUTING.md. This is a consensus discussion where the bulk of the active contributors haven't bothered to comment. IMO, this is an issue worth discussing, but we can't see a consensus without move opinions.

[jmattsson]

I think what we're really trying to avoid is get PRs with umpteen teensy commits in them, and also discourage too large PRs as well. If we need to update the wording in CONTRIBUTING.md then I'm fine with that.

[marcelstoer]

Is this all about 7i) then? If so, feel free to rephrase it. Thanks to GitHub it's now in our hands whether we eventually merge or merge+squash.

[marcelstoer]

I rephrased that a while ago.