Re: [PATCH v3] Add support for GIT_CEILING_DIRS

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2008, 19:46 UTC
Message-ID: <7vej83gxjm.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <482C85C8.90804@facebook.com>

David Reiss <dreiss@facebook.com> writes:

> + * ....  Paths must
> + * be in a canonical form: empty components, or "." or ".." components
> + * are not allowed.  prefix_list may be null, which is like "".

The caller starts from cwd[] and chomps, so you can safely assume that it would not feed anything problematic. But prefix_list comes from user's environment, and it is easy to make mistakes like doubled slashes (which you seem to take care) and also is tempting to use ".." when specifying the ceiling (e.g. "CEIL=$HOME/.."). Perhaps canonicalizing the ceiling would make this easier to use for end users?

Show 5 quoted lines

> +#if 0
> +static void test_longest_ancestor_length()
> +{
> ...
> +	assert(longest_ancestor_length("/foo/bar", "/:/bar/"      ) ==  0);

No test for nonsense/invalid input, like "::/foo" for prefix_list?

Show 29 quoted lines

> diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
> new file mode 100755
> index 0000000..091baad
> --- /dev/null
> +++ b/t/t1504-ceiling-dirs.sh
> @@ -0,0 +1,156 @@
> +#!/bin/sh
> +
> +test_description='test GIT_CEILING_DIRS'
> +. ./test-lib.sh
> +
> +test_prefix() {
> +	test_expect_success "$1" \
> +	"test '$2' = \"\$(git rev-parse --show-prefix)\""
> +}
> +
> +test_fail() {
> +	test_expect_code 128 "$1: prefix" \
> +	"git rev-parse --show-prefix"
> +}
> +
> +TRASH_ROOT="$(pwd)"
> +ROOT_PARENT=$(dirname "$TRASH_ROOT")
> +
> +
> +unset GIT_CEILING_DIRS
> +test_prefix no_ceil ""
> +
> +export GIT_CEILING_DIRS=""

Portability.  Instead write this as two separate statements, please.

        VAR=val
        export VAR

Show 22 quoted lines

> +test_prefix ceil_empty ""
> +
> +export GIT_CEILING_DIRS="$ROOT_PARENT"
> +test_prefix ceil_at_parent ""
> +
> +export GIT_CEILING_DIRS="$ROOT_PARENT/"
> +test_prefix ceil_at_parent_slash ""
> +
> +export GIT_CEILING_DIRS="$TRASH_ROOT"
> +test_prefix ceil_at_trash ""
> +
> ...
> +
> +export GIT_CEILING_DIRS="$TRASH_ROOT/subdi"
> +test_prefix subdir_ceil_at_subdi_slash "sub/dir/"
> +
> +
> +export GIT_CEILING_DIRS="foo:$TRASH_ROOT/sub"
> +test_fail second_of_two
> +
> +export GIT_CEILING_DIRS="$TRASH_ROOT/sub:bar"
> +test_fail first_of_two

You may also check stuff like "sub//dir" and "::sub/dir/".

How well would this colon separated list work with msys folks?