Re: [PATCH v2] receive-pack: optionally deny case clone refs

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Jun 4, 2014, 06:06 UTC
Message-ID: <538EB748.3050300@viscovery.net>
In-Reply-To: <1401851607-8255-1-git-send-email-dturner@twitter.com>

Am 6/4/2014 5:13, schrieb David Turner:

Show 43 quoted lines

> It is possible to have two branches which are the same but for case.
> This works great on the case-sensitive filesystems, but not so well on
> case-insensitive filesystems.  It is fairly typical to have
> case-insensitive clients (Macs, say) with a case-sensitive server
> (GNU/Linux).
> 
> Should a user attempt to pull on a Mac when there are case clone
> branches with differing contents, they'll get an error message
> containing something like "Ref refs/remotes/origin/lower is at
> [sha-of-lowercase-branch] but expected [sha-of-uppercase-branch]....
> (unable to update local ref)"
> 
> With a case-insensitive git server, if a branch called capital-M
> Master (that differs from lowercase-m-master) is pushed, nobody else
> can push to (lowercase-m) master until the branch is removed.
> 
> Create the option receive.denycaseclonebranches, which checks pushed
> branches to ensure that they are not case clones of an existing
> branch.  This setting is turned on by default if core.ignorecase is
> set, but not otherwise.
> 
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  Documentation/config.txt           |  6 ++++++
>  Documentation/git-push.txt         |  5 +++--
>  Documentation/glossary-content.txt |  5 +++++
>  builtin/receive-pack.c             | 27 +++++++++++++++++++++++-
>  t/t5400-send-pack.sh               | 43 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 83 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1932e9b..4deddf8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2053,6 +2053,12 @@ receive.unpackLimit::
>  	especially on slow filesystems.  If not set, the value of
>  	`transfer.unpackLimit` is used instead.
>  
> +receive.denyCaseCloneBranches::
> +	If set to true, git-receive-pack will deny a ref update that creates
> +	a ref which is the same but for case as an existing ref.  This is
> +	useful when clients are on a case-insensitive filesystem, which
> +	will cause errors when given refs which differ only in case.

Shouldn't this better be named 'receive.denyCaseCloneRefs'?

How about 'denyCaseInsensitiveRefs', 'denyIgnoreCaseRefs'?

Is this entry really so important that it must be the first in the list of receive.deny* list, which is not alphabetically sorted?

> +
>  receive.denyDeletes::
>  	If set to true, git-receive-pack will deny a ref update that deletes
>  	the ref. Use this to prevent such a ref deletion via a push.

Show 10 quoted lines

> --- a/t/t5400-send-pack.sh
> +++ b/t/t5400-send-pack.sh
> @@ -129,6 +129,49 @@ test_expect_success 'denyNonFastforwards trumps --force' '
>  	test "$victim_orig" = "$victim_head"
>  '
>  
> +test_expect_success 'denyCaseCloneBranches works' '
> +	(
> +	    cd victim &&
> +	    git config receive.denyCaseCloneBranches true

Broken && chain.

Show 11 quoted lines

> +	    git config receive.denyDeletes false
> +	) &&
> +	git send-pack ./victim HEAD:refs/heads/caseclone &&
> +	orig_ver=$(git rev-parse HEAD) &&
> +	test_must_fail git send-pack ./victim HEAD^:refs/heads/CaseClone &&
> +	#confirm that this had no effect upstream
> +	(
> +	    cd victim &&
> +	    test_must_fail git rev-parse CaseClone &&
> +	    remote_ver=$(git rev-parse caseclone) &&
> +	    test $orig_ver = $remote_ver

Please use double-quotes around the variable expansions: There could be a failure mode where remote_ver (and even orig_ver) are empty, which would lead to a syntax error or a wrong result.

BTW, on a case-insensitive file system, is there not a chance that 'git rev-parse CaseClone' succeeds even though the ref is stored in victim/.git/refs/heads/caseclone? Perhaps you should inspect the output of 'git for-each-ref' for the expected result? (Mental note: At least a case-preserving file system is required to run the test.)

> +	) &&
> +	git send-pack ./victim HEAD^:refs/heads/notacaseclone &&
> +	test_must_fail git send-pack ./victim :CaseClone &&
> +	#confirm that this had no effect upstream

Please insert a blank after the hash mark.

Show 12 quoted lines

> +	(
> +	    cd victim &&
> +	    test_must_fail git rev-parse CaseClone &&
> +	    remote_ver=$(git rev-parse caseclone) &&
> +	    test $orig_ver = $remote_ver
> +	) &&
> +	git send-pack ./victim :caseclone &&
> +	#confirm that this took effect upstream
> +	(
> +	    cd victim &&
> +	    test_must_fail git rev-parse caseclone
> +	)

Broken && chain.

> +	#check that we can recreate a branch after deleting a
> +	#case-clone of it
> +	case_clone_ver=$(git rev-parse HEAD^)

Broken && chain.

Show 13 quoted lines

> +	git send-pack ./victim HEAD^:CaseClone &&
> +	(
> +	    cd victim &&
> +	    test_must_fail git rev-parse caseclone &&
> +	    remote_ver=$(git rev-parse CaseClone) &&
> +	    test $case_clone_ver = $remote_ver
> +	)
> +'
> +
>  test_expect_success 'push --all excludes remote-tracking hierarchy' '
>  	mkdir parent &&
>  	(
>

-- Hannes