Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid network timeout on large histories #7

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Avoid network timeout on large histories #7

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a515ff5
add support for when extracting HEAD:1 times out the network connecti…
Aug 23, 2013
e6c88ca
add support for when extracting HEAD:1 times out the network connecti…
Aug 23, 2013
744be6c
Merge pull request #1 from brlcad/unstuck_svn_log
brlcad Aug 23, 2013
be678aa
PeterDP noted that run() does not return a truth value; read a snippe…
Aug 26, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/scm/adapters/svn_chain/chain.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,16 @@ def first_commit(after=0)
# Returns the first commit with a revision number greater than the provided revision number
def next_revision_xml(after=0)
return "<?xml?>" if after.to_i >= head_token
run "svn log --trust-server-cert --non-interactive --verbose --xml --stop-on-copy -r #{after.to_i+1}:#{final_token || 'HEAD'} --limit 1 #{opt_auth} '#{SvnAdapter.uri_encode(File.join(self.root, self.branch_name))}@#{final_token || 'HEAD'}'"
back_to_front = "svn log --trust-server-cert --non-interactive --verbose --xml --stop-on-copy -r #{after.to_i+1}:#{final_token || 'HEAD'} --limit 1 #{opt_auth} '#{SvnAdapter.uri_encode(File.join(self.root, self.branch_name))}@#{final_token || 'HEAD'}'"
front_to_back = "svn log --trust-server-cert --non-interactive --verbose --xml --stop-on-copy -r #{final_token || 'HEAD'}:#{after.to_i+1} #{opt_auth} '#{SvnAdapter.uri_encode(File.join(self.root, self.branch_name))}@#{final_token || 'HEAD'}'"
# if the repository history is so extensive or
# the remote server so slow that the network
# connection times out before there is a
# response, use a slower and more resource
# intensive method that should always succeed:
# receive the revisions in order and extract
# the last entry.
run(back_to_front) || Scm::Parsers::SvnXmlParser.parse(run(front_to_back)).last
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that run(back_to_front) will return a truthy value. Please refer to Scm::Adapters::AbstractAdapter. This calls Shellout#run, which calls Shellout.execute, which returns an array of [status, outbuf.string, errbuf.string]

It would be very helpful to have test coverage of this next_revision_xml method with the different pathways covered.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The perils of writing code one cannot easily test. Fully concur that there's a problem as written. Looks like I misread a similar snippet in lib/scm/adapters/git/head.rb that let me to think a return code would result.

What about pushing the conditional into the command begin run, then making the callee always just check parse().last?
run("#{back_to_front} || #{front_to_back}")

I can redo a push with that change if the explanation is unclear.

end

# If the passed diff represents the wholesale movement of the entire
Expand Down