On Thu, Mar 8, 2012 at 5:55 PM, Patrick Ohly <[email protected]> wrote:
> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly <[email protected]> wrote:
>> > On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
>> >> Now that you've merged for-master/new-master into master we are
>> >> rebasing onto master.
>> >
>> > How is that going?
>> >
>>
>> It's been rebased and we've squashed the changes into about a dozen
>> commits. The tests seem to give us the same results as before the
>> rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
>> for the rebased and squashed changes. I'm putting the final touches on
>> the cleanup know.
>
> 057a19f18c600fe4158fd06087aed10b44f20c4e
>
> syncevo: Added methods to add and get environment variables to ForkExec
>
> The parent may need to pass additional environment variables to the
> child process.
>
> This commit adds an Add method to the parent process and a Get method
> for the child process.
>
> + /*
> + * Return the value for the environment variable name or and empty
> + * string is not found or set to ForkExecEnvVarEmpty.
> + */
> + std::string getEnvVar(const std::string &name);
>
> Why is this ForkExecEnvVarEmpty necessary?
>
Ah, originally I wanted to use the just the fact that the variable
existed without setting a value but some value needs to be set for env
variables. However, somewhere along the line I changed my mind. I'll
get rid of this.
> Note that there is already a GetEnv() in util.h which returns an empty
> std::string when getenv() returns NULL.
>
Thanks, I wasn't aware. I'll change it to use that.
> +std::string ForkExecChild::getEnvVar(const std::string &name)
> +{
> + gchar *pValue = getenv(name.c_str());
> + if(!pValue || boost::iequals(pValue, ForkExecEnvVarEmpty)) {
> + return std::string();
> + } else {
> + return std::string(pValue);
> +
>
> getenv() returns "char *", not "gchar *". I don't know where that makes
> a difference, but if it is guaranteed to be the same, why introduce
> gchar? That implies to me that there might be a difference somewhere.
>
You're right. Moot point now though.
>
> d85da736dec0975962b610e5c3b0b0dc52b612b3
>
> ForkExec.cpp: Fix the build without --enable-dbus-service.
>
> The .h file's contents are inside an ifdef, so the implementation in
> .cpp should be too.
>
> --------------------------- src/syncevo/ForkExec.cpp
> ---------------------------
> index 0395c33..1f71331 100644
> @@ -18,8 +18,10 @@
> */
>
> #include "ForkExec.h"
>
> +#if defined(HAVE_GLIB) && defined(DBUS_SERVICE)
> +
> SE_BEGIN_CXX
>
> #if defined(HAVE_GLIB)
>
> @@ -292,4 +294,5 @@ const char *ForkExecChild::getParentDBusAddress()
> #endif // HAVE_GLIB
>
> SE_END_CXX
>
> +#endif // HAVE_GLIB
>
> The if and endif don't match, and HAVE_GLIB is tested twice. I suggest
>
> +#if defined(HAVE_GLIB) && defined(DBUS_SERVICE)
> ...
> -#if defined(HAVE_GLIB)
> ...
> -#endif // HAVE_GLIB
> ...
> +#endif // HAVE_GLIB && DBUS_SERVICE
>
Yeah, just saw that too.
> dd0a4ab2d584004095f5f0e7cd30770ae3d952c8
>
> Add some common DBus callbacks.
>
> These callbacks will be used once sync sessions run in their own
> process.
>
> nullCb: a function that does nothing.
> counterCb: a function that calls given callback when given counter
> drops to zero.
>
> Example use:
> ...
>
> The documentation should be in comments in the header file, not in the
> commit message. That way it is more readily available when reading the
> source code in the future.
>
> Note that global functions normally start with a a capital letter, to
> distinguish them from member methods, which start with a lower capital.
>
I'll make these changes and squash them into the for-review branch.
Cheers,
Chris
_______________________________________________
SyncEvolution mailing list
[email protected]
http://lists.syncevolution.org/listinfo/syncevolution
