Page MenuHomeFreeBSD

git: document how to merge a pull request from github.
ClosedPublic

Authored by imp on Jun 1 2021, 12:18 AM.

Details

Diff Detail

Repository
R9 FreeBSD doc repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp requested review of this revision.Jun 1 2021, 12:18 AM
imp created this revision.

update per jrtc27 and Darius on irc

note closing at the end.

The documentation team prefers to avoid use of 'you' in the handbook (and any other documentation).

These are just examples, though - feel free to tweak as needed.

documentation/content/en/articles/committers-guide/_index.adoc
2272
2273
2284
2285
2314
2316
2317
ceri added inline comments.
documentation/content/en/articles/committers-guide/_index.adoc
2267

committed

2268

superfluous; suggest dropping this sentence

2270

verb missing

2270

exactly

2282

contain

2287

Can we avoid this term as it is confusing when compared to "git cherry-pick", unless it means the same thing?

2305

multiple

ceri requested changes to this revision.Jun 1 2021, 8:27 AM
ceri added a reviewer: ceri.

As per comments.

This revision now requires changes to proceed.Jun 1 2021, 8:28 AM
imp marked 14 inline comments as done.Jun 1 2021, 4:03 PM

I don't like the passive voice, due to forced conditioning in high school Freshman English.... however, I know that's the style here...

documentation/content/en/articles/committers-guide/_index.adoc
2287

It means exactly the same thing as 'git cherry-pick' in this context.

2317

minor tweaks here.

imp marked 2 inline comments as done.

review feedback

ceri requested changes to this revision.Jun 1 2021, 4:24 PM
ceri added inline comments.
documentation/content/en/articles/committers-guide/_index.adoc
2271

beginning

2271

This section doesn't seem to document setting origins, unfortunately. I can see that you're reusing text from another section though, so probably a bit harsh to reject this on that basis - unless you want to fix that at the same time, I'll take a separate PR to fix that.

This revision now requires changes to proceed.Jun 1 2021, 4:24 PM
documentation/content/en/articles/committers-guide/_index.adoc
2271

Earlier feedback had me take it out

documentation/content/en/articles/committers-guide/_index.adoc
2271

I may be doing something wrong, but I still see it when I download the diff.

documentation/content/en/articles/committers-guide/_index.adoc
2271

I had the whole origin / fetch stuff in as commands and I took it out.

I do think the origin setting stuff should be in #keeping_current link instead... and I'd prefer to improve that async to this review.

In D30589#687275, @imp wrote:

I do think the origin setting stuff should be in #keeping_current link instead... and I'd prefer to improve that async to this review.

Yeah, happy with that.

documentation/content/en/articles/committers-guide/_index.adoc
2271

Ah, I see.

Just the “beginning” typo then lgtm.

In D30589#687207, @imp wrote:

I don't like the passive voice, due to forced conditioning in high school Freshman English.... however, I know that's the style here...

Believe me, I'm no more fond of it than you, but The Style is The Style, if you'll permit me to be tautological. :)

@ceri @debdrup ok. I think this is ready to land.

The content looks good to me, just some minor nits.

documentation/content/en/articles/committers-guide/_index.adoc
2265

I'm wondering if we need to use "GitHub" instead of github...

2267

have them be committed => have them committed?

2271

Similarly, we should probably settle git vs Git across the doc.

2272

setup => set up?

2285

This line sounds a bit confusing to me from a reader's perspective - [what] can also work, without conflict [against what], when there are [?]. Perhaps breaking into two sentences and rephrasing it?

2304

IMO the first comma is not needed

2310

with?

Also, should we avoid the word "merge" here? Since IMO it means continue/finish the rebase here.

imp marked 7 inline comments as done.Jun 1 2021, 11:39 PM
imp added inline comments.
documentation/content/en/articles/committers-guide/_index.adoc
2265

We don't currently, and such a change would be beyond the scope of this change.
I'll change it here though.

2267

-> "and have them pushed into the FreeBSD's tree from there."
might be better. what do you think?

2271

I tried to do that in the draft docs before we landed them into the tree, so I'll do this here.

2272

Or omit it entirely.

2285

Will give it a shot.

2304

sure

2310

reworded.

imp marked 7 inline comments as done.

ygy

documentation/content/en/articles/committers-guide/_index.adoc
2254
2256

Do we need to/encourage reviewing the diff again after fast-forwarding? IMO it's fine 99.9% of the time but... (question mark)

2257

Huh... What does this mean ;)

2262

If we are closing the PR on the web interface anyways (as the above sentence stated), why does "requiring an account" depend on HTTPS or not?

more feedback fdrom ygy

All but the last thing.

Thanks for making all the changes! This looks good to me, pending other reviewers' approvals.

This revision is now accepted and ready to land.Jun 2 2021, 8:40 AM
documentation/content/en/articles/committers-guide/_index.adoc
2262

You can do all the steps of fetching the changes, preparing them for commit and pushing them into the FreeBSD w/o a github account.
To close the pull request at the end, though requires a github account.

Is there a better way to say that?