From: Duy Nguyen <pclouds@gmail.com>
Date: Thu, 29 Mar 2018 14:53:42 GMT
Subject: Re: [PATCH 2/4] add chdir-notify API
Message-ID: <CACsJy8CCvshwb8a5ZozJ+x0D1kAYbeLSgJ0LDm4Z=uUZLtpsjg@mail.gmail.com>
In-Reply-To: <20180328174041.GB16274@sigill.intra.peff.net>

On Wed, Mar 28, 2018 at 7:40 PM, Jeff King <peff@peff.net> wrote:
> +static void reparent_cb(const char *old_cwd,
> +                       const char *new_cwd,
> +                       void *data)
> +{
> +       char **path = data;

Maybe check data == NULL and return early. This is just for
convenience, e.g. instead of doing

path = getenv("foo");
if (path)
    chdir_notify_reparent(&path);

I could do

path = getenv("foo");
chdir_notify_reparent(&path);

> +       char *tmp = *path;
> +       *path = reparent_relative_path(old_cwd, new_cwd, tmp);
> +       free(tmp);
> +}
> +
> +void chdir_notify_reparent(char **path)
> +{
> +       if (!is_absolute_path(*path))

I think this check is a bit early. There could be a big gap between
chdir_notify_reparent() and reparent_cb(). During that time, *path may
be updated and become a relative path. We can check for absolute path
inside reparent_cb() instead.

> +               chdir_notify_register(reparent_cb, path);
> +}

Overall, I like this API very much! I will add another one similar to
chdir_notify_reparent() but it takes an env name instead and the
callback will setenv() appropriately. The result looks super good.
-- 
Duy