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

[PATCH] 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>
---
 builtin/receive-pack.c | 29 ++++++++++++++++++++++++++++-
 t/t5400-send-pack.sh   | 20 ++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..0894ded 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 = -1;
 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 = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.denydeletes") == 0) {
 		deny_deletes = git_config_bool(var, value);
 		return 0;
@@ -468,6 +474,24 @@ 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 +502,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 +1196,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 == -1)
+		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..099c0e3 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -129,6 +129,26 @@ test_expect_success 'denyNonFastforwards trumps --force' '
 	test "$victim_orig" = "$victim_head"
 '
 
+if ! test_have_prereq CASE_INSENSITIVE_FS
+then
+test_expect_success 'denyCaseCloneBranches works' '
+	(
+	    cd victim &&
+	    git config receive.denyCaseCloneBranches true
+	    git config receive.denyDeletes false
+	) &&
+	git checkout -b caseclone &&
+	git send-pack ./victim caseclone &&
+	git checkout -b CaseClone &&
+	test_must_fail git send-pack ./victim CaseClone &&
+	git checkout -b notacaseclone &&
+	git send-pack ./victim notacaseclone &&
+	test_must_fail git send-pack ./victim :CaseClone &&
+	git send-pack ./victim :caseclone &&
+	git send-pack ./victim CaseClone
+'
+fi
+
 test_expect_success 'push --all excludes remote-tracking hierarchy' '
 	mkdir parent &&
 	(
-- 
2.0.0.rc1.18.gf763c0f

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

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

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

I do not object to this new feature in principle, but I do not know
if we want to introduce a new word "case-clone refs" without adding
it to the glossary documentation.

It feels a bit funny to tie this to core.ignorecase, which is an
attribute of the filesystem used for the working tree, though.

Updates to Documentation/config.txt and Documentation/git-push.txt
are also needed.

>  builtin/receive-pack.c | 29 ++++++++++++++++++++++++++++-
>  t/t5400-send-pack.sh   | 20 ++++++++++++++++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index c323081..0894ded 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 = -1;
>  static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
>  static enum deny_action deny_delete_current = DENY_UNCONFIGURED;

Would it make sense to reuse the enum deny_action for this new
settings, with an eye to later convert the older boolean ones also
to use enum deny_action to make them consistent and more flexible?

> @@ -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 = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>  	if (strcmp(var, "receive.denydeletes") == 0) {
>  		deny_deletes = git_config_bool(var, value);
>  		return 0;
> @@ -468,6 +474,24 @@ 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;

We write C not C++ around here; the pointer-asterisk sticks to the
variable name, not typename.

> +	return !strcasecmp(refname, incoming_refname) &&
> +		strcmp(refname, incoming_refname);

(Mental note to the reviewer himself) This returns true iff there is
an existing ref whose name is only different in case, and cause
for-each-ref to return early with true.  In a sane case of not
receiving problematic refs, this will have to iterate over all the
existing refnames.  Wonder if there are better ways to optimize this
in a repository with hundreds or thousands of refs, which is not all
that uncommon.

> +}
> +
> +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);
> +

The trailing blank line inside a function at the end is somewhat
unusual.

> +}
> +

> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> index 0736bcb..099c0e3 100755
> --- a/t/t5400-send-pack.sh
> +++ b/t/t5400-send-pack.sh
> @@ -129,6 +129,26 @@ test_expect_success 'denyNonFastforwards trumps --force' '
>  	test "$victim_orig" = "$victim_head"
>  '
>  
> +if ! test_have_prereq CASE_INSENSITIVE_FS
> +then

Hmm, don't we want the feature to kick in for both case sensitive
and case insensitive filesystems?

> +test_expect_success 'denyCaseCloneBranches works' '
> +	(
> +	    cd victim &&
> +	    git config receive.denyCaseCloneBranches true
> +	    git config receive.denyDeletes false
> +	) &&
> +	git checkout -b caseclone &&
> +	git send-pack ./victim caseclone &&
> +	git checkout -b CaseClone &&
> +	test_must_fail git send-pack ./victim CaseClone &&

