[v2,2/2,RFC] push: allow delete one level ref

Commit Message

ZheNing Hu Feb. 4, 2023, 4:48 p.m. UTC

From: ZheNing Hu <[e-mail]>

Git will reject the deletion of one level refs e,g. "refs/foo"
through "git push -d", however, some users want to be able to
clean up these branches that were created unexpectedly on the
remote.

Therefore, when updating branches on the server with
"git receive-pack", by checking whether it is a branch deletion
operation, it will determine whether to allow the update of
one level refs. This avoids creating/updating such one level
branches, but allows them to be deleted.

On the client side, "git push" also does not properly fill in
the old-oid of one level refs, which causes the server-side
"git receive-pack" to think that the ref's old-oid has changed
when deleting one level refs, this causes the push to be rejected.

So the solution is to fix the client to be able to delete
one level refs by properly filling old-oid.

Signed-off-by: ZheNing Hu <[e-mail]>
---
 builtin/receive-pack.c | 4 +++-
 connect.c              | 3 ++-
 t/t5516-fetch-push.sh  | 7 +++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Feb. 6, 2023, 10:13 p.m. UTC | #1

"ZheNing Hu via GitGitGadget" <[e-mail]> writes:

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 13ff9fae3ba..77088f5ba8d 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1463,7 +1463,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
>  		find_shared_symref(worktrees, "HEAD", name);
>  
>  	/* 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, is_null_oid(new_oid) ?
> +				 REFNAME_ALLOW_ONELEVEL : 0)) {

I am somehow smelling that this is about "refs/stash" and it may be
a good thing to allow removing a leftover refs/stash remotely.  I am
not sure why we need to treat it differently while still blocking
update/modification.

Is it that we are actively discouraging one-level refs like
refs/stash, so removing is fine but once removed we do not allow
creating or updating?  It somewhat smells a hard to explain rule to
the users, at least to me.

> diff --git a/connect.c b/connect.c
> index 63e59641c0d..7a396ad72e9 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -30,7 +30,8 @@ static int check_ref(const char *name, unsigned int flags)
>  		return 0;
>  
>  	/* REF_NORMAL means that we don't want the magic fake tag refs */
> -	if ((flags & REF_NORMAL) && check_refname_format(name, 0))
> +	if ((flags & REF_NORMAL) && check_refname_format(name,
> +							 REFNAME_ALLOW_ONELEVEL))
>  		return 0;

This side of the change does make sense.

Thanks.

ZheNing Hu Feb. 24, 2023, 6:02 a.m. UTC | #2

Junio C Hamano <[e-mail]> 于2023年2月7日周二 06:13写道：
>
> "ZheNing Hu via GitGitGadget" <[e-mail]> writes:
>
> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index 13ff9fae3ba..77088f5ba8d 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -1463,7 +1463,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
> >               find_shared_symref(worktrees, "HEAD", name);
> >
> >       /* 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, is_null_oid(new_oid) ?
> > +                              REFNAME_ALLOW_ONELEVEL : 0)) {
>
> I am somehow smelling that this is about "refs/stash" and it may be
> a good thing to allow removing a leftover refs/stash remotely.  I am
> not sure why we need to treat it differently while still blocking
> update/modification.
>
> Is it that we are actively discouraging one-level refs like
> refs/stash, so removing is fine but once removed we do not allow
> creating or updating?  It somewhat smells a hard to explain rule to
> the users, at least to me.
>

As I mentioned before, the problem we encountered was that
the refs/master created unexpectedly cannot be deleted. Additionally,
some programs only search for references in the command space of
refs/heads/ or refs/tags/, so they may ignore one-level references
such as refs/master. Therefore, generally speaking, it's better not to
let users create/update special one-level references. This can be achieved
directly through git receive-pack or implemented by upper-layer
services in positions such as pre-receive-hook.

Certainly, I can remove the previous section as you requested.

> > diff --git a/connect.c b/connect.c
> > index 63e59641c0d..7a396ad72e9 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -30,7 +30,8 @@ static int check_ref(const char *name, unsigned int flags)
> >               return 0;
> >
> >       /* REF_NORMAL means that we don't want the magic fake tag refs */
> > -     if ((flags & REF_NORMAL) && check_refname_format(name, 0))
> > +     if ((flags & REF_NORMAL) && check_refname_format(name,
> > +                                                      REFNAME_ALLOW_ONELEVEL))
> >               return 0;
>
> This side of the change does make sense.
>
> Thanks.

