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

From: Justin Tobler <jltobler@gmail.com>
Date: Mar 4, 2026, 20:39 UTC
Message-ID: <aaiSFpWY0YQ6XQcM@denethor>
In-Reply-To: <20260223-b4-pks-odb-source-pluggable-v1-4-253bac1db598@pks.im>

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

Show 20 quoted lines

> 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]

Show 23 quoted lines

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

Show 19 quoted lines

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

Show 25 quoted lines

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

Show 22 quoted lines

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

Show 21 quoted lines

> 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