Opened 7 weeks ago

Closed 6 weeks ago

Last modified 6 weeks ago

#32708 closed task (fixed)

manpage: alphabetize General Options

Reported by: catalyst Owned by: catalyst
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: documentation tor-client manpage easy gsod
Cc: swati, teor Actual Points: 1
Parent ID: #4310 Points: 1
Reviewer: teor Sponsor:

Description

This ticket is for Swati's alphabetizing change in #4310. Making this a child ticket so we can create other child tickets for working on alphabetizing the other sections.

Child Tickets

Change History (10)

comment:1 Changed 7 weeks ago by catalyst

Type: defecttask

comment:2 Changed 7 weeks ago by catalyst

Status: assignedneeds_review

This is based on Swati's earlier pull request in #4310.

My pull request is at https://github.com/torproject/tor/pull/1590.

I separated the unrelated changes, and undid the (unintentional) indentation changes. It looks good to me, but I would like someone else to quickly look it over.

comment:3 Changed 7 weeks ago by catalyst

Cc: teor added
Reviewer: teor

Assigning review to teor, because they looked at a previous iteration in #4310. teor, please let me know if you'd rather someone else looked at it.

comment:4 Changed 7 weeks ago by catalyst

I updated the pull request with some additional commits that make things render more correctly in Asciidoctor tools. I also did a force-push to fix a minor formatting problem caused by alphabetization.

comment:5 Changed 6 weeks ago by teor

Status: needs_reviewneeds_revision

All these changes look good, but the options are not actually in alphabetical order. (There are some cases where word breaks make ordering ambiguous, and others where the options are just out of order.)

$ head -930 doc/tor.1.txt | tail -701 | grep '\[\[' | cut -d [ -f 3 | cut -d ] -f 1 > general_options.txt
$ sort general_options.txt > general_options_sorted.txt
$ diff -u general_options.txt general_options_sorted.txt
...

I suggest that we add a test to make sure the options in the general section are actually in order.

comment:6 in reply to:  5 ; Changed 6 weeks ago by catalyst

Replying to teor:

All these changes look good, but the options are not actually in alphabetical order. (There are some cases where word breaks make ordering ambiguous, and others where the options are just out of order.)

$ head -930 doc/tor.1.txt | tail -701 | grep '\[\[' | cut -d [ -f 3 | cut -d ] -f 1 > general_options.txt
$ sort general_options.txt > general_options_sorted.txt
$ diff -u general_options.txt general_options_sorted.txt
...

Thanks for the review and checking script! There are far fewer out-of-order option names if I do a case-insensitive sort.

 Schedulers
-KISTSchedRunInterval
-KISTSockBufSizeFactor

swati mentioned to me that this ordering is intentional because it's more logical to put the KIST options together with the Schedulers option.

-Socks5ProxyUsername
 Socks5ProxyPassword
+Socks5ProxyUsername

It seems odd to put Password in front of Username, so maybe this can also deviate from strict alphabetical order?

 Log
 Log2
 Log3
 Log4
+LogMessageDomains

This one looks reasonable to move.

I'll make a revision to move LogMessageDomains, and put comments near the other out-of-order options explaining why they're intentionally not in order.

I suggest that we add a test to make sure the options in the general section are actually in order.

I think you filed #32621 for this.

comment:7 in reply to:  6 Changed 6 weeks ago by arma

Replying to catalyst:

It seems odd to put Password in front of Username, so maybe this can also deviate from strict alphabetical order?

I agree that we shouldn't stick to alphabetical when it makes things worse.

It also seems really easy to just accumulate more items in the future and have it all go messy again.

Maybe that argues for grouping config options by topic? Or for having more 'areas', where each area is small and alphabetized? Or for writing down some heuristics for how we picked the current order, so that when we add things in the future we can have a prayer of keeping things organized?

I don't have any good answers. So I am just here to say that even though I wrote 'alphabetized' in a trac ticket eight years ago does not mean that I am certain it is the best way to go. :)

comment:8 Changed 6 weeks ago by catalyst

Status: needs_revisionneeds_review

I pushed a fixup commit that moves LogMessageDomains and adds comments about alphabetical order (and exceptions). Please let me know if you would like me to force-push a squashed version.

I think this pull request is a net improvement on the existing situation, and doesn't introduce any long-term problems. Right now swati's work is blocked on getting this merged so if we could get it merged soon, that would be great.

comment:9 Changed 6 weeks ago by teor

Resolution: fixed
Status: needs_reviewclosed

Merged to master.

The fixup didn't apply, so I modified its commit message to get around our "no fixups" rules.

Please remember to fill in actual points!

comment:10 Changed 6 weeks ago by catalyst

Actual Points: 1
Note: See TracTickets for help on using tickets.