David Turner <dtur...@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 <dtur...@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 &&
> (
--
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
