Page MenuHomePhabricator

Unify email patch byte/line/time limit behaviors across Differential/Diffusion
Open, LowPublic

Description

Inlining and attaching patches in email is currently controlled by several different configuration options:

  • metamta.diffusion.attach-patches (bool)
  • metamta.diffusion.inline-patches (int)
  • metamta.diffusion.byte-limit (int)
  • metamta.diffusion.time-limit (int)
  • metamta.differential.attach-patches (bool)
  • metamta.differential.inline-patches (int)
  • metamta.email-body-limit (int, indirect)

Differential had no byte or time limit at all until T11748, which added an implicit byte limit based on the line limit to inlined patches to deal with patches with a single 100MB line.

These options should ideally be collapsed to a single inline-patches (int) and attach-patches (int) which control behavior for both Differential and Diffusion, and describe a line limit which implies a reasonable byte and time limit (e.g., if the value is set to 100, the line limit is 100; the byte limit is 100 * 128; the time limit is 15s + (10ms * 100) or similar).

We can almost certainly collapse this to four options (inline/attach for Differential/Diffusion). It's possible that some installs feel strongly about Differential and Diffusion having different patch behavior and that we can't collapse them down to two (inline/attach for all applications), but it seems like most installs are reasonably likely to want patches in both applications if they want them in one.


Patch generation and mail itself has some adjacent issues:

  • We generate patches even if they are so large that they can not possibly be included in email. Commits which generate patches larger than 2GB probably fail to publish by hitting PHP's string limits. Instead, we should pass an upper limit (e.g., the email body limit) to patch generation and abort after reading enough data that we're sure we will not be able to attach or inline the patch.
  • When we discard inline patches or attached patches (after D18598) we do not include a message about it in the body. We should include a note that data was dropped because it hit limits.
  • metamta.email-body-limit currently applies to ONLY the text body. The HTML body can still be arbitrarily large, as can the headers and attachments. For bodies and attachments, we should probably: drop the HTML body if the text body gets truncated or the entire HTML body does not fit (truncating HTML is hard), and start dropping attachments if they won't fit in the remaining space. When we drop the HTML body or attachments, we should include a note to this effect. We should also probably drop the HTML body first, since the text body is fairly good and it's probably better to get plain text mail with attachments than HTML mail with no attachments.
  • I'm less sure what to do about headers, although perhaps we count them up first and then limit the text body based on the header size.

Event Timeline

epriestley renamed this task from Unify patch byte/line limit behaviors to Unify email patch byte/line/time limit behaviors across Differential/Diffusion.Oct 17 2016, 10:10 PM
epriestley added a project: Differential.

An additional inconsistency is that the diffusion.byte-limit applies to attached patches, but the differential.byte-limit does not. There's some vague argument for this behavior (you "shouldn't" be reviewing patches which are so enormous that they can not be emailed) but consistent behavior is probably better.

The metamta.email-body-limit option also applies too late to matter, but we could treat that as an upper bound on the implied/computed byte limit for inlined patches. We won't be able to deliver a larger inline patch even if we build it.

Differential also currently does not substitute a diagnostic message when declining to inline a patch: it should. ("Patch is too big to inline because limit X is set to Y." or similar.)

epriestley edited projects, added Diffusion (v3); removed Diffusion.