Re: [PATCH 2/4] add chdir-notify API

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 29, 2018, 14:53 UTC
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:

Show 5 quoted lines

> +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);

Show 8 quoted lines

> +       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