{"thread":{"id":"62271","subject":"[Question] local paths when USE_THE_REPOSITORY_VARIABLE is not defined","startedAt":"2024-10-07T16:54:55Z","lastAt":"2024-10-09T06:20:12Z","messageCount":5,"participants":["Kousik Sanagavarapu","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"504346","messageId":"ZwQSWcmr6HWTxxGL@five231003","threadId":"62271","inReplyTo":null,"subject":"[Question] local paths when USE_THE_REPOSITORY_VARIABLE is not defined","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-10-07T16:54:49Z","receivedAt":"2024-10-07T16:54:55Z","isPatch":false,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Hi,\n\nI have two questions but a bit of a background first -\n\n102de880d2 (path.c: migrate global git_path_* to take a repository\nargument, 2018-05-17) made global git_path_* functions take a repo\nargument.  The commit msg mentions that this migration doesn't change\nthe local path functions in various builtins - which were defined using\nGIT_PATH_FUNC.  This was also the commit which introduced the macro\nREPO_GIT_PATH_FUNC.\n\nSkip to 7ac16649ec (path: hide functions using `the_repository` by\ndefault, 2024-08-13), GIT_PATH_FUNC is hidden under\nUSE_THE_REPOSITORY_VARIABLE and the REPO_GIT_PATH_FUNC is made its\narbitrary repo equivalent - which can be inferred from the following\nportion of the diff\n\n@@ -165,19 +130,10 @@ void report_linked_checkout_garbage(struct repository *r);\n /*\n  * You can define a static memoized git path like:\n  *\n- *    static GIT_PATH_FUNC(git_path_foo, \"FOO\")\n+ *    static REPO_GIT_PATH_FUNC(git_path_foo, \"FOO\")\n  *\n  * or use one of the global ones below.\n  */\n-#define GIT_PATH_FUNC(func, filename) \\\n-       const char *func(void) \\\n-       { \\\n-               static char *ret; \\\n-               if (!ret) \\\n-                       ret = git_pathdup(filename); \\\n-               return ret; \\\n-       }\n-\n #define REPO_GIT_PATH_FUNC(var, filename) \\\n        const char *git_path_##var(struct repository *r) \\\n        { \\\n\n(the GIT_PATH_FUNC macro is moved to be under USE_THE_REPOSITORY_VARIABLE)\n\nLooking at the expansion of REPO_GIT_PATH_FUNC ...\n\n#define REPO_GIT_PATH_FUNC(var, filename) \\\n\tconst char *git_path_##var(struct repository *r) \\\n\t{ \\\n\t\tif (!r->cached_paths.var) \\\n\t\t\tr->cached_paths.var = repo_git_path(r, filename); \\\n\t\treturn r->cached_paths.var; \\\n\t}\n\nIt seems that REPO_GIT_PATH_FUNC isn't an exact equivalent of\nGIT_PATH_FUNC.  That is, REPO_GIT_PATH_FUNC expects even a local path to be\na field of the \"struct repo_path_cache\".  An example of a local path is\nEDIT_DESCRIPTION from \"git branch --edit-description\" (which inturn gets\nused by \"git format-patch\").\n\nSo my question is - do we want, in the future in which we are free from\nthe dependency on \"the_repository\", for all the local paths to be a part\nof \"struct repo_path_cache\"?  Which in my gut feels wrong - one alternative\nthen is that  we will have to refactor REPO_GIT_PATH_FUNC - or am I missing\nsomething here?\n\nI got into this when I was trying to refactor builtin/branch.c to be\nindependent of \"the_repository\".  It was a very naive approach of just\nmanual conversion of all the git_* calls to repo_* calls and similar\nchanges but the compiler started to complain since I overlooked\nGIT_PATH_FUNC and some variables in environment.h which are also hidden\nunder USE_THE_REPOSITORY_VARIABLE.\n\nWhich raises another question - why are variables such as\n\"comment_line_str\" and \"default_abbrev\" hidden under\nUSE_THE_REPOSITORY_VARIABLE?[1]  They don't seem to be dependent on\n\"the_repository\"?  Again, I might be missing something here but am not\nsure what.\n\nBy the way I don't expect this \"naive approach\" to be the right method\nof doing this - I was just tinkering to get to know\nUSE_THE_REPOSITORY_VARIABLE better - since builtin/branch.c also calls\ninto ref-filter which heavily relies on \"the_repository\" so changes\nthere also would be appropriate for the complete picture.\n\n[1] See\n\n- f2d70847bd (environment: reorder header to split out `the_repository`-free\n  section, 2024-09-12)\n\n- 673af418d0 (environment: guard state depending on a repository, 2024-09-12)\n"},{"id":"504418","messageId":"ZwUkUuQgxaE2-djk@pks.im","threadId":"62271","inReplyTo":"ZwQSWcmr6HWTxxGL@five231003","subject":"Re: [Question] local paths when USE_THE_REPOSITORY_VARIABLE is not defined","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-08T12:23:54Z","receivedAt":"2024-10-08T12:23:58Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 07, 2024 at 10:24:49PM +0530, Kousik Sanagavarapu wrote:\n> Hi,\n> \n> I have two questions but a bit of a background first -\n> \n> 102de880d2 (path.c: migrate global git_path_* to take a repository\n> argument, 2018-05-17) made global git_path_* functions take a repo\n> argument.  The commit msg mentions that this migration doesn't change\n> the local path functions in various builtins - which were defined using\n> GIT_PATH_FUNC.  This was also the commit which introduced the macro\n> REPO_GIT_PATH_FUNC.\n> \n> Skip to 7ac16649ec (path: hide functions using `the_repository` by\n> default, 2024-08-13), GIT_PATH_FUNC is hidden under\n> USE_THE_REPOSITORY_VARIABLE and the REPO_GIT_PATH_FUNC is made its\n> arbitrary repo equivalent - which can be inferred from the following\n> portion of the diff\n> \n> @@ -165,19 +130,10 @@ void report_linked_checkout_garbage(struct repository *r);\n>  /*\n>   * You can define a static memoized git path like:\n>   *\n> - *    static GIT_PATH_FUNC(git_path_foo, \"FOO\")\n> + *    static REPO_GIT_PATH_FUNC(git_path_foo, \"FOO\")\n>   *\n>   * or use one of the global ones below.\n>   */\n> -#define GIT_PATH_FUNC(func, filename) \\\n> -       const char *func(void) \\\n> -       { \\\n> -               static char *ret; \\\n> -               if (!ret) \\\n> -                       ret = git_pathdup(filename); \\\n> -               return ret; \\\n> -       }\n> -\n>  #define REPO_GIT_PATH_FUNC(var, filename) \\\n>         const char *git_path_##var(struct repository *r) \\\n>         { \\\n> \n> (the GIT_PATH_FUNC macro is moved to be under USE_THE_REPOSITORY_VARIABLE)\n> \n> Looking at the expansion of REPO_GIT_PATH_FUNC ...\n> \n> #define REPO_GIT_PATH_FUNC(var, filename) \\\n> \tconst char *git_path_##var(struct repository *r) \\\n> \t{ \\\n> \t\tif (!r->cached_paths.var) \\\n> \t\t\tr->cached_paths.var = repo_git_path(r, filename); \\\n> \t\treturn r->cached_paths.var; \\\n> \t}\n> \n> It seems that REPO_GIT_PATH_FUNC isn't an exact equivalent of\n> GIT_PATH_FUNC.  That is, REPO_GIT_PATH_FUNC expects even a local path to be\n> a field of the \"struct repo_path_cache\".  An example of a local path is\n> EDIT_DESCRIPTION from \"git branch --edit-description\" (which inturn gets\n> used by \"git format-patch\").\n> \n> So my question is - do we want, in the future in which we are free from\n> the dependency on \"the_repository\", for all the local paths to be a part\n> of \"struct repo_path_cache\"?  Which in my gut feels wrong - one alternative\n> then is that  we will have to refactor REPO_GIT_PATH_FUNC - or am I missing\n> something here?\n\nWhat I don't quite understand: what is the problem with making it part\nof the `struct repo_path_cache`? Does this cause an actual issue, or is\nit merely that you feel it is unnecessary complexity?\n\n> I got into this when I was trying to refactor builtin/branch.c to be\n> independent of \"the_repository\".  It was a very naive approach of just\n> manual conversion of all the git_* calls to repo_* calls and similar\n> changes but the compiler started to complain since I overlooked\n> GIT_PATH_FUNC and some variables in environment.h which are also hidden\n> under USE_THE_REPOSITORY_VARIABLE.\n> \n> Which raises another question - why are variables such as\n> \"comment_line_str\" and \"default_abbrev\" hidden under\n> USE_THE_REPOSITORY_VARIABLE?[1]  They don't seem to be dependent on\n> \"the_repository\"?  Again, I might be missing something here but am not\n> sure what.\n\nThey do depend on `the_repository`, but implicitly only. The problem is\nthat those variables are populated via the config, and that may include\nrepository-local configuration. As such they contain values that have\nbeen derived via `the_repository`, and those values may not be the\ncorrect value when you handle multiple repositories in a single process,\nbecause those may have a different value for e.g. \"core.commentChar\".\n\n> By the way I don't expect this \"naive approach\" to be the right method\n> of doing this - I was just tinkering to get to know\n> USE_THE_REPOSITORY_VARIABLE better - since builtin/branch.c also calls\n> into ref-filter which heavily relies on \"the_repository\" so changes\n> there also would be appropriate for the complete picture.\n\nYeah, in the ideal case you'd first adapt any underlying code that you\nhappen to spot that relies on `the_repository`. That doesn't always\nwork as it is easy to miss that something implicitly depends on the\nvariable. But in case such a dependency is missed it will get to light\neventually as we continue with our quest to remove `the_repository`.\n\nPatrick\n"},{"id":"504462","messageId":"ZwVzF9Xgn72tT5Ee@five231003","threadId":"62271","inReplyTo":"ZwUkUuQgxaE2-djk@pks.im","subject":"Re: [Question] local paths when USE_THE_REPOSITORY_VARIABLE is not defined","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-10-08T17:59:51Z","receivedAt":"2024-10-08T17:59:55Z","isPatch":false,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Tue, Oct 08, 2024 at 02:23:54PM +0200, Patrick Steinhardt wrote:\n> On Mon, Oct 07, 2024 at 10:24:49PM +0530, Kousik Sanagavarapu wrote:\n> > Hi,\n> > \n> > I have two questions but a bit of a background first -\n> > \n> > [...]\n> > \n> > So my question is - do we want, in the future in which we are free from\n> > the dependency on \"the_repository\", for all the local paths to be a part\n> > of \"struct repo_path_cache\"?  Which in my gut feels wrong - one alternative\n> > then is that  we will have to refactor REPO_GIT_PATH_FUNC - or am I missing\n> > something here?\n> \n> What I don't quite understand: what is the problem with making it part\n> of the `struct repo_path_cache`? Does this cause an actual issue, or is\n> it merely that you feel it is unnecessary complexity?\n\nI feel it is unnecessary complexity.\n\n\t$ git grep -E \"(static GIT_PATH_FUNC|^GIT_PATH_FUNC)\" | wc -l\n\t65\n\nMeaning each of these would have to have an entry in\n\"struct repo_path_cache\" in the world where we don't rely on\n\"the_repository\".  Some of these are also not direct \".git/some-file\" but\n\".git/dir/files\" where \".git/dir\" is also given by a seperate path func,\nlike \".git/rebase-merges\" and \".git/rebase-merges/head-name\".\n\nSo why hold pointers to such filenames instead of just calling\nrepo_git_path() manually - all these filenames are \"local\" anyways - unlike\nsay files such as \"SQUASH_MSG\"?\n\n> > I got into this when I was trying to refactor builtin/branch.c to be\n> > independent of \"the_repository\".  It was a very naive approach of just\n> > manual conversion of all the git_* calls to repo_* calls and similar\n> > changes but the compiler started to complain since I overlooked\n> > GIT_PATH_FUNC and some variables in environment.h which are also hidden\n> > under USE_THE_REPOSITORY_VARIABLE.\n> > \n> > Which raises another question - why are variables such as\n> > \"comment_line_str\" and \"default_abbrev\" hidden under\n> > USE_THE_REPOSITORY_VARIABLE?[1]  They don't seem to be dependent on\n> > \"the_repository\"?  Again, I might be missing something here but am not\n> > sure what.\n> \n> They do depend on `the_repository`, but implicitly only. The problem is\n> that those variables are populated via the config, and that may include\n> repository-local configuration. As such they contain values that have\n> been derived via `the_repository`, and those values may not be the\n> correct value when you handle multiple repositories in a single process,\n> because those may have a different value for e.g. \"core.commentChar\".\n\nI see.  Guess I didn't do my research right - didn't know about\n\"core.commentChar\".\n\n> > By the way I don't expect this \"naive approach\" to be the right method\n> > of doing this - I was just tinkering to get to know\n> > USE_THE_REPOSITORY_VARIABLE better - since builtin/branch.c also calls\n> > into ref-filter which heavily relies on \"the_repository\" so changes\n> > there also would be appropriate for the complete picture.\n> \n> Yeah, in the ideal case you'd first adapt any underlying code that you\n> happen to spot that relies on `the_repository`. That doesn't always\n> work as it is easy to miss that something implicitly depends on the\n> variable. But in case such a dependency is missed it will get to light\n> eventually as we continue with our quest to remove `the_repository`.\n\nThanks for such a nice explanation.\n"},{"id":"504513","messageId":"ZwX7ieAvmjQma45E@pks.im","threadId":"62271","inReplyTo":"ZwVzF9Xgn72tT5Ee@five231003","subject":"Re: [Question] local paths when USE_THE_REPOSITORY_VARIABLE is not defined","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-09T03:42:01Z","receivedAt":"2024-10-09T03:42:07Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 08, 2024 at 11:29:51PM +0530, Kousik Sanagavarapu wrote:\n> On Tue, Oct 08, 2024 at 02:23:54PM +0200, Patrick Steinhardt wrote:\n> > On Mon, Oct 07, 2024 at 10:24:49PM +0530, Kousik Sanagavarapu wrote:\n> > > Hi,\n> > > \n> > > I have two questions but a bit of a background first -\n> > > \n> > > [...]\n> > > \n> > > So my question is - do we want, in the future in which we are free from\n> > > the dependency on \"the_repository\", for all the local paths to be a part\n> > > of \"struct repo_path_cache\"?  Which in my gut feels wrong - one alternative\n> > > then is that  we will have to refactor REPO_GIT_PATH_FUNC - or am I missing\n> > > something here?\n> > \n> > What I don't quite understand: what is the problem with making it part\n> > of the `struct repo_path_cache`? Does this cause an actual issue, or is\n> > it merely that you feel it is unnecessary complexity?\n> \n> I feel it is unnecessary complexity.\n> \n> \t$ git grep -E \"(static GIT_PATH_FUNC|^GIT_PATH_FUNC)\" | wc -l\n> \t65\n> \n> Meaning each of these would have to have an entry in\n> \"struct repo_path_cache\" in the world where we don't rely on\n> \"the_repository\".  Some of these are also not direct \".git/some-file\" but\n> \".git/dir/files\" where \".git/dir\" is also given by a seperate path func,\n> like \".git/rebase-merges\" and \".git/rebase-merges/head-name\".\n> \n> So why hold pointers to such filenames instead of just calling\n> repo_git_path() manually - all these filenames are \"local\" anyways - unlike\n> say files such as \"SQUASH_MSG\"?\n\nIt does make the interface easier to use at times because you don't have\nto worry about freeing returned strings. In other situations it likely\nis unnecessary.\n\nIn any case, not all cases must strictly be converted to REPO_PATH_FUNC.\nA refactoring may also decide that using e.g. `repo_git_path()` or\n`repo_common_pathv()` might be better alternatives.\n\nPatrick\n"},{"id":"504517","messageId":"ZwYgmNe6qk6jBZVT@five231003","threadId":"62271","inReplyTo":"ZwX7ieAvmjQma45E@pks.im","subject":"Re: [Question] local paths when USE_THE_REPOSITORY_VARIABLE is not defined","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2024-10-09T06:20:08Z","receivedAt":"2024-10-09T06:20:12Z","isPatch":false,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Wed, Oct 09, 2024 at 05:42:01AM +0200, Patrick Steinhardt wrote:\n> On Tue, Oct 08, 2024 at 11:29:51PM +0530, Kousik Sanagavarapu wrote:\n> > On Tue, Oct 08, 2024 at 02:23:54PM +0200, Patrick Steinhardt wrote:\n> > > On Mon, Oct 07, 2024 at 10:24:49PM +0530, Kousik Sanagavarapu wrote:\n> > > > Hi,\n> > > > \n> > > > I have two questions but a bit of a background first -\n> > > > \n> > > > [...]\n> > > > \n> > > > So my question is - do we want, in the future in which we are free from\n> > > > the dependency on \"the_repository\", for all the local paths to be a part\n> > > > of \"struct repo_path_cache\"?  Which in my gut feels wrong - one alternative\n> > > > then is that  we will have to refactor REPO_GIT_PATH_FUNC - or am I missing\n> > > > something here?\n> > > \n> > > What I don't quite understand: what is the problem with making it part\n> > > of the `struct repo_path_cache`? Does this cause an actual issue, or is\n> > > it merely that you feel it is unnecessary complexity?\n> > \n> > I feel it is unnecessary complexity.\n> > \n> > \t$ git grep -E \"(static GIT_PATH_FUNC|^GIT_PATH_FUNC)\" | wc -l\n> > \t65\n> > \n> > Meaning each of these would have to have an entry in\n> > \"struct repo_path_cache\" in the world where we don't rely on\n> > \"the_repository\".  Some of these are also not direct \".git/some-file\" but\n> > \".git/dir/files\" where \".git/dir\" is also given by a seperate path func,\n> > like \".git/rebase-merges\" and \".git/rebase-merges/head-name\".\n> > \n> > So why hold pointers to such filenames instead of just calling\n> > repo_git_path() manually - all these filenames are \"local\" anyways - unlike\n> > say files such as \"SQUASH_MSG\"?\n> \n> It does make the interface easier to use at times because you don't have\n> to worry about freeing returned strings. In other situations it likely\n> is unnecessary.\n> \n> In any case, not all cases must strictly be converted to REPO_PATH_FUNC.\n> A refactoring may also decide that using e.g. `repo_git_path()` or\n> `repo_common_pathv()` might be better alternatives.\n\nGot it.  Thanks again for the nice explanations.\n"}]}