[PATCH 0/3] Improve consistency of git-var

[PATCH 0/3] Improve consistency of git-var

[Sean Allred via GitGitGadget]

This patch series makes a few distinct improvements to git-var to support
the change to git_editor() prompted here
[https://lore.kernel.org/git/xmqq1qpwwwxg.fsf@gitster.g/] and ultimately
support that patch to introduce GIT_SEQUENCE_EDITOR as a handled logical
variable.

We first have to pull apart the errors of 'the given logical variable is
unknown/meaningless' and 'the given logical variable is known, but its value
is undefined'. For example, if GIT_EDITOR (and its fallbacks) was completely
unset, git var GIT_EDITOR would end up inappropriately printing a usage
message. This is fixed in var.c by returning the git_var struct itself in
the search on git_vars (to see if the variable is known) and then calling
git_var->read() -- allowing us to handle the cases of 'git_var is null' and
'read() returned null' separately.

After this is done, we're able to remove the handling in var.c:editor()
that's been duplicated in editor.c -- allowing editor() to return NULL and
follow the logic prepared above.

Sean Allred (3):
  var: do not print usage() with a correct invocation
  var: remove read_var
  var: allow GIT_EDITOR to return null

 Documentation/git-var.txt |  3 +-
 builtin/var.c             | 26 +++++++--------
 t/t0007-git-var.sh        | 69 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 15 deletions(-)


base-commit: a0789512c5a4ae7da935cd2e419f253cb3cb4ce7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1434%2Fvermiculus%2Fsa%2Fvar-improvements-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1434/vermiculus/sa/var-improvements-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1434
-- 
gitgitgadget

[PATCH 1/3] var: do not print usage() with a correct invocation

[Sean Allred via GitGitGadget]

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
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'.

Signed-off-by: Sean Allred <allred.sean@gmail.com>
---
 Documentation/git-var.txt |  3 ++-
 builtin/var.c             | 19 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

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..776f1778ae1 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -56,6 +56,17 @@ static void list_vars(void)
 			printf("%s=%s\n", ptr->name, val);
 }
 
