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

[Jeff King <peff@peff.net>]

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?).

I guess it's not that much code to be careful, though.

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

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.

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

Ooh, that's clever.

I had also thought of extending it to other notifications besides
chdir(), like one string depends on another. But then we'd basically
re-inventing a cascading data-flow system, which is probably getting a
bit magical. :)

-Peff