At this point, we would want to see not just that send-pack fails
but also that "victim" does not have CaseClone branch and does have
caseclone branch pointing at the original value (i.e. we do not want
to see "caseclone" updated to a value that would have gone to
CaseClone with this push).

Each push in the sequence should be preceded by a "git commit" or
something so that we can verify the object at the tip of the ref in
the "victim" repository, I would think.  Otherwise it is hard to
tell an expected no-op that has failed and a no-op because it
mistakenly pushed the same value to a wrong ref.

> +	git checkout -b notacaseclone &&
> +	git send-pack ./victim notacaseclone &&
> +	test_must_fail git send-pack ./victim :CaseClone &&

This is expected to fail we expect that earlier push of CaseClone
has failed and we do not have such a branch, OK.

> +	git send-pack ./victim :caseclone &&

This is expected to succeed because we expect that earlier push of
CaseClone has failed and we still have caseclone intact.

> +	git send-pack ./victim CaseClone
> +'
> +fi
> +
>  test_expect_success 'push --all excludes remote-tracking hierarchy' '
>  	mkdir parent &&
>  	(

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

[David Turner]

On Tue, 2014-06-03 at 14:33 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > 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>
> > ---
> 
> I do not object to this new feature in principle, but I do not know
> if we want to introduce a new word "case-clone refs" without adding
> it to the glossary documentation.

I would be happy to add "case-clone" to the glossary -- would that be OK
with you?  I do not immediately think of the better term.

> It feels a bit funny to tie this to core.ignorecase, which is an
> attribute of the filesystem used for the working tree, though.

It seems like it's an attribute of the filesystem used for the GIT_DIR
(at least, that's what's actually tested in order to set it).  So I
think this is OK.

> Updates to Documentation/config.txt and Documentation/git-push.txt
> are also needed.
> ...
> Would it make sense to reuse the enum deny_action for this new
> settings, with an eye to later convert the older boolean ones also
> to use enum deny_action to make them consistent and more flexible?
>...
> We write C not C++ around here; the pointer-asterisk sticks to the
> variable name, not typename.
>...[moved]
> The trailing blank line inside a function at the end is somewhat
> unusual.

Will fix these, thank you.  Do you happen to know if there's a style
checker available that I could run before committing?

> > +	return !strcasecmp(refname, incoming_refname) &&
> > +		strcmp(refname, incoming_refname);
> 
> (Mental note to the reviewer himself) This returns true iff there is
> an existing ref whose name is only different in case, and cause
> for-each-ref to return early with true.  In a sane case of not
> receiving problematic refs, this will have to iterate over all the
> existing refnames.  Wonder if there are better ways to optimize this
> in a repository with hundreds or thousands of refs, which is not all
> that uncommon.

My expectation is that on average a push will involve a small number of
refs -- usually exactly one.  We could put the refs into a
case-insensitive hashmap if we expect many refs.  This ties into the
general question of whether ref handling can be made O(1) or O(log N),
which I think the list has not come to a satisfactory solution to.

> > diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> > index 0736bcb..099c0e3 100755
> > --- a/t/t5400-send-pack.sh
> > +++ b/t/t5400-send-pack.sh
> > @@ -129,6 +129,26 @@ test_expect_success 'denyNonFastforwards trumps --force' '
> >  	test "$victim_orig" = "$victim_head"
> >  '
> >  
> > +if ! test_have_prereq CASE_INSENSITIVE_FS
> > +then
> 
> Hmm, don't we want the feature to kick in for both case sensitive
> and case insensitive filesystems?

Yes, but it's harder to test on case-insensitive filesystems because we
cannot have coexisting local case-clone branches.  We could test by
making sure to first locally deleting the case-clone branches, I guess.
I'll do that.

> > +test_expect_success 'denyCaseCloneBranches works' '
> > +	(
> > +	    cd victim &&
> > +	    git config receive.denyCaseCloneBranches true
> > +	    git config receive.denyDeletes false
> > +	) &&
> > +	git checkout -b caseclone &&
> > +	git send-pack ./victim caseclone &&
> > +	git checkout -b CaseClone &&
> > +	test_must_fail git send-pack ./victim CaseClone &&
> 
> At this point, we would want to see not just that send-pack fails
> but also that "victim" does not have CaseClone branch and does have
> caseclone branch pointing at the original value (i.e. we do not want
> to see "caseclone" updated to a value that would have gone to
> CaseClone with this push).
> 
> Each push in the sequence should be preceded by a "git commit" or
> something so that we can verify the object at the tip of the ref in
> the "victim" repository, I would think.  Otherwise it is hard to
> tell an expected no-op that has failed and a no-op because it
> mistakenly pushed the same value to a wrong ref.

Will fix!

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

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> I would be happy to add "case-clone" to the glossary -- would that be OK
> with you?  I do not immediately think of the better term.

Somehow "case-clone" sounds strange, though X-<.

>> (Mental note to the reviewer himself) This returns true iff there is
>> an existing ref whose name is only different in case, and cause
>> for-each-ref to return early with true.  In a sane case of not
>> receiving problematic refs, this will have to iterate over all the
>> existing refnames.  Wonder if there are better ways to optimize this
>> in a repository with hundreds or thousands of refs, which is not all
>> that uncommon.
>
> My expectation is that on average a push will involve a small number of
> refs -- usually exactly one.

It does not matter that _you_ push only one, because the number of
existing refs at the receiving end is what determines how many times
the for-each-ref loop spins, no?

> Yes, but it's harder to test on case-insensitive filesystems because we
> cannot have coexisting local case-clone branches.

You do not have to (and you should not) do "git checkout -b" to
create various local branches in the first place.  For example:

	git send-pack ./victim HEAD^1:refs/heads/caseclone &&
	test_must_fail git send-pack ./victim HEAD:refs/heads/CaseClone

would let you push the parent of the current tip to caseclone and
then attempt to push the current tip to CaseClone.  If the receiving
end could incorrectly fast-forwards caseclone, you found a bug ;-)

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

