Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Nov 17, 2014, 16:19 UTC
Message-ID: <546A1FFC.60605@alum.mit.edu>
In-Reply-To: <xmqqbno5zvdm.fsf@gitster.dls.corp.google.com>

On 11/17/2014 04:42 PM, Junio C Hamano wrote:

Show 11 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This seems like a one-off bug caused by a specific instance of odd code.
>> It could only recur if somebody were to remove the line that I added,
>> which would be a *very* odd mistake to make given that its purpose is
>> pretty obvious.
> 
> Or some other code that comes _after_ your new code touches the
> file.
> 
> You cannot anticipate what mistake others make.

And yet we do so all the time, adding tests for the things we consider most likely to break in the future and omitting them for breakages that seem unlikely. Otherwise, what frees us from the obligation to add a '! test -x "$GIT_DIR/config"' following every single git operation?

But it's OK with me, we can add a test.

Show 24 quoted lines

> Shouldn't something like this be sufficient?
> 
>  t/t0001-init.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index e62c0ff..acdc1df 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -12,6 +12,13 @@ check_config () {
>  		echo "expected a directory $1, a file $1/config and $1/refs"
>  		return 1
>  	fi
> +
> +	if ! test_have_prereq POSIXPERM || test -x "$1/config"
> +	then
> +		echo "$1/config is executable?"
> +		return 1
> +	fi
> +
>  	bare=$(cd "$1" && git config --bool core.bare)
>  	worktree=$(cd "$1" && git config core.worktree) ||
>  	worktree=unset
>

I think the logic should be

	if test_have_prereq POSIXPERM && test -x "$1/config"

, right? If the system doesn't have POSIXPERM, then aren't all bets off regarding file permissions?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu