Subversion: Fix argument order when using --username and --password

These need to go after the subcommand, not after the "svn"

Task-number: QTCREATORBUG-13066
Change-Id: Id3de0af8b7b2f07159d5ddb1af2e5fa00dd0dbb3
Reviewed-by: Orgad Shaneh <orgads@gmail.com>

src/plugins/subversion/checkoutwizard.cpp

@@ -76,19 +76,14 @@ VcsCommand *CheckoutWizard::createCommand(FileName *checkoutDir)
     const QString directory = cwp->directory();
     QStringList args;
     args << QLatin1String("checkout");
+    args << SubversionClient::addAuthenticationOptions(settings) << cwp->repository() << directory;
     args << QLatin1String(Constants::NON_INTERACTIVE_OPTION);
     if (cwp->trustServerCert())
         args << QLatin1String("--trust-server-cert");
     args << cwp->repository() << directory;
     const QString workingDirectory = cwp->path();
-
     *checkoutDir = FileName::fromString(workingDirectory + QLatin1Char('/') + directory);
 
-    if (settings.hasAuthentication()) {
-        const QString user = settings.stringValue(SubversionSettings::userKey);
-        const QString pwd = settings.stringValue(SubversionSettings::passwordKey);
-        args = SubversionClient::addAuthenticationOptions(args, user, pwd);
-    }
     VcsCommand *command = new VcsCommand(binary, workingDirectory,
                                          QProcessEnvironment::systemEnvironment());
     command->addJob(args, -1);

src/plugins/subversion/subversionclient.cpp

