GitHub PullRequestReviewCommentEvent at 2014-06-24 16:45:23
Repository: libgit2
Actor: arrbee
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/libgit2/libgit2/pulls/comments/14139889","id":14139889,"diff_hunk":"@@ -26,20 +30,126 @@ size_t git_mwindow__mapped_limit = DEFAULT_MAPPED_LIMIT;\n /* Whenever you want to read or modify this, grab git__mwindow_mutex */\n static git_mwindow_ctl mem_ctl;\n \n-/*\n- * Free all the windows in a sequence, typically because we're done\n- * with the file\n+/* Global list of mwindow files, to open packs once across repos */\n+git_strmap *git__pack_cache = NULL;\n+\n+/**\n+ * Run under mwindow lock\n  */\n-void git_mwindow_free_all(git_mwindow_file *mwf)\n+int git_mwindow_files_init(void)\n {\n-\tgit_mwindow_ctl *ctl = &mem_ctl;\n-\tsize_t i;\n+\tif (git__pack_cache)\n+\t\treturn 0;\n+\n+\treturn git_strmap_alloc(&git__pack_cache);\n+}\n+\n+void git_mwindow_files_free(void)\n+{\n+\tgit_strmap *tmp = git__pack_cache;\n+\n+\tgit__pack_cache = NULL;\n+\tgit_strmap_free(tmp);\n+}\n+\n+int git_mwindow_get_pack(struct git_pack_file **out, const char *path)\n+{\n+\tint error;\n+\tchar *packname;\n+\tgit_strmap_iter pos;\n+\tstruct git_pack_file *pack;\n+\n+\tif ((error = git_packfile__name(&packname, path)) < 0)\n+\t\treturn error;\n+\n+\tif (git_mutex_lock(&git__mwindow_mutex) < 0)\n+\t\treturn -1;\n+\n+\tif (git_mwindow_files_init() < 0) {\n+\t\tgit_mutex_unlock(&git__mwindow_mutex);\n+\t\treturn -1;\n+\t}\n+\n+\tpos = git_strmap_lookup_index(git__pack_cache, packname);\n+\tgit__free(packname);\n+\n+\tif (git_strmap_valid_index(git__pack_cache, pos)) {\n+\t\tpack = git_strmap_value_at(git__pack_cache, pos);\n+\t\tgit_atomic_inc(&pack->refcount);\n+\n+\t\tgit_mutex_unlock(&git__mwindow_mutex);\n+\t\t*out = pack;\n+\t\treturn 0;\n+\t}\n+\n+\t/* If we didn't find it, we need to create it */\n+\tif ((error = git_packfile_alloc(&pack, path)) < 0) {\n+\t\tgit_mutex_unlock(&git__mwindow_mutex);\n+\t\treturn error;\n+\t}\n+\n+\tgit_atomic_inc(&pack->refcount);\n+\n+\tgit_strmap_insert(git__pack_cache, pack->pack_name, pack, error);\n+\tgit_mutex_unlock(&git__mwindow_mutex);\n+\n+\tif (error < 0)\n+\t\treturn -1;\n \n+\t*out = pack;\n+\treturn 0;\n+}\n+\n+int git_mwindow_put_pack(struct git_pack_file *pack)","path":"src/mwindow.c","position":91,"original_position":91,"commit_id":"b3b66c57930358467395fc3a5bca87edefd25cf4","original_commit_id":"b3b66c57930358467395fc3a5bca87edefd25cf4","user":{"login":"arrbee","id":103350,"avatar_url":"https://avatars.githubusercontent.com/u/103350?","gravatar_id":"66e82696b38beea60f0e0afe40b0c870","url":"https://api.github.com/users/arrbee","html_url":"https://github.com/arrbee","followers_url":"https://api.github.com/users/arrbee/followers","following_url":"https://api.github.com/users/arrbee/following{/other_user}","gists_url":"https://api.github.com/users/arrbee/gists{/gist_id}","starred_url":"https://api.github.com/users/arrbee/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arrbee/subscriptions","organizations_url":"https://api.github.com/users/arrbee/orgs","repos_url":"https://api.github.com/users/arrbee/repos","events_url":"https://api.github.com/users/arrbee/events{/privacy}","received_events_url":"https://api.github.com/users/arrbee/received_events","type":"User","site_admin":true},"body":"There are a couple of places in this function where we return an error without setting an error message, but looking at how this is actually used, the error return value is never looked at - this is just supposed to release and the error return is ignored. What do you think about making this a `void` function instead?\r\n\r\nRelated, since some error paths do set an error and some don't and the calling code doesn't look at the return value, this can lead to misattributed error messages where the global error message is left set even though the caller ignored the error.","created_at":"2014-06-24T16:45:23Z","updated_at":"2014-06-24T16:45:23Z","html_url":"https://github.com/libgit2/libgit2/pull/2429#discussion_r14139889","pull_request_url":"https://api.github.com/repos/libgit2/libgit2/pulls/2429","_links":{"self":{"href":"https://api.github.com/repos/libgit2/libgit2/pulls/comments/14139889"},"html":{"href":"https://github.com/libgit2/libgit2/pull/2429#discussion_r14139889"},"pull_request":{"href":"https://api.github.com/repos/libgit2/libgit2/pulls/2429"}}},"pull_request":{"url":"https://api.github.com/repos/libgit2/libgit2/pulls/2429","id":17301556,"html_url":"https://github.com/libgit2/libgit2/pull/2429","diff_url":"https://github.com/libgit2/libgit2/pull/2429.diff","patch_url":"https://github.com/libgit2/libgit2/pull/2429.patch","issue_url":"https://api.github.com/repos/libgit2/libgit2/issues/2429","number":2429,"state":"closed","title":"Share packs across repository instances","user":{"login":"carlosmn","id":335443,"avatar_url":"https://avatars.githubusercontent.com/u/335443?","gravatar_id":"023e1295851b018af03706f391b76931","url":"https://api.github.com/users/carlosmn","html_url":"https://github.com/carlosmn","followers_url":"https://api.github.com/users/carlosmn/followers","following_url":"https://api.github.com/users/carlosmn/following{/other_user}","gists_url":"https://api.github.com/users/carlosmn/gists{/gist_id}","starred_url":"https://api.github.com/users/carlosmn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/carlosmn/subscriptions","organizations_url":"https://api.github.com/users/carlosmn/orgs","repos_url":"https://api.github.com/users/carlosmn/repos","events_url":"https://api.github.com/users/carlosmn/events{/privacy}","received_events_url":"https://api.github.com/users/carlosmn/received_events","type":"User","site_admin":false},"body":"Opening the same repository multiple times will currently open the same\r\nfile multiple times, as well as map the same region of the file multiple\r\ntimes. This is not necessary, as the packfile data is immutable.\r\n\r\nInstead of opening and closing packfiles directly, introduce an\r\nindirection and allocate packfiles globally. This does mean locking on\r\neach packfile open, but we already use this lock for the global mwindow\r\nlist so it doesn't introduce a new contention point.\r\n\r\n--- \r\n\r\nWe probably want to add a test to actually make sure we're allocating the right number of packs, but I wanted to get some early feedback.\r\n\r\nOne thing that this means is that once you have the pack open, we won't check for permissions anymore, so if you open a repo as root and then drop to a normal user, we would still have it open as root, not that anybody should be doing anything of the sort.\r\n\r\n/cc @vmg ","created_at":"2014-06-18T15:48:11Z","updated_at":"2014-06-24T16:45:23Z","closed_at":"2014-06-24T15:47:54Z","merged_at":null,"merge_commit_sha":"8e2bf19d39e60e330362b56ac86bbad19d62a38e","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/libgit2/libgit2/pulls/2429/commits","review_comments_url":"https://api.github.com/repos/libgit2/libgit2/pulls/2429/comments","review_comment_url":"https://api.github.com/repos/libgit2/libgit2/pulls/comments/{number}","comments_url":"https://api.github.com/repos/libgit2/libgit2/issues/2429/comments","statuses_url":"https://api.github.com/repos/libgit2/libgit2/statuses/b3b66c57930358467395fc3a5bca87edefd25cf4","head":{"label":"libgit2:cmn/global-mwf","ref":"cmn/global-mwf","sha":"b3b66c57930358467395fc3a5bca87edefd25cf4","user":{"login":"libgit2","id":394745,"avatar_url":"https://avatars.githubusercontent.com/u/394745?","gravatar_id":"1c56acccc41c591705dc92da5f6ab603","url":"https://api.github.com/users/libgit2","html_url":"https://github.com/libgit2","followers_url":"https://api.github.com/users/libgit2/followers","following_url":"https://api.github.com/users/libgit2/following{/other_user}","gists_url":"https://api.github.com/users/libgit2/gists{/gist_id}","starred_url":"https://api.github.com/users/libgit2/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/libgit2/subscriptions","organizations_url":"https://api.github.com/users/libgit2/orgs","repos_url":"https://api.github.com/users/libgit2/repos","events_url":"https://api.github.com/users/libgit2/events{/privacy}","received_events_url":"https://api.github.com/users/libgit2/received_events","type":"Organization","site_admin":false},"repo":{"id":901662,"name":"libgit2","full_name":"libgit2/libgit2","owner":{"login":"libgit2","id":394745,"avatar_url":"https://avatars.githubusercontent.com/u/394745?","gravatar_id":"1c56acccc41c591705dc92da5f6ab603","url":"https://api.github.com/users/libgit2","html_url":"https://github.com/libgit2","followers_url":"https://api.github.com/users/libgit2/followers","following_url":"https://api.github.com/users/libgit2/following{/other_user}","gists_url":"https://api.github.com/users/libgit2/gists{/gist_id}","starred_url":"https://api.github.com/users/libgit2/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/libgit2/subscriptions","organizations_url":"https://api.github.com/users/libgit2/orgs","repos_url":"https://api.github.com/users/libgit2/repos","events_url":"https://api.github.com/users/libgit2/events{/privacy}","received_events_url":"https://api.github.com/users/libgit2/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/libgit2/libgit2","description":"The Library","fork":false,"url":"https://api.github.com/repos/libgit2/libgit2","forks_url":"https://api.github.com/repos/libgit2/libgit2/forks","keys_url":"https://api.github.com/repos/libgit2/libgit2/keys{/key_id}","collaborators_url":"https://api.github.com/repos/libgit2/libgit2/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/libgit2/libgit2/teams","hooks_url":"https://api.github.com/repos/libgit2/libgit2/hooks","issue_events_url":"https://api.github.com/repos/libgit2/libgit2/issues/events{/number}","events_url":"https://api.github.com/repos/libgit2/libgit2/events","assignees_url":"https://api.github.com/repos/libgit2/libgit2/assignees{/user}","branches_url":"https://api.github.com/repos/libgit2/libgit2/branches{/branch}","tags_url":"https://api.github.com/repos/libgit2/libgit2/tags","blobs_url":"https://api.github.com/repos/libgit2/libgit2/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/libgit2/libgit2/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/libgit2/libgit2/git/refs{/sha}","trees_url":"https://api.github.com/repos/libgit2/libgit2/git/trees{/sha}","statuses_url":"https://api.github.com/repos/libgit2/libgit2/statuses/{sha}","languages_url":"https://api.github.com/repos/libgit2/libgit2/languages","stargazers_url":"https://api.github.com/repos/libgit2/libgit2/stargazers","contributors_url":"https://api.github.com/repos/libgit2/libgit2/contributors","subscribers_url":"https://api.github.com/repos/libgit2/libgit2/subscribers","subscription_url":"https://api.github.com/repos/libgit2/libgit2/subscription","commits_url":"https://api.github.com/repos/libgit2/libgit2/commits{/sha}","git_commits_url":"https://api.github.com/repos/libgit2/libgit2/git/commits{/sha}","comments_url":"https://api.github.com/repos/libgit2/libgit2/comments{/number}","issue_comment_url":"https://api.github.com/repos/libgit2/libgit2/issues/comments/{number}","contents_url":"https://api.github.com/repos/libgit2/libgit2/contents/{+path}","compare_url":"https://api.github.com/repos/libgit2/libgit2/compare/{base}...{head}","merges_url":"https://api.github.com/repos/libgit2/libgit2/merges","archive_url":"https://api.github.com/repos/libgit2/libgit2/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/libgit2/libgit2/downloads","issues_url":"https://api.github.com/repos/libgit2/libgit2/issues{/number}","pulls_url":"https://api.github.com/repos/libgit2/libgit2/pulls{/number}","milestones_url":"https://api.github.com/repos/libgit2/libgit2/milestones{/number}","notifications_url":"https://api.github.com/repos/libgit2/libgit2/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/libgit2/libgit2/labels{/name}","releases_url":"https://api.github.com/repos/libgit2/libgit2/releases{/id}","created_at":"2010-09-10T16:17:48Z","updated_at":"2014-06-24T15:57:44Z","pushed_at":"2014-06-24T15:57:43Z","git_url":"git://github.com/libgit2/libgit2.git","ssh_url":"git@github.com:libgit2/libgit2.git","clone_url":"https://github.com/libgit2/libgit2.git","svn_url":"https://github.com/libgit2/libgit2","homepage":"http://libgit2.github.com","size":80574,"stargazers_count":4541,"watchers_count":4541,"language":"C","has_issues":true,"has_downloads":true,"has_wiki":false,"forks_count":999,"mirror_url":null,"open_issues_count":110,"forks":999,"open_issues":110,"watchers":4541,"default_branch":"master"}},"base":{"label":"libgit2:development","ref":"development","sha":"1589aa0c4d48fb130d8a5db28c45cd3d173cde6d","user":{"login":"libgit2","id":394745,"avatar_url":"https://avatars.githubusercontent.com/u/394745?","gravatar_id":"1c56acccc41c591705dc92da5f6ab603","url":"https://api.github.com/users/libgit2","html_url":"https://github.com/libgit2","followers_url":"https://api.github.com/users/libgit2/followers","following_url":"https://api.github.com/users/libgit2/following{/other_user}","gists_url":"https://api.github.com/users/libgit2/gists{/gist_id}","starred_url":"https://api.github.com/users/libgit2/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/libgit2/subscriptions","organizations_url":"https://api.github.com/users/libgit2/orgs","repos_url":"https://api.github.com/users/libgit2/repos","events_url":"https://api.github.com/users/libgit2/events{/privacy}","received_events_url":"https://api.github.com/users/libgit2/received_events","type":"Organization","site_admin":false},"repo":{"id":901662,"name":"libgit2","full_name":"libgit2/libgit2","owner":{"login":"libgit2","id":394745,"avatar_url":"https://avatars.githubusercontent.com/u/394745?","gravatar_id":"1c56acccc41c591705dc92da5f6ab603","url":"https://api.github.com/users/libgit2","html_url":"https://github.com/libgit2","followers_url":"https://api.github.com/users/libgit2/followers","following_url":"https://api.github.com/users/libgit2/following{/other_user}","gists_url":"https://api.github.com/users/libgit2/gists{/gist_id}","starred_url":"https://api.github.com/users/libgit2/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/libgit2/subscriptions","organizations_url":"https://api.github.com/users/libgit2/orgs","repos_url":"https://api.github.com/users/libgit2/repos","events_url":"https://api.github.com/users/libgit2/events{/privacy}","received_events_url":"https://api.github.com/users/libgit2/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/libgit2/libgit2","description":"The Library","fork":false,"url":"https://api.github.com/repos/libgit2/libgit2","forks_url":"https://api.github.com/repos/libgit2/libgit2/forks","keys_url":"https://api.github.com/repos/libgit2/libgit2/keys{/key_id}","collaborators_url":"https://api.github.com/repos/libgit2/libgit2/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/libgit2/libgit2/teams","hooks_url":"https://api.github.com/repos/libgit2/libgit2/hooks","issue_events_url":"https://api.github.com/repos/libgit2/libgit2/issues/events{/number}","events_url":"https://api.github.com/repos/libgit2/libgit2/events","assignees_url":"https://api.github.com/repos/libgit2/libgit2/assignees{/user}","branches_url":"https://api.github.com/repos/libgit2/libgit2/branches{/branch}","tags_url":"https://api.github.com/repos/libgit2/libgit2/tags","blobs_url":"https://api.github.com/repos/libgit2/libgit2/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/libgit2/libgit2/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/libgit2/libgit2/git/refs{/sha}","trees_url":"https://api.github.com/repos/libgit2/libgit2/git/trees{/sha}","statuses_url":"https://api.github.com/repos/libgit2/libgit2/statuses/{sha}","languages_url":"https://api.github.com/repos/libgit2/libgit2/languages","stargazers_url":"https://api.github.com/repos/libgit2/libgit2/stargazers","contributors_url":"https://api.github.com/repos/libgit2/libgit2/contributors","subscribers_url":"https://api.github.com/repos/libgit2/libgit2/subscribers","subscription_url":"https://api.github.com/repos/libgit2/libgit2/subscription","commits_url":"https://api.github.com/repos/libgit2/libgit2/commits{/sha}","git_commits_url":"https://api.github.com/repos/libgit2/libgit2/git/commits{/sha}","comments_url":"https://api.github.com/repos/libgit2/libgit2/comments{/number}","issue_comment_url":"https://api.github.com/repos/libgit2/libgit2/issues/comments/{number}","contents_url":"https://api.github.com/repos/libgit2/libgit2/contents/{+path}","compare_url":"https://api.github.com/repos/libgit2/libgit2/compare/{base}...{head}","merges_url":"https://api.github.com/repos/libgit2/libgit2/merges","archive_url":"https://api.github.com/repos/libgit2/libgit2/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/libgit2/libgit2/downloads","issues_url":"https://api.github.com/repos/libgit2/libgit2/issues{/number}","pulls_url":"https://api.github.com/repos/libgit2/libgit2/pulls{/number}","milestones_url":"https://api.github.com/repos/libgit2/libgit2/milestones{/number}","notifications_url":"https://api.github.com/repos/libgit2/libgit2/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/libgit2/libgit2/labels{/name}","releases_url":"https://api.github.com/repos/libgit2/libgit2/releases{/id}","created_at":"2010-09-10T16:17:48Z","updated_at":"2014-06-24T15:57:44Z","pushed_at":"2014-06-24T15:57:43Z","git_url":"git://github.com/libgit2/libgit2.git","ssh_url":"git@github.com:libgit2/libgit2.git","clone_url":"https://github.com/libgit2/libgit2.git","svn_url":"https://github.com/libgit2/libgit2","homepage":"http://libgit2.github.com","size":80574,"stargazers_count":4541,"watchers_count":4541,"language":"C","has_issues":true,"has_downloads":true,"has_wiki":false,"forks_count":999,"mirror_url":null,"open_issues_count":110,"forks":999,"open_issues":110,"watchers":4541,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/libgit2/libgit2/pulls/2429"},"html":{"href":"https://github.com/libgit2/libgit2/pull/2429"},"issue":{"href":"https://api.github.com/repos/libgit2/libgit2/issues/2429"},"comments":{"href":"https://api.github.com/repos/libgit2/libgit2/issues/2429/comments"},"review_comments":{"href":"https://api.github.com/repos/libgit2/libgit2/pulls/2429/comments"},"review_comment":{"href":"https://api.github.com/repos/libgit2/libgit2/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/libgit2/libgit2/pulls/2429/commits"},"statuses":{"href":"https://api.github.com/repos/libgit2/libgit2/statuses/b3b66c57930358467395fc3a5bca87edefd25cf4"}}}}