| Message ID | 20230622195059.320593-3-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Additional variables for git var | expand |
On 6/22/2023 3:50 PM, brian m. carlson wrote: > struct git_var { > const char *name; > const char *(*read)(int); > + int free; > }; I see you're expanding the git_var struct, and you do it more in patch 3. At that point, there will be two consecutive 'int' parameters, which can make things unclear as to how to proceed. > static struct git_var git_vars[] = { > - { "GIT_COMMITTER_IDENT", git_committer_info }, > - { "GIT_AUTHOR_IDENT", git_author_info }, > - { "GIT_EDITOR", editor }, > - { "GIT_SEQUENCE_EDITOR", sequence_editor }, > - { "GIT_PAGER", pager }, > - { "GIT_DEFAULT_BRANCH", default_branch }, > - { "GIT_SHELL_PATH", shell_path }, > + { "GIT_COMMITTER_IDENT", git_committer_info, 0 }, > + { "GIT_AUTHOR_IDENT", git_author_info, 0 }, This also makes for a big diff like this. One way to solve for this is to use the more modern style when initializing the structs: static struct git_var git_vars[] = { { .name = "GIT_COMMITTER_IDENT", .read = git_author_info, .free = 0, }, ... } Bonus points if you do this conversion before modifying the struct, as later changes will only add lines to the existing initialization... static struct git_var git_vars[] = { { .name = "GIT_COMMITTER_IDENT", .read = git_author_info, + .multivalued = 0, .free = 0, }, ... } Thanks, -Stolee
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > -static const char *git_etc_gitattributes(void) > +const char *git_etc_gitattributes(void) > { > static const char *system_wide; > if (!system_wide) > @@ -878,7 +878,7 @@ static const char *git_etc_gitattributes(void) > return system_wide; > } > > -static const char *get_home_gitattributes(void) > +const char *get_home_gitattributes(void) > { > if (!git_attributes_file) > git_attributes_file = xdg_config_home("attributes"); > @@ -886,7 +886,7 @@ static const char *get_home_gitattributes(void) > return git_attributes_file; > } These are sensible, but ... > -static int git_attr_system(void) > +int git_attr_system(void) > { > return !git_env_bool("GIT_ATTR_NOSYSTEM", 0); > } ... even though this is not exactly a new problem, but this function is misnamed and exposing it outside the file is not exactly nice. We would want to rename it to perhaps git_attr_use_system() or something? "allow" would also work as a verb there, but the point is without any verb, it is easily confused with what the function git_etc_git_attributes() wants to do. side note: arguably the "etc-gitattributes" and "home-gitattributes" are also suboptimal public names to give these functions, even though they are fine as long as they are confined in a subsystem. For global functions, it would be better to give name that match what the end-users think of them, when possible. The former would be "git_attr_system()" and the latter would be "git_attr_global()". > struct git_var { > const char *name; > const char *(*read)(int); > + int free; > }; > static struct git_var git_vars[] = { > - { "GIT_COMMITTER_IDENT", git_committer_info }, > - { "GIT_AUTHOR_IDENT", git_author_info }, > - { "GIT_EDITOR", editor }, > - { "GIT_SEQUENCE_EDITOR", sequence_editor }, > - { "GIT_PAGER", pager }, > - { "GIT_DEFAULT_BRANCH", default_branch }, > - { "GIT_SHELL_PATH", shell_path }, > + { "GIT_COMMITTER_IDENT", git_committer_info, 0 }, > + { "GIT_AUTHOR_IDENT", git_author_info, 0 }, > + { "GIT_EDITOR", editor, 0 }, > + { "GIT_SEQUENCE_EDITOR", sequence_editor, 0 }, > + { "GIT_PAGER", pager, 0 }, > + { "GIT_DEFAULT_BRANCH", default_branch, 0 }, > + { "GIT_SHELL_PATH", shell_path, 0 }, > + { "GIT_ATTR_SYSTEM", git_attr_val_system, 1 }, > + { "GIT_ATTR_GLOBAL", git_attr_val_global, 1 }, > { "", NULL }, > }; I am not sure if the "free" (which stands for "allocated") is worth adding here for such a single-use-and-then-exit command. It is a maintenance burden for anybody who adds new variables. Either mark these values with UNLEAK(), or make the ones that do not allocate xstrdup() so that they can be free'd blindly, would be less unwieldy option, I guess. > diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh > index 270bd4e512..6a2cc94abb 100755 > --- a/t/t0007-git-var.sh > +++ b/t/t0007-git-var.sh > @@ -159,6 +159,26 @@ test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' ' > grep "sh\$" shell > ' > > +test_expect_success 'GIT_ATTR_SYSTEM points to the correct location' ' I found it somewhat funny to claim we are checking that the variable points to the "correct" location, while the test is happy as long as it points at any path. > + test_must_fail env GIT_ATTR_NOSYSTEM=1 git var GIT_ATTR_SYSTEM && > + ( > + sane_unset GIT_ATTR_NOSYSTEM && > + git var GIT_ATTR_SYSTEM >path && > + test "$(cat path)" != "" > + ) > +' > + > +test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' ' > + TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" && > + XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL >path && > + test "$(cat path)" = "$TRASHDIR/.config/git/attributes" && > + ( > + sane_unset XDG_CONFIG_HOME && > + HOME="$TRASHDIR" git var GIT_ATTR_GLOBAL >path && > + test "$(cat path)" = "$TRASHDIR/.config/git/attributes" > + ) > +' This one is much less funny ;-) Very nice.
On 2023-06-22 at 20:19:46, Derrick Stolee wrote: > One way to solve for this is to use the more modern style > when initializing the structs: > > static struct git_var git_vars[] = { > { > .name = "GIT_COMMITTER_IDENT", > .read = git_author_info, > .free = 0, > }, > ... > } Good idea, I've got this in for a v2.
On Thu, Jun 22, 2023 at 4:06 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > Currently, there are some programs which would like to read and parse > the gitattributes files at the global or system levels. However, it's > not always obvious where these files live, especially for the system > file, which may have been hard-coded at compile time or computed > dynamically based on the runtime prefix. > > It's not reasonable to expect all callers of Git to intuitively know > where the Git distributor or user has configured these locations to > be, so add some entries to allow us to determine their location. Honor > the GIT_ATTR_NOSYSTEM environment variable if one is specified. Expose > the accessor functions in a way that we can reuse them from within the > var code. > > In order to make our paths consistent on Windows and also use the same > form as paths use in "git rev-parse", let's normalize the path before we > return it. This results in Windows-style paths that use slashes, which > is convenient for making our tests function in a consistent way across > platforms. Note that this requires that some of our values be freed, so > let's add a flag about whether the value needs to be freed and use it > accordingly. > > Signed-off-by: brian m. carlson <bk2204@github.com> > --- > diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh > @@ -159,6 +159,26 @@ test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' ' > +test_expect_success 'GIT_ATTR_SYSTEM points to the correct location' ' > + test_must_fail env GIT_ATTR_NOSYSTEM=1 git var GIT_ATTR_SYSTEM && > + ( > + sane_unset GIT_ATTR_NOSYSTEM && > + git var GIT_ATTR_SYSTEM >path && > + test "$(cat path)" != "" > + ) > +' Same observation as in patch [1/3]: no need for a temporary file: p=$(git var GIT_ATTR_SYSTEM) && test -n "$p" > +test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' ' > + TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" && The reference to $(pwd) is unnecessary, thus potentially confusing. Simpler: TRASHDIR="$(test-tool path-utils normalize_path_copy .)" && > + XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL >path && > + test "$(cat path)" = "$TRASHDIR/.config/git/attributes" && Same observation about unnecessary temporary file: p=$(XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL) && test "$p" = "$TRASHDIR/.config/git/attributes" && > + ( > + sane_unset XDG_CONFIG_HOME && > + HOME="$TRASHDIR" git var GIT_ATTR_GLOBAL >path && > + test "$(cat path)" = "$TRASHDIR/.config/git/attributes" > + ) And here too. > +'
On Thu, Jun 22, 2023 at 4:06 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > Currently, there are some programs which would like to read and parse > the gitattributes files at the global or system levels. However, it's > not always obvious where these files live, especially for the system > file, which may have been hard-coded at compile time or computed > dynamically based on the runtime prefix. > > It's not reasonable to expect all callers of Git to intuitively know > where the Git distributor or user has configured these locations to > be, so add some entries to allow us to determine their location. Honor > the GIT_ATTR_NOSYSTEM environment variable if one is specified. Expose > the accessor functions in a way that we can reuse them from within the > var code. > > In order to make our paths consistent on Windows and also use the same > form as paths use in "git rev-parse", let's normalize the path before we > return it. This results in Windows-style paths that use slashes, which > is convenient for making our tests function in a consistent way across > platforms. Note that this requires that some of our values be freed, so > let's add a flag about whether the value needs to be freed and use it > accordingly. > > Signed-off-by: brian m. carlson <bk2204@github.com> > --- > diff --git a/attr.h b/attr.h > @@ -227,4 +227,8 @@ void git_attr_set_direction(enum git_attr_direction new_direction); > +const char *git_etc_gitattributes(void); > +const char *get_home_gitattributes(void); > +int git_attr_system(void); I forgot to mention that it would be appreciated if you document these new public declarations. Even a one-sentence comment would be helpful. For instance: /* Return the location of the personal .gitattributes file. */ const char *get_home_gitattributes(void);
On 2023-06-22 at 21:18:46, Eric Sunshine wrote: > The reference to $(pwd) is unnecessary, thus potentially confusing. Simpler: > > TRASHDIR="$(test-tool path-utils normalize_path_copy .)" && Maybe. I spent a lot of time fighting with Windows and its path handling here (as much as writing the initial series). If that's possible and produces an acceptable result there, I can consider it for v2.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2023-06-22 at 20:19:46, Derrick Stolee wrote: >> One way to solve for this is to use the more modern style >> when initializing the structs: >> >> static struct git_var git_vars[] = { >> { >> .name = "GIT_COMMITTER_IDENT", >> .read = git_author_info, >> .free = 0, >> }, >> ... >> } > > Good idea, I've got this in for a v2. Yeah, this would make it palatable ;-)
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt index f0f647e14a..dfbe5cb52b 100644 --- a/Documentation/git-var.txt +++ b/Documentation/git-var.txt @@ -74,6 +74,12 @@ GIT_DEFAULT_BRANCH:: GIT_SHELL_PATH:: The path of the binary providing the POSIX shell for commands which use the shell. +GIT_ATTR_SYSTEM:: + The path to the system linkgit:gitattributes[5] file, if one is enabled. + +GIT_ATTR_GLOBAL:: + The path to the global (per-user) linkgit:gitattributes[5] file. + SEE ALSO -------- linkgit:git-commit-tree[1] diff --git a/attr.c b/attr.c index d45d34058d..822d214053 100644 --- a/attr.c +++ b/attr.c @@ -870,7 +870,7 @@ static struct attr_stack *read_attr(struct index_state *istate, return res; } -static const char *git_etc_gitattributes(void) +const char *git_etc_gitattributes(void) { static const char *system_wide; if (!system_wide) @@ -878,7 +878,7 @@ static const char *git_etc_gitattributes(void) return system_wide; } -static const char *get_home_gitattributes(void) +const char *get_home_gitattributes(void) { if (!git_attributes_file) git_attributes_file = xdg_config_home("attributes"); @@ -886,7 +886,7 @@ static const char *get_home_gitattributes(void) return git_attributes_file; } -static int git_attr_system(void) +int git_attr_system(void) { return !git_env_bool("GIT_ATTR_NOSYSTEM", 0); } diff --git a/attr.h b/attr.h index 676bd17ce2..05a2e4c622 100644 --- a/attr.h +++ b/attr.h @@ -227,4 +227,8 @@ void git_attr_set_direction(enum git_attr_direction new_direction); void attr_start(void); +const char *git_etc_gitattributes(void); +const char *get_home_gitattributes(void); +int git_attr_system(void); + #endif /* ATTR_H */ diff --git a/builtin/var.c b/builtin/var.c index f97178eed1..b9e2f23697 100644 --- a/builtin/var.c +++ b/builtin/var.c @@ -4,6 +4,7 @@ * Copyright (C) Eric Biederman, 2005 */ #include "builtin.h" +#include "attr.h" #include "config.h" #include "editor.h" #include "ident.h" @@ -41,18 +42,41 @@ static const char *shell_path(int flag) return SHELL_PATH; } +static const char *git_attr_val_system(int flag) +{ + if (git_attr_system()) { + char *file = xstrdup(git_etc_gitattributes()); + normalize_path_copy(file, file); + return file; + } + return NULL; +} + +static const char *git_attr_val_global(int flag) +{ + char *file = xstrdup(get_home_gitattributes()); + if (file) { + normalize_path_copy(file, file); + return file; + } + return NULL; +} + struct git_var { const char *name; const char *(*read)(int); + int free; }; static struct git_var git_vars[] = { - { "GIT_COMMITTER_IDENT", git_committer_info }, - { "GIT_AUTHOR_IDENT", git_author_info }, - { "GIT_EDITOR", editor }, - { "GIT_SEQUENCE_EDITOR", sequence_editor }, - { "GIT_PAGER", pager }, - { "GIT_DEFAULT_BRANCH", default_branch }, - { "GIT_SHELL_PATH", shell_path }, + { "GIT_COMMITTER_IDENT", git_committer_info, 0 }, + { "GIT_AUTHOR_IDENT", git_author_info, 0 }, + { "GIT_EDITOR", editor, 0 }, + { "GIT_SEQUENCE_EDITOR", sequence_editor, 0 }, + { "GIT_PAGER", pager, 0 }, + { "GIT_DEFAULT_BRANCH", default_branch, 0 }, + { "GIT_SHELL_PATH", shell_path, 0 }, + { "GIT_ATTR_SYSTEM", git_attr_val_system, 1 }, + { "GIT_ATTR_GLOBAL", git_attr_val_global, 1 }, { "", NULL }, }; @@ -62,8 +86,11 @@ static void list_vars(void) const char *val; for (ptr = git_vars; ptr->read; ptr++) - if ((val = ptr->read(0))) + if ((val = ptr->read(0))) { printf("%s=%s\n", ptr->name, val); + if (ptr->free) + free((void *)val); + } } static const struct git_var *get_git_var(const char *var) @@ -110,6 +137,8 @@ int cmd_var(int argc, const char **argv, const char *prefix UNUSED) return 1; printf("%s\n", val); + if (git_var->free) + free((void *)val); return 0; } diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh index 270bd4e512..6a2cc94abb 100755 --- a/t/t0007-git-var.sh +++ b/t/t0007-git-var.sh @@ -159,6 +159,26 @@ test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' ' grep "sh\$" shell ' +test_expect_success 'GIT_ATTR_SYSTEM points to the correct location' ' + test_must_fail env GIT_ATTR_NOSYSTEM=1 git var GIT_ATTR_SYSTEM && + ( + sane_unset GIT_ATTR_NOSYSTEM && + git var GIT_ATTR_SYSTEM >path && + test "$(cat path)" != "" + ) +' + +test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' ' + TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" && + XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL >path && + test "$(cat path)" = "$TRASHDIR/.config/git/attributes" && + ( + sane_unset XDG_CONFIG_HOME && + HOME="$TRASHDIR" git var GIT_ATTR_GLOBAL >path && + test "$(cat path)" = "$TRASHDIR/.config/git/attributes" + ) +' + # For git var -l, we check only a representative variable; # testing the whole output would make our test too brittle with # respect to unrelated changes in the test suite's environment.