+static const struct git_var *get_git_var(const char *var)
+{
+	struct git_var *ptr;
+	for (ptr = git_vars; ptr->read; ptr++) {
+		if (strcmp(var, ptr->name) == 0) {
+			return ptr;
+		}
+	}
+	return NULL;
+}
+
 static const char *read_var(const char *var)
 {
 	struct git_var *ptr;
@@ -81,6 +92,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,9 +103,14 @@ int cmd_var(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 	git_config(git_default_config, NULL);
+
+	git_var = get_git_var(argv[1]);
+	if (!git_var)
+		usage(var_usage);
+
 	val = read_var(argv[1]);
 	if (!val)
-		usage(var_usage);
+		return 1;
 
 	printf("%s\n", val);
 
-- 
gitgitgadget

[PATCH 2/3] var: remove read_var

[Sean Allred via GitGitGadget]

From: Sean Allred <allred.sean@gmail.com>

With our target git_var value now available, we no longer need to call
into read_var() to find its read() function again. This does avoid a
second loop through git_vars, but mostly it just removes a lot of
duplicated logic.

Signed-off-by: Sean Allred <allred.sean@gmail.com>
---
 builtin/var.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index 776f1778ae1..e215cd3b0c0 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -67,20 +67,6 @@ static const struct git_var *get_git_var(const char *var)
 	return NULL;
 }
 
-static const char *read_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 val;
-}
-
 static int show_config(const char *var, const char *value, void *cb)
 {
 	if (value)
@@ -108,7 +94,7 @@ int cmd_var(int argc, const char **argv, const char *prefix)
 	if (!git_var)
 		usage(var_usage);
 
-	val = read_var(argv[1]);
+	val = git_var->read(IDENT_STRICT);
 	if (!val)
 		return 1;
 
-- 
gitgitgadget

Re: [PATCH 2/3] var: remove read_var

[Junio C Hamano]

"Sean Allred via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sean Allred <allred.sean@gmail.com>
>
> With our target git_var value now available, we no longer need to call
> into read_var() to find its read() function again. This does avoid a
> second loop through git_vars, but mostly it just removes a lot of
> duplicated logic.

If I were doing this series, I would probably have written a single
patch for the steps 1 & 2 from the beginning.  That way, reviewers
can clearly see what the differences in behaviour between
get_git_var() and read_var() in that patch to see that the single
step is a strict improvement.

Other than that, both patches 1 & 2 look good.

Thanks.


> Signed-off-by: Sean Allred <allred.sean@gmail.com>
> ---
>  builtin/var.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/builtin/var.c b/builtin/var.c
> index 776f1778ae1..e215cd3b0c0 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -67,20 +67,6 @@ static const struct git_var *get_git_var(const char *var)
>  	return NULL;
>  }
>  
> -static const char *read_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 val;
> -}
> -
>  static int show_config(const char *var, const char *value, void *cb)
>  {
>  	if (value)
> @@ -108,7 +94,7 @@ int cmd_var(int argc, const char **argv, const char *prefix)
>  	if (!git_var)
>  		usage(var_usage);
>  
> -	val = read_var(argv[1]);
> +	val = git_var->read(IDENT_STRICT);
>  	if (!val)
>  		return 1;

[PATCH 3/3] var: allow GIT_EDITOR to return null

[Sean Allred via GitGitGadget]

From: Sean Allred <allred.sean@gmail.com>

The handling to die early when there is no EDITOR is valuable when
used in normal code (i.e., editor.c). In git-var, where
null/empty-string is a perfectly valid value to return, it doesn't
make as much sense.

Remove this handling from `git var GIT_EDITOR` so that it does not
fail so noisily when there is no defined editor.

Signed-off-by: Sean Allred <allred.sean@gmail.com>
---
 builtin/var.c      |  7 +----
 t/t0007-git-var.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index e215cd3b0c0..77e9ef3081a 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -11,12 +11,7 @@ static const char var_usage[] = "git var (-l | <variable>)";
 
 static const char *editor(int flag)
 {
-	const char *pgm = git_editor();
-
-	if (!pgm && flag & IDENT_STRICT)
-		die("Terminal is dumb, but EDITOR unset");
-
-	return pgm;
+    return git_editor();
 }
 
 static const char *pager(int flag)
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index e56f4b9ac59..bdef271c92a 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -47,6 +47,75 @@ test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '
 	)
 '
 
+test_expect_success 'get GIT_EDITOR without configuration' '
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		>expect &&
+		! git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with configuration' '
+	test_config core.editor foo &&
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo foo >expect &&
+		git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with environment variable GIT_EDITOR' '
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo bar >expect &&
+		GIT_EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with environment variable EDITOR' '
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo bar >expect &&
+		EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with configuration and environment variable GIT_EDITOR' '
+	test_config core.editor foo &&
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo bar >expect &&
+		GIT_EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '
+	test_config core.editor foo &&
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo foo >expect &&
+		EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
 # 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.
-- 
gitgitgadget

[PATCH v2 0/2] Improve consistency of git-var

[Sean Allred via GitGitGadget]

This patch series makes a few distinct improvements to git-var to support
the change to git_editor() prompted [here][1] and ultimately support that
patch to introduce GIT_SEQUENCE_EDITOR as a handled logical variable.

Changes since v1:

 * Fix a whitespace issue in var.c:editor() (where I have my editor
   configured to use spaces instead of tabs; whoops)
 * Squash this down to two patches as suggested. I typically organize my
   commits to make it clear they don't actively break something, but I can
   certainly see the value in organizing them differently when there is
   already an extremely robust body of automated tests like there is for
   Git.
 * Rebased on current main; no conflicts.

Sean Allred (2):
  var: do not print usage() with a correct invocation
  var: allow GIT_EDITOR to return null

 Documentation/git-var.txt |  3 +-
 builtin/var.c             | 26 +++++++--------
 t/t0007-git-var.sh        | 69 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 15 deletions(-)


