{"thread":{"id":"36826","subject":"[PATCH] receive-pack: optionally deny case-clone refs","startedAt":"2014-06-03T19:14:56Z","lastAt":"2014-06-04T03:05:44Z","messageCount":5,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243203","messageId":"1401822896-816-1-git-send-email-dturner@twitter.com","threadId":"36826","inReplyTo":null,"subject":"[PATCH] receive-pack: optionally deny case-clone refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-06-03T19:14:56Z","receivedAt":"2014-06-03T19:14:56Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"It is possible to have two branches which are the same but for case.\nThis works great on the case-sensitive filesystems, but not so well on\ncase-insensitive filesystems.  It is fairly typical to have\ncase-insensitive clients (Macs, say) with a case-sensitive server\n(GNU/Linux).\n\nShould a user attempt to pull on a Mac when there are case-clone\nbranches with differing contents, they'll get an error message\ncontaining something like \"Ref refs/remotes/origin/lower is at\n[sha-of-lowercase-branch] but expected [sha-of-uppercase-branch]....\n(unable to update local ref)\"\n\nWith a case-insensitive git server, if a branch called capital-M\nMaster (that differs from lowercase-m-master) is pushed, nobody else\ncan push to (lowercase-m) master until the branch is removed.\n\nCreate the option receive.denycaseclonebranches, which checks pushed\nbranches to ensure that they are not case-clones of an existing\nbranch.  This setting is turned on by default if core.ignorecase is\nset, but not otherwise.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n builtin/receive-pack.c | 29 ++++++++++++++++++++++++++++-\n t/t5400-send-pack.sh   | 20 ++++++++++++++++++++\n 2 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c323081..0894ded 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -27,6 +27,7 @@ enum deny_action {\n \n static int deny_deletes;\n static int deny_non_fast_forwards;\n+static int deny_case_clone_branches = -1;\n static enum deny_action deny_current_branch = DENY_UNCONFIGURED;\n static enum deny_action deny_delete_current = DENY_UNCONFIGURED;\n static int receive_fsck_objects = -1;\n@@ -69,6 +70,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \tif (status)\n \t\treturn status;\n \n+\tif (strcmp(var, \"receive.denycaseclonebranches\") == 0) {\n+\t\tdeny_case_clone_branches = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.denydeletes\") == 0) {\n \t\tdeny_deletes = git_config_bool(var, value);\n \t\treturn 0;\n@@ -468,6 +474,24 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n+static int is_case_clone(const char *refname, const unsigned char *sha1,\n+\t\t\tint flags, void *cb_data)\n+{\n+\tconst char* incoming_refname = cb_data;\n+\treturn !strcasecmp(refname, incoming_refname) &&\n+\t\tstrcmp(refname, incoming_refname);\n+}\n+\n+static int ref_is_denied_case_clone(const char *name)\n+{\n+\n+\tif (!deny_case_clone_branches)\n+\t\treturn 0;\n+\n+\treturn for_each_ref(is_case_clone, (void *) name);\n+\n+}\n+\n static const char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n@@ -478,7 +502,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tstruct ref_lock *lock;\n \n \t/* only refs/... are allowed */\n-\tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n+\tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0) ||\n+\t    ref_is_denied_case_clone(name)) {\n \t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n \t\treturn \"funny refname\";\n \t}\n@@ -1171,6 +1196,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \n \tgit_config(receive_pack_config, NULL);\n+\tif (deny_case_clone_branches == -1)\n+\t\tdeny_case_clone_branches = ignore_case;\n \n \tif (0 <= transfer_unpack_limit)\n \t\tunpack_limit = transfer_unpack_limit;\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 0736bcb..099c0e3 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -129,6 +129,26 @@ test_expect_success 'denyNonFastforwards trumps --force' '\n \ttest \"$victim_orig\" = \"$victim_head\"\n '\n \n+if ! test_have_prereq CASE_INSENSITIVE_FS\n+then\n+test_expect_success 'denyCaseCloneBranches works' '\n+\t(\n+\t    cd victim &&\n+\t    git config receive.denyCaseCloneBranches true\n+\t    git config receive.denyDeletes false\n+\t) &&\n+\tgit checkout -b caseclone &&\n+\tgit send-pack ./victim caseclone &&\n+\tgit checkout -b CaseClone &&\n+\ttest_must_fail git send-pack ./victim CaseClone &&\n+\tgit checkout -b notacaseclone &&\n+\tgit send-pack ./victim notacaseclone &&\n+\ttest_must_fail git send-pack ./victim :CaseClone &&\n+\tgit send-pack ./victim :caseclone &&\n+\tgit send-pack ./victim CaseClone\n+'\n+fi\n+\n test_expect_success 'push --all excludes remote-tracking hierarchy' '\n \tmkdir parent &&\n \t(\n-- \n2.0.0.rc1.18.gf763c0f\n"},{"id":"243210","messageId":"xmqqioohwud5.fsf@gitster.dls.corp.google.com","threadId":"36826","inReplyTo":"1401822896-816-1-git-send-email-dturner@twitter.com","subject":"Re: [PATCH] receive-pack: optionally deny case-clone refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-03T21:33:58Z","receivedAt":"2014-06-03T21:33:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> It is possible to have two branches which are the same but for case.\n> This works great on the case-sensitive filesystems, but not so well on\n> case-insensitive filesystems.  It is fairly typical to have\n> case-insensitive clients (Macs, say) with a case-sensitive server\n> (GNU/Linux).\n>\n> Should a user attempt to pull on a Mac when there are case-clone\n> branches with differing contents, they'll get an error message\n> containing something like \"Ref refs/remotes/origin/lower is at\n> [sha-of-lowercase-branch] but expected [sha-of-uppercase-branch]....\n> (unable to update local ref)\"\n>\n> With a case-insensitive git server, if a branch called capital-M\n> Master (that differs from lowercase-m-master) is pushed, nobody else\n> can push to (lowercase-m) master until the branch is removed.\n>\n> Create the option receive.denycaseclonebranches, which checks pushed\n> branches to ensure that they are not case-clones of an existing\n> branch.  This setting is turned on by default if core.ignorecase is\n> set, but not otherwise.\n>\n> Signed-off-by: David Turner <dturner@twitter.com>\n> ---\n\nI do not object to this new feature in principle, but I do not know\nif we want to introduce a new word \"case-clone refs\" without adding\nit to the glossary documentation.\n\nIt feels a bit funny to tie this to core.ignorecase, which is an\nattribute of the filesystem used for the working tree, though.\n\nUpdates to Documentation/config.txt and Documentation/git-push.txt\nare also needed.\n\n>  builtin/receive-pack.c | 29 ++++++++++++++++++++++++++++-\n>  t/t5400-send-pack.sh   | 20 ++++++++++++++++++++\n>  2 files changed, 48 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index c323081..0894ded 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -27,6 +27,7 @@ enum deny_action {\n>  \n>  static int deny_deletes;\n>  static int deny_non_fast_forwards;\n> +static int deny_case_clone_branches = -1;\n>  static enum deny_action deny_current_branch = DENY_UNCONFIGURED;\n>  static enum deny_action deny_delete_current = DENY_UNCONFIGURED;\n\nWould it make sense to reuse the enum deny_action for this new\nsettings, with an eye to later convert the older boolean ones also\nto use enum deny_action to make them consistent and more flexible?\n\n> @@ -69,6 +70,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n>  \tif (status)\n>  \t\treturn status;\n>  \n> +\tif (strcmp(var, \"receive.denycaseclonebranches\") == 0) {\n> +\t\tdeny_case_clone_branches = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \tif (strcmp(var, \"receive.denydeletes\") == 0) {\n>  \t\tdeny_deletes = git_config_bool(var, value);\n>  \t\treturn 0;\n> @@ -468,6 +474,24 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n>  \treturn 0;\n>  }\n>  \n> +static int is_case_clone(const char *refname, const unsigned char *sha1,\n> +\t\t\tint flags, void *cb_data)\n> +{\n> +\tconst char* incoming_refname = cb_data;\n\nWe write C not C++ around here; the pointer-asterisk sticks to the\nvariable name, not typename.\n\n> +\treturn !strcasecmp(refname, incoming_refname) &&\n> +\t\tstrcmp(refname, incoming_refname);\n\n(Mental note to the reviewer himself) This returns true iff there is\nan existing ref whose name is only different in case, and cause\nfor-each-ref to return early with true.  In a sane case of not\nreceiving problematic refs, this will have to iterate over all the\nexisting refnames.  Wonder if there are better ways to optimize this\nin a repository with hundreds or thousands of refs, which is not all\nthat uncommon.\n\n> +}\n> +\n> +static int ref_is_denied_case_clone(const char *name)\n> +{\n> +\n> +\tif (!deny_case_clone_branches)\n> +\t\treturn 0;\n> +\n> +\treturn for_each_ref(is_case_clone, (void *) name);\n> +\n\nThe trailing blank line inside a function at the end is somewhat\nunusual.\n\n> +}\n> +\n\n> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\n> index 0736bcb..099c0e3 100755\n> --- a/t/t5400-send-pack.sh\n> +++ b/t/t5400-send-pack.sh\n> @@ -129,6 +129,26 @@ test_expect_success 'denyNonFastforwards trumps --force' '\n>  \ttest \"$victim_orig\" = \"$victim_head\"\n>  '\n>  \n> +if ! test_have_prereq CASE_INSENSITIVE_FS\n> +then\n\nHmm, don't we want the feature to kick in for both case sensitive\nand case insensitive filesystems?\n\n> +test_expect_success 'denyCaseCloneBranches works' '\n> +\t(\n> +\t    cd victim &&\n> +\t    git config receive.denyCaseCloneBranches true\n> +\t    git config receive.denyDeletes false\n> +\t) &&\n> +\tgit checkout -b caseclone &&\n> +\tgit send-pack ./victim caseclone &&\n> +\tgit checkout -b CaseClone &&\n> +\ttest_must_fail git send-pack ./victim CaseClone &&\n\nAt this point, we would want to see not just that send-pack fails\nbut also that \"victim\" does not have CaseClone branch and does have\ncaseclone branch pointing at the original value (i.e. we do not want\nto see \"caseclone\" updated to a value that would have gone to\nCaseClone with this push).\n\nEach push in the sequence should be preceded by a \"git commit\" or\nsomething so that we can verify the object at the tip of the ref in\nthe \"victim\" repository, I would think.  Otherwise it is hard to\ntell an expected no-op that has failed and a no-op because it\nmistakenly pushed the same value to a wrong ref.\n\n> +\tgit checkout -b notacaseclone &&\n> +\tgit send-pack ./victim notacaseclone &&\n> +\ttest_must_fail git send-pack ./victim :CaseClone &&\n\nThis is expected to fail we expect that earlier push of CaseClone\nhas failed and we do not have such a branch, OK.\n\n> +\tgit send-pack ./victim :caseclone &&\n\nThis is expected to succeed because we expect that earlier push of\nCaseClone has failed and we still have caseclone intact.\n\n> +\tgit send-pack ./victim CaseClone\n> +'\n> +fi\n> +\n>  test_expect_success 'push --all excludes remote-tracking hierarchy' '\n>  \tmkdir parent &&\n>  \t(\n"},{"id":"243254","messageId":"1401832959.18134.120.camel@stross","threadId":"36826","inReplyTo":"xmqqioohwud5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] receive-pack: optionally deny case-clone refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-06-03T22:02:39Z","receivedAt":"2014-06-03T22:02:39Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-06-03 at 14:33 -0700, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n> > It is possible to have two branches which are the same but for case.\n> > This works great on the case-sensitive filesystems, but not so well on\n> > case-insensitive filesystems.  It is fairly typical to have\n> > case-insensitive clients (Macs, say) with a case-sensitive server\n> > (GNU/Linux).\n> >\n> > Should a user attempt to pull on a Mac when there are case-clone\n> > branches with differing contents, they'll get an error message\n> > containing something like \"Ref refs/remotes/origin/lower is at\n> > [sha-of-lowercase-branch] but expected [sha-of-uppercase-branch]....\n> > (unable to update local ref)\"\n> >\n> > With a case-insensitive git server, if a branch called capital-M\n> > Master (that differs from lowercase-m-master) is pushed, nobody else\n> > can push to (lowercase-m) master until the branch is removed.\n> >\n> > Create the option receive.denycaseclonebranches, which checks pushed\n> > branches to ensure that they are not case-clones of an existing\n> > branch.  This setting is turned on by default if core.ignorecase is\n> > set, but not otherwise.\n> >\n> > Signed-off-by: David Turner <dturner@twitter.com>\n> > ---\n> \n> I do not object to this new feature in principle, but I do not know\n> if we want to introduce a new word \"case-clone refs\" without adding\n> it to the glossary documentation.\n\nI would be happy to add \"case-clone\" to the glossary -- would that be OK\nwith you?  I do not immediately think of the better term.\n\n> It feels a bit funny to tie this to core.ignorecase, which is an\n> attribute of the filesystem used for the working tree, though.\n\nIt seems like it's an attribute of the filesystem used for the GIT_DIR\n(at least, that's what's actually tested in order to set it).  So I\nthink this is OK.\n\n> Updates to Documentation/config.txt and Documentation/git-push.txt\n> are also needed.\n> ...\n> Would it make sense to reuse the enum deny_action for this new\n> settings, with an eye to later convert the older boolean ones also\n> to use enum deny_action to make them consistent and more flexible?\n>...\n> We write C not C++ around here; the pointer-asterisk sticks to the\n> variable name, not typename.\n>...[moved]\n> The trailing blank line inside a function at the end is somewhat\n> unusual.\n\nWill fix these, thank you.  Do you happen to know if there's a style\nchecker available that I could run before committing?\n\n> > +\treturn !strcasecmp(refname, incoming_refname) &&\n> > +\t\tstrcmp(refname, incoming_refname);\n> \n> (Mental note to the reviewer himself) This returns true iff there is\n> an existing ref whose name is only different in case, and cause\n> for-each-ref to return early with true.  In a sane case of not\n> receiving problematic refs, this will have to iterate over all the\n> existing refnames.  Wonder if there are better ways to optimize this\n> in a repository with hundreds or thousands of refs, which is not all\n> that uncommon.\n\nMy expectation is that on average a push will involve a small number of\nrefs -- usually exactly one.  We could put the refs into a\ncase-insensitive hashmap if we expect many refs.  This ties into the\ngeneral question of whether ref handling can be made O(1) or O(log N),\nwhich I think the list has not come to a satisfactory solution to.\n\n> > diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\n> > index 0736bcb..099c0e3 100755\n> > --- a/t/t5400-send-pack.sh\n> > +++ b/t/t5400-send-pack.sh\n> > @@ -129,6 +129,26 @@ test_expect_success 'denyNonFastforwards trumps --force' '\n> >  \ttest \"$victim_orig\" = \"$victim_head\"\n> >  '\n> >  \n> > +if ! test_have_prereq CASE_INSENSITIVE_FS\n> > +then\n> \n> Hmm, don't we want the feature to kick in for both case sensitive\n> and case insensitive filesystems?\n\nYes, but it's harder to test on case-insensitive filesystems because we\ncannot have coexisting local case-clone branches.  We could test by\nmaking sure to first locally deleting the case-clone branches, I guess.\nI'll do that.\n\n> > +test_expect_success 'denyCaseCloneBranches works' '\n> > +\t(\n> > +\t    cd victim &&\n> > +\t    git config receive.denyCaseCloneBranches true\n> > +\t    git config receive.denyDeletes false\n> > +\t) &&\n> > +\tgit checkout -b caseclone &&\n> > +\tgit send-pack ./victim caseclone &&\n> > +\tgit checkout -b CaseClone &&\n> > +\ttest_must_fail git send-pack ./victim CaseClone &&\n> \n> At this point, we would want to see not just that send-pack fails\n> but also that \"victim\" does not have CaseClone branch and does have\n> caseclone branch pointing at the original value (i.e. we do not want\n> to see \"caseclone\" updated to a value that would have gone to\n> CaseClone with this push).\n> \n> Each push in the sequence should be preceded by a \"git commit\" or\n> something so that we can verify the object at the tip of the ref in\n> the \"victim\" repository, I would think.  Otherwise it is hard to\n> tell an expected no-op that has failed and a no-op because it\n> mistakenly pushed the same value to a wrong ref.\n\nWill fix!\n"},{"id":"243256","messageId":"xmqq61khwsj8.fsf@gitster.dls.corp.google.com","threadId":"36826","inReplyTo":"1401832959.18134.120.camel@stross","subject":"Re: [PATCH] receive-pack: optionally deny case-clone refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-03T22:13:31Z","receivedAt":"2014-06-03T22:13:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> I would be happy to add \"case-clone\" to the glossary -- would that be OK\n> with you?  I do not immediately think of the better term.\n\nSomehow \"case-clone\" sounds strange, though X-<.\n\n>> (Mental note to the reviewer himself) This returns true iff there is\n>> an existing ref whose name is only different in case, and cause\n>> for-each-ref to return early with true.  In a sane case of not\n>> receiving problematic refs, this will have to iterate over all the\n>> existing refnames.  Wonder if there are better ways to optimize this\n>> in a repository with hundreds or thousands of refs, which is not all\n>> that uncommon.\n>\n> My expectation is that on average a push will involve a small number of\n> refs -- usually exactly one.\n\nIt does not matter that _you_ push only one, because the number of\nexisting refs at the receiving end is what determines how many times\nthe for-each-ref loop spins, no?\n\n> Yes, but it's harder to test on case-insensitive filesystems because we\n> cannot have coexisting local case-clone branches.\n\nYou do not have to (and you should not) do \"git checkout -b\" to\ncreate various local branches in the first place.  For example:\n\n\tgit send-pack ./victim HEAD^1:refs/heads/caseclone &&\n\ttest_must_fail git send-pack ./victim HEAD:refs/heads/CaseClone\n\nwould let you push the parent of the current tip to caseclone and\nthen attempt to push the current tip to CaseClone.  If the receiving\nend could incorrectly fast-forwards caseclone, you found a bug ;-)\n"},{"id":"243272","messageId":"1401851144.18134.128.camel@stross","threadId":"36826","inReplyTo":"xmqq61khwsj8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] receive-pack: optionally deny case-clone refs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-06-04T03:05:44Z","receivedAt":"2014-06-04T03:05:44Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-06-03 at 15:13 -0700, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n> > I would be happy to add \"case-clone\" to the glossary -- would that be OK\n> > with you?  I do not immediately think of the better term.\n> \n> Somehow \"case-clone\" sounds strange, though X-<.\n\nCase clones case clones roly poly case clones; case clones case clones\neat them up yum.  Yeah, I wish I could think of a name that did not call\nto mind Tatiana Maslany, but unfortunately, that is all I can think of.\nAt least it's easy to search for.\n\n> >> (Mental note to the reviewer himself) This returns true iff there is\n> >> an existing ref whose name is only different in case, and cause\n> >> for-each-ref to return early with true.  In a sane case of not\n> >> receiving problematic refs, this will have to iterate over all the\n> >> existing refnames.  Wonder if there are better ways to optimize this\n> >> in a repository with hundreds or thousands of refs, which is not all\n> >> that uncommon.\n> >\n> > My expectation is that on average a push will involve a small number of\n> > refs -- usually exactly one.\n> \n> It does not matter that _you_ push only one, because the number of\n> existing refs at the receiving end is what determines how many times\n> the for-each-ref loop spins, no?\n\nYes, but that loop is an inner loop; so the total cost is O(refs pushed\n* existing refs).  An in-memory hashmap would be O(existing refs) with a\nhigher constant factor.  An on-disk hashmap would probably scale best,\nbut a fair amount more work.\n\n> > Yes, but it's harder to test on case-insensitive filesystems because we\n> > cannot have coexisting local case-clone branches.\n> \n> You do not have to (and you should not) do \"git checkout -b\" to\n> create various local branches in the first place.  For example:\n> \n> \tgit send-pack ./victim HEAD^1:refs/heads/caseclone &&\n> \ttest_must_fail git send-pack ./victim HEAD:refs/heads/CaseClone\n> \n> would let you push the parent of the current tip to caseclone and\n> then attempt to push the current tip to CaseClone.  If the receiving\n> end could incorrectly fast-forwards caseclone, you found a bug ;-)\n\nThanks.\n"}]}