#2013 too strict rules for branches deletion alongside with norules for theirs creations
Closed: Insufficient data 5 years ago by bowlofeggs. Opened 5 years ago by jvanek.

Hello!

Recently I changed directories, and instead of in my fork, I created and pushed https://src.fedoraproject.org/rpms/java-11-openjdk/branch/cups-libs

First of all, I was surprised it allowed me to create it so simply, thus I was sure it will eb easy to delete:
 git push --delete origin cups-libs
remote: FATAL: + refs/heads/cups-libs rpms/java-11-openjdk jvanek DENIED by fallthru
remote: error: hook declined to update refs/heads/cups-libs
To ssh://pkgs.fedoraproject.org/rpms/java-11-openjdk
! [remote rejected] cups-libs (hook declined)
error: failed to push some refs to 'ssh://jvanek@pkgs.fedoraproject.org/rpms/java-11-openjdk'

Not so surprising. What If I would by accident delete master or so. So a bit of searching led to https://fedoraproject.org/wiki/Remove_dist-git_branches and a bit of attempts later to fact that only dist-git admin can do that.
So I found one on #fedora-releng, and he was kind enough to delte this branch.
However later he was rebuked that no one is allwoed to delete branch. So he restoresd branch:)

I hope you see the irony of the thing:
- anybody with push permissions can create a branch
- but cant delete his own accientaly created branch
- any admin still can delete branch, but is bound only politicaly
- any admin can of course create any branch anywhere

Best solution:
- anybody with push permissions can create a branch
- anybody with push permissions can delete branch he created
- project admin can remove brnaches in project
- no one, not even admin, nor git-admin can delete system branches

acceptable solution
- delete my garbage of https://src.fedoraproject.org/rpms/java-11-openjdk/branch/cups-libs please
- again, allow relengs to delete branches, eg after ticket with review or similarly....

Thanx!
J.


Historically, this restriction is for legal compliance as we need to keep all commits that have been used to generate builds in Koji. For most of Koji's history, it did not log the commit it used to make builds, so the only way for us to achieve the compliance goal was to disallow deleting branches. Koji does now log commit hashes for builds, but we still don't want to delete branches with commits that happened prior to that feature so it is not simple.

Thanx for explanation.

This branch is less then day old, and except CI, never built from. The commit it had on top of master, si already merged.

For the record, right now the branch points to the same commit as master - removing it wouldn't lead to loosing any commits. In cases like that I think it would be good to allow deleting such branches.

to enforce similar check to responsible person may be unnecessary burden. On contrary, allow deleltion of such automatically checked branches (again, instead of system ones) may solve the political part of issue.

Is there any particular proposal you would like FESCo to decide on here? Is it just the one branch, or are you looking for a more general policy change?

Metadata Update from @bowlofeggs:
- Issue tagged with: meeting

5 years ago

We will discuss this in the FESCo meeting Monday at 15:00UTC in #fedora-meeting-1 on
irc.freenode.net.

Is there any particular proposal you would like FESCo to decide on here? Is it just the one branch, or are you looking for a more general policy change?

The reason is, that the good one releneg deleted the branch, and then recreated it, as he was rebuked he cant.

So I'm searching for decision, to allow to delete branches. Obviously, not all, but the rules are on to you. TY!

The reason that branch deletion is forbidden is that it eventually results in the loss of those commits from the history (whenever the next time the git gc garbage collection runs). So, if we've ever used that branch for anything official (even merging it into master), those commits must not be lost.

However, I agree that it's too strict to refuse to delete an accidental push, if we inform rel-eng about it immediately and don't use the branch for anything. (I actually made such a mistake yesterday myself...)

I propose that we update the policy to be as follows:
"Branch-deletion is forbidden if git branch -a --contains <branchname> returns anything other than a single entry, which matches <branchname> and where this branch has never been supplied as a stream branch for a module."

If we want to be especially paranoid, we can also add to the policy "When deleting a branch, a tag must be created of the form deleted-branchname-YYYYMMDD at the current HEAD of that branch." This will prevent git's garbage collection from removing any of the commits that used to be on that branch, since they will still be referenced as a tag. Thus, if we ever miss a case where a commit unique to that branch was ever used for something, it will never be deleted. We will no longer see the branch as visible, but there will be a tag around that can be used to recreate it if needed.

I spoke with @pingou about this and he pointed out that dist-git does allow tag deletion, so the "especially paranoid" suggestion probably doesn't add much, but the --contains is a good suggestion I think.