base-commit: c000d916380bb59db69c78546928eadd076b9c7d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1434%2Fvermiculus%2Fsa%2Fvar-improvements-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1434/vermiculus/sa/var-improvements-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1434

Range-diff vs v1:

 1:  d5f571f0bb3 ! 1:  a7ff842a3e8 var: do not print usage() with a correct invocation
     @@ builtin/var.c: 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;
     -+	for (ptr = git_vars; ptr->read; ptr++) {
     -+		if (strcmp(var, ptr->name) == 0) {
     -+			return ptr;
     -+		}
     -+	}
     -+	return NULL;
     -+}
     -+
     - static const char *read_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)
      @@ builtin/var.c: static int show_config(const char *var, const char *value, void *cb)
       
       int cmd_var(int argc, const char **argv, const char *prefix)
     @@ builtin/var.c: 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 = read_var(argv[1]);
     - 	if (!val)
     --		usage(var_usage);
     -+		return 1;
     + 		usage(var_usage);
       
     ++	val = git_var->read(IDENT_STRICT);
     ++	if (!val)
     ++		return 1;
     ++
       	printf("%s\n", val);
       
     + 	return 0;
 2:  905b109b458 < -:  ----------- var: remove read_var
 3:  8d49a718038 ! 2:  427cb7b55ac var: allow GIT_EDITOR to return null
     @@ builtin/var.c: static const char var_usage[] = "git var (-l | <variable>)";
      -		die("Terminal is dumb, but EDITOR unset");
      -
      -	return pgm;
     -+    return git_editor();
     ++	return git_editor();
       }
       
       static const char *pager(int flag)

-- 
gitgitgadget

[PATCH v2 1/2] var: do not print usage() with a correct invocation

[Sean Allred via GitGitGadget]

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
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'.

Signed-off-by: Sean Allred <allred.sean@gmail.com>
---
 Documentation/git-var.txt |  3 ++-
 builtin/var.c             | 19 +++++++++++--------
 2 files changed, 13 insertions(+), 9 deletions(-)

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;
-- 
gitgitgadget

Re: [PATCH v2 1/2] var: do not print usage() with a correct invocation

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH v2 1/2] var: do not print usage() with a correct invocation

[Sean Allred]

Æ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

[PATCH v2 2/2] var: allow GIT_EDITOR to return null

[Sean Allred via GitGitGadget]

From: Sean Allred <allred.sean@gmail.com>

The handling to die early when there is no EDITOR is valuable when
used in normal code (i.e., editor.c). In git-var, where
null/empty-string is a perfectly valid value to return, it doesn't
make as much sense.

Remove this handling from `git var GIT_EDITOR` so that it does not
fail so noisily when there is no defined editor.

Signed-off-by: Sean Allred <allred.sean@gmail.com>
---
 builtin/var.c      |  7 +----
 t/t0007-git-var.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index e215cd3b0c0..5678ec68bfe 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -11,12 +11,7 @@ static const char var_usage[] = "git var (-l | <variable>)";
 
 static const char *editor(int flag)
 {
-	const char *pgm = git_editor();
-
-	if (!pgm && flag & IDENT_STRICT)
-		die("Terminal is dumb, but EDITOR unset");
-
-	return pgm;
+	return git_editor();
 }
 
 static const char *pager(int flag)
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index e56f4b9ac59..bdef271c92a 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -47,6 +47,75 @@ test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '
 	)
 '
 
