On Wed, Mar 28, 2018 at 01:58:18PM -0400, Eric Sunshine wrote:
> On Wed, Mar 28, 2018 at 1:40 PM, Jeff King <peff@xxxxxxxx> wrote:
> > [...]
> > Let's provide an API to let code that stores relative paths
> > "subscribe" to updates to the current working directory.
> > This means that callers of chdir() don't need to know about
> > all subscribers ahead of time; they can simply consult a
> > dynamically built list.
> > [...]
> > Signed-off-by: Jeff King <peff@xxxxxxxx>
> > ---
> > diff --git a/chdir-notify.c b/chdir-notify.c
> > @@ -0,0 +1,71 @@
> > +int chdir_notify(const char *new_cwd)
> > +{
> > + struct strbuf old_cwd = STRBUF_INIT;
> > + struct list_head *pos;
> > +
> > + if (strbuf_getcwd(&old_cwd) < 0)
> > + return -1;
> > + if (chdir(new_cwd) < 0)
> > + return -1;
>
> This 'return' is leaking 'old_cwd', isn't it?
Whoops, yes. I wrote it the other way around first to avoid the leak,
but of course that has other problems. ;)
> > diff --git a/chdir-notify.h b/chdir-notify.h
> > @@ -0,0 +1,64 @@
> > + * In practice most callers will want to move a relative path to the new root;
> > + * they can use the reparent_relative_path() helper for that. If that's all
> > + * you're doing, you can also use the convenience function:
> > + *
> > + * chdir_notify_reparent(&my_path);
> > + */
> > +typedef void (*chdir_notify_callback)(const char *old_cwd,
> > + const char *new_cwd,
> > + void *data);
> > +void chdir_notify_register(chdir_notify_callback cb, void *data);
> > +void chdir_notify_reparent(char **path);
>
> Can we have some documentation here (or in the chdir_notify_reparent()
> example above) explaining that *path is a heap-allocated value? I had
> to consult the implementation to understand ownership.
Sure. I originally had reparent_relative_path() handle the freeing and
allocation, and it explained the ownership in its docstring. But as I
revised that to simply return its value, that got lost. I'll add it back
in here.
-Peff
