| Message ID | a7ff842a3e8d30cad7f18427bc812f542b998efc.1669395151.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve consistency of git-var | expand |
On Fri, Nov 25 2022, Sean Allred via GitGitGadget wrote: > From: Sean Allred <allred.sean@gmail.com> > > Before, git-var could print usage() even if the command was invoked > correctly with a variable defined in git_vars -- provided that its > read() function returned NULL. > > Now, we only print usage() only if it was called with a logical "we only ... only if", drop/combine some "only"? > variable that wasn't defined -- regardless of read(). > > Since we now know the variable is valid when we call read_var(), we > can avoid printing usage() here (and exiting with code 129) and > instead exit quietly with code 1. While exiting with a different code > can be a breaking change, it's far better than changing the exit > status more generally from 'failure' to 'success'. I honestly don't still don't grok what was different here before/after, whatever we are now/should be doing here, a test as part of this change asserting the new behavior would be really useful. > -static const char *read_var(const char *var) > +static const struct git_var *get_git_var(const char *var) > { > struct git_var *ptr; > - const char *val; > - val = NULL; > for (ptr = git_vars; ptr->read; ptr++) { > if (strcmp(var, ptr->name) == 0) { > - val = ptr->read(IDENT_STRICT); > - break; > + return ptr; > } > { > + const struct git_var *git_var = NULL; This assignment to "NULL" can be dropped, i.e.... > const char *val = NULL; > if (argc != 2) > usage(var_usage); > @@ -91,10 +89,15 @@ int cmd_var(int argc, const char **argv, const char *prefix) > return 0; > } > git_config(git_default_config, NULL); > - val = read_var(argv[1]); > - if (!val) > + > + git_var = get_git_var(argv[1]); ...we first assign to it here, and if we use it uninit'd before the compiler will tell us.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I honestly don't still don't grok what was different here before/after, > whatever we are now/should be doing here, a test as part of this change > asserting the new behavior would be really useful. Sadly I don't think there are any logical variables that could be tested for this behavior until the second patch in the series (where quite a few tests are added). I did some brief testing with GIT_COMMITTER_IDENT as the most obvious candidate, but it will still die early if GIT_COMMITTER_NAME is unset, so it's not a good test case. If you've got a test case that'll work before the second patch, I'd be happy to include it here. >> { >> + const struct git_var *git_var = NULL; > > This assignment to "NULL" can be dropped, i.e.... > >> const char *val = NULL; >> if (argc != 2) >> usage(var_usage); >> @@ -91,10 +89,15 @@ int cmd_var(int argc, const char **argv, const char *prefix) >> return 0; >> } >> git_config(git_default_config, NULL); >> - val = read_var(argv[1]); >> - if (!val) >> + >> + git_var = get_git_var(argv[1]); > > ...we first assign to it here, and if we use it uninit'd before the > compiler will tell us. Nice catch! I've removed the premature assignment to both `git_var` and `val`. I've updated my branch with this change; I'll send out a v3 later today. -- Sean Allred
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt index 6aa521fab23..0ab5bfa7d72 100644 --- a/Documentation/git-var.txt +++ b/Documentation/git-var.txt @@ -13,7 +13,8 @@ SYNOPSIS DESCRIPTION ----------- -Prints a Git logical variable. +Prints a Git logical variable. Exits with code 1 if the variable has +no value. OPTIONS ------- diff --git a/builtin/var.c b/builtin/var.c index 491db274292..e215cd3b0c0 100644 --- a/builtin/var.c +++ b/builtin/var.c @@ -56,18 +56,15 @@ static void list_vars(void) printf("%s=%s\n", ptr->name, val); } -static const char *read_var(const char *var) +static const struct git_var *get_git_var(const char *var) { struct git_var *ptr; - const char *val; - val = NULL; for (ptr = git_vars; ptr->read; ptr++) { if (strcmp(var, ptr->name) == 0) { - val = ptr->read(IDENT_STRICT); - break; + return ptr; } } - return val; + return NULL; } static int show_config(const char *var, const char *value, void *cb) @@ -81,6 +78,7 @@ static int show_config(const char *var, const char *value, void *cb) int cmd_var(int argc, const char **argv, const char *prefix) { + const struct git_var *git_var = NULL; const char *val = NULL; if (argc != 2) usage(var_usage); @@ -91,10 +89,15 @@ int cmd_var(int argc, const char **argv, const char *prefix) return 0; } git_config(git_default_config, NULL); - val = read_var(argv[1]); - if (!val) + + git_var = get_git_var(argv[1]); + if (!git_var) usage(var_usage); + val = git_var->read(IDENT_STRICT); + if (!val) + return 1; + printf("%s\n", val); return 0;