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

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

[David Turner]

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.
+
 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.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 21cd455..0786fb3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -323,8 +323,9 @@ remote rejected::
 	of the following safety options in effect:
 	`receive.denyCurrentBranch` (for pushes to the checked out
 	branch), `receive.denyNonFastForwards` (for forced
-	non-fast-forward updates), `receive.denyDeletes` or
-	`receive.denyDeleteCurrent`.  See linkgit:git-config[1].
+	non-fast-forward updates), `receive.denyDeletes`,
+	`receive.denyCaseCloneBranches` or `receive.denyDeleteCurrent`.
+	See linkgit:git-config[1].
 
 remote failure::
 	The remote end did not report the successful update of the ref,
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index be0858c..ed5ac23 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -31,6 +31,11 @@
 [[def_cache]]cache::
 	Obsolete for: <<def_index,index>>.
 
+[[def_case_clone]]case clone::
+	Two entities (e.g. filenames or refs) that differ only in case.
+	These can cause problems on case-insensitive filesystems, and
+	Git has machinery to prevent these problems in various cases.
+
 [[def_chain]]chain::
 	A list of objects, where each <<def_object,object>> in the list contains
 	a reference to its successor (for example, the successor of a
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..4df99c5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -27,6 +27,7 @@ enum deny_action {
 
 static int deny_deletes;
 static int deny_non_fast_forwards;
+static int deny_case_clone_branches = DENY_UNCONFIGURED;
 static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
 static enum deny_action deny_delete_current = DENY_UNCONFIGURED;
 static int receive_fsck_objects = -1;
@@ -69,6 +70,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 	if (status)
 		return status;
 
+	if (strcmp(var, "receive.denycaseclonebranches") == 0) {
+		deny_case_clone_branches = parse_deny_action(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.denydeletes") == 0) {
 		deny_deletes = git_config_bool(var, value);
 		return 0;
@@ -468,6 +474,22 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	return 0;
 }
 
+static int is_case_clone(const char *refname, const unsigned char *sha1,
+			int flags, void *cb_data)
+{
+	const char *incoming_refname = cb_data;
+	return !strcasecmp(refname, incoming_refname) &&
+		strcmp(refname, incoming_refname);
+}
+
+static int ref_is_denied_case_clone(const char *name)
+{
+	if (!deny_case_clone_branches)
+		return 0;
+
+	return for_each_ref(is_case_clone, (void *) name);
+}
+
 static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
@@ -478,7 +500,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	struct ref_lock *lock;
 
 	/* only refs/... are allowed */
-	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
+	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0) ||
+	    ref_is_denied_case_clone(name)) {
 		rp_error("refusing to create funny ref '%s' remotely", name);
 		return "funny refname";
 	}
@@ -1171,6 +1194,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		die("'%s' does not appear to be a git repository", dir);
 
 	git_config(receive_pack_config, NULL);
+	if (deny_case_clone_branches == DENY_UNCONFIGURED)
+		deny_case_clone_branches = ignore_case;
 
 	if (0 <= transfer_unpack_limit)
 		unpack_limit = transfer_unpack_limit;
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 0736bcb..fd95231 100755
--- 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
+	    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
+	) &&
+	git send-pack ./victim HEAD^:refs/heads/notacaseclone &&
+	test_must_fail git send-pack ./victim :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
+	) &&
+	git send-pack ./victim :caseclone &&
+	#confirm that this took effect upstream
+	(
+	    cd victim &&
+	    test_must_fail git rev-parse caseclone
+	)
+	#check that we can recreate a branch after deleting a
+	#case-clone of it
+	case_clone_ver=$(git rev-parse HEAD^)
+	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 &&
 	(
-- 
2.0.0.rc1.18.gf763c0f

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

[Johannes Sixt]

Am 6/4/2014 5:13, schrieb David Turner:
> 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.

> --- 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.

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

> +	(
> +	    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.

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

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

[David Turner]

On Wed, 2014-06-04 at 08:06 +0200, Johannes Sixt wrote:

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

Yes.  I'll fix that.

> How about 'denyCaseInsensitiveRefs', 'denyIgnoreCaseRefs'?

I don't love these, because it's not the case-insensitivity that's being
denied but the duplication. 

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

I think the list should be sorted alphabetically, so that we don't have
to worry about what is most important. 

<snip issues that I'll fix when I reroll>

> 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.)

I'll look into that and fix if necessary.  Thanks.

<snip more issues that I'll fix when I reroll>