CONTRIBUTING.md: Add in for github pull requests

[imp]

Summary

Create a slightly longer version of the inforamtion available in the
handbook in the file that Github displays for more information about
contributing.

Sponsored by: Netflix

[grahamperrin]

CONTRIBUTING.md

6–7

29

33

36

48

51

57

Missing a word?

58–59

62

… or:

Occasionally, changes

63

(What uses?)

68

70–71

If only one occurrence of style(9) will be linked, make it the first occurrence.

74

85

86

86

Is this a command, should it appear as mandoc -lint?

98

[salvadore]

CONTRIBUTING.md

32

I am a bit unfamiliar with Signed-off-by:. In particular, I would like to know:

• what is the relation between such lines and the Developer Certificate of Origin? Is it written somewhere that if someone adds such a line then (s)he is adhering to the Developer Certificate of Origin? Or does it mean something else?

• I read in imp's comment it is something new that we are trying: should we start requiring those lines or not yet? I remind that I usually work in the ports repository, sometimes in the doc repository and in the GitHub freebsd/freebsd-quarterly repository (soon to be archived, see below);

• the freebsd/freebsd-quarterly repository will soon be archived and, starting in March, we (the status team) are going to accept pull requests directly in the doc repository (one of three possible ways of submission). Should we start to require a Signed-off-by line? Should we require it only for submissions sent through GitHub pull requests, or also through Phabricator and by mails? Or for none?

• I read in imp's comment that such lines can be generated with "git commit -s": maybe write it in the CONTRIBUTING.md file as well. Knowing that it is indeed easy is helpful.

Thanks.

[diizzy]

CONTRIBUTING.md

8

Capital "I" but I would rephrase that sentence rather than starting with "It".

[imp]

comments, but I think I Got all of these except @salvadore 's which I'm mulling over how to address.

CONTRIBUTING.md

28

no, it's asciidoctor. I'll convert

51

i have the link to style(9) below..

70–71

I reworded above, so will ignore this as this is now the first occurrence :)

[grahamperrin]

Blindly accepting, without attempting to read the whole thing :-)

Sorry, I can't cope with Phabricator for things such as this …

CONTRIBUTING.md

6–7

This suggestion above is shown as Done, but it's not (not changed).

An oversight, or intentional, and/or is this a peculiarity of Phabricator? (Like when it shows not done for things that were marked done …)

[imp]

update this for real this time (rather than accidentally creating a new one)

[imp]

CONTRIBUTING.md

6–7

I created a new rev rather than updating this rev. Oops. Fixed now.

[imp]

signed-off-by

[imp]

address @salvadore's comments. We should be really close with this revision, but if more than trivial changes are needed, I may open a new review for real this time.

CONTRIBUTING.md

32

Signed-off-by: are ubiquitous in other projects. Non-project members making contributions likely are familiar with them, as they are required in so many other projects. Having people assert this helps the project know that these minor contributions are offered freely. It's strongly suggested, but not required. We also use it as a backup for Authorship and as a cross-check.

At this point, I don't think we should require it for project members at this time. They've made the agreement already, at least implicitly, when they joined the project. Plus, their identities are well known, so we don't need it from either perspective (though we don't filter them out so people are free to do that if they want).

For pull requests to other repos: It's an open question. It would be useful for the reasons above, but nothing more. Plus, for ports, there's a longer tradition of using bugzilla with properly formatted patches. For docs, it's a toss up.

For the quarterly reports, I'd say the same: there's not a huge reason to be super careful about IP here: people are submitting reports to be published, so the risks are smaller (but admittedly not zero: the cost to make it zero is too high imho). Maybe suggest it for the reasons above?

Finally, I've added some words at the end of the file that spells the relevant bits of this out.

[pauamma_gundo.com]

LGTM as far as English is concerned with these typos fixed and the spurious word removed.

CONTRIBUTING.md

103

108

110

[imp]

pauamma's typo suggestions

[emaste]

Overall looks good to me, at a high level the information we want to present. I have a few tweaks I might suggest but I'd do that as part of an update as we get some more feedback / find out how this is working. One of the things I would do is make it more prescriptive -- "do it this way" rather than "it can be done this way." But we really want to have the experience/comfort with things before doing so.

[grahamperrin]

Errors in two places (changes requested).

The other suggestions can be negligible, if you'd like to get this diff over and done with. Thanks.