Thanks.

Junio C Hamano Feb. 24, 2023, 5:12 p.m. UTC | #3

ZheNing Hu <[e-mail]> writes:

> ... Therefore, generally speaking, it's better not to let users
> create/update special one-level references.

The question was "Is it that we are actively discouraging one-level
refs like refs/stash, so removing is fine but once removed we do not
allow creating or updating?"

You could just have given a single word answer, Yes, then.

> Certainly, I can remove the previous section as you requested.

I didn't request to do anything to the change.  I asked you to
explain why you allow only delete without create/update, and without
knowing why, I didn't have enough information to make such a request.

I think "we discourage a single-level refnames so allow deleting one
that may have been created by mistake, but do not allow creation or
deletion as before" does make sense.  As long as that is explained
in the proposed log message and in the end-user facing documentation,
I am happy with the new behaviour.

Thanks.

ZheNing Hu Feb. 25, 2023, 2:11 p.m. UTC | #4

Junio C Hamano <[e-mail]> 于2023年2月25日周六 01:12写道：
>
> ZheNing Hu <[e-mail]> writes:
>
> > ... Therefore, generally speaking, it's better not to let users
> > create/update special one-level references.
>
> The question was "Is it that we are actively discouraging one-level
> refs like refs/stash, so removing is fine but once removed we do not
> allow creating or updating?"
>
> You could just have given a single word answer, Yes, then.
>
> > Certainly, I can remove the previous section as you requested.
>
> I didn't request to do anything to the change.  I asked you to
> explain why you allow only delete without create/update, and without
> knowing why, I didn't have enough information to make such a request.
>
> I think "we discourage a single-level refnames so allow deleting one
> that may have been created by mistake, but do not allow creation or
> deletion as before" does make sense.  As long as that is explained
> in the proposed log message and in the end-user facing documentation,
> I am happy with the new behaviour.
>

I understand. I didn't mention the reason for not allowing the
creation/update of one-level refs in the commit message, I will add it later.

> Thanks.

Patch

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 13ff9fae3ba..77088f5ba8d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1463,7 +1463,9 @@  static const char *update(struct command *cmd, struct shallow_info *si)
 		find_shared_symref(worktrees, "HEAD", name);
 
 	/* 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, is_null_oid(new_oid) ?
+				 REFNAME_ALLOW_ONELEVEL : 0)) {
 		rp_error("refusing to update funny ref '%s' remotely", name);
 		ret = "funny refname";
 		goto out;
diff --git a/connect.c b/connect.c
index 63e59641c0d..7a396ad72e9 100644
--- a/connect.c
+++ b/connect.c
@@ -30,7 +30,8 @@  static int check_ref(const char *name, unsigned int flags)
 		return 0;
 
 	/* REF_NORMAL means that we don't want the magic fake tag refs */
-	if ((flags & REF_NORMAL) && check_refname_format(name, 0))
+	if ((flags & REF_NORMAL) && check_refname_format(name,
+							 REFNAME_ALLOW_ONELEVEL))
 		return 0;
 
 	/* REF_HEADS means that we want regular branch heads */
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f37861efc40..19ebefa5ace 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -903,6 +903,13 @@  test_expect_success 'push --delete refuses empty string' '
 	test_must_fail git push testrepo --delete ""
 '
 
+test_expect_success 'push --delete onelevel refspecs' '
+	mk_test testrepo heads/main &&
+	git -C testrepo update-ref refs/onelevel refs/heads/main &&
+	git push testrepo --delete refs/onelevel &&
+	test_must_fail git -C testrepo rev-parse --verify refs/onelevel
+'
+
 test_expect_success 'warn on push to HEAD of non-bare repository' '
 	mk_test testrepo heads/main &&
 	(