Re: [PATCH v3] Add support for GIT_CEILING_DIRS

[Junio C Hamano <gitster@pobox.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?


> +#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?

> 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

> +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?