| Message ID | 20230626190008.644769-7-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Additional variables for git var | expand |
On Mon, Jun 26, 2023 at 07:00:07PM +0000, brian m. carlson wrote: > @@ -51,6 +52,26 @@ static char *shell_path(int flag) > return xstrdup(SHELL_PATH); > } > > +static char *git_attr_val_system(int flag) > +{ > + if (git_attr_system_is_enabled()) { > + char *file = xstrdup(git_attr_system_file()); > + normalize_path_copy(file, file); > + return file; > + } > + return NULL; > +} These new ones would ideally mark the "flag" variable with the UNUSED attribute (in preparation for building with -Wunused-parameter). I can also come through later and fix them up in a separate patch. It's slightly awkward, just because I was about to post a patch that fixed the existing functions in that file, and I'd have to either rebase on top, or make a second pass once this is merged. That said, I also renamed the "flag" variable in my patch because it's super confusing (see my patch below for reference). So adjusting your new callers to match (without my changes) would be a little weird. The least-weird thing would be sticking my patch at the front of your series, but I don't want to make you do extra work. So I dunno. I'm mostly giving a heads-up, and seeing if you (or other reviewers in the thread) have an idea to make this "flag" thing less awful. I'm also happy to just do my topic separately, and then eventually circle back after yours is merged. -- >8 -- Subject: [PATCH] var: mark unused parameters in git_var callbacks We abstract the set of variables into a table, with a "read" callback to provide the value of each. Each callback takes a "flag" argument, but most callbacks don't make use of it. This flag is a bit odd. It may be set to IDENT_STRICT, which make sense for ident-based callbacks, but is just confusing for things like GIT_EDITOR. At first glance, it seems like this is just a hack to let us directly stick the generic git_committer_info() and git_author_info() functions into our table. And we'd be better off to wrap them with local functions which pass IDENT_STRICT, and have our callbacks take no option at all. But that doesn't quite work. We pass IDENT_STRICT when the caller asks for a specific variable, but otherwise do not (so that "git var -l" does not bail if the committer ident cannot be formed). So we really do need to pass in the flag to each invocation, even if the individual callback doesn't care about it. Let's mark the unused ones so that -Wunused-parameter does not complain. And while we're here, let's rename them so that it's clear that the flag values we get will be from the IDENT_* set. That may prevent confusion for future readers of the code. Another option would be to define our own local "strict" flag for the callbacks, and then have wrappers that translate that to IDENT_STRICT where it matters. But that would be more boilerplate for little gain (most functions would still ignore the "strict" flag anyway). Signed-off-by: Jeff King <peff@peff.net> --- builtin/var.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/var.c b/builtin/var.c index 2149998980..10ee62f84c 100644 --- a/builtin/var.c +++ b/builtin/var.c @@ -12,17 +12,17 @@ static const char var_usage[] = "git var (-l | <variable>)"; -static const char *editor(int flag) +static const char *editor(int ident_flag UNUSED) { return git_editor(); } -static const char *sequence_editor(int flag) +static const char *sequence_editor(int ident_flag UNUSED) { return git_sequence_editor(); } -static const char *pager(int flag) +static const char *pager(int ident_flag UNUSED) { const char *pgm = git_pager(1); @@ -31,7 +31,7 @@ static const char *pager(int flag) return pgm; } -static const char *default_branch(int flag) +static const char *default_branch(int ident_flag UNUSED) { return git_default_branch_name(1); }
On 2023-06-27 at 07:05:57, Jeff King wrote: > On Mon, Jun 26, 2023 at 07:00:07PM +0000, brian m. carlson wrote: > > > @@ -51,6 +52,26 @@ static char *shell_path(int flag) > > return xstrdup(SHELL_PATH); > > } > > > > +static char *git_attr_val_system(int flag) > > +{ > > + if (git_attr_system_is_enabled()) { > > + char *file = xstrdup(git_attr_system_file()); > > + normalize_path_copy(file, file); > > + return file; > > + } > > + return NULL; > > +} > > These new ones would ideally mark the "flag" variable with the UNUSED > attribute (in preparation for building with -Wunused-parameter). > > I can also come through later and fix them up in a separate patch. It's > slightly awkward, just because I was about to post a patch that fixed > the existing functions in that file, and I'd have to either rebase on > top, or make a second pass once this is merged. > > That said, I also renamed the "flag" variable in my patch because it's > super confusing (see my patch below for reference). So adjusting your > new callers to match (without my changes) would be a little weird. The > least-weird thing would be sticking my patch at the front of your > series, but I don't want to make you do extra work. > > So I dunno. I'm mostly giving a heads-up, and seeing if you (or other > reviewers in the thread) have an idea to make this "flag" thing less > awful. I'm also happy to just do my topic separately, and then > eventually circle back after yours is merged. I've picked up your patch as the first patch in the series and will send it out in v3 in just a few minutes. Since I plan to have v3 be the last round of this series, I'll let you send out any further changes as fixups on top of that.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: >> So I dunno. I'm mostly giving a heads-up, and seeing if you (or other >> reviewers in the thread) have an idea to make this "flag" thing less >> awful. I'm also happy to just do my topic separately, and then >> eventually circle back after yours is merged. > > I've picked up your patch as the first patch in the series and will send > it out in v3 in just a few minutes. Since I plan to have v3 be the last > round of this series, I'll let you send out any further changes as > fixups on top of that. Sounds like a sensible way to go, at least to me. Thanks for working well together. I wish all conflicting topics worked nicely with each other like these ;-)
On Tue, Jun 27, 2023 at 04:12:27PM +0000, brian m. carlson wrote: > > So I dunno. I'm mostly giving a heads-up, and seeing if you (or other > > reviewers in the thread) have an idea to make this "flag" thing less > > awful. I'm also happy to just do my topic separately, and then > > eventually circle back after yours is merged. > > I've picked up your patch as the first patch in the series and will send > it out in v3 in just a few minutes. Since I plan to have v3 be the last > round of this series, I'll let you send out any further changes as > fixups on top of that. Thanks, that is optimal from my perspective. :) The resulting series looks good to me, both the changes you integrated from me, but also the whole thing overall. It is a little funny to stuff the GIT_CONFIG_GLOBAL values into a string only to re-split them (versus passing around a string_list itself), but I think it's a reasonable compromise given the function interface. -Peff
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/builtin/var.c b/builtin/var.c index ce6a2231ca..7cec05a49a 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" @@ -51,6 +52,26 @@ static char *shell_path(int flag) return xstrdup(SHELL_PATH); } +static char *git_attr_val_system(int flag) +{ + if (git_attr_system_is_enabled()) { + char *file = xstrdup(git_attr_system_file()); + normalize_path_copy(file, file); + return file; + } + return NULL; +} + +static char *git_attr_val_global(int flag) +{ + char *file = xstrdup(git_attr_global_file()); + if (file) { + normalize_path_copy(file, file); + return file; + } + return NULL; +} + struct git_var { const char *name; char *(*read)(int); @@ -84,6 +105,14 @@ static struct git_var git_vars[] = { .name = "GIT_SHELL_PATH", .read = shell_path, }, + { + .name = "GIT_ATTR_SYSTEM", + .read = git_attr_val_system, + }, + { + .name = "GIT_ATTR_GLOBAL", + .read = git_attr_val_global, + }, { .name = "", .read = NULL, diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh index e35f07afcb..374d9f49e9 100755 --- a/t/t0007-git-var.sh +++ b/t/t0007-git-var.sh @@ -162,6 +162,26 @@ test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' ' esac ' +test_expect_success 'GIT_ATTR_SYSTEM produces expected output' ' + test_must_fail env GIT_ATTR_NOSYSTEM=1 git var GIT_ATTR_SYSTEM && + ( + sane_unset GIT_ATTR_NOSYSTEM && + systempath=$(git var GIT_ATTR_SYSTEM) && + test "$systempath" != "" + ) +' + +test_expect_success 'GIT_ATTR_GLOBAL points to the correct location' ' + TRASHDIR="$(test-tool path-utils normalize_path_copy "$(pwd)")" && + globalpath=$(XDG_CONFIG_HOME="$TRASHDIR/.config" git var GIT_ATTR_GLOBAL) && + test "$globalpath" = "$TRASHDIR/.config/git/attributes" && + ( + sane_unset XDG_CONFIG_HOME && + globalpath=$(HOME="$TRASHDIR" git var GIT_ATTR_GLOBAL) && + test "$globalpath" = "$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.