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.

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

Custom repo names #382

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
[submodule "test/fixtures/test_git_repo"]
path = test/fixtures/test_git_repo
url = git://github.com/ooyala/barkeep_integration_tests.git
[submodule "test/fixtures/base/test_git_repo"]
path = test/fixtures/base/test_git_repo
url = [email protected]:michaelstorm/barkeep_compound_name_integration_tests.git
6 changes: 3 additions & 3 deletions barkeep_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -297,10 +297,10 @@ def ensure_required_params(*required_params)
end
end

get "/commits/:repo_name/:sha" do
get %r{^/commits/(.+)/(.+)$} do
MetaRepo.instance.scan_for_new_repos
repo_name = params[:repo_name]
sha = params[:sha]
repo_name = params[:captures][0]
sha = params[:captures][1]
halt 404, "No such repository: #{repo_name}" unless GitRepo[:name => repo_name]
commit = MetaRepo.instance.db_commit(repo_name, sha)
unless commit
Expand Down
4 changes: 2 additions & 2 deletions client/lib/barkeep/commit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ def self.commit(configuration)

def self.parse_commit(commit_specification)
case commit_specification
when %r{^[^/]+/#{SHA_REGEX}$} # foo/abc123
commit_specification.split "/"
when %r{^(\S+)/(#{SHA_REGEX})$} # foo/abc123
[$1, $2]
Copy link
Contributor

Choose a reason for hiding this comment

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

These magic vars are hard to read. Use Regexp.last_match instead.

when /^#{SHA_REGEX}$/ # abc123
repo = File.basename(`git rev-parse --show-toplevel`).strip
if repo.empty?
Expand Down
3 changes: 2 additions & 1 deletion lib/admin_routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,9 @@ class BarkeepServer < Sinatra::Base
# - url
post "/admin/repos/create_new_repo" do
halt 400, "'url' is required." if (params[:url] || "").strip.empty?
halt 400, "'name' is required." if (params[:name] || "").strip.empty?
begin
add_repo params[:url]
add_repo params[:url], params[:name]
rescue RuntimeError => e
halt 400, e.message
end
Expand Down
5 changes: 2 additions & 3 deletions lib/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@
require "resque_jobs/clone_new_repo"

module Api
def add_repo(url)
def add_repo(repo_url, repo_name)
raise "This is not a valid URL." unless Addressable::URI.parse(url)
repo_name = File.basename(url, ".*")
repo_path = File.join(REPOS_ROOT, repo_name)
raise "There is already a folder named \"#{repo_name}\" in #{REPOS_ROOT}." if File.exists?(repo_path)
Resque.enqueue(CloneNewRepo, repo_name, url)
Resque.enqueue(CloneNewRepo, repo_name, repo_url)
end

# Generate a random API key or API secret for a user.
Expand Down
21 changes: 13 additions & 8 deletions lib/api_routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ def api_error(status, message)
post "/api/add_repo" do
ensure_required_params :url
begin
add_repo params[:url]
url = params[:url]
name = params[:name] || File.basename(url, ".*")
add_repo url, name
rescue RuntimeError => e
api_error 400, e.message
end
Expand All @@ -49,30 +51,33 @@ def api_error(status, message)
nil
end

get "/api/commits/:repo_name/:sha" do
get %r{^/api/commits/(.+)/(.+)$} do
fields = params[:fields] ? params[:fields].split(",") : nil
repo_name = params[:captures][0]
sha = params[:captures][1]
begin
commit = Commit.prefix_match params[:repo_name], params[:sha]
commit = Commit.prefix_match repo_name, sha
rescue RuntimeError => e
api_error 404, e.message
end
format_commit_data(commit, params[:repo_name], fields).to_json
format_commit_data(commit, repo_name, fields).to_json
end

# NOTE(caleb): Large GET requests are rejected by the Ruby web servers we use. (Unicorn, in particular,
# doesn't seem to like paths > 1k and rejects them silently.) Hence, to batch-request commit data, we must
# use a POST.
post "/api/commits/:repo_name" do
post %r{^/api/commits/(.+)$} do
shas = params[:shas].split(",")
fields = params[:fields] ? params[:fields].split(",") : nil
repo_name = params[:captures][0]
commits = {}
shas.each do |sha|
begin
commit = Commit.prefix_match params[:repo_name], sha
commit = Commit.prefix_match repo_name, sha
rescue RuntimeError => e
api_error 404, e.message
end
commits[commit.sha] = format_commit_data(commit, params[:repo_name], fields)
commits[commit.sha] = format_commit_data(commit, repo_name, fields)
end
commits.to_json
end
Expand All @@ -86,7 +91,7 @@ def format_commit_data(commit, repo_name, fields)
:approved_by => commit.approved? ? "#{approver.name} <#{approver.email}>" : nil,
:approved_at => commit.approved? ? commit.approved_at.to_i : nil,
:comment_count => commit.comment_count,
:link => "http://#{BARKEEP_HOSTNAME}/commits/#{params[:repo_name]}/#{commit.sha}"
:link => "http://#{BARKEEP_HOSTNAME}/commits/#{repo_name}/#{commit.sha}"
}
fields ? commit_data.select { |key, value| fields.include? key.to_s } : commit_data
end
Expand Down
33 changes: 23 additions & 10 deletions lib/meta_repo.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,27 +26,41 @@ def configure(logger, repos_root)
attr_reader :repos

def initialize(repos_root)
@repos_root = repos_root
@repos_root = Pathname.new(repos_root).realdirpath.to_s
Thread.abort_on_exception = true
load_repos
end

# Loads in any new repos from the repos_root.
def scan_for_new_repos() load_repos if repos_out_of_date? end

def all_repo_names_recurse(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can find all .git folders using Dir.glob, and then you can just iterate over the matches, obviating the need for a recursive helper.

if File.exist?("#{path}/.git")
[Pathname.new(path).relative_path_from(Pathname.new(@repos_root)).to_s]
else
names = []
repo_paths = Dir.glob("#{path}/*/")
repo_paths.each do |sub_path|
names += all_repo_names_recurse(sub_path)
end
names
end
end

def all_repo_names
all_repo_names_recurse(@repos_root)
end

def load_repos
@repos = []
@repo_names_and_ids_to_repos = {}
@repo_name_to_id = {}

repo_paths = Dir.glob("#{@repos_root}/*/")

repo_paths.each do |path|
path = Pathname.new(path).realpath.to_s # Canonical path
name = File.basename(path)
all_repo_names.each do |name|
path = Pathname.new("#{@repos_root}/#{name}").realpath.to_s # Canonical path
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why pathname.realpath instead of File.expand_path?

id = GitRepo.find_or_create(:name => name, :path => path).id
grit_repo = create_grit_repo_for_name(name)
next unless grit_repo && grit_repo.has_refs?
return unless grit_repo && grit_repo.has_refs?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why return instead of next?

@repos << grit_repo
@repo_name_to_id[name] = id
@repo_names_and_ids_to_repos[name] = grit_repo
Expand All @@ -70,7 +84,7 @@ def grit_commit(repo_name_or_id, sha)
grit_commit = grit_repo.commit(sha)
return nil unless grit_commit

grit_commit.repo_name = File.basename(grit_repo.working_dir)
grit_commit.repo_name = Pathname.new(grit_repo.working_dir).relative_path_from(Pathname.new(@repos_root)).to_s
grit_commit
end

Expand Down Expand Up @@ -353,8 +367,7 @@ def self.git_command_options(search_options)
private

def repos_out_of_date?
repo_names = Dir.glob("#{@repos_root}/*/").map { |path| File.basename(path) }
Set.new(repo_names) != Set.new(@repos.map(&:name))
Set.new(all_repo_names) != Set.new(@repos.map(&:name))
end

# Creates a new Grit::Repo object for the given path.
Expand Down
16 changes: 14 additions & 2 deletions public/coffee/repos.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,26 @@ window.Repos =
init: ->
$("button#clone").click =>
repoUrl = $("#newRepoUrl").val()
repoName = $("#newRepoName").val()
$.ajax
type: "post"
url: "/admin/repos/create_new_repo"
data: { url: repoUrl }
data: { url: repoUrl, name: repoName }
dataType: "json"
success: => @showConfirmationMessage("#{repoUrl} has been scheduled to be cloned.")
success: => @showConfirmationMessage("#{repoUrl} has been scheduled to be cloned as #{repoName}.")
error: (response) => @showConfirmationMessage(response.responseText)

$("#newRepoUrl").bind "propertychange keyup input paste", =>
repoUrl = $("#newRepoUrl").val()
match = /.*(?:\/|:)([^/:]+)\/*$/.exec repoUrl
if match
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to a circuit: return unless match

name = match[1]
name = (/\s*([^\s]+)/.exec name)[1]
suffix_match = /(.*)\.git$/.exec name
if suffix_match
name = suffix_match[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine the if and its body into one line.

$("#newRepoName").val(name)

$(".trash").click (e) =>
repoRow = $(e.target).closest("tr")
repoName = repoRow.find("td:nth-of-type(1)").html()
Expand Down
4 changes: 3 additions & 1 deletion public/css/admin.scss
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,12 @@
#cloneForm {
label { font-weight: bold; }
input {
width: 450px;
font-size: 18px;
margin: 8px 0;
}
#newRepoUrl {
width: 450px;
}
button {
padding: 4px 6px;
font-size: 14px;
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/base/test_git_repo
Submodule test_git_repo added at 88e5ee
36 changes: 20 additions & 16 deletions test/integration/clone_new_repo_integration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,38 +10,42 @@ class CloneNewRepoIntegrationTest < Scope::TestCase
include IntegrationTestHelper

setup_once do
@@test_repo_name = "clone_repo_integration_test"
@@test_repo_names = ["clone_repo_integration_test", "base/clone_repo_integration_test"]
@@bad_test_repo_names = ["bad_repo_integration_test", "bad_base/bad_repo_integration_test"]

# We could also clone this repo from [email protected]:ooyala/barkeep_integration_tests.git, but that takes
# about 5 seconds. Cloning from the local disk is much faster.
@@test_repo_url = File.expand_path(File.join(File.dirname(__FILE__), "../fixtures/test_git_repo"))

delete_repo(@@test_repo_name)
@@test_repo_names.each { |repo_name| delete_repo(repo_name) }
end

teardown_once do
delete_repo(@@test_repo_name)
@@test_repo_names.each { |repo_name| delete_repo(repo_name) }
end

should "clone a new repo to the correct place on disk" do
assert_equal false, File.exists?(repo_path(@@test_repo_name))
CloneNewRepo.perform(@@test_repo_name, @@test_repo_url)
assert File.exists?(repo_path(@@test_repo_name)), "repo should've been cloned, but it's not on disk."

# Ensure it successfully completed a full clone (i.e. commits can now be read by Grit).
first_commit = "65a0045e7ac5329d76e6644aa2fb427b78100a7b"
grit = Grit::Repo.new(repo_path(@@test_repo_name))
assert_equal 1, grit.commits(first_commit).size
@@test_repo_names.each do |repo_name|
assert_equal false, File.exists?(repo_path(repo_name))
CloneNewRepo.perform(repo_name, @@test_repo_url)
assert File.exists?(repo_path(repo_name)), "repo should've been cloned, but it's not on disk."

# Ensure it successfully completed a full clone (i.e. commits can now be read by Grit).
first_commit = "65a0045e7ac5329d76e6644aa2fb427b78100a7b"
grit = Grit::Repo.new(repo_path(repo_name))
assert_equal 1, grit.commits(first_commit).size
end
end

should "if a repo cannot be cloned, no trace of it should be left on disk" do
repo_name = "bad_repo_integration_test"
FileUtils.rm_rf(repo_path(repo_name))
CloneNewRepo.perform(repo_name, "bad_repo/url")
assert_equal false, File.exists?(repo_path(repo_name))
@@bad_test_repo_names.each do |repo_name|
FileUtils.rm_rf(repo_path(repo_name))
CloneNewRepo.perform(repo_name, "bad_repo/url")
assert_equal false, File.exists?(repo_path(repo_name))
end
end

def repo_path(repo_name) File.join(REPOS_ROOT, @@test_repo_name) end
def repo_path(repo_name) File.join(REPOS_ROOT, repo_name) end
def delete_repo(repo_name)
FileUtils.rm_rf(repo_path(repo_name)) if File.exists?(repo_path(repo_name))
end
Expand Down
6 changes: 3 additions & 3 deletions test/stub_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@ module StubHelper

# Creates a Commit.
# - user: a User who is the author of this commit.
def stub_commit(sha, user)
def stub_commit(repo_name, sha, user)
commit = Commit.new(:sha => sha)
stub(commit).git_repo { GitRepo.new(:name => "my_repo") }
stub(commit).git_repo { GitRepo.new(:name => repo_name) }
stub(commit).user { user }

commit_author = user.name.dup
stub(commit_author).user { user }
grit_commit = OpenStruct.new(
:id => sha, :sha => sha, :id_abbrev => sha,
:repo_name => "my_repo",
:repo_name => repo_name,
:short_message => "message", :author => Grit::Actor.new(user.name, user.email),
:date => Time.now, :diffs => [])
stub(commit).grit_commit { grit_commit }
Expand Down
Loading