+1

There's a SOP for removing problematic commits, I believe. We've had to do this before. If I remember correctly, we move the branches to a ref not fetched by default and clean the main refs.

If I remember correctly, we do not do this unless there is a specific legal need for it (such as the tarball committed included content that was not legally permissible to redistribute). Otherwise, changing the commit history causes problems for us, because the commits used to build official packages in Fedora will have changed and be unreachable.

So, unless this is being done for legal reasons, I'm -1 to this change.

We did it for the Java recently, though. That was for a technical reason and not a legal one.

We did it for blender as well. The commit history is not lost; it's just stashed away.

Right, this is not messing up history. The ref is moved to an archive one thats not cloned by default, but it's still there/reachable.

But the real question is:

When should releng do this? On any request? only after fesco approval? With some critera (ie, must be larger than ABC)?

I think any reasonable request is fine. If someone checked in binary artifacts into Git, it's almost always bad in the long-term, so dealing with it sooner rather than later is a good idea.

Ah right, I'd forgotten we figured out a way to archive the "bad" commits. (Which is ironic, because IIRC it was due to the git-fsck issue I had discovered.)

OK, with that in mind, I think I can be a +1 on this.

For a future policy, I propose:

Releng is empowered to follow its branch rewrite policy if one of the following conditions are met:

1. The checked-in artifact exceeds 512 KiB in size and exists in (or is replaced on the modified history with an entry in) the lookaside cache with a matching hash.
2. The checked-in artifact includes content whose redistribution status is either impermissible or indeterminate.
3. The checked-in artifact is wholly unrelated to the package (e.g. accidentally checking in temporary files or an IDE configuration).

For any other request to rewrite history, a FESCo ticket must be filed to decide.

Accidentally checking in temporary files or an IDE configuration alone does not sound like a good enough reason to me.

I'm with Miro here, I think point 3 is a bit superfluous. OTOH, this just about approval, and it does not mean that people will ask for the removal to happen. So I'm fine with the point being there or not.

+1 for removal following the SOP

Thank you for your responses.

There's a SOP for removing problematic commits, I believe. We've had to do this before. If I remember correctly, we move the branches to a ref not fetched by default and clean the main refs.

Yeah, this one, but it's not ideal. I will update it when we finish processing.

Releng is empowered to follow its branch rewrite policy if one of the following conditions are met:

1) The checked-in artifact exceeds 512 KiB in size and exists in (or is replaced on the modified history with an entry in) the lookaside cache with a matching hash.
2) The checked-in artifact includes content whose redistribution status is either impermissible or indeterminate.
3) The checked-in artifact is wholly unrelated to the package (e.g. accidentally checking in temporary files or an IDE configuration).

Thanks for grouping those conditions. I will add them to SOP documentation. So, if the request has one of those reasons, Releng can proceed with SOP without authorization from FESCo? Or those are the conditions when FESCO might approve such changes?

Please hold. That was a proposal by @sgallagh I don't know that all of fesco agrees with it. Please don't use it/add to policy until this ticket is fully completed?

I don't think points 2 or 3 make much sense.

How about;

If the artifact is > 1MB and is either duplicated in the lookaside cache or not ever used in a build?

Could we have a server-side hook that rejects pushes with files exceeding 1 MB (or even less)? That way, no future cases like this could exist and the remaining cases could be dealt with all at once, with no need for a remedial policy, only a preventive one, enforced by code.

Please hold. That was a proposal by @sgallagh I don't know that all of fesco agrees with it. Please don't use it/add to policy until this ticket is fully completed?

I don't think points 2 or 3 make much sense.

I can see Miro's argument for point 3 and I'm fine with dropping that one if it's the only sticking point, but point 2 directly addresses the "we committed proprietary code to public dist-git" problem. I just tried to make it generic enough to apply to any of a wide variety of cases where something is non-distributable. If I made it too vague, we can workshop the phrasing.

How about;

If the artifact is > 1MB and is either duplicated in the lookaside cache or not ever used in a build?

Could we have a server-side hook that rejects pushes with files exceeding 1 MB (or even less)? That way, no future cases like this could exist and the remaining cases could be dealt with all at once, with no need for a remedial policy, only a preventive one, enforced by code.

centpkg sets up a client-side hook for this on CentOS Stream; we could look at having something similar done for fedpkg. I think there's a reason why it's not done on server-side, but I would have to really dig to figure out what it was.

Please hold. That was a proposal by @sgallagh I don't know that all of fesco agrees with it. Please don't use it/add to policy until this ticket is fully completed?

Sure, I'm not make any changes yet, just suggested that if it will be approved I will add this part to documentation.

centpkg sets up a client-side hook for this on CentOS Stream; we could look at having something similar done for fedpkg. I think there's a reason why it's not done on server-side, but I would have to really dig to figure out what it was.

It a good option. As a starting point we can just make an notification like "are you sure you want to push file that large, if not type I_didnt_want_to_push or just n"

I would be +1 to granting releng a blanket fesco approval for cases 1. and 2. outlined in https://pagure.io/fesco/issue/3496#comment-992081 (though 512 KiB seems rather low, maybe "file or files exceeding 1 MiB in total"?)

I'm +1 on releng blanket fesco approval.

