Re: [PATCH 04/17] odb: move reparenting logic into respective subsystems

[Justin Tobler <jltobler@gmail.com>]

On 26/02/23 05:17PM, Patrick Steinhardt wrote:
> The primary object database source may be initialized with a relative
> path. When reparenting the process to a different working directory we

I find the wording here a bit confusing. Maybe something like this would
be a bit clearer:

  When the process changes its current working directory...

> thus have to update this path and have it point to the same path, but
> relative to the new working directory.
> 
> This logic is handled in the object database layer. It consists of three
> steps:
> 
>   1. We undo any potential temporary object directory, which are used
>      for transactions. This is done so that we don't end up modifying
>      the temporary object database source that got applied for the
>      transaction.
> 
>   2. We then iterate through the non-transactional sources and reparent
>      their respective paths.
> 
>   3. We reapply the temporary object directory, but update its path.
> 
> All of this logic is heavily tied to how the object database source
> handles paths in the first place. It's an internal implementation
> detail, and as sources may not even use an on-disk path at all it is not
> a mechanism that applies to all potential sources.

Indeed this mechanism is directly coupled to how the "files" backend
operates.

> Refactor the code so that the logic to reparent the sources is hosted by
> the "files" source and the temporary object directory subsystems,
> respectively. This logic is easier to reason about, but it also ensures
> that this logic is handled at the correct level.

Makes sense.
 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> diff --git a/odb/source-files.c b/odb/source-files.c
> index a43a197157..df0ea9ee62 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -1,13 +1,28 @@
>  #include "git-compat-util.h"
> +#include "abspath.h"
> +#include "chdir-notify.h"
>  #include "object-file.h"
>  #include "odb/source.h"
>  #include "odb/source-files.h"
>  #include "packfile.h"
>  
> +static void odb_source_files_reparent(const char *name UNUSED,
> +				      const char *old_cwd,
> +				      const char *new_cwd,
> +				      void *cb_data)
> +{
> +	struct odb_source_files *files = cb_data;
> +	char *path = reparent_relative_path(old_cwd, new_cwd,
> +					    files->base.path);
> +	free(files->base.path);
> +	files->base.path = path;

I do find it a bit curious that we consider the "path" to be specific to
the "files" backend, but still track it as part of the "base" ODB
source. I suspect this will eventually change though?

> +}
> +
>  void odb_source_files_free(struct odb_source_files *files)
>  {
>  	if (!files)
>  		return;
> +	chdir_notify_unregister(NULL, odb_source_files_reparent, files);
>  	odb_source_loose_free(files->loose);
>  	packfile_store_free(files->packed);
>  	odb_source_release(&files->base);
> @@ -25,5 +40,13 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
>  	files->loose = odb_source_loose_new(&files->base);
>  	files->packed = packfile_store_new(&files->base);
>  
> +	/*
> +	 * Ideally, we would only ever store absolute paths in the source. This
> +	 * is not (yet) possible though because we access and assume relative
> +	 * paths in the primary ODB source in some user-facing functionality.
> +	 */

Should this be a NEEDSWORK comment? Or do we expect it to remain this
way for the forseeable future?

> +	if (!is_absolute_path(path))
> +		chdir_notify_register(NULL, odb_source_files_reparent, files);

Ok so now a callback to reparent the path is set up for the "files"
source when it is created. If there are multiple "files" sources
created, each source will be handled separately.

> +
>  	return files;
>  }
> diff --git a/tmp-objdir.c b/tmp-objdir.c
> index 9f5a1788cd..e436eed07e 100644
> --- a/tmp-objdir.c
> +++ b/tmp-objdir.c
> @@ -36,6 +36,21 @@ static void tmp_objdir_free(struct tmp_objdir *t)
>  	free(t);
>  }
>  
> +static void tmp_objdir_reparent(const char *name UNUSED,
> +				const char *old_cwd,
> +				const char *new_cwd,
> +				void *cb_data)
> +{
> +	struct tmp_objdir *t = cb_data;
> +	char *path;
> +
> +	path = reparent_relative_path(old_cwd, new_cwd,
> +				      t->path.buf);
> +	strbuf_reset(&t->path);
> +	strbuf_addstr(&t->path, path);
> +	free(path);
> +}

Ok, at first I was a bit confused as to why we needed this logic for the
tmpdir as well. I thought reparenting as only applied to the primary
ODB, but it looks like the tmpdir was also reparented via
tmp_objdir_reapply_primary_odb().

> +
>  int tmp_objdir_destroy(struct tmp_objdir *t)
>  {
>  	int err;
> @@ -51,6 +66,7 @@ int tmp_objdir_destroy(struct tmp_objdir *t)
>  
>  	err = remove_dir_recursively(&t->path, 0);
>  
> +	chdir_notify_unregister(NULL, tmp_objdir_reparent, t);
>  	tmp_objdir_free(t);
>  
>  	return err;
> @@ -137,6 +153,9 @@ struct tmp_objdir *tmp_objdir_create(struct repository *r,
>  	strbuf_addf(&t->path, "%s/tmp_objdir-%s-XXXXXX",
>  		    repo_get_object_directory(r), prefix);
>  
> +	if (!is_absolute_path(t->path.buf))
> +		chdir_notify_register(NULL, tmp_objdir_reparent, t);
> +
>  	if (!mkdtemp(t->path.buf)) {
>  		/* free, not destroy, as we never touched the filesystem */
>  		tmp_objdir_free(t);
[snip]
> diff --git a/tmp-objdir.h b/tmp-objdir.h
> index fceda14979..ccf800faa7 100644
> --- a/tmp-objdir.h
> +++ b/tmp-objdir.h
> @@ -68,19 +68,4 @@ void tmp_objdir_add_as_alternate(const struct tmp_objdir *);
>   */
>  void tmp_objdir_replace_primary_odb(struct tmp_objdir *, int will_destroy);
>  
> -/*
> - * If the primary object database was replaced by a temporary object directory,
> - * restore it to its original value while keeping the directory contents around.
> - * Returns NULL if the primary object database was not replaced.
> - */
> -struct tmp_objdir *tmp_objdir_unapply_primary_odb(void);
> -
> -/*
> - * Reapplies the former primary temporary object database, after potentially
> - * changing its relative path.
> - */
> -void tmp_objdir_reapply_primary_odb(struct tmp_objdir *, const char *old_cwd,
> -		const char *new_cwd);

These functions are no longer needed because each of the sources have
their paths updated directly via separate registered callbacks. Makes
sense.

-Justin