Re: [PATCH] grep: die gracefully when outside repository

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2023, 16:42 UTC
Message-ID: <xmqqmswhjj48.fsf@gitster.g>
In-Reply-To: <087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name>

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

Show 14 quoted lines

> diff --git a/pathspec.c b/pathspec.c
> index 3a3a5724c44..e115832f17a 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -468,6 +468,9 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
>  					   &prefixlen, copyfrom);
>  		if (!match) {
>  			const char *hint_path = get_git_work_tree();
> +			if (!have_git_dir())
> +				die(_("'%s' is outside the directory tree"),
> +				    copyfrom);
>  			if (!hint_path)
>  				hint_path = get_git_dir();
>  			die(_("%s: '%s' is outside repository at '%s'"), elt,

It is curious that the original has two sources of hint_path (i.e., get_git_dir() is used as a fallback for get_git_work_tree()). Are we certain that the check is at the right place? If we do not have a repository, then both would fail by returning NULL, so it should not matter if we add the new check before we check either or both, or even after we checked both before dying.

I wonder if

	const char *hint_path = get_git_work_tree();

	if (!hint_path)
	        hint_path = get_git_dir();
	if (hint_path)
		die(_("%s: '%s' is outside repository at '%s'"),
		    elt, copyfrom, absolute_path(hint_path));
	else
		die(_("%s: '%s' is outside the directory tree"),
		    elt, copyfrom);

makes the intent of the code clearer. We want to hint the location of the repository by computing hint_path, and if we can compute it, we use it in the error message, but otherwise we don't add hint. And we apply that conditional whether we have repository or not---what we care about is the NULL-ness of the hint string we computed.

Show 18 quoted lines

> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 39d6d713ecb..b976f81a166 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
>  	)
>  '
>  
> +test_expect_success 'outside of git repository with pathspec outside the directory tree' '
> +	test_when_finished rm -fr non &&
> +	rm -fr non &&
> +	mkdir -p non/git/sub &&
> +	(
> +		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd non/git &&
> +		test_expect_code 128 git grep --no-index search .. 2>error &&
> +		grep "is outside the directory tree" error

Excellent. This is a very good use of the GIT_CEILING_DIRECTORIES facility.

Show 8 quoted lines

> +	)
> +'
> +
>  test_expect_success 'inside git repository but with --no-index' '
>  	rm -fr is &&
>  	mkdir -p is/git/sub &&
>
> base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40