From: Justin Tobler Date: Wed, 04 Mar 2026 20:39:57 GMT Subject: Re: [PATCH 04/17] odb: move reparenting logic into respective subsystems Message-ID: 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... > 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 > --- [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