Re: [PATCH v2 4/8] refs: unregister reference stores from "chdir_notify"

[Justin Tobler <jltobler@gmail.com>]

On 26/06/15 03:56PM, Patrick Steinhardt wrote:
> When creating reference stores we register them with the "chdir_notify"
> subsystem. This is required because some of the paths we track may be
> relative paths, so we have to reparent them in case the current working
> directory changes.
> 
> But while we register the reference stores, we never unregister them.
> This can have multiple outcomes:
> 
>   - For a repository's main reference database we essentially keep the
>     pointer alive. We never free that database, either, and our leak
>     checker doesn't notice because it's still registered.
> 
>   - For submodule and worktree reference databases we do eventually free
>     them in `repo_clear()`, so we may keep pointers to free'd memory
>     registered. We never notice though as we don't tend to chdir around
>     in the middle of the process.
> 
> We never noticed either of these symptoms, but they are obviously bad.
> 
> Partially fix those issues by unregistering the reference stores when
> releasing them. The leak of the main reference database will be fixed in
> a subsequent commit.
> 
> Note that this requires us to use `chdir_notify_register()` instead of
> `chdir_notify_reparent()`, as there is no infrastructure to unregister the
> latter. It ultimately doesn't matter much though: in a subsequent commit
> we'll drop this infrastructure completely. We merely require this step
> here so that we can fix the memory leaks ahead of time.

Since this version of the series dropped the last patch which stopped
using `chdir_notify_reparent()`, does the log message here need to be
updated?

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/files-backend.c    | 22 +++++++++++++++++++---
>  refs/packed-backend.c   | 16 +++++++++++++++-
>  refs/reftable-backend.c | 16 +++++++++++++++-
>  3 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index a4c7858787..296981584b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -100,6 +100,23 @@ static void clear_loose_ref_cache(struct files_ref_store *refs)
>  	}
>  }
>  
> +static void files_ref_store_reparent(const char *name UNUSED,
> +				     const char *old_cwd,
> +				     const char *new_cwd,
> +				     void *payload)
> +{
> +	struct files_ref_store *refs = payload;
> +	char *tmp;
> +
> +	tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
> +	free(refs->base.gitdir);
> +	refs->base.gitdir = tmp;
> +
> +	tmp = reparent_relative_path(old_cwd, new_cwd, refs->gitcommondir);
> +	free(refs->gitcommondir);
> +	refs->gitcommondir = tmp;
> +}

Ok, here is introduce a callback specific to the file ref store to
handle reparenting both the gitdir and commondir.

>  /*
>   * Create a new submodule ref cache and add it to the internal
>   * set of caches.
> @@ -128,9 +145,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
>  
>  	repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs);
>  
> -	chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
> -	chdir_notify_reparent("files-backend $GIT_COMMONDIR",
> -			      &refs->gitcommondir);
> +	chdir_notify_register(NULL, files_ref_store_reparent, refs);

We use the new callback here instead of relying on the generic callback
used by `chdir_notify_reparent()`.

>  	strbuf_release(&refdir);
>  
> @@ -182,6 +197,7 @@ static void files_ref_store_release(struct ref_store *ref_store)
>  	free(refs->gitcommondir);
>  	ref_store_release(refs->packed_ref_store);
>  	free(refs->packed_ref_store);
> +	chdir_notify_unregister(NULL, files_ref_store_reparent, refs);

This allows us to unregister the callback and avoid holding on
references which may have been free'd. Makes sense.

The rest of the patch does the exact same for the packed ref store and
reftable BE which look correct too.

-Justin