From cd71098110e209c686ad99a1d78499b20576c5c8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 26 Sep 2011 14:58:42 -0700 Subject: [PATCH] Detect commits by hash relationships Summary: When we discover a new commit and it has a known local commit or tree hash, mark it committed. This supports Mercurial and Git-Immutable workflows, and improves hybrid-Git-Mutable workflows and covers some cases where poeple just make mistakes or whatever. Test Plan: Parsed Mercurial, Git and SVN commits. Reviewers: Makinde Reviewed By: Makinde CC: aran, Makinde Differential Revision: 963 --- ...torRepositoryCommitMessageParserWorker.php | 42 ++++++++++++++++++- .../commitmessageparser/base/__init__.php | 2 + ...RepositoryGitCommitMessageParserWorker.php | 25 ++++++++--- .../commitmessageparser/git/__init__.php | 2 +- ...toryMercurialCommitMessageParserWorker.php | 13 +++++- .../mercurial/__init__.php | 1 + ...RepositorySvnCommitMessageParserWorker.php | 7 ++++ 7 files changed, 82 insertions(+), 10 deletions(-) diff --git a/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php index 794a0d16b6..cb01c48b48 100644 --- a/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php @@ -19,6 +19,10 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker extends PhabricatorRepositoryCommitParserWorker { + abstract protected function getCommitHashes( + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit); + final protected function updateCommitData($author, $message) { $commit = $this->commit; @@ -45,13 +49,47 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $data->save(); + $conn_w = id(new DifferentialRevision())->establishConnection('w'); + + // NOTE: The `differential_commit` table has a unique ID on `commitPHID`, + // preventing more than one revision from being associated with a commit. + // Generally this is good and desirable, but with the advent of hash + // tracking we may end up in a situation where we match several different + // revisions. We just kind of ignore this and pick one, we might want to + // revisit this and do something differently. (If we match several revisions + // someone probably did something very silly, though.) + $revision_id = $data->getCommitDetail('differential.revisionID'); + if (!$revision_id) { + $hashes = $this->getCommitHashes( + $this->repository, + $this->commit); + if ($hashes) { + $sql = array(); + foreach ($hashes as $info) { + list($type, $hash) = $info; + $sql[] = qsprintf( + $conn_w, + '(type = %s AND hash = %s)', + $type, + $hash); + } + $revision = queryfx_one( + $conn_w, + 'SELECT revisionID FROM %T WHERE %Q LIMIT 1', + DifferentialRevisionHash::TABLE_NAME, + implode(' OR ', $sql)); + if ($revision) { + $revision_id = $revision['revisionID']; + } + } + } + if ($revision_id) { $revision = id(new DifferentialRevision())->load($revision_id); if ($revision) { - queryfx( - $revision->establishConnection('w'), + $conn_w, 'INSERT IGNORE INTO %T (revisionID, commitPHID) VALUES (%d, %s)', DifferentialRevision::TABLE_COMMIT, $revision->getID(), diff --git a/src/applications/repository/worker/commitmessageparser/base/__init__.php b/src/applications/repository/worker/commitmessageparser/base/__init__.php index 2646add245..59dcb12de3 100644 --- a/src/applications/repository/worker/commitmessageparser/base/__init__.php +++ b/src/applications/repository/worker/commitmessageparser/base/__init__.php @@ -7,11 +7,13 @@ phutil_require_module('phabricator', 'applications/differential/constants/action'); +phutil_require_module('phabricator', 'applications/differential/constants/revisionhash'); phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus'); phutil_require_module('phabricator', 'applications/differential/editor/comment'); phutil_require_module('phabricator', 'applications/differential/storage/revision'); phutil_require_module('phabricator', 'applications/repository/storage/commitdata'); phutil_require_module('phabricator', 'applications/repository/worker/base'); +phutil_require_module('phabricator', 'storage/qsprintf'); phutil_require_module('phabricator', 'storage/queryfx'); phutil_require_module('phutil', 'symbols'); diff --git a/src/applications/repository/worker/commitmessageparser/git/PhabricatorRepositoryGitCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/git/PhabricatorRepositoryGitCommitMessageParserWorker.php index 89cec4fc84..420c80f0bf 100644 --- a/src/applications/repository/worker/commitmessageparser/git/PhabricatorRepositoryGitCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/git/PhabricatorRepositoryGitCommitMessageParserWorker.php @@ -23,13 +23,10 @@ class PhabricatorRepositoryGitCommitMessageParserWorker PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { - $local_path = $repository->getDetail('local-path'); - // NOTE: %B was introduced somewhat recently in git's history, so pull // commit message information with %s and %b instead. - list($info) = execx( - '(cd %s && git log -n 1 --pretty=format:%%an%%x00%%s%%n%%n%%b %s)', - $local_path, + list($info) = $repository->execxLocalCommand( + 'log -n 1 --pretty=format:%%an%%x00%%s%%n%%n%%b %s', $commit->getCommitIdentifier()); list($author, $message) = explode("\0", $info); @@ -52,4 +49,22 @@ class PhabricatorRepositoryGitCommitMessageParserWorker } } + protected function getCommitHashes( + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit) { + + list($stdout) = $repository->execxLocalCommand( + 'log -n 1 --format=%s %s --', + '%T', + $commit->getCommitIdentifier()); + + $commit_hash = $commit->getCommitIdentifier(); + $tree_hash = trim($stdout); + + return array( + array(DifferentialRevisionHash::HASH_GIT_COMMIT, $commit_hash), + array(DifferentialRevisionHash::HASH_GIT_TREE, $tree_hash), + ); + } + } diff --git a/src/applications/repository/worker/commitmessageparser/git/__init__.php b/src/applications/repository/worker/commitmessageparser/git/__init__.php index 4f42580c9c..cbe011c05f 100644 --- a/src/applications/repository/worker/commitmessageparser/git/__init__.php +++ b/src/applications/repository/worker/commitmessageparser/git/__init__.php @@ -6,10 +6,10 @@ +phutil_require_module('phabricator', 'applications/differential/constants/revisionhash'); phutil_require_module('phabricator', 'applications/repository/worker/commitmessageparser/base'); phutil_require_module('phabricator', 'infrastructure/daemon/workers/storage/task'); -phutil_require_module('phutil', 'future/exec'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/repository/worker/commitmessageparser/mercurial/PhabricatorRepositoryMercurialCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/mercurial/PhabricatorRepositoryMercurialCommitMessageParserWorker.php index 70bc86d8ca..8447c068e0 100644 --- a/src/applications/repository/worker/commitmessageparser/mercurial/PhabricatorRepositoryMercurialCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/mercurial/PhabricatorRepositoryMercurialCommitMessageParserWorker.php @@ -23,8 +23,6 @@ class PhabricatorRepositoryMercurialCommitMessageParserWorker PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { - $local_path = $repository->getDetail('local-path'); - list($stdout) = $repository->execxLocalCommand( 'log --template %s --rev %s', '{author}\\n{desc}', @@ -50,4 +48,15 @@ class PhabricatorRepositoryMercurialCommitMessageParserWorker } } + protected function getCommitHashes( + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit) { + + $commit_hash = $commit->getCommitIdentifier(); + + return array( + array(DifferentialRevisionHash::HASH_MERCURIAL_COMMIT, $commit_hash), + ); + } + } diff --git a/src/applications/repository/worker/commitmessageparser/mercurial/__init__.php b/src/applications/repository/worker/commitmessageparser/mercurial/__init__.php index 1f22433b1c..a0bdf0a813 100644 --- a/src/applications/repository/worker/commitmessageparser/mercurial/__init__.php +++ b/src/applications/repository/worker/commitmessageparser/mercurial/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/differential/constants/revisionhash'); phutil_require_module('phabricator', 'applications/repository/worker/commitmessageparser/base'); phutil_require_module('phabricator', 'infrastructure/daemon/workers/storage/task'); diff --git a/src/applications/repository/worker/commitmessageparser/svn/PhabricatorRepositorySvnCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/svn/PhabricatorRepositorySvnCommitMessageParserWorker.php index c8c8d95478..c27c848016 100644 --- a/src/applications/repository/worker/commitmessageparser/svn/PhabricatorRepositorySvnCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/svn/PhabricatorRepositorySvnCommitMessageParserWorker.php @@ -48,4 +48,11 @@ class PhabricatorRepositorySvnCommitMessageParserWorker } } + protected function getCommitHashes( + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit) { + return array(); + } + + }