Opened 15 months ago

Last modified 10 months ago

#27662 needs_revision defect

refactor networkstatus_parse_vote_from_string()

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt, refactor, long-functions, cthulhucode, 040-deferred-20190220
Cc: catalyst Actual Points:
Parent ID: #22408 Points:
Reviewer: Sponsor:

Description

As of 1c62adb65baa99c92f937318c452955306301643, the function is 628 lines long. It could be split into calling 3 helper functions instead.

Child Tickets

Change History (11)

comment:1 Changed 15 months ago by cyberpunks

See branch parsevotesplit1 at https://gitgud.io/onionk/tor.git

comment:2 Changed 15 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.6.x-final
Status: newneeds_review

comment:3 Changed 15 months ago by teor

Status: needs_reviewneeds_revision

Hi,

The unit tests fail CI on rust builds with:

protover/vote:
  FAIL src/test/test_protover.c:175: assert(result OP_EQ ""): <Bar=2> vs <>
  [vote FAILED]

https://travis-ci.org/teor2345/tor/jobs/428431227#L6130
https://travis-ci.org/teor2345/tor/builds/428431225

comment:4 in reply to:  3 ; Changed 15 months ago by cyberpunks

Replying to teor:

Hi,

The unit tests fail CI on rust builds with:

That was a failure from the master branch that was fixed since then, not related to this refactor. So a merge or rebase makes it go away.

Last edited 15 months ago by cyberpunks (previous) (diff)

comment:5 in reply to:  4 Changed 15 months ago by cyberpunks

What revision is needed here?

comment:6 Changed 14 months ago by teor

Status: needs_revisionneeds_review

This branch can be reviewed, but I wonder if it conflicts with other code changes that have already been merged. I guess we'll find out.

comment:7 in reply to:  6 ; Changed 14 months ago by teor

Status: needs_reviewneeds_revision

Replying to teor:

This branch can be reviewed, but I wonder if it conflicts with other code changes that have already been merged. I guess we'll find out.

This branch conflicts with some changes in master.
(We split some files as part of a refactor. Sorry about that.)

Can you please rebase it on master?
If you can't, just let us know, and we will do the rebase eventually.

comment:8 in reply to:  7 ; Changed 14 months ago by cyberpunks

Replying to teor:

This branch conflicts with some changes in master.

It actually doesn't, the branch was already rebased a good while ago.

Last edited 14 months ago by cyberpunks (previous) (diff)

comment:9 in reply to:  8 Changed 13 months ago by teor

Replying to cyberpunks:

Replying to teor:

This branch conflicts with some changes in master.

It actually doesn't, the branch was already rebased a good while ago.

I opened a pull request at:
https://github.com/torproject/tor/pull/427

GitHub says:

This branch has conflicts that must be resolved
Use the web editor or the  to resolve conflicts.
Conflicting files
src/feature/dirparse/ns_parse.c

When I try to do the merge manually, there are conflicts:

$ git checkout -b parsevotesplit1-master tp/master
Branch 'parsevotesplit1-master' set up to track remote branch 'master' from 'tp'.
Switched to a new branch 'parsevotesplit1-master'
$ git merge onionk/parsevotesplit1
Auto-merging src/feature/dirparse/ns_parse.h
Auto-merging src/feature/dirparse/ns_parse.c
CONFLICT (content): Merge conflict in src/feature/dirparse/ns_parse.c
Recorded preimage for 'src/feature/dirparse/ns_parse.c'
Automatic merge failed; fix conflicts and then commit the result.
Exit 1

comment:10 Changed 13 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:11 Changed 10 months ago by nickm

Keywords: 040-deferred-20190220 added
Milestone: Tor: 0.4.0.x-finalTor: unspecified

Deferring 51 tickets from 0.4.0.x-final. Tagging them with 040-deferred-20190220 for visibility. These are the tickets that did not get 040-must, 040-can, or tor-ci.

Note: See TracTickets for help on using tickets.