Tag deletion is allowed, but I think we can make a reasonable policy that no one should ever delete a tag with the format described above. (Also, if such a tag was deleted accidentally, there's a grace period where we can use git reflog to find and restore it before the git gc makes it unrecoverable).

So, if we've ever used that branch for anything official (even merging it into master)

Nothing special is required for commits which were merged into master (or any other branch). No commits that are ancestors of a branch are deleted while that branch lives.

Branch-deletion is forbidden if git branch -a --contains <branchname> returns anything other than a single entry

So actually the rule needs to be based on the opposite of this, i.e. we need to consider commits which are not part of any other branch.

We care about commits, not branches. So git branch ... --contains <branchname> is not useful. Instead the check needs to be over all commits in that branch. Of course we can immediately filter out any commits which are on origin/master. We could for example loop over origin/master..origin/<branchname>, and do something like git branch -r --contains <commit> | grep -v origin/<branchname>. If we get empty, those commits are not referenced elsewhere and would be lost if we delete the branch.

For the commits that would be lost, we should check if there were any builds done.

So I see two possibilities:
1. Write a script that does the loop described above and queries koji if any commits that would be lost were used for builds.
2. A simpler proposal: do that same as in
https://pagure.io/releng/issue/7265#comment-499371 and instead of deleting, move branches to refs/archive.

I am for allowing deletion of branches if all the commits are part of another branch. Archiving these branches if nobody needs them only pollutes the repositories IMHO.

OK, revised proposal then:

We will allow the deletion of a branch if one of the following cases is true:

  • All commits in this branch since its branch-point exist in at least one other branch.
  • None of the commits in this branch have ever been used as part of a tagged build in Koji

I imagine that I could write a tool to prove the first one fairly simply. I think the second case would be harder, but I'll defer to @nirik or someone else with a better knowledge of the Koji API than I have.

So one reason why this was in place was to avoid complex and burdensome work added to releng folks.

If we have a clear, easy to run script that releng can use thats fine. If we make releng spend a lot of time investigating each case I think thats undue burden on them.

I am not sure if there's any way to always answer "None of the commits in this branch have ever been used as part of a tagged build in Koji", as in the past we could in some cases only see that a build was against HEAD or something instead of a actual commit. Is someone here committing to writing the tool?

OK, revised proposal then:
We will allow the deletion of a branch if one of the following cases is true:

All commits in this branch since its branch-point exist in at least one other branch.

+1, but it can be simplified to "All commits in this branch exist in at least one other branch".

None of the commits in this branch have ever been used as part of a tagged build in Koji

+1

I imagine that I could write a tool to prove the first one fairly simply. I think the second case would be harder, but I'll defer to @nirik or someone else with a better knowledge of the Koji API than I have.

If we have a tool for the first case, then we could also add automation for this. We can allow the second reason as well but also add a note that there is no tooling, yet, to detect this. Therefore someone who wants to take advantage of this needs to provide the tool first. ;-)

I'm fine with @sgallagh's proposal, but unless someone actually writes the tool for releng to run, we're just talking hypotheticals here...

None of the commits in this branch have ever been used as part of a tagged build in Koji

That doesn't really work. If I create a branch from master, a lot of previous commits were used as part of a tagged build in Koji and they are immediately in this branch.

The "All commits in this branch exist in at least one other branch" rule makes sure all koji builds are reachable in git. However if you want the extra possibility for branches that diverged but never built, it would have to be:

None of the commits only reachable from this branch have ever been used as part of a tagged build in Koji.

We discussed this in FESCo today:

AGREED: we'll wait for a volunteer to write the script to do the checks, and revisit the ticket when we have an implementation proposal (+5, 0, 0)

Metadata Update from @bowlofeggs:
- Issue untagged with: meeting

5 years ago

Metadata Update from @jsmith:
- Issue tagged with: meeting

5 years ago

Can we enable protection against this by default instead?

+1, I think it should be opt-out

EDIT: I don't agree with the "instead" part, i.e. if we get the script to delete existing branches, I think we should allow it to be used.

Metadata Update from @zbyszek:
- Issue untagged with: meeting

5 years ago

Metadata Update from @psabata:
- Issue tagged with: pending announcement

5 years ago

Metadata Update from @psabata:
- Issue untagged with: pending announcement

5 years ago

Metadata Update from @psabata:
- Issue close_status updated to: Accepted
- Issue status updated to: Closed (was: Open)

5 years ago

Metadata Update from @zbyszek:
- Issue status updated to: Open (was: Closed)

5 years ago

I don't think we can close this. We had a resolution to wait for an implementation, this hasn't happened yet. There was a proposal to make an opt-out block for branch creation, but this has neither received enough votes nor been implemented.

See #2018 for the "Prevent creating new branches by git push" part.

Waits for implementation.

Metadata Update from @churchyard:
- Issue tagged with: stalled

5 years ago

I'm going to close this due to inactivity.

Metadata Update from @bowlofeggs:
- Issue close_status updated to: Insufficient data
- Issue status updated to: Closed (was: Open)

5 years ago

Login to comment on this ticket.

Metadata