gitserver: cleanup

It all started from
  src/jamidht/gitserver.cpp: In member function ‘bool jami::GitServer::Impl::parseOrder(const uint8_t*, std::size_t)’:
  src/jamidht/gitserver.cpp:111:9: warning: ‘pkt_len’ may be used uninitialized [-Wmaybe-uninitialized]

Change-Id: I2ac85ed1c21ecf346aecd71cd1d0ccf9d6cf6024

src/jamidht/gitserver.cpp

@@ -34,6 +34,7 @@ using namespace std::string_view_literals;
 constexpr auto FLUSH_PKT = "0000"sv;
 constexpr auto NAK_PKT = "0008NAK\n"sv;
 constexpr auto DONE_PKT = "0009done\n"sv;
+constexpr auto DONE_CMD = "done\n"sv;
 constexpr auto WANT_CMD = "want"sv;
 constexpr auto HAVE_CMD = "have"sv;
 constexpr auto SERVER_CAPABILITIES
@@ -55,11 +56,8 @@ public:
             std::lock_guard<std::mutex> lk(destroyMtx_);
             if (isDestroying_)
                 return len;
-            std::string data(buf, buf + len);
-            auto needMoreParsing = parseOrder(buf, len);
-            while (needMoreParsing) {
-                needMoreParsing = parseOrder();
-            };
+            if (parseOrder(std::string_view((const char*)buf, len)))
+                while(parseOrder());
             return len;
         });
     }
@@ -72,15 +70,14 @@ public:
             socket_->shutdown();
         }
     }
-    bool parseOrder(const uint8_t* buf = nullptr, std::size_t len = 0);
+    bool parseOrder(std::string_view buf = {});
 
     void sendReferenceCapabilities(bool sendVersion = false);
-    void answerToWantOrder();
     bool NAK();
     void ACKCommon();
     bool ACKFirst();
     void sendPackData();
-    std::map<std::string, std::string> getParameters(const std::string& pkt_line);
+    std::map<std::string, std::string> getParameters(std::string_view pkt_line);
 
     std::string repositoryId_ {};
     std::string repository_ {};
@@ -95,36 +92,65 @@ public:
 };
 
 bool
