Add amend to stateful_repo_ops #1537
Conversation
| # TODO: this should have failed earlier on the model step. the model to | ||
| # need to fix the model here as well |
There was a problem hiding this comment.
making sure people know this comment is by a human by making it incoherent :)
I think I mean that the incorrect branches should never be expired becuase that's all downstream of expiring the correct snapshots. The bug I was seeing was that we were overeager in expriing snapshots, which was then leading to overeager branch/tag expiration. This comment is overspecific to that bug. I'll remove it. Not really sure what I meant with the last sentence.
There was a problem hiding this comment.
I have removed this. I think i meant that I was surprised this wasn't catching the excessive expiration for v1 spec, delete_expired tags true case the model is not doing quite the right thing in these lines:
expired_snaps = set()
branch_pointees = set(self.branch_heads.values())
tag_pointees = set(map(operator.attrgetter("commit_id"), self.tags.values()))
for snap in self.commits.values():
if (
snap.written_at < older_than
and snap.parent_id is not None
and (delete_expired_tags or snap.id not in tag_pointees)
and (
(delete_expired_branches and self.branch_heads["main"] != snap.id)
or snap.id not in branch_pointees
)
):
expired_snaps.add(snap.id)but now im not 100% sure what the right expiration behavior is. If there is a branch that has the commit as it's head. should it not be expired?
of if there is a expired snapshot that is the head, but there is other history for that branch what should happen?
| has_tag = any(tag.commit_id == old_head for tag in self.tags.values()) | ||
| # if none of the above then delete the commit | ||
| if not is_other_branch_head and not is_parent and not has_tag: | ||
| self.commits.pop(old_head) |
There was a problem hiding this comment.
this pop was wrong; amending keep the snapshot ID in the repo object file, so we shouldn't pop from commits.
| def amend(self, snap: SnapshotInfo) -> None: | ||
| """Amend the HEAD commit.""" | ||
| # this is simpe because we aren't modeling the branch as a list of commits | ||
| self.commit(snap) |
There was a problem hiding this comment.
very simple now since we only model branch heads
| self.commits[new_commit_id] = CommitModel.from_snapshot_and_store( | ||
| snap, copy.deepcopy(self.store) | ||
| ) | ||
| self.commits[new_commit_id].parent_id = parent_id |
There was a problem hiding this comment.
the error here is that we forgot to update self.ondisk_snaps I've chosen to just call self.commit to avoid such errors in the future.
| assert actual.written_at == expected.written_at | ||
|
|
||
| @invariant() | ||
| def checks(self) -> None: |
There was a problem hiding this comment.
making the output less verbose.
This required checking if a commit was anywhere in any branches history so I also had to update to storing a full commit history for each branch. I made a branchModel object to make it a bit nicer to do
branch.headinstead of grabbing the last item in a list.I also added some more checks to the expiry that would have caught issues #1520 and #1534
This addresses most, but not all of @dcherian review comments from #1515 but I think this is a nice standalone change. I can work on deeper paramaterization of the model and spec separately if thats ok (ref #1515 (comment))
These tests currently fail due to #1534 and #1520
This is the first half of #1513