On Thu, Mar 29, 2018 at 7:48 PM, Jeff King <peff@xxxxxxxx> wrote:
> 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@xxxxxxxx> 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?).
It actually exists in 'next', repository.c where we have this line
repo->alternate_db = xstrdup_or_null(o->alternate_db);
> I guess it's not that much code to be careful, though.
>> > +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 think we could leave such optimization to the caller. I'm ok with
keeping this "if" too, but you may need to clarify it in the big
comment block in chdir-notify.h because this behavior to me is
surprising.
> 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.
Well, given the number of call sites is rather small at the moment, I
think chdir-notify can just stay dumb and let the caller deal with
duplicate registration. One thing I like about your linked list design
though, is it makes it quite easy to remove (or even update) a
callback. You can simply return the (opaque) pointer to the entire
chdir_notify_entry as a handle and we can free/unlink the entry based
on it. It's kinda hard to me to do it with array-based design.
--
Duy