[David Turner]

On Tue, 2014-06-03 at 15:13 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > I would be happy to add "case-clone" to the glossary -- would that be OK
> > with you?  I do not immediately think of the better term.
> 
> Somehow "case-clone" sounds strange, though X-<.

Case clones case clones roly poly case clones; case clones case clones
eat them up yum.  Yeah, I wish I could think of a name that did not call
to mind Tatiana Maslany, but unfortunately, that is all I can think of.
At least it's easy to search for.

> >> (Mental note to the reviewer himself) This returns true iff there is
> >> an existing ref whose name is only different in case, and cause
> >> for-each-ref to return early with true.  In a sane case of not
> >> receiving problematic refs, this will have to iterate over all the
> >> existing refnames.  Wonder if there are better ways to optimize this
> >> in a repository with hundreds or thousands of refs, which is not all
> >> that uncommon.
> >
> > My expectation is that on average a push will involve a small number of
> > refs -- usually exactly one.
> 
> It does not matter that _you_ push only one, because the number of
> existing refs at the receiving end is what determines how many times
> the for-each-ref loop spins, no?

Yes, but that loop is an inner loop; so the total cost is O(refs pushed
* existing refs).  An in-memory hashmap would be O(existing refs) with a
higher constant factor.  An on-disk hashmap would probably scale best,
but a fair amount more work.

> > Yes, but it's harder to test on case-insensitive filesystems because we
> > cannot have coexisting local case-clone branches.
> 
> You do not have to (and you should not) do "git checkout -b" to
> create various local branches in the first place.  For example:
> 
> 	git send-pack ./victim HEAD^1:refs/heads/caseclone &&
> 	test_must_fail git send-pack ./victim HEAD:refs/heads/CaseClone
> 
> would let you push the parent of the current tip to caseclone and
> then attempt to push the current tip to CaseClone.  If the receiving
> end could incorrectly fast-forwards caseclone, you found a bug ;-)

Thanks.