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

> On Thu, Mar 29, 2018 at 04:53:42PM +0200, Duy Nguyen wrote:
>
>> 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);
>
> You'd need to xstrdup(path) there anyway. I guess you could use
> xstrdup_or_null(), but it seems somewhat unlikely (unless your work on
> top really does add such a callsite?).

>> > +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.
>
> My thinking was that we'd be effectively zero-cost for an absolute path,
> though really adding an element to the notification list is probably not
> a big deal.

> I also assumed that whoever changed it to a relative path would then
> install the notification handler. One thing that my series kind of
> glosses over is whether somebody might call any of these functions
> twice, which would involve setting up the handler twice. So e.g. if you
> did:
>
>   set_git_dir("foo");
>   set_git_dir("bar");
>
> we'd have two handlers, which would do the wrong thing when the second
> one triggered. I hoped we could eventually add a BUG() if that happens,
> but maybe we should simply do a:
>
>   static int registered_chdir;
>
>   if (!registered_chdir) {
>         chdir_notify_reparent(&path);
>         registered_chdir = 1;
>   }
>
> at each call-site. Another alternative would be to make it a noop to
> feed the same path twice. That requires a linear walk through the list,
> but it's a pretty small list.