Opened 3 months ago

Closed 3 months ago

#32003 closed task (implemented)

Unify handling of command-line option parsing

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-august
Cc: nickm, teor Actual Points: .4
Parent ID: #29211 Points: .4
Reviewer: teor Sponsor: Sponsor31-can

Description

Our function for parsing the command line does not actually set up various command-line-only options, leaving that to be done elsewhere. We should make this table-driven, so we can simplify our code.

Child Tickets

Change History (11)

comment:1 Changed 3 months ago by nickm

branch is cmdline_refactor with PT at https://github.com/torproject/tor/pull/1400. I'll wait for CI and coverage before I merge_ready.

comment:2 Changed 3 months ago by nickm

Status: assignedneeds_review

CI has passed; I've squashed some fixups, split a commit, and re-pushed. Putting in needs_review under the presumption it will pass again.

comment:3 Changed 3 months ago by teor

I didn't do a full review here, but I noticed a typo in a comment.

comment:4 Changed 3 months ago by nickm

Reviewer: teor

comment:5 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

I added some comments on the PR.

Do we need more command-line tests?
We might not need a test for every option, but one per mode/quiet level would be nice.

comment:6 Changed 3 months ago by nickm

I've made the requested changes. For the quiet levels, I think I can do that, but what do you mean by "mode"?

I'd hoped that the existing tests might be enough for this patch, since they cover 100% of affected lines; I'd be okay with writing more, though.

comment:7 Changed 3 months ago by nickm

Status: needs_revisionneeds_review

comment:8 Changed 3 months ago by teor

Ok, if we have 100% coverage, we don't need more tests.

There's one fix remaining: the CMD_OTHER rename didn't reach the nt_service files:
https://ci.appveyor.com/project/torproject/tor/builds/28177080/job/svbqubudyuw6c38i#L3075

When that's fixed, and CI passes, feel free to merge this ticket.

comment:9 Changed 3 months ago by nickm

ok, pushed a fix and waiting for CI.

comment:10 Changed 3 months ago by nickm

(To be clear, the 100% coverage is on the lines modified by this branch; I hope that's ok)

comment:11 Changed 3 months ago by nickm

Actual Points: .3.4
Resolution: implemented
Status: needs_reviewclosed

CI passed; squashed and merging.

Note: See TracTickets for help on using tickets.