As for pre-commit hook, what about putting a filter for binary content, instead of the size?

FESCo documentation (Ticket Votes section) is saying:

At the end of one week, if a proposal has at least 3 votes of +1 and no votes of -1 in the ticket, it is approved and may proceed with implementation.

So may I consider it as approval to proceed with actions?

As far as I can see, the original proposal still has a "-1 but can be +1 with ..." from @sgallagh so this needs to be discussed during the next meeting.

As for @sgallagh 's proposal for a blanket approval, there were "I could agree to this but either point 2. and / or 3. don't make sense to me" so this needs more bikeshedding too before it can receive a final vote (without "I could agree to this, if only it were different").

I'll tag it with "meeting" for next week. For now, I don't think either the original request nor the policy amendment are approved.

Yes, there's enough discussion here that we at least need a revised proposal to vote on. I'll offer another one that we can discuss between now and then.

It sounds like people are neutral-or-against my point 3, so I'm going to just drop it and leave that situation at the status quo (which means that someone wishing to remove content for this reason would need to file a FESCo ticket for consideration).

Proposal:

Releng is empowered to follow its branch rewrite policy if one of the following conditions are met:

1. The checked-in artifact exceeds 1 MiB in size and exists in (or is replaced on the modified history with an entry in) the lookaside cache with a matching hash.
2. The checked-in artifact includes content whose redistribution status is either impermissible or indeterminate (including, but not limited to: an unacceptable license or patent-encumbered content)

Revision of the git history for any other reason must be submitted as a FESCo ticket for consideration.

-1 to the proposal as-is.

I don't understand the restriction that the file must exist in the lookaside cache after the rewrite on top of the >1MiB size threshold. The point 1. as written does not cover accidentally committed large files period, i.e. files that should be neither tracked by git nor in the lookaside cache.

For example, I'm thinking about accidentally committed *.src.rpm files. Those definitely don't belong in git, but they also definitely don't belong in the lookaside cache.

I would replace point 1 with something like: "The file(s) exceed 1 MiB in size in total and are either 1) unused or 2) exist as (or are moved to) equivalent file(s) in the lookaside cache."

Also unsure why you used "artifact" instead of "file", using fancy words like that just makes it harder to understand. (It's also misspelled, it's "artefact", not "artifact".)

Also unsure why you used "artifact" instead of "file", using fancy words like that just makes it harder to understand. (It's also misspelled, it's "artefact", not "artifact".)

Point of fact: artifact and artefact are both valid spellings. The former is archaic in British English, the latter is archaic in American English.

-1 to the proposal as-is.

I don't understand the restriction that the file must exist in the lookaside cache after the rewrite on top of the >1MiB size threshold. The point 1. as written does not cover accidentally committed large files period, i.e. files that should be neither tracked by git nor in the lookaside cache.

For example, I'm thinking about accidentally committed *.src.rpm files. Those definitely don't belong in git, but they also definitely don't belong in the lookaside cache.

I would replace point 1 with something like: "The file(s) exceed 1 MiB in size in total and are either 1) unused or 2) exist as (or are moved to) equivalent file(s) in the lookaside cache."

You're correct; I didn't account for the situation where it's unused. I actually originally intended that to be covered by point 3), but I actually managed to drop that aspect in rewrites before I published my first proposal. Oops.

Also unsure why you used "artifact" instead of "file", using fancy words like that just makes it harder to understand. (It's also misspelled, it's "artefact", not "artifact".)

I suppose "file" is probably simpler here. (Also, that's a dialect difference; it really is "artifact" in US English).

Revised Proposal

Releng is empowered to follow its branch rewrite policy if one of the following conditions are met:

1. The checked-in file exceeds 1 MiB in size and is either a) unused or b) exists in (or is moved to) the lookaside cache.
2. The checked-in file includes content whose redistribution status is either impermissible or indeterminate (including, but not limited to: an unacceptable license or patent-encumbered content)

Revision of the git history for any other reason must be submitted as a FESCo ticket for consideration.

+1 for the latest "revised proposal"

I don't get to vote, but the latest proposal seems reasonable to me.

+1, looks good to me now, thanks! (you replaced "artifact" with "file" in point 1 but left it in point 2, but that's probably fine. :))

+1, looks good to me now, thanks! (you replaced "artifact" with "file" in point 1 but left it in point 2, but that's probably fine. :))

Retroactively fixed now. That was an oversight and I can reasonably assume that people voting knew it was meant to be "file".

I'm fine with 1.

Should 2 perhaps be split out from this? Using this process for such things doesn't actually solve the issue. It makes it more obscure, but anyone can still clone the repo and look in it to find the content, it just doesn't get checked out by default. For that we may want to consider policy being to force push history and break everything. ;(

+1 to the revised proposal

We're at +4 for the revised proposal, but I take @kevin's comment as -1.
Let's discuss this today.

Let's hope that the meeting today happens. This will be on the agenda.

+1

We discussed this during the FESCo meeting today:
AGREED: Proposal in https://pagure.io/fesco/issue/3496#comment-993193 is APPROVED (+6, 0, -1)

@amedvede can you handle this now?

@amedvede can you handle this now?

Yeah, thanks for approving. I'll proceed with actions.