Re: [PATCH 2/2] Revert "rev-parse: remove restrictions on some options"

[John Keeping <john@keeping.me.uk>]

On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:
> This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.
[snip]
> The original patch was not spurred by an actual bug report,
> but by an observation[1] that was essentially "eh, this
> looks unnecessarily restrictive". It _is_ restrictive, but
> it turns out to be necessarily so. :)

The aim of the original series was to improve the documentation, so I
don't think it's unreasonable to consider this a regression and revert
the functional change.  Although I think we can improve the behaviour
slightly (see below).

> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index b6c6326..0f2bb9b 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -28,7 +28,8 @@ OPTIONS
>  Operation Modes
>  ~~~~~~~~~~~~~~~
>  
> -Each of these options must appear first on the command line.
> +Each of these options must appear first on the command line; they do not
> +need to be run in a git repository.
>  
>  --parseopt::
>  	Use 'git rev-parse' in option parsing mode (see PARSEOPT section below).
> @@ -38,6 +39,18 @@ Each of these options must appear first on the command line.
>  	section below). In contrast to the `--sq` option below, this
>  	mode does only quoting. Nothing else is done to command input.
>  
> +--local-env-vars::
> +	List the GIT_* environment variables that are local to the
> +	repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
> +	Only the names of the variables are listed, not their value,
> +	even if they are set.

I think we should add:

	No other arguments may be supplied.

> +--resolve-git-dir <path>::
> +	Check if <path> is a valid repository or a gitfile that
> +	points at a valid repository, and print the location of the
> +	repository.  If <path> is a gitfile then the resolved path
> +	to the real repository is printed.

Again I think this should say that only the `path` argument may be
supplied.

>  --git-path <path>::
>  	Resolve "$GIT_DIR/<path>" and takes other path relocation
>  	variables such as $GIT_OBJECT_DIRECTORY,
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index cf8487b..ccc0328 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -518,6 +518,21 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  	if (argc > 1 && !strcmp("--sq-quote", argv[1]))
>  		return cmd_sq_quote(argc - 2, argv + 2);
>  
> +	if (argc == 2 && !strcmp("--local-env-vars", argv[1])) {

Maybe:

	if (argc > 1 && !strcmp("--local-env-vars", argv[1])) {
		if (argc != 2)
			die("--local-env-vars must be the only argument");

since the behaviour of:

	$ git rev-parse --local-env-vars --
	--local-env-vars
	--

is quite surprising.

> +		int i;
> +		for (i = 0; local_repo_env[i]; i++)
> +			printf("%s\n", local_repo_env[i]);
> +		return 0;
> +	}
> +
> +	if (argc > 2 && !strcmp(argv[1], "--resolve-git-dir")) {

This is less bad, but again it might be nice to provide a better error
if the path argument isn't supplied.

> +		const char *gitdir = resolve_gitdir(argv[2]);
> +		if (!gitdir)
> +			die("not a gitdir '%s'", argv[2]);
> +		puts(gitdir);
> +		return 0;
> +	}
> +
>  	if (argc > 1 && !strcmp("-h", argv[1]))
>  		usage(builtin_rev_parse_usage);
>  
> @@ -706,12 +721,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  				add_ref_exclusion(&ref_excludes, arg + 10);
>  				continue;
>  			}
> -			if (!strcmp(arg, "--local-env-vars")) {

What about leaving this in and replacing the body of the if statement
with:

	die("--local-env-vars must be the first argument");

?  I expect this will significantly reduce debugging time if anyone is
relying on the current behaviour.

> -				int i;
> -				for (i = 0; local_repo_env[i]; i++)
> -					printf("%s\n", local_repo_env[i]);
> -				continue;
> -			}
>  			if (!strcmp(arg, "--show-toplevel")) {
>  				const char *work_tree = get_git_work_tree();
>  				if (work_tree)
> @@ -767,16 +776,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  				puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
>  				continue;
>  			}
> -			if (!strcmp(arg, "--resolve-git-dir")) {
> -				const char *gitdir = argv[++i];
> -				if (!gitdir)
> -					die("--resolve-git-dir requires an argument");
> -				gitdir = resolve_gitdir(gitdir);
> -				if (!gitdir)
> -					die("not a gitdir '%s'", argv[i]);
> -				puts(gitdir);
> -				continue;
> -			}
>  			if (!strcmp(arg, "--is-inside-git-dir")) {
>  				printf("%s\n", is_inside_git_dir() ? "true"
>  						: "false");
> diff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh
> index ae33093..3ec2971 100755
> --- a/t/t1515-rev-parse-outside-repo.sh
> +++ b/t/t1515-rev-parse-outside-repo.sh
> @@ -27,14 +27,14 @@ test_expect_success 'rev-parse --sq-quote' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'rev-parse --local-env-vars' '
> +test_expect_success 'rev-parse --local-env-vars' '
>  	git rev-parse --local-env-vars >actual &&
>  	# we do not want to depend on the complete list here,
>  	# so just look for something plausible
>  	grep ^GIT_DIR actual
>  '
>  
> -test_expect_failure 'rev-parse --resolve-git-dir' '
> +test_expect_success 'rev-parse --resolve-git-dir' '
>  	git init --separate-git-dir repo dir &&
>  	test_must_fail git rev-parse --resolve-git-dir . &&
>  	echo "$(pwd)/repo" >expect &&
> -- 
> 2.7.2.767.g705917e
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html