nit: IInstructs -> Instructs

But this won't be possible to do, since we have parts that pass infinitepush bookmarks, and they need to be handled differently - we can't save them to the bundle. So we always need to handle some parts differently, don't we?

I think it's possible to send infinitepush bundle, which without changegroup part. For example, infinitepush bundle, that just sends infinitepush bookmarks. We don't want to raise exception in that case, do we?

Sure, some parts can be handled special, like how scratch branch parts are handled above. But this logic here opens the doors for migrating to not using custom parts, so it could be a completely server side extension (except for the backup push commands).

The storebundle logic only works if it has a changegroup part params available, so I assume it only worked if a changegroup part was sent. Is there a test for sending just bookmarks? If not, what would the repro be? Put a bookmark on master and push a scratch bookmark with no commits?

Actually, you comment is written twice.

There are tests that cover this case. The problem is that infinitepush backups don't use standard push codepath to avoid taking locks. That means that line "bundler.addparam("infinitepush", "True")" is skipped and new logic is not tested on infinitepush backups tests.

If you add this diff:

diff --git a/infinitepush/backupcommands.py b/infinitepush/backupcommands.py
--- a/infinitepush/backupcommands.py
+++ b/infinitepush/backupcommands.py
@@ -424,6 +424,7 @@
     wrapfunction(changegroup.cg2packer, 'deltaparent', _deltaparent)
     try:
         bundler = _createbundler(ui, repo, other)
+        bundler.addparam("infinitepush", "True")
         backup = False
         if outgoing and outgoing.missing:
             backup = True

and run test-infinitepush-backup.t, then you'll see test failures.

It seems that new logic is tested on normal (i.e. non pushbackup) tests, but please double check it.

BTW, why do you need it?

Also this code is not covered with tests. If we need it, can we cover it with tests?

Why do we need handleallparts? So we can store other parts, like treepacks and obsmarkers, without having to create custom part types for them. In the long run, allowing the server to take over handling each part will allow us to send normal part types to the server and the server can decide what to do with them.

The last patch in the stack adds a test that runs this code, though it doesn't set the config. The config isn't really ready to be used, but having it available to flip helps us figure out where new bundle parts aren't working with infinitepush.

Good catch. Will fix

No, sorry, I was interested in this code:

op.records.add(part.type, {
    'return': 1,
})

Why do you need to set return?

For backwards compatibility. If we ship this to the servers, and there are still some old hg clients out there, they would be pushing without the infinitepush=True flag.

I think we'll get around to remove the part handlers later, when we stop sending custom part types.