Re: [PATCH 2/2] [RFC] push: allow delete one level ref

[ZheNing Hu <adlternative@gmail.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2023年1月17日周二 21:18写道：
>
>
> On Tue, Jan 17 2023, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > 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 <adlternative@gmail.com>
> > ---
> >  builtin/receive-pack.c |  5 ++++-
> >  connect.c              |  2 +-
> >  t/t5516-fetch-push.sh  | 13 +++++++++++++
> >  3 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index 13ff9fae3ba..ad21877ea1b 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -1463,7 +1463,10 @@ 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)) {
>
> Style nit: We tend to wrap at 79 characters, adn with argument lists you
> "keep going" until you hit that limit.
>
> In this case strictly following that rule will lead to funny
> indentation, as we'll have to wrap at "is_null_oid(...)" etc.
>
> But even when avoiding that (which seems good in this case) this should
> be:
>
>         if (!starts_with(name, "refs/") ||
>             check_refname_format(name + 5, is_null_oid(new_oid) ?
>                                  REFNAME_ALLOW_ONELEVEL : 0)) {
>
>

Make sense, I'm going to fix the formatting here.

>
> >               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..b841ae58e03 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -30,7 +30,7 @@ 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))
>
> Here we should wrap after "name,", we end up with a too-long line.
>

To be honest, sometimes it's hard to notice if the current line is longer
than 79 characters, it's often a matter of intuition. Is there any ci script
that can detect this kind of problem?

> >               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..dec8950a392 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -903,6 +903,19 @@ 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 &&
> > +     (
> > +             cd testrepo &&
> > +             git update-ref refs/onelevel refs/heads/main
> > +     ) &&
>
> Avoid the subshell here with:
>
>         git -C update-ref ....
>

OK.

> > +     git push testrepo --delete refs/onelevel &&
> > +     (
> > +             cd testrepo &&
> > +             test_must_fail git rev-parse --verify refs/onelevel
>
> Ditto.


Thanks,
ZheNing Hu