Re: [PATCH 1/3] get rid of std::string from bugzilla

[Denys Vlasenko]

On Thu, 2010-08-12 at 16:28 +0200, Nikola Pajkovsky wrote:
> --- a/lib/plugins/Bugzilla.cpp
> +++ b/lib/plugins/Bugzilla.cpp
> @@ -31,6 +31,8 @@
>  #define XML_RPC_SUFFIX      "/xmlrpc.cgi"
>  #define MAX_HOPS            5
>
> +#include "strbuf.h"

I usually put all includes together at the top of the module.
> if ((Login == "") || (Password == ""))
> if ((strcmp(login, "") == 0) || (strcmp(password, "") == 0))

strcmp(login, "") == 0   =>   !login[0]   or   =>    login[0] == '\0'
>    SSLVerify = string_to_bool(settings["SSLVerify"].c_str());
>    login = settings["Login"].c_str();
>    password = settings["Password"].c_str();
>    bugzilla_xmlrpc = settings["BugzillaXMLRPC"].c_str();
>    bugzilla_url = settings["BugzillaURL"].c_str();
>    no_ssl_verify = string_to_bool(settings["NoSSLVerify"].c_str());

...
> ctx bz_server(BugzillaXMLRPC.c_str(), SSLVerify);
> ctx bz_server(bugzilla_xmlrpc, no_ssl_verify);

Looks like you went back to "no SSL verify" name. We decided that having
a negative name is confusing. Is it a mistake?
>            throw_if_xml_fault_occurred(&bz_server.env);
>            throw_xml_fault(&bz_server.env);

This change is unrelated, and should be in separate change.
> @@ -919,58 +936,72 @@ void CReporterBugzilla::SetSettings(const map_plugin_settings_t& pSettings)
>  map_plugin_settings_t::const_iterator end = pSettings.end();
>  map_plugin_settings_t::const_iterator it;
> it = pSettings.find("BugzillaURL");
>    if (it != end)
>    {

...
>  }
> it = pSettings.find("Login");
>    if (it != end)
>    {

...
>  }
> it = pSettings.find("Password");

These small unrelated changes are better go in separately.
Imagine you need to cherry pick a change later.
> VERB3 log("%s:%s:%s:%s:%s", __FILE__, __func__, m_bugzilla_url, m_login, m_password);

Looks like forgotten debugging
> /* Should not be deleted (why?) */
> +/*
>  const map_plugin_settings_t& CReporterBugzilla::GetSettings()
>  {
> +<<<<<<< HEAD
>      m_pSettings["BugzillaURL"] = m_sBugzillaURL;
>      m_pSettings["Login"] = m_sLogin;
>      m_pSettings["Password"] = m_sPassword;
>      m_pSettings["SSLVerify"] = m_bSSLVerify ? "yes" : "no";
>      m_pSettings["RatingRequired"] = m_bRatingRequired ? "yes" : "no";
> +=======
> m_pSettings["BugzillaURL"] = m_bugzilla_url;
> m_pSettings["Login"] = (m_login)? m_login: "";
> m_pSettings["Password"] = (m_password)? m_password: "";
> m_pSettings["NoSSLVerify"] = m_no_ssl_verify ? "yes" : "no";
>
> +>>>>>>> rid of std::string from bugzilla
>  return m_pSettings;
>
> }
> +*/

Looks like this needs fixing...
> --- a/lib/utils/make_descr.cpp
> +++ b/lib/utils/make_descr.cpp
> @@ -29,13 +29,16 @@
>  # define _(S) (S)
>  #endif
>
> +#include "strbuf.h"

Here too please move it up to other includes.
> return howToReproduce + comment;
> if (!repro && !comment)
>    return NULL;

Is caller expecting that the result can be NULL, not merely ""?