Opened 19 months ago

Closed 7 months ago

#18146 closed defect (fixed)

Tor's control protocol misspells "Dependent"

Reported by: teor Owned by: jryans
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Trivial Keywords: doc tor-controller
Cc: jryans@…, atagar Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In getinfo_helper_config(), tor misspells "Dependent" as "Dependant".

There's probably nothing we can do about this, because it's part of the controller interface.

But we should at least document it, preferably in the code and the control spec.

Child Tickets

Change History (18)

comment:1 Changed 19 months ago by teor

Milestone: Tor: very long termTor: 0.2.8.x-final

comment:2 Changed 19 months ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

It is impossible that we will fix all 277 currently open 028 tickets before 028 releases. Time to move some out. This is my first pass through the "new" and "reopened" tickets, looking for things to move to ???.

comment:3 Changed 9 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:4 Changed 8 months ago by jryans

Cc: jryans@… added
Owner: set to jryans
Status: newassigned

comment:5 Changed 8 months ago by jryans

Status: assignedneeds_review

comment:6 Changed 8 months ago by nickm

Milestone: Tor: 0.3.???Tor: 0.3.0.x-final

comment:7 Changed 8 months ago by arma

Is there some way to fix it, and tolerate both for now or something, so in the distant future we can get rid of the typo?

comment:8 Changed 8 months ago by arma

Cc: atagar added

Damian, does arm or stem use the "getinfo config/names" feature? Maybe we can just fix it and break backward compatibility? :)

comment:9 Changed 8 months ago by teor

Since it's an output of the control protocol, we have to choose one or the other.

I suggest we fix the bug in 0.3.0, and note that it's a breaking change, and then say in the control spec that controllers should accept both, noting the version that the fix was made in (0.3.0.1-alpha, if we're lucky).

comment:10 Changed 8 months ago by teor

Status: needs_reviewneeds_revision

comment:11 Changed 8 months ago by atagar

Damian, does arm or stem use the "getinfo config/names" feature? Maybe we can just fix it and break backward compatibility? :)

Yes, both Stem and Nyx use 'GETINFO config/names'. Its used for tor-prompt's auto-completion for instance...

https://gitweb.torproject.org/stem.git/tree/stem/interpreter/autocomplete.py#n40

comment:12 Changed 8 months ago by nickm

It's not the end of the world to leave a misspelled string in an interface. HTTP has "Referer", after all.

comment:13 Changed 8 months ago by jryans

Status: needs_revisionneeds_information

It seems nice to try fixing the misspelling if it's possible to do so. Maybe the community of Tor control protocol clients is small enough that it's within reason to try...?

Doing a quick skim of Stem and Nyx, it looks like:

  • Stem only uses the config name from getinfo config/names, not the type, so it would be unaffected by the proposed change here
  • Nyx does use the config type for a few cases, but seems to ignore this specific type (currently misspelled as Dependant)

atagar, is this analysis correct?

comment:14 Changed 8 months ago by atagar

Hi jryans, sorry about the delay. Was down for a week with a cold. Yup, looks right to me. I'm not spotting anywhere in Stem or Nyx where renaming this would impact us.

comment:15 Changed 8 months ago by jryans

Status: needs_informationneeds_review

Thanks for the confirmation atagar!

Here's a different branch where I've make a breaking change to fix the spelling as teor suggested in comment 9, since it appears to be safe for the most common control protocol clients.

tor: https://github.com/torproject/tor/compare/master...jryans:dependant-corrected
torspec: https://github.com/torproject/torspec/compare/master...jryans:dependant-corrected

comment:16 Changed 8 months ago by teor

Status: needs_reviewneeds_revision
Version: Tor: unspecified

Ok, one fix needed:

Replace "Tor XXX" with "Tor 0.3.0.2-alpha", as our best guess at the version this will be merged into.

comment:17 Changed 8 months ago by jryans

Status: needs_revisionmerge_ready

Okay, updated with the expected fix version. (I assume another round of review is not needed for this change.)

comment:18 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.