mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-27 09:12:41 +01:00
Make "metamta.differential.inline-patches" imply a reasonable byte limit, not just a line limit
Summary: Fixes T11748. This option currently implies a line limit (e.g., inline patches that are less than 100 lines long). This breaks down if a diff has a 10MB line, like a huge blob of JSON all on one line. For now, imply a reasonable byte limit (256 bytes per line). See T11767 for future work to make this and related options more cohesive. Test Plan: - With option at `1000`: sent Differential email, saw patches inlined. - With option at `10`: sent Differential email, saw patches dropped because of the byte limit. - `var_dump()`'d the actual limits and used `bin/worker execute --id ...` to sanity check that things were working properly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11748 Differential Revision: https://secure.phabricator.com/D16714
This commit is contained in:
parent
ac8e11359d
commit
dad17fb98a
2 changed files with 37 additions and 11 deletions
|
@ -70,6 +70,19 @@ final class PhabricatorDifferentialConfigOptions
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$inline_description = $this->deformat(
|
||||||
|
pht(<<<EOHELP
|
||||||
|
To include patches inline in email bodies, set this option to a positive
|
||||||
|
integer. Patches will be inlined if they are at most that many lines and at
|
||||||
|
most 256 times that many bytes.
|
||||||
|
|
||||||
|
For example, a value of 100 means "inline patches if they are at not more than
|
||||||
|
100 lines long and not more than 25,600 bytes large".
|
||||||
|
|
||||||
|
By default, patches are not inlined.
|
||||||
|
EOHELP
|
||||||
|
));
|
||||||
|
|
||||||
return array(
|
return array(
|
||||||
$this->newOption(
|
$this->newOption(
|
||||||
'differential.fields',
|
'differential.fields',
|
||||||
|
@ -254,13 +267,7 @@ final class PhabricatorDifferentialConfigOptions
|
||||||
'int',
|
'int',
|
||||||
0)
|
0)
|
||||||
->setSummary(pht('Inline patches in email, as body text.'))
|
->setSummary(pht('Inline patches in email, as body text.'))
|
||||||
->setDescription(
|
->setDescription($inline_description),
|
||||||
pht(
|
|
||||||
"To include patches inline in email bodies, set this to a ".
|
|
||||||
"positive integer. Patches will be inlined if they are at most ".
|
|
||||||
"that many lines. For instance, a value of 100 means 'inline ".
|
|
||||||
"patches if they are no longer than 100 lines'. By default, ".
|
|
||||||
"patches are not inlined.")),
|
|
||||||
$this->newOption(
|
$this->newOption(
|
||||||
'metamta.differential.patch-format',
|
'metamta.differential.patch-format',
|
||||||
'enum',
|
'enum',
|
||||||
|
|
|
@ -1264,11 +1264,30 @@ final class DifferentialTransactionEditor
|
||||||
$config_attach = PhabricatorEnv::getEnvConfig($config_key_attach);
|
$config_attach = PhabricatorEnv::getEnvConfig($config_key_attach);
|
||||||
|
|
||||||
if ($config_inline || $config_attach) {
|
if ($config_inline || $config_attach) {
|
||||||
$patch = $this->buildPatchForMail($diff);
|
$body_limit = PhabricatorEnv::getEnvConfig('metamta.email-body-limit');
|
||||||
$lines = substr_count($patch, "\n");
|
|
||||||
|
|
||||||
if ($config_inline && ($lines <= $config_inline)) {
|
$patch = $this->buildPatchForMail($diff);
|
||||||
|
if ($config_inline) {
|
||||||
|
$lines = substr_count($patch, "\n");
|
||||||
|
$bytes = strlen($patch);
|
||||||
|
|
||||||
|
// Limit the patch size to the smaller of 256 bytes per line or
|
||||||
|
// the mail body limit. This prevents degenerate behavior for patches
|
||||||
|
// with one line that is 10MB long. See T11748.
|
||||||
|
$byte_limits = array();
|
||||||
|
$byte_limits[] = (256 * $config_inline);
|
||||||
|
$byte_limits[] = $body_limit;
|
||||||
|
$byte_limit = min($byte_limits);
|
||||||
|
|
||||||
|
$lines_ok = ($lines <= $config_inline);
|
||||||
|
$bytes_ok = ($bytes <= $byte_limit);
|
||||||
|
|
||||||
|
if ($lines_ok && $bytes_ok) {
|
||||||
$this->appendChangeDetailsForMail($object, $diff, $patch, $body);
|
$this->appendChangeDetailsForMail($object, $diff, $patch, $body);
|
||||||
|
} else {
|
||||||
|
// TODO: Provide a helpful message about the patch being too
|
||||||
|
// large or lengthy here.
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if ($config_attach) {
|
if ($config_attach) {
|
||||||
|
|
Loading…
Reference in a new issue