From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 05 Mar 2026 13:23:31 GMT
Subject: Re: [PATCH 04/17] odb: move reparenting logic into respective subsystems
Message-ID: <aamD0yUNTb0r2JQZ@pks.im>
In-Reply-To: <aaiSFpWY0YQ6XQcM@denethor>

On Wed, Mar 04, 2026 at 02:39:57PM -0600, Justin Tobler wrote:
> 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...

Yup, this reads clearer indeed.

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

Yeah, this will change eventually, but it's going to take a while to get
there. I plan to drop the "path" pointer from the base completely, as
other sources may not even have a path in the first place. But that
first requires us to address all instances where we directly access the
path

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

Once we are able to drop the `struct odb_source::path` field it should
become feasible. So I don't think we should add a NEEDSWORK comment now,
as it might mislead fellow developers to think it's already doable and
can be worked on right away.

Patrick