Re: [PATCH 1/2] git: add NO_INTERNAL_HELP flag for builtins

From

Jonathan Nieder <jrnieder@gmail.com>

Date

Oct 17, 2017, 17:39 UTC

Message-ID

<20171017173920.hzsvftikaoi4y6zt@aiede.mtv.corp.google.com>

In-Reply-To

<76c3e710-0655-7788-f6f4-af1e59d65edd@web.de>

Hi,

René Scharfe wrote:

> Builtin commands have skipped repo setup when called with just a single
> option -h and thus shown their short help text even outside of
> repositories since 99caeed05d3 (Let 'git <command> -h' show usage
> without a git dir).
>
> Add the flag NO_INTERNAL_HELP for builtins to opt out of this special
> treatment and provide a list of commands that are exempt from the
> corresponding test in t0012.  That allows builtins to handle -h
> themselves.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  git.c           | 6 +++++-
>  t/t0012-help.sh | 9 ++++++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)

Makes sense.

[...]

> +++ b/git.c

[...]

> @@ -312,7 +313,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>  	const char *prefix;
>  
>  	prefix = NULL;
> -	help = argc == 2 && !strcmp(argv[1], "-h");
> +	if (p->option & NO_INTERNAL_HELP)
> +		help = 0;
> +	else
> +		help = argc == 2 && !strcmp(argv[1], "-h");

optional: this could be part of the same expression:

	help = !(p->option & NO_INTERNAL_HELP) && argc == 2 && ...;

[...]

> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -53,12 +53,19 @@ test_expect_success 'generate builtin list' '
>  	git --list-builtins >builtins
>  '
>  
> +cat >no_internal_help <<EOF
> +EOF
> +
> +test_expect_success 'generate list of builtins with internal help' '
> +	grep -w -v -f no_internal_help <builtins >builtins_internal_help
> +'

	comm -1 -3 no_internal_help builtins >builtins_internal_help

Other nits:

- preparatory 'cat' commands like the above tend to go inside
  test_expect_success these days

- test that set up for later tests get marked as 'setup' or 'set up'

With whatever subset of the changes below looks good squashed in,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

diff --git i/git.c w/git.c
index 9d1b8c4716..e4b340df7d 100644
--- i/git.c
+++ w/git.c
@@ -313,10 +313,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	const char *prefix;
 
 	prefix = NULL;
-	if (p->option & NO_INTERNAL_HELP)
-		help = 0;
-	else
-		help = argc == 2 && !strcmp(argv[1], "-h");
+	help = !(p->option & NO_INTERNAL_HELP) &&
+			argc == 2 && !strcmp(argv[1], "-h");
 	if (!help) {
 		if (p->option & RUN_SETUP)
 			prefix = setup_git_directory();
diff --git i/t/t0012-help.sh w/t/t0012-help.sh
index c5a748837c..73fdfd99ab 100755
--- i/t/t0012-help.sh
+++ w/t/t0012-help.sh
@@ -49,15 +49,11 @@ test_expect_success "--help does not work for guides" "
 	test_i18ncmp expect actual
 "
 
-test_expect_success 'generate builtin list' '
-	git --list-builtins >builtins
-'
-
-cat >no_internal_help <<EOF
-EOF
-
-test_expect_success 'generate list of builtins with internal help' '
-	grep -w -v -f no_internal_help <builtins >builtins_internal_help
+test_expect_success 'set up list of builtins with internal help' '
+	cat >no_internal_help <<-\EOF &&
+	EOF
+	git --list-builtins >builtins &&
+	comm -1 -3 no_internal_help builtins >builtins_internal_help
 '
 
 while read builtin