push: allow delete one level ref
cover push: allow delete one level ref
This might be an odd request: allow git push to delete one level refs like "ref/foo".
Some users on my side often push "refs/for/master" to the remote for code review, but due to a user's typo, "refs/master" is pushed to the remote.
Pushing a one level ref like "refs/foo" should not have been successful, but since my server side directly updated the ref during the proc-receive-hook phase of git receive-pack, "refs/foo" was mistakenly left at on the server.
But for some reasons, users cannot delete this special branch through "git push -d".
First, I executed git update-ref --stdin inside my proc-receive-hook to delete the branch. As a result, update-ref reported an error: "cannot lock ref 'refs/foo': reference already exists".
So I tried GIT_TRACKET_PACKET to debug and found that git push did not seem to pass the correct ref old-oid, which is why update-ref reported an error.
"18:13:41.214872 pkt-line.c:80 packet: receive-pack< 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/foo\0 report-status-v2 side-band-64k object-format=sha1 agent=git/2.39.0.227.g262c45b6a1"
Tracing it back, the check_ref() in the git push link didn't record the old-oid for the remote "refs/foo".
At the same time, the server update() process will reject the one level ref by default and return a "funny refname" error.
It is worth mentioning that since I deleted the branch, the error message returned here is "refusing to create funny ref 'refs/foo' remotely", which is also worth fixing.
So this patch series do:
v1.
1. fix funny refname error message from "create" to "update".
2. let server allow delete one level ref.
3. let client pass correct one level ref old-oid.
ZheNing Hu (2):
receive-pack: fix funny ref error messsage
[RFC] push: allow delete one level ref
builtin/receive-pack.c | 7 +++++-- connect.c | 2 +- t/t5516-fetch-push.sh | 18 ++++++++++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-)
base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1465%2Fadlternative%2Fzh%2Fdelete-one-level-ref-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1465/adlternative/zh/delete-one-level-ref-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1465
Signature · 1 line
gitgitgadget
PATCH 1/2 receive-pack: fix funny ref error messsage
From: ZheNing Hu <[email protected]>
When the user deletes the remote one level branch through "git push origin -d refs/foo", remote will return an error:
"refusing to create funny ref 'refs/foo' remotely", here we are not creating "refs/foo" instead wants to delete it, so a better error description here would be: "refusing to update funny ref 'refs/foo' remotely".
--- builtin/receive-pack.c | 2 +- t/t5516-fetch-push.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
builtin/receive-pack.c +1 -1
@@ -1464,7 +1464,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)/* only refs/... are allowed */ if (!starts_with (name,"refs/" ) ||check_refname_format (name +5 ,0 )) { -rp_error ("refusing to create funny ref '%s' remotely" , name); +rp_error ("refusing to update funny ref '%s' remotely" , name); ret ="funny refname" ; goto out; }
t/t5516-fetch-push.sh +5 -0
@@ -401,6 +401,11 @@ test_expect_success 'push with ambiguity' ' ' +test_expect_success 'push with onelevel ref' ' + mk_test testrepo heads/main && + test_must_fail git push testrepo HEAD:refs/onelevel +' + test_expect_success 'push with colon-less refspec (1)' ' mk_test testrepo heads/frotz tags/frotz &&
Signature · 1 line
gitgitgadget
RFC PATCH 2/2 push: allow delete one level ref
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
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.
--- builtin/receive-pack.c | 5 ++++- connect.c | 2 +- t/t5516-fetch-push.sh | 13 +++++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-)
builtin/receive-pack.c +4 -1
@@ -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 )) {rp_error ("refusing to update funny ref '%s' remotely" , name); ret ="funny refname" ; goto out;
connect.c +1 -1
@@ -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 ))return 0 ;/* REF_HEADS means that we want regular branch heads */
t/t5516-fetch-push.sh +13 -0
@@ -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 + ) && + git push testrepo --delete refs/onelevel && + ( + cd testrepo && + test_must_fail git rev-parse --verify refs/onelevel + ) +' + test_expect_success 'warn on push to HEAD of non-bare repository' ' mk_test testrepo heads/main && (
Signature · 1 line
gitgitgadget
Quoted 33 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
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 <[email protected]>--- builtin/receive-pack.c | 5 ++++- connect.c | 2 +- t/t5516-fetch-push.sh | 13 +++++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-)builtin/receive-pack.c +0
-0
builtin/receive-pack.c static const char *update(struct command *cmd, struct shallow_info *si) · line 1469
Quoted 8 lines
builtin/receive-pack.c +4
-1@@ -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)) {connect.c static int check_ref(const char *name, unsigned int flags) · line 33
Quoted 13 lines
builtin/receive-pack.c +0
-0rp_error ("refusing to update funny ref '%s' remotely" , name); ret ="funny refname" ; goto out;connect.c +1
-1@@ -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.
Quoted 8 lines
connect.c +0
-0return 0 ;/* REF_HEADS means that we want regular branch heads */ t/t5516-fetch-push.sh +0
-0
t/t5516-fetch-push.sh test_expect_success 'push --delete refuses empty string' ' · line 911
Quoted 9 lines
t/t5516-fetch-push.sh +6
-0@@ -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 ....
t/t5516-fetch-push.sh test_expect_success 'push --delete refuses empty string' ' · line 915
Quoted 4 lines
t/t5516-fetch-push.sh +4
-0+ git push testrepo --delete refs/onelevel && + ( + cd testrepo && + test_must_fail git rev-parse --verify refs/onelevel
Ditto.
Quoted 52 lines · Ævar Arnfjörð Bjarmason <[email protected]> 于2023年1月17日周二 21:18写道：
Quoted 33 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
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 <[email protected]>--- builtin/receive-pack.c | 5 ++++- connect.c | 2 +- t/t5516-fetch-push.sh | 13 +++++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-)builtin/receive-pack.c +0
-0builtin/receive-pack.c static const char *update(struct command *cmd, struct shallow_info *si) · line 1469
Quoted 8 lines
builtin/receive-pack.c +4
-1@@ -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.
Quoted 8 lines
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.
Quoted 11 lines
Quoted 11 lines
builtin/receive-pack.c +0
-0rp_error ("refusing to update funny ref '%s' remotely" , name); ret ="funny refname" ; goto out;connect.c +0
-0@@ -30,7 +30,7 @@ static int check_ref(const char *name, unsigned int flags)return 0 ;
Quoted 6 lines
connect.c static int check_ref(const char *name, unsigned int flags) · line 33
Quoted 3 lines
connect.c +1
-1/* 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?
Quoted 20 lines
Quoted 8 lines
connect.c +0
-0return 0 ;/* REF_HEADS means that we want regular branch heads */ t/t5516-fetch-push.sh +0
-0t/t5516-fetch-push.sh test_expect_success 'push --delete refuses empty string' ' · line 911
Quoted 9 lines
t/t5516-fetch-push.sh +6
-0@@ -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:
Quoted 2 lines
git -C update-ref ....
OK.
Quoted 6 lines
t/t5516-fetch-push.sh test_expect_success 'push --delete refuses empty string' ' · line 915
Quoted 4 lines
t/t5516-fetch-push.sh +4
-0+ git push testrepo --delete refs/onelevel && + ( + cd testrepo && + test_must_fail git rev-parse --verify refs/onelevelDitto.
Thanks,
ZheNing Hu
v2 cover push: allow delete one level ref
In reply to ZheNing Hu via GitGitGadgetThis might be an odd request: allow git push to delete one level refs like "ref/foo".
Some users on my side often push "refs/for/master" to the remote for code review, but due to a user's typo, "refs/master" is pushed to the remote.
Pushing a one level ref like "refs/foo" should not have been successful, but since my server side directly updated the ref during the proc-receive-hook phase of git receive-pack, "refs/foo" was mistakenly left at on the server.
But for some reasons, users cannot delete this special branch through "git push -d".
First, I executed git update-ref --stdin inside my proc-receive-hook to delete the branch. As a result, update-ref reported an error: "cannot lock ref 'refs/foo': reference already exists".
So I tried GIT_TRACKET_PACKET to debug and found that git push did not seem to pass the correct ref old-oid, which is why update-ref reported an error.
"18:13:41.214872 pkt-line.c:80 packet: receive-pack< 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/foo\0 report-status-v2 side-band-64k object-format=sha1 agent=git/2.39.0.227.g262c45b6a1"
Tracing it back, the check_ref() in the git push link didn't record the old-oid for the remote "refs/foo".
At the same time, the server update() process will reject the one level ref by default and return a "funny refname" error.
It is worth mentioning that since I deleted the branch, the error message returned here is "refusing to create funny ref 'refs/foo' remotely", which is also worth fixing.
So this patch series do:
v1.
1. fix funny refname error message from "create" to "update".
2. let server allow delete one level ref.
3. let client pass correct one level ref old-oid.
v2.
1. fix code style.
ZheNing Hu (2):
receive-pack: fix funny ref error messsage
[RFC] push: allow delete one level ref
builtin/receive-pack.c | 6 ++++-- connect.c | 3 ++- t/t5516-fetch-push.sh | 12 ++++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-)
base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1465%2Fadlternative%2Fzh%2Fdelete-one-level-ref-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1465/adlternative/zh/delete-one-level-ref-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1465
Range-diff vs v1:
1: 214a2b662e3 = 1: 214a2b662e3 receive-pack: fix funny ref error messsage
2: 605b95bf8ab ! 2: 966cb49c388 [RFC] push: allow delete one level ref
@@ builtin/receive-pack.c: static const char *update(struct command *cmd, struct sh
/* 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) ?
++ 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";
@@ connect.c: static int check_ref(const char *name, unsigned int flags)
/* 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))
++ if ((flags & REF_NORMAL) && check_refname_format(name,
++ REFNAME_ALLOW_ONELEVEL))
return 0;
/* REF_HEADS means that we want regular branch heads */
@@ t/t5516-fetch-push.sh: test_expect_success 'push --delete refuses empty string'
+test_expect_success 'push --delete onelevel refspecs' '
+ mk_test testrepo heads/main &&
-+ (
-+ cd testrepo &&
-+ git update-ref refs/onelevel refs/heads/main
-+ ) &&
++ git -C testrepo update-ref refs/onelevel refs/heads/main &&
+ git push testrepo --delete refs/onelevel &&
-+ (
-+ cd testrepo &&
-+ test_must_fail git rev-parse --verify 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' 'Signature · 1 line
gitgitgadget
PATCH v2 1/2 receive-pack: fix funny ref error messsage
From: ZheNing Hu <[email protected]>
When the user deletes the remote one level branch through "git push origin -d refs/foo", remote will return an error:
"refusing to create funny ref 'refs/foo' remotely", here we are not creating "refs/foo" instead wants to delete it, so a better error description here would be: "refusing to update funny ref 'refs/foo' remotely".
--- builtin/receive-pack.c | 2 +- t/t5516-fetch-push.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
builtin/receive-pack.c +1 -1
@@ -1464,7 +1464,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)/* only refs/... are allowed */ if (!starts_with (name,"refs/" ) ||check_refname_format (name +5 ,0 )) { -rp_error ("refusing to create funny ref '%s' remotely" , name); +rp_error ("refusing to update funny ref '%s' remotely" , name); ret ="funny refname" ; goto out; }
t/t5516-fetch-push.sh +5 -0
@@ -401,6 +401,11 @@ test_expect_success 'push with ambiguity' ' ' +test_expect_success 'push with onelevel ref' ' + mk_test testrepo heads/main && + test_must_fail git push testrepo HEAD:refs/onelevel +' + test_expect_success 'push with colon-less refspec (1)' ' mk_test testrepo heads/frotz tags/frotz &&
Signature · 1 line
gitgitgadget
RFC PATCH v2 2/2 push: allow delete one level ref
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
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.
--- builtin/receive-pack.c | 4 +++- connect.c | 3 ++- t/t5516-fetch-push.sh | 7 +++++++ 3 files changed, 12 insertions(+), 2 deletions(-)
builtin/receive-pack.c +3 -1
@@ -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;
connect.c +2 -1
@@ -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 */
t/t5516-fetch-push.sh +7 -0
@@ -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 && (
Signature · 1 line
gitgitgadget
builtin/receive-pack.c static const char *update(struct command *cmd, struct shallow_info *si) · line 1468
Quoted 12 lines · ZheNing Hu via GitGitGadget
builtin/receive-pack.c +3
-1@@ -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.
connect.c static int check_ref(const char *name, unsigned int flags) · line 35
Quoted 12 lines
connect.c +2
-1@@ -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.
Quoted 21 lines · Junio C Hamano <[email protected]> 于2023年2月7日周二 06:13写道：
builtin/receive-pack.c static const char *update(struct command *cmd, struct shallow_info *si) · line 1468
Quoted 12 lines · ZheNing Hu via GitGitGadget
builtin/receive-pack.c +3
-1@@ -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.
Quoted 5 lines
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.
Quoted 15 lines
connect.c static int check_ref(const char *name, unsigned int flags) · line 35
Quoted 12 lines
connect.c +2
-1@@ -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.
Quoted 1 line
Thanks.
Thanks.
Quoted 2 lines · ZheNing Hu
... 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.
Quoted 1 line
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.
Quoted 18 lines · Junio C Hamano <[email protected]> 于2023年2月25日周六 01:12写道：
Quoted 2 lines · ZheNing Hu
... 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.
Quoted 1 line
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.
Quoted 6 lines
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.
Quoted 1 line
Thanks.
v3 cover push: allow delete one level ref
In reply to ZheNing Hu via GitGitGadgetThis might be an odd request: allow git push to delete one level refs like "ref/foo".
Some users on my side often push "refs/for/master" to the remote for code review, but due to a user's typo, "refs/master" is pushed to the remote.
Pushing a one level ref like "refs/foo" should not have been successful, but since my server side directly updated the ref during the proc-receive-hook phase of git receive-pack, "refs/foo" was mistakenly left at on the server.
But for some reasons, users cannot delete this special branch through "git push -d".
First, I executed git update-ref --stdin inside my proc-receive-hook to delete the branch. As a result, update-ref reported an error: "cannot lock ref 'refs/foo': reference already exists".
So I tried GIT_TRACKET_PACKET to debug and found that git push did not seem to pass the correct ref old-oid, which is why update-ref reported an error.
"18:13:41.214872 pkt-line.c:80 packet: receive-pack< 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/foo\0 report-status-v2 side-band-64k object-format=sha1 agent=git/2.39.0.227.g262c45b6a1"
Tracing it back, the check_ref() in the git push link didn't record the old-oid for the remote "refs/foo".
At the same time, the server update() process will reject the one level ref by default and return a "funny refname" error.
It is worth mentioning that since I deleted the branch, the error message returned here is "refusing to create funny ref 'refs/foo' remotely", which is also worth fixing.
So this patch series do:
v1.
1. fix funny refname error message from "create" to "update".
2. let server allow delete one level ref.
3. let client pass correct one level ref old-oid.
v2.
1. fix code style.
v3.
1. clarify in the docs why single-level refs are allowed to be deleted but not created/updated.
ZheNing Hu (2):
receive-pack: fix funny ref error messsage
[RFC] push: allow delete single-level ref
builtin/receive-pack.c | 6 ++++-- connect.c | 3 ++- t/t5516-fetch-push.sh | 12 ++++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-)
base-commit: dadc8e6dacb629f46aee39bde90b6f09b73722eb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1465%2Fadlternative%2Fzh%2Fdelete-one-level-ref-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1465/adlternative/zh/delete-one-level-ref-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1465
Range-diff vs v2:
1: 214a2b662e3 = 1: 857d2435caf receive-pack: fix funny ref error messsage 2: 966cb49c388 ! 2: 4dc75f5b961 [RFC] push: allow delete one level ref
@@ Metadata
Author: ZheNing Hu <[email protected]>
## Commit message ##
- [RFC] push: allow delete one level ref
+ [RFC] push: allow delete single-level ref
- 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.
+ We discourage the creation/update of single-level refs
+ because some upper-layer applications only work in specified
+ reference namespaces, such as "refs/heads/*" or "refs/tags/*",
+ these single-level refnames may not be recognized. However,
+ we still hope users can delete them which have been created
+ by mistake.
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.
+ a single-level refs. This avoids creating/updating such
+ single-level refs, 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
+ the old-oid of single-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.
+ when deleting single-level refs, this causes the push to be
+ rejected. So the solution is to fix the client to be able to
+ delete single-level refs by properly filling old-oid.
Signature · 1 line
gitgitgadget
PATCH v3 1/2 receive-pack: fix funny ref error messsage
From: ZheNing Hu <[email protected]>
When the user deletes the remote one level branch through "git push origin -d refs/foo", remote will return an error:
"refusing to create funny ref 'refs/foo' remotely", here we are not creating "refs/foo" instead wants to delete it, so a better error description here would be: "refusing to update funny ref 'refs/foo' remotely".
--- builtin/receive-pack.c | 2 +- t/t5516-fetch-push.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
builtin/receive-pack.c +1 -1
@@ -1464,7 +1464,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)/* only refs/... are allowed */ if (!starts_with (name,"refs/" ) ||check_refname_format (name +5 ,0 )) { -rp_error ("refusing to create funny ref '%s' remotely" , name); +rp_error ("refusing to update funny ref '%s' remotely" , name); ret ="funny refname" ; goto out; }
t/t5516-fetch-push.sh +5 -0
@@ -401,6 +401,11 @@ test_expect_success 'push with ambiguity' ' ' +test_expect_success 'push with onelevel ref' ' + mk_test testrepo heads/main && + test_must_fail git push testrepo HEAD:refs/onelevel +' + test_expect_success 'push with colon-less refspec (1)' ' mk_test testrepo heads/frotz tags/frotz &&
Signature · 1 line
gitgitgadget
Re: [PATCH v3 1/2] receive-pack: fix funny ref error messsage
Quoted 8 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
When the user deletes the remote one level branch through "git push origin -d refs/foo", remote will return an error:
"refusing to create funny ref 'refs/foo' remotely", here we are not creating "refs/foo" instead wants to delete it, so a better error description here would be: "refusing to update funny ref 'refs/foo' remotely".
OK, update() works on each ref affected, not just the ones that are updated, but also created and deleted. The updated wording may probably be better.
t/t5516-fetch-push.sh test_expect_success 'push with ambiguity' ' · line 408
Quoted 12 lines
Signed-off-by ZheNing Hu <[email protected]>--- builtin/receive-pack.c | 2 +- t/t5516-fetch-push.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)...
t/t5516-fetch-push.sh +5
-0+test_expect_success 'push with onelevel ref' ' + mk_test testrepo heads/main && + test_must_fail git push testrepo HEAD:refs/onelevel +' +
I am not sure what relevance this new test has to the proposed update to the message, though.
Thanks.
Re: [PATCH v3 1/2] receive-pack: fix funny ref error messsage
Quoted 18 lines · Junio C Hamano <[email protected]> 于2023年2月28日周二 00:07写道：
Quoted 8 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
When the user deletes the remote one level branch through "git push origin -d refs/foo", remote will return an error:
"refusing to create funny ref 'refs/foo' remotely", here we are not creating "refs/foo" instead wants to delete it, so a better error description here would be: "refusing to update funny ref 'refs/foo' remotely".OK,
update()works on each ref affected, not just the ones that are updated, but also created and deleted. The updated wording may probably be better.Quoted 1 line
Quoted 15 lines
t/t5516-fetch-push.sh test_expect_success 'push with ambiguity' ' · line 408
Quoted 11 lines
Signed-off-by ZheNing Hu <[email protected]>--- builtin/receive-pack.c | 2 +- t/t5516-fetch-push.sh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)...
t/t5516-fetch-push.sh +5
-0+test_expect_success 'push with onelevel ref' ' + mk_test testrepo heads/main && + test_must_fail git push testrepo HEAD:refs/onelevel +' +I am not sure what relevance this new test has to the proposed update to the message, though.
Ah, I should have incorporated this part of the test into another patch test.
Quoted 1 line
Thanks.
RFC PATCH v3 2/2 push: allow delete single-level ref
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
We discourage the creation/update of single-level refs because some upper-layer applications only work in specified reference namespaces, such as "refs/heads/*" or "refs/tags/*", these single-level refnames may not be recognized. However, we still hope users can delete them which have been created by mistake.
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 a single-level refs. This avoids creating/updating such single-level refs, but allows them to be deleted.
On the client side, "git push" also does not properly fill in the old-oid of single-level refs, which causes the server-side "git receive-pack" to think that the ref's old-oid has changed when deleting single-level refs, this causes the push to be rejected. So the solution is to fix the client to be able to delete single-level refs by properly filling old-oid.
--- builtin/receive-pack.c | 4 +++- connect.c | 3 ++- t/t5516-fetch-push.sh | 7 +++++++ 3 files changed, 12 insertions(+), 2 deletions(-)
builtin/receive-pack.c +3 -1
@@ -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;
connect.c +2 -1
@@ -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 */
t/t5516-fetch-push.sh +7 -0
@@ -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 && (
Signature · 1 line
gitgitgadget
v4 cover push: allow delete one level ref
In reply to ZheNing Hu via GitGitGadgetThis might be an odd request: allow git push to delete one level refs like "ref/foo".
Some users on my side often push "refs/for/master" to the remote for code review, but due to a user's typo, "refs/master" is pushed to the remote.
Pushing a one level ref like "refs/foo" should not have been successful, but since my server side directly updated the ref during the proc-receive-hook phase of git receive-pack, "refs/foo" was mistakenly left at on the server.
But for some reasons, users cannot delete this special branch through "git push -d".
First, I executed git update-ref --stdin inside my proc-receive-hook to delete the branch. As a result, update-ref reported an error: "cannot lock ref 'refs/foo': reference already exists".
So I tried GIT_TRACKET_PACKET to debug and found that git push did not seem to pass the correct ref old-oid, which is why update-ref reported an error.
"18:13:41.214872 pkt-line.c:80 packet: receive-pack< 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/foo\0 report-status-v2 side-band-64k object-format=sha1 agent=git/2.39.0.227.g262c45b6a1"
Tracing it back, the check_ref() in the git push link didn't record the old-oid for the remote "refs/foo".
At the same time, the server update() process will reject the one level ref by default and return a "funny refname" error.
It is worth mentioning that since I deleted the branch, the error message returned here is "refusing to create funny ref 'refs/foo' remotely", which is also worth fixing.
So this patch series do:
v1.
1. fix funny refname error message from "create" to "update".
2. let server allow delete one level ref.
3. let client pass correct one level ref old-oid.
v2.
1. fix code style.
v3.
1. clarify in the docs why single-level refs are allowed to be deleted but not created/updated.
v4.
1. move the testing part of the first patch to the second patch.
ZheNing Hu (2):
receive-pack: fix funny ref error messsage
push: allow delete single-level ref
builtin/receive-pack.c | 6 ++++-- connect.c | 3 ++- t/t5516-fetch-push.sh | 12 ++++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-)
base-commit: dadc8e6dacb629f46aee39bde90b6f09b73722eb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1465%2Fadlternative%2Fzh%2Fdelete-one-level-ref-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1465/adlternative/zh/delete-one-level-ref-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1465
Range-diff vs v3:
1: 857d2435caf ! 1: eef7bca63c6 receive-pack: fix funny ref error messsage
@@ builtin/receive-pack.c: static const char *update(struct command *cmd, struct sh
ret = "funny refname";
goto out;
}
-
- ## t/t5516-fetch-push.sh ##
-@@ t/t5516-fetch-push.sh: test_expect_success 'push with ambiguity' '
-
- '
-
-+test_expect_success 'push with onelevel ref' '
-+ mk_test testrepo heads/main &&
-+ test_must_fail git push testrepo HEAD:refs/onelevel
-+'
-+
- test_expect_success 'push with colon-less refspec (1)' '
-
- mk_test testrepo heads/frotz tags/frotz && 2: 4dc75f5b961 ! 2: 022818f0e9f [RFC] push: allow delete single-level ref
@@ Metadata
Author: ZheNing Hu <[email protected]>
## Commit message ##
- [RFC] push: allow delete single-level ref
+ push: allow delete single-level ref
We discourage the creation/update of single-level refs because some upper-layer applications only work in specified
@@ connect.c: static int check_ref(const char *name, unsigned int flags)
/* REF_HEADS means that we want regular branch heads */
## t/t5516-fetch-push.sh ##
+@@ t/t5516-fetch-push.sh: test_expect_success 'push with ambiguity' ' +
+ '
+
++test_expect_success 'push with onelevel ref' '
++ mk_test testrepo heads/main &&
++ test_must_fail git push testrepo HEAD:refs/onelevel
++'
++
+ test_expect_success 'push with colon-less refspec (1)' '
+
+ mk_test testrepo heads/frotz tags/frotz &&
@@ t/t5516-fetch-push.sh: test_expect_success 'push --delete refuses empty string' '
test_must_fail git push testrepo --delete ""
'
Signature · 1 line
gitgitgadget
PATCH v4 1/2 receive-pack: fix funny ref error messsage
From: ZheNing Hu <[email protected]>
When the user deletes the remote one level branch through "git push origin -d refs/foo", remote will return an error:
"refusing to create funny ref 'refs/foo' remotely", here we are not creating "refs/foo" instead wants to delete it, so a better error description here would be: "refusing to update funny ref 'refs/foo' remotely".
--- builtin/receive-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/receive-pack.c +1 -1
@@ -1464,7 +1464,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)/* only refs/... are allowed */ if (!starts_with (name,"refs/" ) ||check_refname_format (name +5 ,0 )) { -rp_error ("refusing to create funny ref '%s' remotely" , name); +rp_error ("refusing to update funny ref '%s' remotely" , name); ret ="funny refname" ; goto out; }
Signature · 1 line
gitgitgadget
PATCH v4 2/2 push: allow delete single-level ref
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
We discourage the creation/update of single-level refs because some upper-layer applications only work in specified reference namespaces, such as "refs/heads/*" or "refs/tags/*", these single-level refnames may not be recognized. However, we still hope users can delete them which have been created by mistake.
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 a single-level refs. This avoids creating/updating such single-level refs, but allows them to be deleted.
On the client side, "git push" also does not properly fill in the old-oid of single-level refs, which causes the server-side "git receive-pack" to think that the ref's old-oid has changed when deleting single-level refs, this causes the push to be rejected. So the solution is to fix the client to be able to delete single-level refs by properly filling old-oid.
--- builtin/receive-pack.c | 4 +++- connect.c | 3 ++- t/t5516-fetch-push.sh | 12 ++++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-)
builtin/receive-pack.c +3 -1
@@ -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;
connect.c +2 -1
@@ -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 */
t/t5516-fetch-push.sh +12 -0
@@ -401,6 +401,11 @@ test_expect_success 'push with ambiguity' ' ' +test_expect_success 'push with onelevel ref' ' + mk_test testrepo heads/main && + test_must_fail git push testrepo HEAD:refs/onelevel +' + test_expect_success 'push with colon-less refspec (1)' ' mk_test testrepo heads/frotz tags/frotz &&
@@ -898,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 && (
Signature · 1 line
gitgitgadget