On Tue, Oct 08, 2024 at 02:23:54PM +0200, Patrick Steinhardt wrote: > 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"? > > 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". > > 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.
