Re: [PATCH] receive-pack: optionally deny case-clone refs
On Tue, 2014-06-03 at 15:13 -0700, Junio C Hamano wrote: > David Turner 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] receive-pack: optionally deny case-clone refs
David Turner 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 ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] receive-pack: optionally deny case-clone refs
On Tue, 2014-06-03 at 14:33 -0700, Junio C Hamano wrote: > David Turner 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 > > --- > > 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
Re: [PATCH] receive-pack: optionally deny case-clone refs
David Turner 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
> ---
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 va
[PATCH] receive-pack: optionally deny case-clone refs
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/maj