-GitServer::Impl::parseOrder(const uint8_t* buf, std::size_t len)
+GitServer::Impl::parseOrder(std::string_view buf)
 {
-    std::string pkt = cachedPkt_;
-    if (buf)
-        pkt += std::string({buf, buf + len});
-    cachedPkt_.clear();
+    std::string pkt = std::move(cachedPkt_);
+    if (!buf.empty())
+        pkt += buf;
 
     // Parse pkt len
     // Reference: https://github.com/git/git/blob/master/Documentation/technical/protocol-common.txt#L51
     // The first four bytes define the length of the packet and 0000 is a FLUSH pkt
 
-    unsigned int pkt_len;
-    std::from_chars(pkt.data(), pkt.data() + 4, pkt_len, 16);
+    unsigned int pkt_len = 0;
+    auto [p, ec] = std::from_chars(pkt.data(), pkt.data() + 4, pkt_len, 16);
+    if (ec != std::errc()) {
+        JAMI_ERROR("Can't parse packet size");
+    }
     if (pkt_len != pkt.size()) {
         // Store next packet part
         if (pkt_len == 0) {
             // FLUSH_PKT
             pkt_len = 4;
         }
-        cachedPkt_ = pkt.substr(pkt_len, pkt.size() - pkt_len);
+        cachedPkt_ = pkt.substr(pkt_len);
+    }
+
+    auto pack = std::string_view(pkt).substr(4, pkt_len - 4);
+    if (pack == DONE_CMD) {
+        // Reference:
+        // https://github.com/git/git/blob/master/Documentation/technical/pack-protocol.txt#L390 Do
+        // not do multi-ack, just send ACK + pack file
+        // In case of no common base, send NAK
+        JAMI_INFO("Peer negotiation is done. Answering to want order");
+        bool sendData;
+        if (common_.empty())
+            sendData = NAK();
+        else
+            sendData = ACKFirst();
+        if (sendData)
+            sendPackData();
+        return !cachedPkt_.empty();
+    } else if (pack.empty()) {
+        if (!haveRefs_.empty()) {
+            // Reference:
+            // https://github.com/git/git/blob/master/Documentation/technical/pack-protocol.txt#L390
+            // Do not do multi-ack, just send ACK + pack file In case of no common base ACK
+            ACKCommon();
+            NAK();
+        }
+        return !cachedPkt_.empty();
     }
-    // NOTE: do not remove the size to detect the 0009done packet
-    pkt = pkt.substr(0, pkt_len);
 
-    if (pkt.find(UPLOAD_PACK_CMD) == 4) {
+    auto lim = pack.find(' ');
+    auto cmd = pack.substr(0, lim);
+    auto dat = (lim < pack.size()) ? pack.substr(lim+1) : std::string_view{};
+    if (cmd == UPLOAD_PACK_CMD) {
         // Cf: https://github.com/git/git/blob/master/Documentation/technical/pack-protocol.txt#L166
         // References discovery
         JAMI_INFO("Upload pack command detected.");
         auto version = 1;
-        auto parameters = getParameters(pkt);
+        auto parameters = getParameters(dat);
         auto versionIt = parameters.find("version");
         bool sendVersion = false;
         if (versionIt != parameters.end()) {
@@ -132,7 +158,7 @@ GitServer::Impl::parseOrder(const uint8_t* buf, std::size_t len)
                 version = std::stoi(versionIt->second);
                 sendVersion = true;
             } catch (...) {
-                JAMI_WARN("Invalid version detected: %s", versionIt->second.c_str());
+                JAMI_WARNING("Invalid version detected: {}", versionIt->second);
             }
         }
         if (version == 1) {
@@ -140,18 +166,14 @@ GitServer::Impl::parseOrder(const uint8_t* buf, std::size_t len)
         } else {
             JAMI_ERR("That protocol version is not yet supported (version: %u)", version);
         }
-    } else if (pkt.find(WANT_CMD) == 4) {
+    } else if (cmd == WANT_CMD) {
         // Reference:
         // https://github.com/git/git/blob/master/Documentation/technical/pack-protocol.txt#L229
         // TODO can have more want
-        auto content = pkt.substr(5, pkt_len - 5);
-        auto commit = content.substr(4, 40);
-        wantedReference_ = commit;
+        wantedReference_ = dat.substr(0, 40);
         JAMI_INFO("Peer want ref: %s", wantedReference_.c_str());
-    } else if (pkt.find(HAVE_CMD) == 4) {
-        auto content = pkt.substr(5, pkt_len - 5);
-        auto commit = content.substr(4, 40);
-        haveRefs_.emplace_back(commit);
+    } else if (cmd == HAVE_CMD) {
+        const auto& commit = haveRefs_.emplace_back(dat.substr(0, 40));
         if (common_.empty()) {
             // Detect first common commit
             // Reference:
@@ -169,29 +191,8 @@ GitServer::Impl::parseOrder(const uint8_t* buf, std::size_t len)
                 common_ = commit;
             }
         }
-    } else if (pkt == DONE_PKT) {
-        // Reference:
-        // https://github.com/git/git/blob/master/Documentation/technical/pack-protocol.txt#L390 Do
-        // not do multi-ack, just send ACK + pack file
-        // In case of no common base, send NAK
-        JAMI_INFO("Peer negotiation is done. Answering to want order");
-        bool sendData;
-        if (common_.empty())
-            sendData = NAK();
-        else
-            sendData = ACKFirst();
-        if (sendData)
-            sendPackData();
-    } else if (pkt == FLUSH_PKT) {
-        if (!haveRefs_.empty()) {
-            // Reference:
-            // https://github.com/git/git/blob/master/Documentation/technical/pack-protocol.txt#L390
-            // Do not do multi-ack, just send ACK + pack file In case of no common base ACK
-            ACKCommon();
-            NAK();
-        }
     } else {
-        JAMI_WARN("Unwanted packet received: %s", pkt.c_str());
+        JAMI_WARNING("Unwanted packet received: {}", pkt);
     }
     return !cachedPkt_.empty();
 }
@@ -432,18 +433,17 @@ GitServer::Impl::sendPackData()
 }
 
 std::map<std::string, std::string>
-GitServer::Impl::getParameters(const std::string& pkt_line)
+GitServer::Impl::getParameters(std::string_view pkt_line)
 {
     std::map<std::string, std::string> parameters;
     std::string key, value;
     auto isKey = true;
     auto nullChar = 0;
-    for (std::size_t i = 0; i < pkt_line.size(); ++i) {
-        auto letter = pkt_line[i];
+    for (auto letter: pkt_line) {
         if (letter == '\0') {
             // parameters such as host or version are after the first \0
             if (nullChar != 0 && !key.empty()) {
-                parameters[key] = value;
+                parameters[std::move(key)] = std::move(value);
             }
             nullChar += 1;
             isKey = true;