+test_expect_success 'get GIT_EDITOR without configuration' '
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		>expect &&
+		! git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with configuration' '
+	test_config core.editor foo &&
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo foo >expect &&
+		git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with environment variable GIT_EDITOR' '
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo bar >expect &&
+		GIT_EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with environment variable EDITOR' '
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo bar >expect &&
+		EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with configuration and environment variable GIT_EDITOR' '
+	test_config core.editor foo &&
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo bar >expect &&
+		GIT_EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '
+	test_config core.editor foo &&
+	(
+		sane_unset GIT_EDITOR &&
+		sane_unset VISUAL &&
+		sane_unset EDITOR &&
+		echo foo >expect &&
+		EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
 # 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.
-- 
gitgitgadget

Re: [PATCH v2 2/2] var: allow GIT_EDITOR to return null

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 25 2022, Sean Allred via GitGitGadget wrote:

> From: Sean Allred <allred.sean@gmail.com>

> +test_expect_success 'get GIT_EDITOR without configuration' '
> +	(
> +		sane_unset GIT_EDITOR &&
> +		sane_unset VISUAL &&
> +		sane_unset EDITOR &&
> +		>expect &&
> +		! git var GIT_EDITOR >actual &&

Negate git with "test_must_fail", not "!", this would e.g. hide
segfaults. See t/README's discussion about it.

> +		test_cmp expect actual

Looks like this should be:

	test_must_fail git ... >out &&
	test_must_be_empty out

> +test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '
> +	test_config core.editor foo &&
> +	(
> +		sane_unset GIT_EDITOR &&
> +		sane_unset VISUAL &&
> +		sane_unset EDITOR &&
> +		echo foo >expect &&
> +		EDITOR=bar git var GIT_EDITOR >actual &&
> +		test_cmp expect actual
> +	)

Perhaps these can all be factored into a helper to hide this repetition
in a function, but maybe not. E.g:

	test_git_var () {
		cat >expect &&
		(
			[...common part of subshell ...]
		        "$@" >actual &&
			test_cmp expect actual
		)
	}

(untested)

Re: [PATCH v2 2/2] var: allow GIT_EDITOR to return null

[Sean Allred]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Negate git with "test_must_fail", not "!", this would e.g. hide
> segfaults. See t/README's discussion about it.
>
>> +		test_cmp expect actual
>
> Looks like this should be:
>
> 	test_must_fail git ... >out &&
> 	test_must_be_empty out

Nice! I don't know why I didn't look for t/README, but I also found
test_expect_code, which seems to be even more specific as to what is
being expected. I assume it has the same segfault detection.

This has now been incorporated in my branch; I'll submit it in v3 later
today.

>> +test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '
>> +	test_config core.editor foo &&
>> +	(
>> +		sane_unset GIT_EDITOR &&
>> +		sane_unset VISUAL &&
>> +		sane_unset EDITOR &&
>> +		echo foo >expect &&
>> +		EDITOR=bar git var GIT_EDITOR >actual &&
>> +		test_cmp expect actual
>> +	)
>
> Perhaps these can all be factored into a helper to hide this repetition
> in a function, but maybe not. E.g:
>
> 	test_git_var () {
> 		cat >expect &&
> 		(
> 			[...common part of subshell ...]
> 		        "$@" >actual &&
> 			test_cmp expect actual
> 		)
> 	}
>
> (untested)

In all honesty, I think too much abstraction would do more harm than
good here. I definitely share the instinct to factor out the common
pieces, but in other codebases I've worked in, that tends to stifle
future changes in the tests themselves.

That said, I can't realistically imagine a world where a
'sane_unset_all_editors' would stifle code changes -- and I think that
accounts for the lion's share of the repetition. I've incorporated such
a helper in my branch now.

If you're not convinced there should be further abstraction, I'd rather
leave things 'stupid simple' -- but if you think this would block merge,
I'd be happy to take a crack at further factoring out what I can.


--
Sean Allred

[PATCH v3 0/2] Improve consistency of git-var

[Sean Allred via GitGitGadget]

This patch series makes a few distinct improvements to git-var to support
the change to git_editor() prompted [here][1] and ultimately support that
patch to introduce GIT_SEQUENCE_EDITOR as a handled logical variable.

Changes since v2:

 * Nix premature assignment of git_var and val, preferring to let the
   compiler tell us when they're being used before init.
 * Factor out sane_unset_all_editors for tests to reduce duplication
 * Use more specific test_* helper functions

Sean Allred (2):
  var: do not print usage() with a correct invocation
  var: allow GIT_EDITOR to return null

 Documentation/git-var.txt |  3 +-
 builtin/var.c             | 29 +++++++++---------
 t/t0007-git-var.sh        | 62 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 16 deletions(-)


base-commit: c000d916380bb59db69c78546928eadd076b9c7d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1434%2Fvermiculus%2Fsa%2Fvar-improvements-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1434/vermiculus/sa/var-improvements-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1434

Range-diff vs v2:

 1:  a7ff842a3e8 ! 1:  889fdf877a1 var: do not print usage() with a correct invocation
     @@ builtin/var.c: static int show_config(const char *var, const char *value, void *
       
       int cmd_var(int argc, const char **argv, const char *prefix)
       {
     -+	const struct git_var *git_var = NULL;
     - 	const char *val = NULL;
     +-	const char *val = NULL;
     ++	const struct git_var *git_var;
     ++	const char *val;
     ++
       	if (argc != 2)
       		usage(var_usage);
     + 
      @@ builtin/var.c: int cmd_var(int argc, const char **argv, const char *prefix)
       		return 0;
       	}
 2:  427cb7b55ac ! 2:  3d8bf3662fe var: allow GIT_EDITOR to return null
     @@ builtin/var.c: static const char var_usage[] = "git var (-l | <variable>)";
       static const char *pager(int flag)
      
       ## t/t0007-git-var.sh ##
     +@@ t/t0007-git-var.sh: test_description='basic sanity checks for git var'
     + TEST_PASSES_SANITIZE_LEAK=true
     + . ./test-lib.sh
     + 
     ++sane_unset_all_editors () {
     ++	sane_unset GIT_EDITOR &&
     ++	sane_unset VISUAL &&
     ++	sane_unset EDITOR
     ++}
     ++
     + test_expect_success 'get GIT_AUTHOR_IDENT' '
     + 	test_tick &&
     + 	echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
      @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '
       	)
       '
       
      +test_expect_success 'get GIT_EDITOR without configuration' '
      +	(
     -+		sane_unset GIT_EDITOR &&
     -+		sane_unset VISUAL &&
     -+		sane_unset EDITOR &&
     -+		>expect &&
     -+		! git var GIT_EDITOR >actual &&
     -+		test_cmp expect actual
     ++		sane_unset_all_editors &&
     ++		test_expect_code 1 git var GIT_EDITOR >out &&
     ++		test_must_be_empty out
      +	)
      +'
      +
      +test_expect_success 'get GIT_EDITOR with configuration' '
      +	test_config core.editor foo &&
      +	(
     -+		sane_unset GIT_EDITOR &&
     -+		sane_unset VISUAL &&
     -+		sane_unset EDITOR &&
     ++		sane_unset_all_editors &&
      +		echo foo >expect &&
      +		git var GIT_EDITOR >actual &&
      +		test_cmp expect actual
     @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configurati
      +
      +test_expect_success 'get GIT_EDITOR with environment variable GIT_EDITOR' '
      +	(
     -+		sane_unset GIT_EDITOR &&
     -+		sane_unset VISUAL &&
     -+		sane_unset EDITOR &&
     ++		sane_unset_all_editors &&
      +		echo bar >expect &&
      +		GIT_EDITOR=bar git var GIT_EDITOR >actual &&
      +		test_cmp expect actual
     @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configurati
      +
      +test_expect_success 'get GIT_EDITOR with environment variable EDITOR' '
      +	(
     -+		sane_unset GIT_EDITOR &&
     -+		sane_unset VISUAL &&
     -+		sane_unset EDITOR &&
     ++		sane_unset_all_editors &&
      +		echo bar >expect &&
      +		EDITOR=bar git var GIT_EDITOR >actual &&
      +		test_cmp expect actual
     @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configurati
      +test_expect_success 'get GIT_EDITOR with configuration and environment variable GIT_EDITOR' '
      +	test_config core.editor foo &&
      +	(
     -+		sane_unset GIT_EDITOR &&
     -+		sane_unset VISUAL &&
     -+		sane_unset EDITOR &&
     ++		sane_unset_all_editors &&
      +		echo bar >expect &&
      +		GIT_EDITOR=bar git var GIT_EDITOR >actual &&
      +		test_cmp expect actual
     @@ t/t0007-git-var.sh: test_expect_success 'get GIT_DEFAULT_BRANCH with configurati
      +test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '
      +	test_config core.editor foo &&
      +	(
     -+		sane_unset GIT_EDITOR &&
     -+		sane_unset VISUAL &&
     -+		sane_unset EDITOR &&
     ++		sane_unset_all_editors &&
      +		echo foo >expect &&
      +		EDITOR=bar git var GIT_EDITOR >actual &&
      +		test_cmp expect actual

-- 
gitgitgadget

[PATCH v3 1/2] var: do not print usage() with a correct invocation

[Sean Allred via GitGitGadget]

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
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'.

Signed-off-by: Sean Allred <allred.sean@gmail.com>
---
 Documentation/git-var.txt |  3 ++-
 builtin/var.c             | 22 +++++++++++++---------
 2 files changed, 15 insertions(+), 10 deletions(-)

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..5cbe32ec890 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,7 +78,9 @@ static int show_config(const char *var, const char *value, void *cb)
 
 int cmd_var(int argc, const char **argv, const char *prefix)
 {
-	const char *val = NULL;
+	const struct git_var *git_var;
+	const char *val;
+
 	if (argc != 2)
 		usage(var_usage);
 
@@ -91,10 +90,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;
-- 
gitgitgadget

[PATCH v3 2/2] var: allow GIT_EDITOR to return null

[Sean Allred via GitGitGadget]

From: Sean Allred <allred.sean@gmail.com>

The handling to die early when there is no EDITOR is valuable when
used in normal code (i.e., editor.c). In git-var, where
null/empty-string is a perfectly valid value to return, it doesn't
make as much sense.

Remove this handling from `git var GIT_EDITOR` so that it does not
fail so noisily when there is no defined editor.

Signed-off-by: Sean Allred <allred.sean@gmail.com>
---
 builtin/var.c      |  7 +-----
 t/t0007-git-var.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index 5cbe32ec890..a1a2522126f 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -11,12 +11,7 @@ static const char var_usage[] = "git var (-l | <variable>)";
 
 static const char *editor(int flag)
 {
-	const char *pgm = git_editor();
-
-	if (!pgm && flag & IDENT_STRICT)
-		die("Terminal is dumb, but EDITOR unset");
-
-	return pgm;
+	return git_editor();
 }
 
 static const char *pager(int flag)
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index e56f4b9ac59..433d242897c 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -5,6 +5,12 @@ test_description='basic sanity checks for git var'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+sane_unset_all_editors () {
+	sane_unset GIT_EDITOR &&
+	sane_unset VISUAL &&
+	sane_unset EDITOR
+}
+
 test_expect_success 'get GIT_AUTHOR_IDENT' '
 	test_tick &&
 	echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
@@ -47,6 +53,62 @@ test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '
 	)
 '
 
+test_expect_success 'get GIT_EDITOR without configuration' '
+	(
+		sane_unset_all_editors &&
+		test_expect_code 1 git var GIT_EDITOR >out &&
+		test_must_be_empty out
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with configuration' '
+	test_config core.editor foo &&
+	(
+		sane_unset_all_editors &&
+		echo foo >expect &&
+		git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with environment variable GIT_EDITOR' '
+	(
+		sane_unset_all_editors &&
+		echo bar >expect &&
+		GIT_EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with environment variable EDITOR' '
+	(
+		sane_unset_all_editors &&
+		echo bar >expect &&
+		EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with configuration and environment variable GIT_EDITOR' '
+	test_config core.editor foo &&
+	(
+		sane_unset_all_editors &&
+		echo bar >expect &&
+		GIT_EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_EDITOR with configuration and environment variable EDITOR' '
+	test_config core.editor foo &&
+	(
+		sane_unset_all_editors &&
+		echo foo >expect &&
+		EDITOR=bar git var GIT_EDITOR >actual &&
+		test_cmp expect actual
+	)
+'
+
 # 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.
-- 
gitgitgadget