From d8e2bb9f0f51c73b1d13427eb494a1d2a3c583ba Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 12 Dec 2018 05:20:36 -0800
Subject: [PATCH] Fix some straggling qsprintf() warnings in repository import

Summary:
Ref T13217. See <https://discourse.phabricator-community.org/t/unsafe-raw-string-warnings-while-importing-git-commits/2191>.

Hunt down and fix two more `qsprintf()` things.

I just converted the "performance optimization" into a normal, safe call since we're dealing with far less SVN stuff nowadays and the actual issue has been lost in the mists of time. If it resurfaces, we can take another look.

Test Plan: Imported some commits, no longer saw these warnings in the daemon logs.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19869
---
 .../pathchange/DiffusionPathChangeQuery.php   |  3 ++-
 ...atorRepositoryCommitChangeParserWorker.php | 24 ++++++++-----------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/src/applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php b/src/applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php
index 8ae9b3d203..850c06f105 100644
--- a/src/applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php
+++ b/src/applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php
@@ -43,12 +43,13 @@ final class DiffusionPathChangeQuery extends Phobject {
 
     $conn_r = $repository->establishConnection('r');
 
-    $limit = '';
     if ($this->limit) {
       $limit = qsprintf(
         $conn_r,
         'LIMIT %d',
         $this->limit + 1);
+    } else {
+      $limit = qsprintf($conn_r, '');
     }
 
     $raw_changes = queryfx_all(
diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php
index b544228561..f5d3af7fd8 100644
--- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php
+++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php
@@ -114,40 +114,36 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker
     PhabricatorRepositoryCommit $commit,
     array $changes) {
 
+    $conn = $repository->establishConnection('w');
+
     $repository_id = (int)$repository->getID();
     $commit_id = (int)$commit->getID();
 
-    // NOTE: This SQL is being built manually instead of with qsprintf()
-    // because some SVN changes affect an enormous number of paths (millions)
-    // and this showed up as significantly slow on a profile at some point.
-
     $changes_sql = array();
     foreach ($changes as $change) {
-      $values = array(
+      $changes_sql[] = qsprintf(
+        $conn,
+        '(%d, %d, %d, %nd, %nd, %d, %d, %d, %d)',
         $repository_id,
         (int)$change->getPathID(),
         $commit_id,
-        nonempty((int)$change->getTargetPathID(), 'null'),
-        nonempty((int)$change->getTargetCommitID(), 'null'),
+        nonempty((int)$change->getTargetPathID(), null),
+        nonempty((int)$change->getTargetCommitID(), null),
         (int)$change->getChangeType(),
         (int)$change->getFileType(),
         (int)$change->getIsDirect(),
-        (int)$change->getCommitSequence(),
-      );
-      $changes_sql[] = '('.implode(', ', $values).')';
+        (int)$change->getCommitSequence());
     }
 
-    $conn_w = $repository->establishConnection('w');
-
     queryfx(
-      $conn_w,
+      $conn,
       'DELETE FROM %T WHERE commitID = %d',
       PhabricatorRepository::TABLE_PATHCHANGE,
       $commit_id);
 
     foreach (PhabricatorLiskDAO::chunkSQL($changes_sql) as $chunk) {
       queryfx(
-        $conn_w,
+        $conn,
         'INSERT INTO %T
           (repositoryID, pathID, commitID, targetPathID, targetCommitID,
             changeType, fileType, isDirect, commitSequence)