From d75289477f3c0c9cb89c2eb26a023329a7b50731 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Wed, 8 Sep 2021 09:45:06 -0400
Subject: [PATCH] swarm: calculate linearizedParent while logging
This makes the log sending messages in the order the client will
show it and avoid to calculate linearized parent for each commit.
However, this linearized parent can change over time. In fact, if
a merge happens, the parents of the messages shown can be
modified.
Change-Id: I26372aa52de0e5f89140680a4beaf5fc7ec52163
GitLab: https://git.jami.net/savoirfairelinux/ring-project/-/issues/1282
---
src/jamidht/conversation.cpp | 3 +-
src/jamidht/conversationrepository.cpp | 87 +++++--------------
src/jamidht/conversationrepository.h | 1 +
test/unitTest/conversation/conversation.cpp | 9 +-
.../conversationRepository.cpp | 1 +
5 files changed, 30 insertions(+), 71 deletions(-)
diff --git a/src/jamidht/conversation.cpp b/src/jamidht/conversation.cpp
index 62659d8f7f..995e638c9e 100644
--- a/src/jamidht/conversation.cpp
+++ b/src/jamidht/conversation.cpp
@@ -404,10 +404,9 @@ Conversation::Impl::convCommitToMap(const ConversationCommit& commit) const
if (!extension.empty())
message["fileId"] += "." + extension;
}
- auto linearizedParent = repository_->linearizedParent(commit.id);
message["id"] = commit.id;
message["parents"] = parents;
- message["linearizedParent"] = linearizedParent ? *linearizedParent : "";
+ message["linearizedParent"] = commit.linearized_parent;
message["author"] = authorId;
message["type"] = type;
message["timestamp"] = std::to_string(commit.timestamp);
diff --git a/src/jamidht/conversationrepository.cpp b/src/jamidht/conversationrepository.cpp
index d6235829c8..46282ab7ed 100644
--- a/src/jamidht/conversationrepository.cpp
+++ b/src/jamidht/conversationrepository.cpp
@@ -119,7 +119,6 @@ public:
bool logIfNotFound = false,
bool fastLog = false,
const std::string& authorUri = "") const;
- std::optional<std::string> linearizedParent(const std::string& commitId) const;
GitObject fileAtTree(const std::string& path, const GitTree& tree) const;
// NOTE! GitDiff needs to be deteleted before repo
@@ -1533,20 +1532,21 @@ ConversationRepository::Impl::log(const std::string& from,
const std::string& authorUri) const
{
std::vector<ConversationCommit> commits {};
+ git_oid oid, oidFrom, oidMerge;
- git_oid oid;
+ // Note: Start from head to get all merge possibilities and correct linearized parent.
auto repo = repository();
- if (!repo)
+ if (!repo or git_reference_name_to_id(&oid, repo.get(), "HEAD") < 0) {
+ JAMI_ERR("Cannot get reference for HEAD");
return commits;
- if (from.empty()) {
- if (git_reference_name_to_id(&oid, repo.get(), "HEAD") < 0) {
- JAMI_ERR("Cannot get reference for HEAD");
- return commits;
- }
- } else {
- if (git_oid_fromstr(&oid, from.c_str()) < 0) {
- JAMI_ERR("Cannot get reference for commit %s", from.c_str());
- return commits;
+ }
+
+ if (from != "" && git_oid_fromstr(&oidFrom, from.c_str()) == 0) {
+ auto isMergeBase = git_merge_base(&oidMerge, repo.get(), &oid, &oidFrom) == 0
+ && git_oid_equal(&oidMerge, &oidFrom);
+ if (!isMergeBase) {
+ // We're logging a non merged branch, so, take this one instead of HEAD
+ oid = oidFrom;
}
}
@@ -1559,23 +1559,30 @@ ConversationRepository::Impl::log(const std::string& from,
JAMI_DBG("Couldn't init revwalker for conversation %s", id_.c_str());
return commits;
}
+
GitRevWalker walker {walker_ptr, git_revwalk_free};
git_revwalk_sorting(walker.get(), GIT_SORT_TOPOLOGICAL | GIT_SORT_TIME);
+ auto startLogging = from == "";
for (auto idx = 0u; !git_revwalk_next(&oid, walker.get()); ++idx) {
- if (n != 0 && idx == n) {
- break;
- }
git_commit* commit_ptr = nullptr;
std::string id = git_oid_tostr_s(&oid);
+ if (!commits.empty()) {
+ // Set linearized parent
+ commits.rbegin()->linearized_parent = id;
+ }
if (git_commit_lookup(&commit_ptr, repo.get(), &oid) < 0) {
JAMI_WARN("Failed to look up commit %s", id.c_str());
break;
}
GitCommit commit {commit_ptr, git_commit_free};
- if (id == to) {
+ if ((n != 0 && commits.size() == n) || (id == to))
break;
- }
+
+ if (!startLogging && from != "" && from == id)
+ startLogging = true;
+ if (!startLogging)
+ continue;
const git_signature* sig = git_commit_author(commit.get());
GitAuthor author;
@@ -1625,49 +1632,9 @@ ConversationRepository::Impl::log(const std::string& from,
git_buf_dispose(&signed_data);
cc->timestamp = git_commit_time(commit.get());
}
-
return commits;
}
-std::optional<std::string>
-ConversationRepository::Impl::linearizedParent(const std::string& commitId) const
-{
- git_oid oid;
- auto repo = repository();
- if (!repo or git_reference_name_to_id(&oid, repo.get(), "HEAD") < 0) {
- JAMI_ERR("Cannot get reference for HEAD");
- return std::nullopt;
- }
-
- git_revwalk* walker_ptr = nullptr;
- if (git_revwalk_new(&walker_ptr, repo.get()) < 0 || git_revwalk_push(walker_ptr, &oid) < 0) {
- GitRevWalker walker {walker_ptr, git_revwalk_free};
- // This fail can be ok in the case we check if a commit exists before pulling (so can fail
- // there). only log if the fail is unwanted.
- return std::nullopt;
- }
- GitRevWalker walker {walker_ptr, git_revwalk_free};
- git_revwalk_sorting(walker.get(), GIT_SORT_TOPOLOGICAL | GIT_SORT_TIME);
-
- auto ret = false;
- for (auto idx = 0u; !git_revwalk_next(&oid, walker.get()); ++idx) {
- git_commit* commit_ptr = nullptr;
- std::string id = git_oid_tostr_s(&oid);
- if (git_commit_lookup(&commit_ptr, repo.get(), &oid) < 0) {
- JAMI_WARN("Failed to look up commit %s", id.c_str());
- break;
- }
- GitCommit commit {commit_ptr, git_commit_free};
-
- if (ret)
- return id;
- if (id == commitId)
- ret = true;
- }
-
- return std::nullopt;
-}
-
GitObject
ConversationRepository::Impl::fileAtTree(const std::string& path, const GitTree& tree) const
{
@@ -2927,12 +2894,6 @@ ConversationRepository::validClone() const
return pimpl_->validCommits(logN("", 0));
}
-std::optional<std::string>
-ConversationRepository::linearizedParent(const std::string& commitId) const
-{
- return pimpl_->linearizedParent(commitId);
-}
-
void
ConversationRepository::removeBranchWith(const std::string& remoteDevice)
{
diff --git a/src/jamidht/conversationrepository.h b/src/jamidht/conversationrepository.h
index b4a73a230c..ed26caa82a 100644
--- a/src/jamidht/conversationrepository.h
+++ b/src/jamidht/conversationrepository.h
@@ -69,6 +69,7 @@ struct ConversationCommit
std::vector<uint8_t> signed_content {};
std::vector<uint8_t> signature {};
std::string commit_msg {};
+ std::string linearized_parent {};
int64_t timestamp {0};
};
diff --git a/test/unitTest/conversation/conversation.cpp b/test/unitTest/conversation/conversation.cpp
index a6214b6328..b407317e6e 100644
--- a/test/unitTest/conversation/conversation.cpp
+++ b/test/unitTest/conversation/conversation.cpp
@@ -1064,14 +1064,11 @@ ConversationTest::testPingPongMessages()
[&](const std::string& accountId,
const std::string& /* conversationId */,
std::map<std::string, std::string> /*message*/) {
- if (accountId == bobId) {
+ if (accountId == bobId)
messageBobReceived += 1;
- }
- if (accountId == aliceId) {
+ if (accountId == aliceId)
messageAliceReceived += 1;
- }
- if (messageAliceReceived == messageBobReceived)
- cv.notify_one();
+ cv.notify_one();
}));
confHandlers.insert(
DRing::exportable_callback<DRing::ConversationSignal::ConversationRequestReceived>(
diff --git a/test/unitTest/conversationRepository/conversationRepository.cpp b/test/unitTest/conversationRepository/conversationRepository.cpp
index 2d8fb93c29..b688cbe0c1 100644
--- a/test/unitTest/conversationRepository/conversationRepository.cpp
+++ b/test/unitTest/conversationRepository/conversationRepository.cpp
@@ -426,6 +426,7 @@ ConversationRepositoryTest::testFetch()
// Clone repository
auto id1 = repository->commitMessage("Commit 1");
+
auto cloned = ConversationRepository::cloneConversation(bobAccount->weak(),
aliceDeviceId.toString(),
repository->id());
--
GitLab