Re: [Question] local paths when USE_THE_REPOSITORY_VARIABLE is not defined

From: Kousik Sanagavarapu <five231003@gmail.com>
Date: Oct 8, 2024, 17:59 UTC
Message-ID: <ZwVzF9Xgn72tT5Ee@five231003>
In-Reply-To: <ZwUkUuQgxaE2-djk@pks.im>

On Tue, Oct 08, 2024 at 02:23:54PM +0200, Patrick Steinhardt wrote:

Show 16 quoted lines

> On Mon, Oct 07, 2024 at 10:24:49PM +0530, Kousik Sanagavarapu wrote:
> > Hi,
> > 
> > I have two questions but a bit of a background first -
> > 
> > [...]
> > 
> > So my question is - do we want, in the future in which we are free from
> > the dependency on "the_repository", for all the local paths to be a part
> > of "struct repo_path_cache"?  Which in my gut feels wrong - one alternative
> > then is that  we will have to refactor REPO_GIT_PATH_FUNC - or am I missing
> > something here?
> 
> What I don't quite understand: what is the problem with making it part
> of the `struct repo_path_cache`? Does this cause an actual issue, or is
> it merely that you feel it is unnecessary complexity?

I feel it is unnecessary complexity.

	$ git grep -E "(static GIT_PATH_FUNC|^GIT_PATH_FUNC)" | wc -l
	65

Meaning each of these would have to have an entry in "struct repo_path_cache" in the world where we don't rely on "the_repository". Some of these are also not direct ".git/some-file" but ".git/dir/files" where ".git/dir" is also given by a seperate path func, like ".git/rebase-merges" and ".git/rebase-merges/head-name".

So why hold pointers to such filenames instead of just calling repo_git_path() manually - all these filenames are "local" anyways - unlike say files such as "SQUASH_MSG"?

Show 19 quoted lines

> > I got into this when I was trying to refactor builtin/branch.c to be
> > independent of "the_repository".  It was a very naive approach of just
> > manual conversion of all the git_* calls to repo_* calls and similar
> > changes but the compiler started to complain since I overlooked
> > GIT_PATH_FUNC and some variables in environment.h which are also hidden
> > under USE_THE_REPOSITORY_VARIABLE.
> > 
> > Which raises another question - why are variables such as
> > "comment_line_str" and "default_abbrev" hidden under
> > USE_THE_REPOSITORY_VARIABLE?[1]  They don't seem to be dependent on
> > "the_repository"?  Again, I might be missing something here but am not
> > sure what.
> 
> They do depend on `the_repository`, but implicitly only. The problem is
> that those variables are populated via the config, and that may include
> repository-local configuration. As such they contain values that have
> been derived via `the_repository`, and those values may not be the
> correct value when you handle multiple repositories in a single process,
> because those may have a different value for e.g. "core.commentChar".

I see. Guess I didn't do my research right - didn't know about "core.commentChar".

Show 11 quoted lines

> > By the way I don't expect this "naive approach" to be the right method
> > of doing this - I was just tinkering to get to know
> > USE_THE_REPOSITORY_VARIABLE better - since builtin/branch.c also calls
> > into ref-filter which heavily relies on "the_repository" so changes
> > there also would be appropriate for the complete picture.
> 
> Yeah, in the ideal case you'd first adapt any underlying code that you
> happen to spot that relies on `the_repository`. That doesn't always
> work as it is easy to miss that something implicitly depends on the
> variable. But in case such a dependency is missed it will get to light
> eventually as we continue with our quest to remove `the_repository`.

Thanks for such a nice explanation.