git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Oct 8, 2024, 12:23 UTC
Message-ID
<ZwUkUuQgxaE2-djk@pks.im>
In-Reply-To
<ZwQSWcmr6HWTxxGL@five231003>
On Mon, Oct 07, 2024 at 10:24:49PM +0530, Kousik Sanagavarapu wrote:
Show 62 quoted lines
> Hi,
> 
> I have two questions but a bit of a background first -
> 
> 102de880d2 (path.c: migrate global git_path_* to take a repository
> argument, 2018-05-17) made global git_path_* functions take a repo
> argument.  The commit msg mentions that this migration doesn't change
> the local path functions in various builtins - which were defined using
> GIT_PATH_FUNC.  This was also the commit which introduced the macro
> REPO_GIT_PATH_FUNC.
> 
> Skip to 7ac16649ec (path: hide functions using `the_repository` by
> default, 2024-08-13), GIT_PATH_FUNC is hidden under
> USE_THE_REPOSITORY_VARIABLE and the REPO_GIT_PATH_FUNC is made its
> arbitrary repo equivalent - which can be inferred from the following
> portion of the diff
> 
> @@ -165,19 +130,10 @@ void report_linked_checkout_garbage(struct repository *r);
>  /*
>   * You can define a static memoized git path like:
>   *
> - *    static GIT_PATH_FUNC(git_path_foo, "FOO")
> + *    static REPO_GIT_PATH_FUNC(git_path_foo, "FOO")
>   *
>   * or use one of the global ones below.
>   */
> -#define GIT_PATH_FUNC(func, filename) \
> -       const char *func(void) \
> -       { \
> -               static char *ret; \
> -               if (!ret) \
> -                       ret = git_pathdup(filename); \
> -               return ret; \
> -       }
> -
>  #define REPO_GIT_PATH_FUNC(var, filename) \
>         const char *git_path_##var(struct repository *r) \
>         { \
> 
> (the GIT_PATH_FUNC macro is moved to be under USE_THE_REPOSITORY_VARIABLE)
> 
> Looking at the expansion of REPO_GIT_PATH_FUNC ...
> 
> #define REPO_GIT_PATH_FUNC(var, filename) \
> 	const char *git_path_##var(struct repository *r) \
> 	{ \
> 		if (!r->cached_paths.var) \
> 			r->cached_paths.var = repo_git_path(r, filename); \
> 		return r->cached_paths.var; \
> 	}
> 
> It seems that REPO_GIT_PATH_FUNC isn't an exact equivalent of
> GIT_PATH_FUNC.  That is, REPO_GIT_PATH_FUNC expects even a local path to be
> a field of the "struct repo_path_cache".  An example of a local path is
> EDIT_DESCRIPTION from "git branch --edit-description" (which inturn gets
> used by "git format-patch").
> 
> 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?

Show 12 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".

Show 5 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`.

Patrick
Previous: Kousik SanagavarapuNext: Kousik Sanagavarapu
Message 2 of 5 in “[Question] local paths when USE_THE_REPOSITORY_VARIABLE is not defined”
  1. Kousik SanagavarapuOct 7, 2024
  2. Patrick SteinhardtOct 8, 2024
  3. Kousik SanagavarapuOct 8, 2024
  4. Patrick SteinhardtOct 9, 2024
  5. Kousik SanagavarapuOct 9, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.