@@ -117,7 +117,7 @@ VcsCommand *SubversionClient::createCommitCmd(const QString &repositoryRoot,
 {
     const QStringList svnExtraOptions =
             QStringList(extraOptions)
-            << authenticationOptions(SubversionClient::CommitCommand)
+            << SubversionClient::addAuthenticationOptions(*settings())
             << QLatin1String(Constants::NON_INTERACTIVE_OPTION)
             << QLatin1String("--encoding") << QLatin1String("utf8")
             << QLatin1String("--file") << commitMessageFile;
@@ -178,28 +178,18 @@ SubversionClient::Version SubversionClient::svnVersion()
     return v;
 }
 
-QStringList SubversionClient::authenticationOptions(VcsCommandTag cmd) const
-{
-    const bool hasAuth = settings()->hasAuthentication();
-    const QString userName = hasAuth ? settings()->stringValue(SubversionSettings::userKey) : QString();
-    const QString password = hasAuth ? settings()->stringValue(SubversionSettings::passwordKey) : QString();
-    QStringList args(vcsCommandString(cmd));
-    args = SubversionClient::addAuthenticationOptions(args, userName, password);
-    args.removeFirst();
-    return args;
-}
-
 // Add authorization options to the command line arguments.
-// SVN pre 1.5 does not accept "--userName" for "add", which is most likely
+QStringList SubversionClient::addAuthenticationOptions(const SubversionSettings &settings)
-// an oversight. As no password is needed for the option, generally omit it.
-QStringList SubversionClient::addAuthenticationOptions(const QStringList &args,
-                                                       const QString &userName,
-                                                       const QString &password)
 {
+    if (!settings.hasAuthentication())
+        return QStringList();
+
+    const QString userName = settings.stringValue(SubversionSettings::userKey);
+    const QString password = settings.stringValue(SubversionSettings::passwordKey);
+
     if (userName.isEmpty())
-        return args;
+        return QStringList();
-    if (!args.empty() && args.front() == QLatin1String("add"))
+
-        return args;
     QStringList rc;
     rc.push_back(QLatin1String("--username"));
     rc.push_back(userName);
@@ -207,7 +197,6 @@ QStringList SubversionClient::addAuthenticationOptions(const QStringList &args,
         rc.push_back(QLatin1String("--password"));
         rc.push_back(password);
     }
-    rc.append(args);
     return rc;
 }
 
@@ -233,14 +222,10 @@ QString SubversionClient::synchronousTopic(const QString &repository)
 void SubversionClient::diff(const QString &workingDir, const QStringList &files,
                            const QStringList &extraOptions)
 {
-    QStringList args(extraOptions);
+    QStringList args;
+    args << addAuthenticationOptions(*settings());
     args.append(QLatin1String("--internal-diff"));
-
+    args << extraOptions;
-    const bool hasAuth = settings()->hasAuthentication();
-    const QString userName = hasAuth ? settings()->stringValue(SubversionSettings::userKey) : QString();
-    const QString password = hasAuth ? settings()->stringValue(SubversionSettings::passwordKey) : QString();
-    args = addAuthenticationOptions(args, userName, password);
-
     VcsBaseClient::diff(workingDir, files, args);
 }
 

src/plugins/subversion/subversionclient.h

@@ -76,10 +76,7 @@ public:
     Version svnVersion();
 
     // Add authorization options to the command line arguments.
-    QStringList authenticationOptions(VcsCommandTag cmd) const;
+    static QStringList addAuthenticationOptions(const SubversionSettings &settings);
-    static QStringList addAuthenticationOptions(const QStringList &args,
-                                                const QString &userName = QString(),
-                                                const QString &password = QString());
 
     QString synchronousTopic(const QString &repository);
 

src/plugins/subversion/subversionplugin.cpp

@@ -583,7 +583,9 @@ void SubversionPlugin::revertAll()
         return;
     // NoteL: Svn "revert ." doesn not work.
     QStringList args;
-    args << QLatin1String("revert") << QLatin1String("--recursive") << state.topLevel();
+    args << QLatin1String("revert");
+    args << SubversionClient::addAuthenticationOptions(settings());
+    args << QLatin1String("--recursive") << state.topLevel();
     const SubversionResponse revertResponse =
             runSvn(state.topLevel(), args, m_settings.timeOutMs(),
                    SshPasswordPrompt|ShowStdOutInLogWindow);
@@ -600,6 +602,7 @@ void SubversionPlugin::revertCurrentFile()
     QTC_ASSERT(state.hasFile(), return);
 
     QStringList args(QLatin1String("diff"));
+    args << SubversionClient::addAuthenticationOptions(settings());
     args.push_back(state.relativeCurrentFile());
 
     const SubversionResponse diffResponse =
@@ -619,7 +622,9 @@ void SubversionPlugin::revertCurrentFile()
 
     // revert
     args.clear();
-    args << QLatin1String("revert") << state.relativeCurrentFile();
+    args << QLatin1String("revert");
+    args << SubversionClient::addAuthenticationOptions(settings());
+    args << state.relativeCurrentFile();
 
     const SubversionResponse revertResponse =
             runSvn(state.currentFileTopLevel(), args, m_settings.timeOutMs(),
@@ -679,6 +684,7 @@ void SubversionPlugin::startCommit(const QString &workingDir, const QStringList
     }
 
     QStringList args(QLatin1String("status"));
+    args << SubversionClient::addAuthenticationOptions(settings());
     args += files;
 
     const SubversionResponse response =
@@ -758,6 +764,7 @@ void SubversionPlugin::svnStatus(const QString &workingDir, const QString &relat
     const VcsBasePluginState state = currentState();
     QTC_ASSERT(state.hasTopLevel(), return);
     QStringList args(QLatin1String("status"));
+    args << SubversionClient::addAuthenticationOptions(settings());
     if (!relativePath.isEmpty())
         args.append(relativePath);
     VcsOutputWindow::setRepository(workingDir);
@@ -772,6 +779,7 @@ void SubversionPlugin::filelog(const QString &workingDir,
 {
     // no need for temp file
     QStringList args(QLatin1String("log"));
+    args << SubversionClient::addAuthenticationOptions(settings());
     if (m_settings.intValue(SubversionSettings::logCountKey) > 0) {
         args << QLatin1String("-l")
              << QString::number(m_settings.intValue(SubversionSettings::logCountKey));
@@ -816,6 +824,7 @@ void SubversionPlugin::updateProject()
 void SubversionPlugin::svnUpdate(const QString &workingDir, const QString &relativePath)
 {
     QStringList args(QLatin1String("update"));
+    args << SubversionClient::addAuthenticationOptions(settings());
     args.push_back(QLatin1String(Constants::NON_INTERACTIVE_OPTION));
     if (!relativePath.isEmpty())
         args.append(relativePath);
@@ -849,6 +858,7 @@ void SubversionPlugin::vcsAnnotate(const QString &workingDir, const QString &fil
     QTextCodec *codec = VcsBaseEditor::getCodec(source);
 
     QStringList args(QLatin1String("annotate"));
+    args << SubversionClient::addAuthenticationOptions(settings());
     if (m_settings.boolValue(SubversionSettings::spaceIgnorantAnnotationKey))
         args << QLatin1String("-x") << QLatin1String("-uw");
     if (!revision.isEmpty())
@@ -908,6 +918,7 @@ void SubversionPlugin::describe(const QString &source, const QString &changeNr)
     // Run log to obtain message (local utf8)
     QString description;
     QStringList args(QLatin1String("log"));
+    args << SubversionClient::addAuthenticationOptions(settings());
     args.push_back(QLatin1String("-r"));
     args.push_back(changeNr);
     const SubversionResponse logResponse =
@@ -919,6 +930,7 @@ void SubversionPlugin::describe(const QString &source, const QString &changeNr)
     // Run diff (encoding via source codec)
     args.clear();
     args.push_back(QLatin1String("diff"));
+    args << SubversionClient::addAuthenticationOptions(settings());
     args.push_back(QLatin1String("-r"));
     QString diffArg;
     QTextStream(&diffArg) << (number - 1) << ':' << number;
@@ -970,22 +982,7 @@ void SubversionPlugin::submitCurrentLog()
     EditorManager::closeDocument(submitEditor()->document());
 }
 
-SubversionResponse
-        SubversionPlugin::runSvn(const QString &workingDir,
-                                 const QStringList &arguments,
-                                 int timeOut,
-                                 unsigned flags,
-                                 QTextCodec *outputCodec) const
-{
-    const bool hasAuth = m_settings.hasAuthentication();
-    return runSvn(workingDir,
-                  hasAuth ? m_settings.stringValue(SubversionSettings::userKey) : QString(),
-                  hasAuth ? m_settings.stringValue(SubversionSettings::passwordKey) : QString(),
-                  arguments, timeOut, flags, outputCodec);
-}
-
 SubversionResponse SubversionPlugin::runSvn(const QString &workingDir,
-                          const QString &userName, const QString &password,
                           const QStringList &arguments, int timeOut,
                           unsigned flags, QTextCodec *outputCodec) const
 {
@@ -997,9 +994,8 @@ SubversionResponse SubversionPlugin::runSvn(const QString &workingDir,
         return response;
     }
 
-    const QStringList completeArguments = SubversionClient::addAuthenticationOptions(arguments, userName, password);
     const SynchronousProcessResponse sp_resp =
-            VcsBasePlugin::runVcs(workingDir, executable, completeArguments, timeOut,
+            VcsBasePlugin::runVcs(workingDir, executable, arguments, timeOut,
                                            flags, outputCodec);
 
     response.error = sp_resp.result != SynchronousProcessResponse::Finished;
@@ -1080,7 +1076,9 @@ bool SubversionPlugin::vcsAdd(const QString &workingDir, const QString &rawFileN
 {
     const QString file = QDir::toNativeSeparators(rawFileName);
     QStringList args;
-    args << QLatin1String("add") << QLatin1String("--parents") << file;
+    args << QLatin1String("add")
+         << SubversionClient::addAuthenticationOptions(settings())
+         << QLatin1String("--parents") << file;
     const SubversionResponse response =
             runSvn(workingDir, args, m_settings.timeOutMs(),
                    SshPasswordPrompt|ShowStdOutInLogWindow);
@@ -1092,7 +1090,8 @@ bool SubversionPlugin::vcsDelete(const QString &workingDir, const QString &rawFi
     const QString file = QDir::toNativeSeparators(rawFileName);
 
     QStringList args;
-    args << QLatin1String("delete") << QLatin1String("--force") << file;
+    args << QLatin1String("delete");
+    args << SubversionClient::addAuthenticationOptions(settings()) << QLatin1String("--force") << file;
 
     const SubversionResponse response =
             runSvn(workingDir, args, m_settings.timeOutMs(),
@@ -1103,6 +1102,7 @@ bool SubversionPlugin::vcsDelete(const QString &workingDir, const QString &rawFi
 bool SubversionPlugin::vcsMove(const QString &workingDir, const QString &from, const QString &to)
 {
     QStringList args(QLatin1String("move"));
+    args << SubversionClient::addAuthenticationOptions(settings());
     args << QDir::toNativeSeparators(from) << QDir::toNativeSeparators(to);
     const SubversionResponse response =
             runSvn(workingDir, args, m_settings.timeOutMs(),
@@ -1118,31 +1118,33 @@ bool SubversionPlugin::vcsCheckout(const QString &directory, const QByteArray &u
     QStringList args = QStringList(QLatin1String("checkout"));
     args << QLatin1String(Constants::NON_INTERACTIVE_OPTION) ;
 
-    if (!username.isEmpty() && !password.isEmpty()) {
+    if (!username.isEmpty()) {
         // If url contains username and password we have to use separate username and password
         // arguments instead of passing those in the url. Otherwise the subversion 'non-interactive'
         // authentication will always fail (if the username and password data are not stored locally),
         // if for example we are logging into a new host for the first time using svn. There seems to
         // be a bug in subversion, so this might get fixed in the future.
         tempUrl.setUserInfo(QString());
+        args << QLatin1String("--username") << username;
+        if (!password.isEmpty())
+            args << QLatin1String("--password") << password;
+    }
+
     args << QLatin1String(tempUrl.toEncoded()) << directory;
-        const SubversionResponse response = runSvn(directory, username, password, args,
+    args << QLatin1String(url) << directory;
+
+    const SubversionResponse response = runSvn(directory, args,
                                                10 * m_settings.timeOutMs(),
                                                VcsBasePlugin::SshPasswordPrompt);
     return !response.error;
-    } else {
+
-        args << QLatin1String(url) << directory;
-        const SubversionResponse response = runSvn(directory, args, 10 * m_settings.timeOutMs(),
-                                                   VcsBasePlugin::SshPasswordPrompt);
-        return !response.error;
-    }
 }
 
 QString SubversionPlugin::vcsGetRepositoryURL(const QString &directory)
 {
     QXmlStreamReader xml;
     QStringList args = QStringList(QLatin1String("info"));
-    args << QLatin1String("--xml");
+    args << SubversionClient::addAuthenticationOptions(settings()) << QLatin1String("--xml");
 
     const SubversionResponse response = runSvn(directory, args, 10 * m_settings.timeOutMs(), SuppressCommandLogging);
     xml.addData(response.stdOut);
@@ -1203,7 +1205,8 @@ bool SubversionPlugin::managesDirectory(const QString &directory, QString *topLe
 bool SubversionPlugin::managesFile(const QString &workingDirectory, const QString &fileName) const
 {
     QStringList args;
-    args << QLatin1String("status") << fileName;
+    args << QLatin1String("status");
+    args << SubversionClient::addAuthenticationOptions(settings()) << fileName;
     SubversionResponse response =
             runSvn(workingDirectory, args, m_settings.timeOutMs(), 0);
     return response.stdOut.isEmpty() || response.stdOut.at(0) != QLatin1Char('?');

src/plugins/subversion/subversionplugin.h

@@ -137,15 +137,9 @@ private:
     Core::IEditor * showOutputInEditor(const QString& title, const QString &output,
                                        int editorType, const QString &source,
                                        QTextCodec *codec);
-    // Run using the settings' authentication options.
     SubversionResponse runSvn(const QString &workingDir,
                               const QStringList &arguments, int timeOut,
                               unsigned flags, QTextCodec *outputCodec = 0) const;
-    // Run using custom authentication options.
-    SubversionResponse runSvn(const QString &workingDir,
-                              const QString &userName, const QString &password,
-                              const QStringList &arguments, int timeOut,
-                              unsigned flags, QTextCodec *outputCodec = 0) const;
 
     void filelog(const QString &workingDir,
                  const QString &file = QString(),