Updating contributing article to include git instead of tar/shar
AbandonedPublic
Actions

Authored by freebsd_ny-central.org on May 19 2024, 2:38 PM.

Details

Reviewers

jrm
fernape
brooks

Group Reviewers

docs

Summary

This patch originated out of PR 278284, which indicated that tar and shar are outdated processes for change submissions.

There may probably be better people to speak to this to me, so I hope this is a step in the right direction. Feedback is very welcome.

I have

added reference to git format-patch as well as git diff
removed tar and shar in terms of the "de-facto" submission path
added Phabricator as review option
removed the hosting question to be asked from project members, since we now have Phabricator

Diff Detail

Repository

R9 FreeBSD doc repository

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 60456
Build 57340: arc lint + arc unit

Event Timeline

freebsd_ny-central.org requested review of this revision.May 19 2024, 2:38 PM

freebsd_ny-central.org created this revision.

LGTM.

Thanks!

This revision is now accepted and ready to land.May 19 2024, 3:46 PM

For non-ports contributions we should really discourage submissions via bugzilla unless it's what a specific committer you're working with wants. Generally, bugzilla is where patches go to die.

documentation/content/en/articles/contributing/_index.adoc
265–266	I think we should drop the mention of tar entirely. For things that Phabricator doesn't handle well (lots of renames, etc) GitHub PRs are more appropriate.
288	I'd drop FTP while you're here, it's archaic.

fernape added inline comments.May 21 2024, 10:41 AM

documentation/content/en/articles/contributing/_index.adoc
265–266	I think we should drop the mention of tar entirely. For things that Phabricator doesn't handle well (lots of renames, etc) GitHub PRs are more appropriate. Agree.
288	Probably worth mentioning "... a web server or other online storage solution." so we cover google drive, dropbox, etc...

brooks added inline comments.May 21 2024, 4:04 PM

documentation/content/en/articles/contributing/_index.adoc
288	I guess so. Personally I want Phab or a GitHub PR. Anything else and the interest bar for looking at such submissions becomes very high.

I've done some additional updates based on your inputs. Additional feedback very
welcome, of course.

I've switched to arcanist now - if something's off with the patch, please let me
know - I might still have some of the config screwed up...

Suggestion for preliminary commit message (once we get there):

Instead of sending tar files and only filing patches as PR on
Bugzilla, contributions should instead by uploaded to Phabricator
or provided as pull requests on Github.

PR:             278274
Reported by:    brooks
Reviewed by:    brooks, fernape

This revision now requires review to proceed.May 30 2024, 7:00 PM

Harbormaster completed remote builds in B57968: Diff 139231.May 30 2024, 7:00 PM

freebsd_ny-central.org marked 4 inline comments as done.May 30 2024, 7:02 PM

freebsd_ny-central.org added inline comments.

documentation/content/en/articles/contributing/_index.adoc
288	I've removed any form of website and recommend Phabricator and Github only - is this a working compromise or should we mention these still?

This is an improvement.

I'm not convinced we should discuss any non-git contribution methods, but can certainly be committed as is.

This revision is now accepted and ready to land.May 31 2024, 2:32 PM

jrm added inline comments.May 31 2024, 3:31 PM

documentation/content/en/articles/contributing/_index.adoc
253–262	Would linking directly to `man:git-format-patch [1]` here make sense? We already link to man:git[1] a few lines above.
262	We could also mention man:git-apply[1] here for when the patch was generated with `git format-patch`. ...(which you may test with man:patch[1] or man:git-format-patch[1])...

[articles][committers-guide]: streamlining stable/N and releng/N.n syntax

As outlined in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279503 there are
various ways we are listing stable/N and releng/N.n - sometimes with numbers,
sometimes not.

I'm suggesting to migrate everything into one common format. What I have not
resolved yet (and would appreciate feedback on): there are still numerous
examples with i.e. stable/13 (I updated from stable/12). It would make sense to
simply change them all into a stable/N and releng/N.n format.

Ed made the point that he received issue reports for people misunderstanding the
placeholder syntax, so maybe it makes sense to leave a handful of examples with
numbers?

PR: 279503

This revision now requires review to proceed.Nov 9 2024, 11:06 AM

Harbormaster completed remote builds in B60456: Diff 146196.Nov 9 2024, 11:06 AM

Mistakenly had another file in there... removed.

Harbormaster completed remote builds in B60457: Diff 146197.Nov 9 2024, 11:07 AM

And this happens when I don't use arc more regularly. I push a new patch into an old revision. Apologies and please ignore this one.