Improve consistency of git-var

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/[email protected]/] 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

From: Sean Allred <[email protected]>

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

---
 Documentation/git-var.txt |  3 ++-
 builtin/var.c             | 19 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

index 6aa521fab23..0ab5bfa7d72 100644
@@ -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
 -------

index 491db274292..776f1778ae1 100644
@@ -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

From: Sean Allred <[email protected]>

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.

---
 builtin/var.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

index 776f1778ae1..e215cd3b0c0 100644
@@ -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

From: Sean Allred <[email protected]>

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.

---
 builtin/var.c      |  7 +----
 t/t0007-git-var.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 6 deletions(-)

index e215cd3b0c0..77e9ef3081a 100644
@@ -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)

gitgitgadget

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.

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

From: Sean Allred <[email protected]>

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

---
 Documentation/git-var.txt |  3 ++-
 builtin/var.c             | 19 +++++++++++--------
 2 files changed, 13 insertions(+), 9 deletions(-)

index 6aa521fab23..0ab5bfa7d72 100644
@@ -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
 -------

index 491db274292..e215cd3b0c0 100644
@@ -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

From: Sean Allred <[email protected]>

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.

---
 builtin/var.c      |  7 +----
 t/t0007-git-var.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 6 deletions(-)

index e215cd3b0c0..5678ec68bfe 100644
@@ -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)

gitgitgadget

"we only ... only if", drop/combine some "only"?

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.

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

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.

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

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

From: Sean Allred <[email protected]>

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

---
 Documentation/git-var.txt |  3 ++-
 builtin/var.c             | 22 +++++++++++++---------
 2 files changed, 15 insertions(+), 10 deletions(-)

index 6aa521fab23..0ab5bfa7d72 100644
@@ -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
 -------

index 491db274292..5cbe32ec890 100644
@@ -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

From: Sean Allred <[email protected]>

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.

---
 builtin/var.c      |  7 +-----
 t/t0007-git-var.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 6 deletions(-)

index 5cbe32ec890..a1a2522126f 100644
@@ -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)

gitgitgadget