Patches as a separate specification?

[canadaduane]

Synopsis: Keep patches, but make a separate spec/protocol for them. Make core Braid simpler.

It seems like the "Patches: N" header and subsequent patch blocks add a fair bit of cognitive and implementation complexity to the Braid protocol. What would it look like if we made them an optional add-on instead of a core part of the spec?

Some thoughts:

• Each "Merge-Type" seems to have its own way of specifying multiple patches in the same request. "Patches: N" is just one more way.
• Could "Patches: N" be specific to a "Merge-Type: Braid" or "Merge-Type: sync9" perhaps?
• There's a lot of usefulness in the Braid protocol without patches, so it would be nice if we could keep cognitive complexity down while still providing a lot of value.
• Relative to the simplicity of the rest of the protocol, implementing patches in a library adds parsing complexity to a Braid protocol library. For reference, here is my PR to Seph's braid-protocol JS library that adds "Patches: N" headers: https://github.com/josephg/braid-protocol/pull/7 (Note that he had already implemented Version headers & body parsing)

[josephg]

Yeah I can see some merit in that. Which headers do you think would end up in the base protocol vs headers for patches? I'd love to see an example of how you're imagining it - though it might be pretty straightforward.

My preference would be to keep versions in the base protocol either way.

[toomim]

Patches are one of the core elements of synchronization, along with (IMO) Versioning, Merging, and Subscriptions, and it makes sense to discuss them in the main Braid-HTTP spec. HTTP today doesn't specify how to include a patch in a GET, PUT or POST request or response, and specifying how to do so is a core contribution in my view.

I see you referencing the "cognitive complexity" of patches, but the actual spec for the Patches: header is very simple to my eye. I believe the entire spec is just these four sentences, along with an example:

   One can send an update in a patch by setting the "Patches" header to
   a number, and then set the Message body to a sequence of that many
   patches, separated by blank lines:

   [ example ]

   In order to distinguish each patch within a Version, we need to know
   the length of each patch.  To know the length of each patch, it MUST
   include the following header:

         Content-Length: N

   This length determines where this patch ends, and next begins.

(Note, this presumes https://github.com/braid-org/braid-spec/pull/75)

I think your proposal is to move these paragraphs and example to a separate file, and ultimately submit it as a separate RFC, and find consensus on it separately. However, to my eye, it looks like overkill to move these 4 sentences and example into their own RFC. Maybe you didn't intend for this to be a separate RFC, but if it's going to be the same RFC, it should be in the same file.

As for implementation complexity, here's the Javascript code that supports patches in the current braidjs:

        if (parsed_headers.patches) {
            // Parse patches until we run out of patches to parse or get
            // all of them
            while (parsed_patches.length < parsed_headers.patches) {
                input_buffer = input_buffer.trimStart()
                var parsed_patch_headers = parse_headers()

                // Catch an error
                if (!parsed_patch_headers) {
                    console.debug("Failed to parse patch headers!")
                    return false
                }
                var patch_headers = parsed_patch_headers.headers
                var header_length = parsed_patch_headers.consumed_length
                var length = parseInt(patch_headers['content-length'])

                // Does our current buffer contain enough data that we
                // have the entire patch?
                if (input_buffer.length < header_length + length) {
                    console.debug("Buffer is too small to contain the rest of the patch.")
                    return false
                }

                // Content-range is of the form '<unit> <range>' e.g. 'json .index'
                var [unit, range] = patch_headers['content-range'].match(/(\S+) (.*)/).slice(1)
                var patch_content = input_buffer.substring(header_length, header_length + length)

                // We've got our patch!
                parsed_patches.push({range, unit, content: patch_content})
                input_buffer = input_buffer.substring(header_length + length + 2)
                console.debug('Successfully parsed a patch.',
                              `We now have ${parsed_patches.length}/${parsed_headers.patches}`)
            }
            if (input_buffer[0] === '\n' && input_buffer[1] === '\n') {
                console.error(input_buffer)
                throw 'parse error'
            }
            console.debug("Parsed all patches.")
            return true
        }

This code is a self-contained block within the parse_version_body() function. I personally find it to be quite simple. Do you find this to be complex?

And to respond to your specific bullet points:

• Each "Merge-Type" seems to have its own way of specifying multiple patches in the same request. "Patches: N" is just one more way.

There is value in separating the specification of merge-type and patch format. This allows synchronizers to specify that they can interoperate with one or the other independently.

• Could "Patches: N" be specific to a "Merge-Type: Braid" or "Merge-Type: sync9" perhaps?

That would prevent patches from being used in places where there is no Merge-Type, for instance, such as the single-writer chat app we are building, which has a central server and sends patch messages to append messages to the chat array.

• Relative to the simplicity of the rest of the protocol, implementing patches in a library adds parsing complexity to a Braid protocol library. For reference, here is my PR to Seph's braid-protocol JS library that adds "Patches: N" headers: https://github.com/josephg/braid-protocol/pull/7 (Note that he had already implemented Version headers & body parsing)

Perhaps the challenges you faced were specific to this implementation?

[toomim]

Finally, the Patches: N header is already optional for implementors. (Although there is work we want to do to clarify that! https://github.com/braid-org/braid-spec/issues/96 https://github.com/braid-org/braid-spec/issues/67 https://github.com/braid-org/braid-spec/issues/49) Moving it to a different file doesn't make it more optional for implementations, but it does separate it into an independent consensus process and eventually RFC.