{"thread":{"id":"58853","subject":"[PATCH 0/3] Improve consistency of git-var","startedAt":"2022-11-24T20:22:56Z","lastAt":"2022-11-26T14:18:19Z","messageCount":15,"participants":["Sean Allred via GitGitGadget","Junio C Hamano","Ævar Arnfjörð Bjarmason","Sean Allred"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"467951","messageId":"pull.1434.git.1669321369.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":null,"subject":"[PATCH 0/3] Improve consistency of git-var","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-24T20:22:45Z","receivedAt":"2022-11-24T20:22:56Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"This patch series makes a few distinct improvements to git-var to support\nthe change to git_editor() prompted here\n[https://lore.kernel.org/git/xmqq1qpwwwxg.fsf@gitster.g/] and ultimately\nsupport that patch to introduce GIT_SEQUENCE_EDITOR as a handled logical\nvariable.\n\nWe first have to pull apart the errors of 'the given logical variable is\nunknown/meaningless' and 'the given logical variable is known, but its value\nis undefined'. For example, if GIT_EDITOR (and its fallbacks) was completely\nunset, git var GIT_EDITOR would end up inappropriately printing a usage\nmessage. This is fixed in var.c by returning the git_var struct itself in\nthe search on git_vars (to see if the variable is known) and then calling\ngit_var->read() -- allowing us to handle the cases of 'git_var is null' and\n'read() returned null' separately.\n\nAfter this is done, we're able to remove the handling in var.c:editor()\nthat's been duplicated in editor.c -- allowing editor() to return NULL and\nfollow the logic prepared above.\n\nSean Allred (3):\n var: do not print usage() with a correct invocation\n var: remove read_var\n var: allow GIT_EDITOR to return null\n\n Documentation/git-var.txt | 3 +-\n builtin/var.c | 26 +++++++--------\n t/t0007-git-var.sh | 69 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 83 insertions(+), 15 deletions(-)\n\n\nbase-commit: a0789512c5a4ae7da935cd2e419f253cb3cb4ce7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1434%2Fvermiculus%2Fsa%2Fvar-improvements-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1434/vermiculus/sa/var-improvements-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1434\n-- \ngitgitgadget\n"},{"id":"467952","messageId":"d5f571f0bb352c0ec9cd8d1162c14cc26ccfa52c.1669321369.git.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":"pull.1434.git.1669321369.gitgitgadget@gmail.com","subject":"[PATCH 1/3] var: do not print usage() with a correct invocation","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-24T20:22:46Z","receivedAt":"2022-11-24T20:22:57Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"From: Sean Allred \n\nBefore, git-var could print usage() even if the command was invoked\ncorrectly with a variable defined in git_vars -- provided that its\nread() function returned NULL.\n\nNow, we only print usage() only if it was called with a logical\nvariable that wasn't defined -- regardless of read().\n\nSince we now know the variable is valid when we call read_var(), we\ncan avoid printing usage() here (and exiting with code 129) and\ninstead exit quietly with code 1. While exiting with a different code\ncan be a breaking change, it's far better than changing the exit\nstatus more generally from 'failure' to 'success'.\n\nSigned-off-by: Sean Allred \n---\n Documentation/git-var.txt | 3 ++-\n builtin/var.c | 19 ++++++++++++++++++-\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 6aa521fab23..0ab5bfa7d72 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -13,7 +13,8 @@ SYNOPSIS\n \n DESCRIPTION\n -----------\n-Prints a Git logical variable.\n+Prints a Git logical variable. Exits with code 1 if the variable has\n+no value.\n \n OPTIONS\n -------\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 491db274292..776f1778ae1 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -56,6 +56,17 @@ static void list_vars(void)\n \t\t\tprintf(\"%s=%s\\n\", ptr->name, val);\n }\n \n+static const struct git_var *get_git_var(const char *var)\n+{\n+\tstruct git_var *ptr;\n+\tfor (ptr = git_vars; ptr->read; ptr++) {\n+\t\tif (strcmp(var, ptr->name) == 0) {\n+\t\t\treturn ptr;\n+\t\t}\n+\t}\n+\treturn NULL;\n+}\n+\n static const char *read_var(const char *var)\n {\n \tstruct git_var *ptr;\n@@ -81,6 +92,7 @@ static int show_config(const char *var, const char *value, void *cb)\n \n int cmd_var(int argc, const char **argv, const char *prefix)\n {\n+\tconst struct git_var *git_var = NULL;\n \tconst char *val = NULL;\n \tif (argc != 2)\n \t\tusage(var_usage);\n@@ -91,9 +103,14 @@ int cmd_var(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \tgit_config(git_default_config, NULL);\n+\n+\tgit_var = get_git_var(argv[1]);\n+\tif (!git_var)\n+\t\tusage(var_usage);\n+\n \tval = read_var(argv[1]);\n \tif (!val)\n-\t\tusage(var_usage);\n+\t\treturn 1;\n \n \tprintf(\"%s\\n\", val);\n \n-- \ngitgitgadget\n\n"},{"id":"467953","messageId":"905b109b458e291da04d9879cbc6b032bbd9a302.1669321369.git.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":"pull.1434.git.1669321369.gitgitgadget@gmail.com","subject":"[PATCH 2/3] var: remove read_var","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-24T20:22:47Z","receivedAt":"2022-11-24T20:23:00Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"From: Sean Allred \n\nWith our target git_var value now available, we no longer need to call\ninto read_var() to find its read() function again. This does avoid a\nsecond loop through git_vars, but mostly it just removes a lot of\nduplicated logic.\n\nSigned-off-by: Sean Allred \n---\n builtin/var.c | 16 +---------------\n 1 file changed, 1 insertion(+), 15 deletions(-)\n\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 776f1778ae1..e215cd3b0c0 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -67,20 +67,6 @@ static const struct git_var *get_git_var(const char *var)\n \treturn NULL;\n }\n \n-static const char *read_var(const char *var)\n-{\n-\tstruct git_var *ptr;\n-\tconst char *val;\n-\tval = NULL;\n-\tfor (ptr = git_vars; ptr->read; ptr++) {\n-\t\tif (strcmp(var, ptr->name) == 0) {\n-\t\t\tval = ptr->read(IDENT_STRICT);\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\treturn val;\n-}\n-\n static int show_config(const char *var, const char *value, void *cb)\n {\n \tif (value)\n@@ -108,7 +94,7 @@ int cmd_var(int argc, const char **argv, const char *prefix)\n \tif (!git_var)\n \t\tusage(var_usage);\n \n-\tval = read_var(argv[1]);\n+\tval = git_var->read(IDENT_STRICT);\n \tif (!val)\n \t\treturn 1;\n \n-- \ngitgitgadget\n\n"},{"id":"467954","messageId":"8d49a718038c1e7504f512b0d04709b9c2d28df7.1669321369.git.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":"pull.1434.git.1669321369.gitgitgadget@gmail.com","subject":"[PATCH 3/3] var: allow GIT_EDITOR to return null","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-24T20:22:48Z","receivedAt":"2022-11-24T20:23:01Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"From: Sean Allred \n\nThe handling to die early when there is no EDITOR is valuable when\nused in normal code (i.e., editor.c). In git-var, where\nnull/empty-string is a perfectly valid value to return, it doesn't\nmake as much sense.\n\nRemove this handling from `git var GIT_EDITOR` so that it does not\nfail so noisily when there is no defined editor.\n\nSigned-off-by: Sean Allred \n---\n builtin/var.c | 7 +----\n t/t0007-git-var.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/var.c b/builtin/var.c\nindex e215cd3b0c0..77e9ef3081a 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -11,12 +11,7 @@ static const char var_usage[] = \"git var (-l | )\";\n \n static const char *editor(int flag)\n {\n-\tconst char *pgm = git_editor();\n-\n-\tif (!pgm && flag & IDENT_STRICT)\n-\t\tdie(\"Terminal is dumb, but EDITOR unset\");\n-\n-\treturn pgm;\n+ return git_editor();\n }\n \n static const char *pager(int flag)\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nindex e56f4b9ac59..bdef271c92a 100755\n--- a/t/t0007-git-var.sh\n+++ b/t/t0007-git-var.sh\n@@ -47,6 +47,75 @@ test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '\n \t)\n '\n \n+test_expect_success 'get GIT_EDITOR without configuration' '\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\t>expect &&\n+\t\t! git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with configuration' '\n+\ttest_config core.editor foo &&\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo foo >expect &&\n+\t\tgit var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with environment variable GIT_EDITOR' '\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo bar >expect &&\n+\t\tGIT_EDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with environment variable EDITOR' '\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo bar >expect &&\n+\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with configuration and environment variable GIT_EDITOR' '\n+\ttest_config core.editor foo &&\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo bar >expect &&\n+\t\tGIT_EDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '\n+\ttest_config core.editor foo &&\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo foo >expect &&\n+\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n # For git var -l, we check only a representative variable;\n # testing the whole output would make our test too brittle with\n # respect to unrelated changes in the test suite's environment.\n-- \ngitgitgadget\n"},{"id":"467970","messageId":"xmqqo7sveg8o.fsf@gitster.g","threadId":"58853","inReplyTo":"905b109b458e291da04d9879cbc6b032bbd9a302.1669321369.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] var: remove read_var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-25T05:48:55Z","receivedAt":"2022-11-25T05:49:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sean Allred via GitGitGadget\" writes:\n\n> From: Sean Allred \n>\n> With our target git_var value now available, we no longer need to call\n> into read_var() to find its read() function again. This does avoid a\n> second loop through git_vars, but mostly it just removes a lot of\n> duplicated logic.\n\nIf I were doing this series, I would probably have written a single\npatch for the steps 1 & 2 from the beginning. That way, reviewers\ncan clearly see what the differences in behaviour between\nget_git_var() and read_var() in that patch to see that the single\nstep is a strict improvement.\n\nOther than that, both patches 1 & 2 look good.\n\nThanks.\n\n\n> Signed-off-by: Sean Allred \n> ---\n> builtin/var.c | 16 +---------------\n> 1 file changed, 1 insertion(+), 15 deletions(-)\n>\n> diff --git a/builtin/var.c b/builtin/var.c\n> index 776f1778ae1..e215cd3b0c0 100644\n> --- a/builtin/var.c\n> +++ b/builtin/var.c\n> @@ -67,20 +67,6 @@ static const struct git_var *get_git_var(const char *var)\n> \treturn NULL;\n> }\n> \n> -static const char *read_var(const char *var)\n> -{\n> -\tstruct git_var *ptr;\n> -\tconst char *val;\n> -\tval = NULL;\n> -\tfor (ptr = git_vars; ptr->read; ptr++) {\n> -\t\tif (strcmp(var, ptr->name) == 0) {\n> -\t\t\tval = ptr->read(IDENT_STRICT);\n> -\t\t\tbreak;\n> -\t\t}\n> -\t}\n> -\treturn val;\n> -}\n> -\n> static int show_config(const char *var, const char *value, void *cb)\n> {\n> \tif (value)\n> @@ -108,7 +94,7 @@ int cmd_var(int argc, const char **argv, const char *prefix)\n> \tif (!git_var)\n> \t\tusage(var_usage);\n> \n> -\tval = read_var(argv[1]);\n> +\tval = git_var->read(IDENT_STRICT);\n> \tif (!val)\n> \t\treturn 1;\n"},{"id":"468001","messageId":"pull.1434.v2.git.1669395151.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":"pull.1434.git.1669321369.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Improve consistency of git-var","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-25T16:52:29Z","receivedAt":"2022-11-25T16:52:40Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"This patch series makes a few distinct improvements to git-var to support\nthe change to git_editor() prompted [here][1] and ultimately support that\npatch to introduce GIT_SEQUENCE_EDITOR as a handled logical variable.\n\nChanges since v1:\n\n * Fix a whitespace issue in var.c:editor() (where I have my editor\n configured to use spaces instead of tabs; whoops)\n * Squash this down to two patches as suggested. I typically organize my\n commits to make it clear they don't actively break something, but I can\n certainly see the value in organizing them differently when there is\n already an extremely robust body of automated tests like there is for\n Git.\n * Rebased on current main; no conflicts.\n\nSean Allred (2):\n var: do not print usage() with a correct invocation\n var: allow GIT_EDITOR to return null\n\n Documentation/git-var.txt | 3 +-\n builtin/var.c | 26 +++++++--------\n t/t0007-git-var.sh | 69 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 83 insertions(+), 15 deletions(-)\n\n\nbase-commit: c000d916380bb59db69c78546928eadd076b9c7d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1434%2Fvermiculus%2Fsa%2Fvar-improvements-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1434/vermiculus/sa/var-improvements-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1434\n\nRange-diff vs v1:\n\n 1: d5f571f0bb3 ! 1: a7ff842a3e8 var: do not print usage() with a correct invocation\n @@ builtin/var.c: static void list_vars(void)\n \t\t\tprintf(\"%s=%s\\n\", ptr->name, val);\n }\n \n +-static const char *read_var(const char *var)\n +static const struct git_var *get_git_var(const char *var)\n -+{\n -+\tstruct git_var *ptr;\n -+\tfor (ptr = git_vars; ptr->read; ptr++) {\n -+\t\tif (strcmp(var, ptr->name) == 0) {\n -+\t\t\treturn ptr;\n -+\t\t}\n -+\t}\n -+\treturn NULL;\n -+}\n -+\n - static const char *read_var(const char *var)\n {\n \tstruct git_var *ptr;\n +-\tconst char *val;\n +-\tval = NULL;\n + \tfor (ptr = git_vars; ptr->read; ptr++) {\n + \t\tif (strcmp(var, ptr->name) == 0) {\n +-\t\t\tval = ptr->read(IDENT_STRICT);\n +-\t\t\tbreak;\n ++\t\t\treturn ptr;\n + \t\t}\n + \t}\n +-\treturn val;\n ++\treturn NULL;\n + }\n + \n + static int show_config(const char *var, const char *value, void *cb)\n @@ builtin/var.c: static int show_config(const char *var, const char *value, void *cb)\n \n int cmd_var(int argc, const char **argv, const char *prefix)\n @@ builtin/var.c: int cmd_var(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \tgit_config(git_default_config, NULL);\n +-\tval = read_var(argv[1]);\n +-\tif (!val)\n +\n +\tgit_var = get_git_var(argv[1]);\n +\tif (!git_var)\n -+\t\tusage(var_usage);\n -+\n - \tval = read_var(argv[1]);\n - \tif (!val)\n --\t\tusage(var_usage);\n -+\t\treturn 1;\n + \t\tusage(var_usage);\n \n ++\tval = git_var->read(IDENT_STRICT);\n ++\tif (!val)\n ++\t\treturn 1;\n ++\n \tprintf(\"%s\\n\", val);\n \n + \treturn 0;\n 2: 905b109b458 < -: ----------- var: remove read_var\n 3: 8d49a718038 ! 2: 427cb7b55ac var: allow GIT_EDITOR to return null\n @@ builtin/var.c: static const char var_usage[] = \"git var (-l | )\";\n -\t\tdie(\"Terminal is dumb, but EDITOR unset\");\n -\n -\treturn pgm;\n -+ return git_editor();\n ++\treturn git_editor();\n }\n \n static const char *pager(int flag)\n\n-- \ngitgitgadget\n"},{"id":"468002","messageId":"a7ff842a3e8d30cad7f18427bc812f542b998efc.1669395151.git.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":"pull.1434.v2.git.1669395151.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] var: do not print usage() with a correct invocation","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-25T16:52:30Z","receivedAt":"2022-11-25T16:52:43Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"From: Sean Allred \n\nBefore, git-var could print usage() even if the command was invoked\ncorrectly with a variable defined in git_vars -- provided that its\nread() function returned NULL.\n\nNow, we only print usage() only if it was called with a logical\nvariable that wasn't defined -- regardless of read().\n\nSince we now know the variable is valid when we call read_var(), we\ncan avoid printing usage() here (and exiting with code 129) and\ninstead exit quietly with code 1. While exiting with a different code\ncan be a breaking change, it's far better than changing the exit\nstatus more generally from 'failure' to 'success'.\n\nSigned-off-by: Sean Allred \n---\n Documentation/git-var.txt | 3 ++-\n builtin/var.c | 19 +++++++++++--------\n 2 files changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 6aa521fab23..0ab5bfa7d72 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -13,7 +13,8 @@ SYNOPSIS\n \n DESCRIPTION\n -----------\n-Prints a Git logical variable.\n+Prints a Git logical variable. Exits with code 1 if the variable has\n+no value.\n \n OPTIONS\n -------\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 491db274292..e215cd3b0c0 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -56,18 +56,15 @@ static void list_vars(void)\n \t\t\tprintf(\"%s=%s\\n\", ptr->name, val);\n }\n \n-static const char *read_var(const char *var)\n+static const struct git_var *get_git_var(const char *var)\n {\n \tstruct git_var *ptr;\n-\tconst char *val;\n-\tval = NULL;\n \tfor (ptr = git_vars; ptr->read; ptr++) {\n \t\tif (strcmp(var, ptr->name) == 0) {\n-\t\t\tval = ptr->read(IDENT_STRICT);\n-\t\t\tbreak;\n+\t\t\treturn ptr;\n \t\t}\n \t}\n-\treturn val;\n+\treturn NULL;\n }\n \n static int show_config(const char *var, const char *value, void *cb)\n@@ -81,6 +78,7 @@ static int show_config(const char *var, const char *value, void *cb)\n \n int cmd_var(int argc, const char **argv, const char *prefix)\n {\n+\tconst struct git_var *git_var = NULL;\n \tconst char *val = NULL;\n \tif (argc != 2)\n \t\tusage(var_usage);\n@@ -91,10 +89,15 @@ int cmd_var(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \tgit_config(git_default_config, NULL);\n-\tval = read_var(argv[1]);\n-\tif (!val)\n+\n+\tgit_var = get_git_var(argv[1]);\n+\tif (!git_var)\n \t\tusage(var_usage);\n \n+\tval = git_var->read(IDENT_STRICT);\n+\tif (!val)\n+\t\treturn 1;\n+\n \tprintf(\"%s\\n\", val);\n \n \treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"468003","messageId":"427cb7b55ac3fead1651cbad7318b9c0bb454b08.1669395151.git.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":"pull.1434.v2.git.1669395151.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] var: allow GIT_EDITOR to return null","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-25T16:52:31Z","receivedAt":"2022-11-25T16:52:45Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"From: Sean Allred \n\nThe handling to die early when there is no EDITOR is valuable when\nused in normal code (i.e., editor.c). In git-var, where\nnull/empty-string is a perfectly valid value to return, it doesn't\nmake as much sense.\n\nRemove this handling from `git var GIT_EDITOR` so that it does not\nfail so noisily when there is no defined editor.\n\nSigned-off-by: Sean Allred \n---\n builtin/var.c | 7 +----\n t/t0007-git-var.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/var.c b/builtin/var.c\nindex e215cd3b0c0..5678ec68bfe 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -11,12 +11,7 @@ static const char var_usage[] = \"git var (-l | )\";\n \n static const char *editor(int flag)\n {\n-\tconst char *pgm = git_editor();\n-\n-\tif (!pgm && flag & IDENT_STRICT)\n-\t\tdie(\"Terminal is dumb, but EDITOR unset\");\n-\n-\treturn pgm;\n+\treturn git_editor();\n }\n \n static const char *pager(int flag)\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nindex e56f4b9ac59..bdef271c92a 100755\n--- a/t/t0007-git-var.sh\n+++ b/t/t0007-git-var.sh\n@@ -47,6 +47,75 @@ test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '\n \t)\n '\n \n+test_expect_success 'get GIT_EDITOR without configuration' '\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\t>expect &&\n+\t\t! git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with configuration' '\n+\ttest_config core.editor foo &&\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo foo >expect &&\n+\t\tgit var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with environment variable GIT_EDITOR' '\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo bar >expect &&\n+\t\tGIT_EDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with environment variable EDITOR' '\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo bar >expect &&\n+\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with configuration and environment variable GIT_EDITOR' '\n+\ttest_config core.editor foo &&\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo bar >expect &&\n+\t\tGIT_EDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '\n+\ttest_config core.editor foo &&\n+\t(\n+\t\tsane_unset GIT_EDITOR &&\n+\t\tsane_unset VISUAL &&\n+\t\tsane_unset EDITOR &&\n+\t\techo foo >expect &&\n+\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n # For git var -l, we check only a representative variable;\n # testing the whole output would make our test too brittle with\n # respect to unrelated changes in the test suite's environment.\n-- \ngitgitgadget\n"},{"id":"468007","messageId":"221125.86tu2mmz1e.gmgdl@evledraar.gmail.com","threadId":"58853","inReplyTo":"a7ff842a3e8d30cad7f18427bc812f542b998efc.1669395151.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] var: do not print usage() with a correct invocation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-25T22:45:04Z","receivedAt":"2022-11-25T22:48:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 25 2022, Sean Allred via GitGitGadget wrote:\n\n> From: Sean Allred \n>\n> Before, git-var could print usage() even if the command was invoked\n> correctly with a variable defined in git_vars -- provided that its\n> read() function returned NULL.\n>\n> Now, we only print usage() only if it was called with a logical\n\n\"we only ... only if\", drop/combine some \"only\"?\n\n> variable that wasn't defined -- regardless of read().\n>\n> Since we now know the variable is valid when we call read_var(), we\n> can avoid printing usage() here (and exiting with code 129) and\n> instead exit quietly with code 1. While exiting with a different code\n> can be a breaking change, it's far better than changing the exit\n> status more generally from 'failure' to 'success'.\n\nI honestly don't still don't grok what was different here before/after,\nwhatever we are now/should be doing here, a test as part of this change\nasserting the new behavior would be really useful.\n\n> -static const char *read_var(const char *var)\n> +static const struct git_var *get_git_var(const char *var)\n> {\n> \tstruct git_var *ptr;\n> -\tconst char *val;\n> -\tval = NULL;\n> \tfor (ptr = git_vars; ptr->read; ptr++) {\n> \t\tif (strcmp(var, ptr->name) == 0) {\n> -\t\t\tval = ptr->read(IDENT_STRICT);\n> -\t\t\tbreak;\n> +\t\t\treturn ptr;\n> \t\t}\n\n> {\n> +\tconst struct git_var *git_var = NULL;\n\nThis assignment to \"NULL\" can be dropped, i.e....\n\n> \tconst char *val = NULL;\n> \tif (argc != 2)\n> \t\tusage(var_usage);\n> @@ -91,10 +89,15 @@ int cmd_var(int argc, const char **argv, const char *prefix)\n> \t\treturn 0;\n> \t}\n> \tgit_config(git_default_config, NULL);\n> -\tval = read_var(argv[1]);\n> -\tif (!val)\n> +\n> +\tgit_var = get_git_var(argv[1]);\n\n...we first assign to it here, and if we use it uninit'd before the\ncompiler will tell us.\n"},{"id":"468008","messageId":"221125.86pmdamyv5.gmgdl@evledraar.gmail.com","threadId":"58853","inReplyTo":"427cb7b55ac3fead1651cbad7318b9c0bb454b08.1669395151.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] var: allow GIT_EDITOR to return null","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-25T22:48:08Z","receivedAt":"2022-11-25T22:51:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 25 2022, Sean Allred via GitGitGadget wrote:\n\n> From: Sean Allred \n\n> +test_expect_success 'get GIT_EDITOR without configuration' '\n> +\t(\n> +\t\tsane_unset GIT_EDITOR &&\n> +\t\tsane_unset VISUAL &&\n> +\t\tsane_unset EDITOR &&\n> +\t\t>expect &&\n> +\t\t! git var GIT_EDITOR >actual &&\n\nNegate git with \"test_must_fail\", not \"!\", this would e.g. hide\nsegfaults. See t/README's discussion about it.\n\n> +\t\ttest_cmp expect actual\n\nLooks like this should be:\n\n\ttest_must_fail git ... >out &&\n\ttest_must_be_empty out\n\n> +test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '\n> +\ttest_config core.editor foo &&\n> +\t(\n> +\t\tsane_unset GIT_EDITOR &&\n> +\t\tsane_unset VISUAL &&\n> +\t\tsane_unset EDITOR &&\n> +\t\techo foo >expect &&\n> +\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n\nPerhaps these can all be factored into a helper to hide this repetition\nin a function, but maybe not. E.g:\n\n\ttest_git_var () {\n\t\tcat >expect &&\n\t\t(\n\t\t\t[...common part of subshell ...]\n\t\t \"$@\" >actual &&\n\t\t\ttest_cmp expect actual\n\t\t)\n\t}\n\n(untested)\n"},{"id":"468011","messageId":"87k03hsv3n.fsf@gmail.com","threadId":"58853","inReplyTo":"221125.86tu2mmz1e.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/2] var: do not print usage() with a correct invocation","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2022-11-26T13:19:35Z","receivedAt":"2022-11-26T13:28:49Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nÆvar Arnfjörð Bjarmason writes:\n> I honestly don't still don't grok what was different here before/after,\n> whatever we are now/should be doing here, a test as part of this change\n> asserting the new behavior would be really useful.\n\nSadly I don't think there are any logical variables that could be tested\nfor this behavior until the second patch in the series (where quite a\nfew tests are added). I did some brief testing with GIT_COMMITTER_IDENT\nas the most obvious candidate, but it will still die early if\nGIT_COMMITTER_NAME is unset, so it's not a good test case.\n\nIf you've got a test case that'll work before the second patch, I'd be\nhappy to include it here.\n\n>> {\n>> +\tconst struct git_var *git_var = NULL;\n>\n> This assignment to \"NULL\" can be dropped, i.e....\n>\n>> \tconst char *val = NULL;\n>> \tif (argc != 2)\n>> \t\tusage(var_usage);\n>> @@ -91,10 +89,15 @@ int cmd_var(int argc, const char **argv, const char *prefix)\n>> \t\treturn 0;\n>> \t}\n>> \tgit_config(git_default_config, NULL);\n>> -\tval = read_var(argv[1]);\n>> -\tif (!val)\n>> +\n>> +\tgit_var = get_git_var(argv[1]);\n>\n> ...we first assign to it here, and if we use it uninit'd before the\n> compiler will tell us.\n\nNice catch! I've removed the premature assignment to both `git_var` and\n`val`. I've updated my branch with this change; I'll send out a v3 later\ntoday.\n\n--\nSean Allred\n"},{"id":"468012","messageId":"87fse5ssyo.fsf@gmail.com","threadId":"58853","inReplyTo":"221125.86pmdamyv5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] var: allow GIT_EDITOR to return null","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2022-11-26T13:54:22Z","receivedAt":"2022-11-26T14:15:00Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nÆvar Arnfjörð Bjarmason writes:\n> Negate git with \"test_must_fail\", not \"!\", this would e.g. hide\n> segfaults. See t/README's discussion about it.\n>\n>> +\t\ttest_cmp expect actual\n>\n> Looks like this should be:\n>\n> \ttest_must_fail git ... >out &&\n> \ttest_must_be_empty out\n\nNice! I don't know why I didn't look for t/README, but I also found\ntest_expect_code, which seems to be even more specific as to what is\nbeing expected. I assume it has the same segfault detection.\n\nThis has now been incorporated in my branch; I'll submit it in v3 later\ntoday.\n\n>> +test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '\n>> +\ttest_config core.editor foo &&\n>> +\t(\n>> +\t\tsane_unset GIT_EDITOR &&\n>> +\t\tsane_unset VISUAL &&\n>> +\t\tsane_unset EDITOR &&\n>> +\t\techo foo >expect &&\n>> +\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n>> +\t\ttest_cmp expect actual\n>> +\t)\n>\n> Perhaps these can all be factored into a helper to hide this repetition\n> in a function, but maybe not. E.g:\n>\n> \ttest_git_var () {\n> \t\tcat >expect &&\n> \t\t(\n> \t\t\t[...common part of subshell ...]\n> \t\t \"$@\" >actual &&\n> \t\t\ttest_cmp expect actual\n> \t\t)\n> \t}\n>\n> (untested)\n\nIn all honesty, I think too much abstraction would do more harm than\ngood here. I definitely share the instinct to factor out the common\npieces, but in other codebases I've worked in, that tends to stifle\nfuture changes in the tests themselves.\n\nThat said, I can't realistically imagine a world where a\n'sane_unset_all_editors' would stifle code changes -- and I think that\naccounts for the lion's share of the repetition. I've incorporated such\na helper in my branch now.\n\nIf you're not convinced there should be further abstraction, I'd rather\nleave things 'stupid simple' -- but if you think this would block merge,\nI'd be happy to take a crack at further factoring out what I can.\n\n\n--\nSean Allred\n"},{"id":"468013","messageId":"pull.1434.v3.git.1669472277.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":"pull.1434.v2.git.1669395151.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] Improve consistency of git-var","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-26T14:17:55Z","receivedAt":"2022-11-26T14:18:13Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"This patch series makes a few distinct improvements to git-var to support\nthe change to git_editor() prompted [here][1] and ultimately support that\npatch to introduce GIT_SEQUENCE_EDITOR as a handled logical variable.\n\nChanges since v2:\n\n * Nix premature assignment of git_var and val, preferring to let the\n compiler tell us when they're being used before init.\n * Factor out sane_unset_all_editors for tests to reduce duplication\n * Use more specific test_* helper functions\n\nSean Allred (2):\n var: do not print usage() with a correct invocation\n var: allow GIT_EDITOR to return null\n\n Documentation/git-var.txt | 3 +-\n builtin/var.c | 29 +++++++++---------\n t/t0007-git-var.sh | 62 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 78 insertions(+), 16 deletions(-)\n\n\nbase-commit: c000d916380bb59db69c78546928eadd076b9c7d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1434%2Fvermiculus%2Fsa%2Fvar-improvements-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1434/vermiculus/sa/var-improvements-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1434\n\nRange-diff vs v2:\n\n 1: a7ff842a3e8 ! 1: 889fdf877a1 var: do not print usage() with a correct invocation\n @@ builtin/var.c: static int show_config(const char *var, const char *value, void *\n \n int cmd_var(int argc, const char **argv, const char *prefix)\n {\n -+\tconst struct git_var *git_var = NULL;\n - \tconst char *val = NULL;\n +-\tconst char *val = NULL;\n ++\tconst struct git_var *git_var;\n ++\tconst char *val;\n ++\n \tif (argc != 2)\n \t\tusage(var_usage);\n + \n @@ builtin/var.c: int cmd_var(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n 2: 427cb7b55ac ! 2: 3d8bf3662fe var: allow GIT_EDITOR to return null\n @@ builtin/var.c: static const char var_usage[] = \"git var (-l | )\";\n static const char *pager(int flag)\n \n ## t/t0007-git-var.sh ##\n +@@ t/t0007-git-var.sh: test_description='basic sanity checks for git var'\n + TEST_PASSES_SANITIZE_LEAK=true\n + . ./test-lib.sh\n + \n ++sane_unset_all_editors () {\n ++\tsane_unset GIT_EDITOR &&\n ++\tsane_unset VISUAL &&\n ++\tsane_unset EDITOR\n ++}\n ++\n + test_expect_success 'get GIT_AUTHOR_IDENT' '\n + \ttest_tick &&\n + \techo \"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\" >expect &&\n @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '\n \t)\n '\n \n +test_expect_success 'get GIT_EDITOR without configuration' '\n +\t(\n -+\t\tsane_unset GIT_EDITOR &&\n -+\t\tsane_unset VISUAL &&\n -+\t\tsane_unset EDITOR &&\n -+\t\t>expect &&\n -+\t\t! git var GIT_EDITOR >actual &&\n -+\t\ttest_cmp expect actual\n ++\t\tsane_unset_all_editors &&\n ++\t\ttest_expect_code 1 git var GIT_EDITOR >out &&\n ++\t\ttest_must_be_empty out\n +\t)\n +'\n +\n +test_expect_success 'get GIT_EDITOR with configuration' '\n +\ttest_config core.editor foo &&\n +\t(\n -+\t\tsane_unset GIT_EDITOR &&\n -+\t\tsane_unset VISUAL &&\n -+\t\tsane_unset EDITOR &&\n ++\t\tsane_unset_all_editors &&\n +\t\techo foo >expect &&\n +\t\tgit var GIT_EDITOR >actual &&\n +\t\ttest_cmp expect actual\n @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configurati\n +\n +test_expect_success 'get GIT_EDITOR with environment variable GIT_EDITOR' '\n +\t(\n -+\t\tsane_unset GIT_EDITOR &&\n -+\t\tsane_unset VISUAL &&\n -+\t\tsane_unset EDITOR &&\n ++\t\tsane_unset_all_editors &&\n +\t\techo bar >expect &&\n +\t\tGIT_EDITOR=bar git var GIT_EDITOR >actual &&\n +\t\ttest_cmp expect actual\n @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configurati\n +\n +test_expect_success 'get GIT_EDITOR with environment variable EDITOR' '\n +\t(\n -+\t\tsane_unset GIT_EDITOR &&\n -+\t\tsane_unset VISUAL &&\n -+\t\tsane_unset EDITOR &&\n ++\t\tsane_unset_all_editors &&\n +\t\techo bar >expect &&\n +\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n +\t\ttest_cmp expect actual\n @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configurati\n +test_expect_success 'get GIT_EDITOR with configuration and environment variable GIT_EDITOR' '\n +\ttest_config core.editor foo &&\n +\t(\n -+\t\tsane_unset GIT_EDITOR &&\n -+\t\tsane_unset VISUAL &&\n -+\t\tsane_unset EDITOR &&\n ++\t\tsane_unset_all_editors &&\n +\t\techo bar >expect &&\n +\t\tGIT_EDITOR=bar git var GIT_EDITOR >actual &&\n +\t\ttest_cmp expect actual\n @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configurati\n +test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '\n +\ttest_config core.editor foo &&\n +\t(\n -+\t\tsane_unset GIT_EDITOR &&\n -+\t\tsane_unset VISUAL &&\n -+\t\tsane_unset EDITOR &&\n ++\t\tsane_unset_all_editors &&\n +\t\techo foo >expect &&\n +\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n +\t\ttest_cmp expect actual\n\n-- \ngitgitgadget\n"},{"id":"468014","messageId":"889fdf877a13067ece785c9c694ed17dcde19b32.1669472277.git.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":"pull.1434.v3.git.1669472277.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] var: do not print usage() with a correct invocation","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-26T14:17:56Z","receivedAt":"2022-11-26T14:18:15Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"From: Sean Allred \n\nBefore, git-var could print usage() even if the command was invoked\ncorrectly with a variable defined in git_vars -- provided that its\nread() function returned NULL.\n\nNow, we only print usage() only if it was called with a logical\nvariable that wasn't defined -- regardless of read().\n\nSince we now know the variable is valid when we call read_var(), we\ncan avoid printing usage() here (and exiting with code 129) and\ninstead exit quietly with code 1. While exiting with a different code\ncan be a breaking change, it's far better than changing the exit\nstatus more generally from 'failure' to 'success'.\n\nSigned-off-by: Sean Allred \n---\n Documentation/git-var.txt | 3 ++-\n builtin/var.c | 22 +++++++++++++---------\n 2 files changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-var.txt b/Documentation/git-var.txt\nindex 6aa521fab23..0ab5bfa7d72 100644\n--- a/Documentation/git-var.txt\n+++ b/Documentation/git-var.txt\n@@ -13,7 +13,8 @@ SYNOPSIS\n \n DESCRIPTION\n -----------\n-Prints a Git logical variable.\n+Prints a Git logical variable. Exits with code 1 if the variable has\n+no value.\n \n OPTIONS\n -------\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 491db274292..5cbe32ec890 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -56,18 +56,15 @@ static void list_vars(void)\n \t\t\tprintf(\"%s=%s\\n\", ptr->name, val);\n }\n \n-static const char *read_var(const char *var)\n+static const struct git_var *get_git_var(const char *var)\n {\n \tstruct git_var *ptr;\n-\tconst char *val;\n-\tval = NULL;\n \tfor (ptr = git_vars; ptr->read; ptr++) {\n \t\tif (strcmp(var, ptr->name) == 0) {\n-\t\t\tval = ptr->read(IDENT_STRICT);\n-\t\t\tbreak;\n+\t\t\treturn ptr;\n \t\t}\n \t}\n-\treturn val;\n+\treturn NULL;\n }\n \n static int show_config(const char *var, const char *value, void *cb)\n@@ -81,7 +78,9 @@ static int show_config(const char *var, const char *value, void *cb)\n \n int cmd_var(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *val = NULL;\n+\tconst struct git_var *git_var;\n+\tconst char *val;\n+\n \tif (argc != 2)\n \t\tusage(var_usage);\n \n@@ -91,10 +90,15 @@ int cmd_var(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \tgit_config(git_default_config, NULL);\n-\tval = read_var(argv[1]);\n-\tif (!val)\n+\n+\tgit_var = get_git_var(argv[1]);\n+\tif (!git_var)\n \t\tusage(var_usage);\n \n+\tval = git_var->read(IDENT_STRICT);\n+\tif (!val)\n+\t\treturn 1;\n+\n \tprintf(\"%s\\n\", val);\n \n \treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"468015","messageId":"3d8bf3662fe92e61805a1d9ffbccf7a17b3d1e8c.1669472277.git.gitgitgadget@gmail.com","threadId":"58853","inReplyTo":"pull.1434.v3.git.1669472277.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] var: allow GIT_EDITOR to return null","fromName":"Sean Allred via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-26T14:17:57Z","receivedAt":"2022-11-26T14:18:19Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"From: Sean Allred \n\nThe handling to die early when there is no EDITOR is valuable when\nused in normal code (i.e., editor.c). In git-var, where\nnull/empty-string is a perfectly valid value to return, it doesn't\nmake as much sense.\n\nRemove this handling from `git var GIT_EDITOR` so that it does not\nfail so noisily when there is no defined editor.\n\nSigned-off-by: Sean Allred \n---\n builtin/var.c | 7 +-----\n t/t0007-git-var.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 5cbe32ec890..a1a2522126f 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -11,12 +11,7 @@ static const char var_usage[] = \"git var (-l | )\";\n \n static const char *editor(int flag)\n {\n-\tconst char *pgm = git_editor();\n-\n-\tif (!pgm && flag & IDENT_STRICT)\n-\t\tdie(\"Terminal is dumb, but EDITOR unset\");\n-\n-\treturn pgm;\n+\treturn git_editor();\n }\n \n static const char *pager(int flag)\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nindex e56f4b9ac59..433d242897c 100755\n--- a/t/t0007-git-var.sh\n+++ b/t/t0007-git-var.sh\n@@ -5,6 +5,12 @@ test_description='basic sanity checks for git var'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+sane_unset_all_editors () {\n+\tsane_unset GIT_EDITOR &&\n+\tsane_unset VISUAL &&\n+\tsane_unset EDITOR\n+}\n+\n test_expect_success 'get GIT_AUTHOR_IDENT' '\n \ttest_tick &&\n \techo \"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\" >expect &&\n@@ -47,6 +53,62 @@ test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '\n \t)\n '\n \n+test_expect_success 'get GIT_EDITOR without configuration' '\n+\t(\n+\t\tsane_unset_all_editors &&\n+\t\ttest_expect_code 1 git var GIT_EDITOR >out &&\n+\t\ttest_must_be_empty out\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with configuration' '\n+\ttest_config core.editor foo &&\n+\t(\n+\t\tsane_unset_all_editors &&\n+\t\techo foo >expect &&\n+\t\tgit var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with environment variable GIT_EDITOR' '\n+\t(\n+\t\tsane_unset_all_editors &&\n+\t\techo bar >expect &&\n+\t\tGIT_EDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with environment variable EDITOR' '\n+\t(\n+\t\tsane_unset_all_editors &&\n+\t\techo bar >expect &&\n+\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with configuration and environment variable GIT_EDITOR' '\n+\ttest_config core.editor foo &&\n+\t(\n+\t\tsane_unset_all_editors &&\n+\t\techo bar >expect &&\n+\t\tGIT_EDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '\n+\ttest_config core.editor foo &&\n+\t(\n+\t\tsane_unset_all_editors &&\n+\t\techo foo >expect &&\n+\t\tEDITOR=bar git var GIT_EDITOR >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n # For git var -l, we check only a representative variable;\n # testing the whole output would make our test too brittle with\n # respect to unrelated changes in the test suite's environment.\n-- \ngitgitgadget\n"}]}