CONTRIBUTING.md

13

15–19

Attempting to improve sentences that begin with the word 'It'.

Also, emphasising that this file is src-specific. For readers to not assume that what applies here generally applies to any other FreeBSD repo.

26

Change requested (grammar).

Either something like the suggested change, or insert 'the' after the word 'follow'.

62

85

Change requested (white space missing, I think).

93–94

106

106

With a first thing in the midst of a paragraph, where's the second thing? (I'd expect to find it in the same paragaph).

[imp]

In D38771#883310, @emaste wrote:

Overall looks good to me, at a high level the information we want to present. I have a few tweaks I might suggest but I'd do that as part of an update as we get some more feedback / find out how this is working. One of the things I would do is make it more prescriptive -- "do it this way" rather than "it can be done this way." But we really want to have the experience/comfort with things before doing so.

I generally agree. Since we're running experiments now, greater latitude helps us find out what will or won't work so we can be more prescriptive later

[imp]

OK. That's a wrap. I'm going to commit after plowing the last minute feedback in.
If something is still wrong, then go ahead and fix it :)

CONTRIBUTING.md

15–19

I found a different way to rework this that I think is shorter and clearer.

106

fair point. I reworked a little.

[ngie]

CONTRIBUTING.md

8

Should this be treated as proper nouns?

17

Problem reports/issue reporting is already disabled on the repo. Does this need to be explicitly mentioned here?

Future backlog: it would be good to have an alternative means for "interacting with the community" posted with the sources -- if folks clone FreeBSD's GitHub repo, they won't necessarily know how to reach out for support, where to have discussions with respect to non-trivial improvements, etc. Anecdotally, this is one of the major hinderances that new FreeBSD contributors face when engaging the project.

23

A bit pedantic, but Cirrus CI is technically how the checks are run today. In the future if the project chooses to do so, some of the checks can be moved to GitHub Actions, Travis CI, etc. This would benefit the project with some of the OS-agnostic checkers, e.g., black, cppcheck, etc, which can be executed from Linux containers, at the cost of some "CI sprawl".

29

Future backlog: there should really be a push hook that prevents fixup commits from being merged in general, so developers and contributors don't accidentally push fixup commits to main.

35

Future backlog item: this seems like it should be a CI check -- I think given the previous criteria (changes should be small and low-touch), merge commits should not be allowed from GitHub.

[ngie]

CONTRIBUTING.md

85

I would recommend make manlint instead.

Future backlog item: make this into a CI check.

93

I would probably make this wording stronger. bash-syntax shell scripts are not likely to work with ash at all.

Future backlog item: add a CI check to ensure that code passes sh -n on FreeBSD.

114

Maybe reword this to be more direct?

[imp]

CONTRIBUTING.md

8

Likely, but other review said lower case. don't care. Someone else can fix it now that this has been committed if this is wrong.

17

It's not terrible to mention it here to reinforce it.

And there's dozens of ways to interact with the community. If someone shows up here with the wrong stuff, we'll redirect. I plan on making sure people get feedback here within a week going forward. If the pull is good, I'll commit it. If it needs review, I'll coordinate. If it should be handled in another venue, I'll redirect. That's kinda why I'm doing this: to have at least one venue that's guaranteed a speedy reply. And we've been doing PRs wrong to date: we have been too shy about rejection (here and in Bugizlla, but bugzilla is too overloaded now for me to do anything with it).

23

We have a half a dozen CI jobs on GitHub Actions.
And none of this is getting in the way of doing that reorg.
There's other people doing work on the developer side for our post-commit CI.
I have a start of a script that does style and other checks that could be quite useful, it can run anywhere with perl since I snagged it from another project.

So I'm going to leave this alone, and encourage people to do the actual work that you suggest... They are good ideas.

29

already there: pre-commit push hooks prevent this...
But it's a bit of a pain when processing the pull request to rework to cope (though the scripting I've done will effectively get rid of these).

35

I don't think that we can do that on pull requests.... Maybe as a GitHub action we can check it... I'm open to experimenting here...

85

Agreed on both points, but I've already pushed this, so I'm not going to change it since it's not a huge increase...

93

I would probably make this wording stronger. bash-syntax shell scripts are not likely to work with ash at all.

Future backlog item: add a CI check to ensure that code passes sh -n on FreeBSD.

both of these are good suggestions, but I'd already committed it...

[imp]

This landed.