Page MenuHomeFreeBSD

git-arc(1): Add manual page
ClosedPublic

Authored by debdrup on Feb 7 2021, 2:52 AM.

Details

Summary

Add manual page based on the usage in the script with a few changes and
hook it up to the build.

Test Plan

Igoring as well as liberal use of mandoc.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 36867
Build 33756: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
tools/tools/git/git-arc.8
43 ↗(On Diff #83487)
44 ↗(On Diff #83487)
48 ↗(On Diff #83487)
56 ↗(On Diff #83487)

I would not use you in man pages.

60 ↗(On Diff #83487)

The specification here looks to be broken, no width provided.

94 ↗(On Diff #83487)
97 ↗(On Diff #83487)

These do not belong in ENVIRONMENT section.

116 ↗(On Diff #83487)

The change above should work with .Bl -item, didn't edit in-place as phab goes crazy. Or check other man pages for EXAMPLES sections.

.Ic doesn't seem to be correct macro here; if you just want bold text, use .Sy.

138 ↗(On Diff #83487)

trailing : seems to be out of place

140 ↗(On Diff #83487)

Sort by section, then alphabetically.

142 ↗(On Diff #83487)
debdrup marked 17 inline comments as done.

Address feedback by yuripw

Capitalize Phabricator and switch from Ic to Cm macro

tools/tools/git/git-arc.1
35–46

Is there a macro which ensures that these arguments are listed on one line each?

debdrup retitled this revision from arc-git(8): Add manual page to git-arc(8): Add manual page.Feb 8 2021, 6:22 AM
debdrup edited the summary of this revision. (Show Details)

I'm planning to do a review of this in the coming days. I'd like to help you with the mdoc part :)

In D28519#638602, @0mp wrote:

I'm planning to do a review of this in the coming days. I'd like to help you with the mdoc part :)

Sure thing, I look forward to it :)

Most of the macro use was either inspired from mdoc(7), manual pages I've written previously, or manual pages that I consider well-written.

tools/tools/git/git-arc.1
50

I would s/or/and/.

54

IMO it would be clearer to state that this is an assumption.

56
57
177

My last name has a 't'. :)

debdrup marked 5 inline comments as done and an inline comment as not done.

Address feedback by markj

Looks ok to me. Thanks for doing this. I would remove the corresponding text from the git-arc usage message but that can be done separately.

tools/tools/git/git-arc.1
185

This script does not get installed by default, so I'm not really sure that a history section makes much sense. At least, "appeared in FreeBSD 14.0" is a bit misleading since it technically not appear in any FreeBSD release.

Huh, you're right, that is somewhat misleading - however, it's also not completely unique:

find /usr/src/tools/ -type f -name "*.[0-9]" -exec grep "appeared" {} +
/usr/src/tools/tools/mcgrab/mcgrab.1:program first appeared in
/usr/src/tools/tools/vimage/vimage.8:Network stack virtualization framework first appeared as a patchset
/usr/src/tools/tools/vimage/vimage.8:Stiching NLNet, integrated virtualized network stack first appeared in
/usr/src/tools/tools/ether_reflect/ether_reflect.1:program first appeared in
/usr/src/tools/tools/git/git-arc.1:utility appeared in
/usr/src/tools/tools/ioat/ioatcontrol.8:driver first appeared in
/usr/src/tools/tools/mctest/mctest.1:program first appeared in

src/tools/tools is included in the src.txz distribution set, but whether that includes being part of a RELEASE is probably outside the scope of this commit ;)
I'm more than happy to change it, but at present I'm not really sure what it should be - I'll leave it unsolved for now, until I find a good way to phrase it.

tools/tools/git/git-arc.1
185

Maybe Appeared in the src tools collection in FreeBSD 14?

debdrup marked 2 inline comments as done.

Address last bit of open feedback by @markj and @emaste

debdrup retitled this revision from git-arc(8): Add manual page to git-arc(1): Add manual page.Feb 8 2021, 9:47 PM
rpokala added inline comments.
tools/tools/git/git-arc.1
185

Or perhaps not in the src tree at all, and part of devel/git or devel/arcanist instead?

tools/tools/git/git-arc.1
185

But it's not part of those ports. Are you suggesting the script be added to one of them instead? It is somewhat specific to FreeBSD development, at least for now, so I'm not sure whether that is the right place.

tools/tools/git/git-arc.1
185

I was thinking it might be added to one of those ports, yes. Thinking aloud, anyway; whatever. <shrug>

(Please move the conversation out of inline comments)
Since it's FreeBSD specific, I'm not sure either git or arcanist upstreams will accept it - which means anyone will need ports approval to get changes made to it.
I don't see the benefit in this, personally.

0mp requested changes to this revision.Feb 9 2021, 6:14 PM

Great job! The content seems pretty solid. I've left some comments regarding the mdoc syntax and style.

tools/tools/git/git-arc.1
32

Perhaps this description could be simplified "git-Phabricator" doesn't look good to me, but maybe I'm just not feeling this expression. That's why I wonder if it could be simplified to make it clearer.

35–46

Kind of. Here's a corrected synopsis:

.Nm
.Cm create
.Op Fl l
.Op Fl r Ar reviewer1 Ns Op Cm \&, Ns Ar reviewer2 ...
.Op Fl s Ar subscriber1 Ns Op Cm \&, Ns Ar subscriber2 ...
.Op Ar commit Ns | Ns Ar commit-range
.Nm
.Cm list Ar commit Ns | Ns Ar commit-range
.Nm
.Cm patch Ar diff1 Ns Op Cm \&, Ns Ar diff2 No
.Nm
.Cm stage
.Op Fl b Oc
.Ar branch Op Ar commit Ns | Ns Ar commit-range
.Nm
.Cm update
.Op Ar branch Oo Ar commit Ns | Ns Ar commit-range

Assuming that -b is optional to the stage verb, and branch is required.

54

Shouldn't we stylize git as Git in such cases?

55

"Differential revisions" seems a bit inconsistent to me. Should that be either "Differential Revisions" or "differential revisions"?

57
62

I think I'd drop -offset indent. That would be consistent with style in pages like ifconfig(8).

74

Since main is a constant in a way, I'd stylize it with the Ql macro.

85

Perhaps this should be EXAMPLES instead? I am not sure if the order of sections is going to be correct then...

86

This is not needed. Please see the output of mandoc -Tlint.

90

I'd add -offset indent for every Bd in this section.

91

Usually, we do not use Sy to make exaples bold. I'd remove them for consistency with other manuals.

96

Typo? s/review/revision/?

109

We should either use .Xr git-config 1 here or something like .Nm git config perhaps.

111

Here and in other cases as well. This [bool] is not a part of a command modifier.

You may also consider the format used in rc.conf(5) for types.

112

We usually try to use the Pq macro for quoting instead of using " directly.

114

I'd also add a note the flag of which program we reference here (I guess arc, but I am not sure).

Also, the default value is missing. OTOH, the default is quite obvious so maybe we can skip that.

119
121

Can we merge this and the next line into one line?

123

Can we merge this and the next line into one line?

126

Should it be expressed in an imperative form to match other definitions for consistency?

127

the -v flag of arc?

166
171
172
173

I wonder if we should add .An -nosplit here... I guess it's up to you.

179
This revision now requires changes to proceed.Feb 9 2021, 6:14 PM
debdrup marked 26 inline comments as done.

Address feedback from @0mp

tools/tools/git/git-arc.1
112

I assume you meant the Qq macro, because Pq is parentheses while the former is double quotes, so that's what I went with, since it matches the text that you rightly spotted should be a macro. :)

tools/tools/git/git-arc.1
95

Could [bool] be dropped now?

112

Sorry for the confusion. Instead of Pq I meant Dq. As documented in style.mdoc we use Dq instead of Qq usually.

debdrup marked 2 inline comments as done.

Address feedback by @0mp

Seems fine! Thanks a lot!

tools/tools/git/git-arc.1
95

I'd probably drop -offset indent as well here. It's up to you :)

102

Typo

This revision is now accepted and ready to land.Feb 12 2021, 9:58 PM
This revision was automatically updated to reflect the committed changes.