A proper commit consists of a one-line title, a newline, then a summary. Phabricator will always add a "Summary:" line above the summary, which we feel is redundant.
There should be a way to instruct DifferentialSummaryField.php to automatically pick out the summary instead of requiring it explicitly.
Description
Description
Event Timeline
Comment Actions
We're unlikely to support this. We do try to parse messages without "Summary:" (i.e., you shouldn't have to add it) but won't write messages out without it.
Fundamentally, using Phabricator changes commit messages from human-readable prose into a structured record (we're not alone in this, and most code review software which interacts with Git does this to some degree, including Git itself with features like --signoff). This implies some tradeoffs which make the messages work less well as human-readable prose. Git and other VCS tools do not provide reasonable alternatives for storing structured metadata (where "reasonable" means both "it exists" and "normal users understand how to interact with it").
The notion that a "proper commit message" should be formatted in a specific way that optimizes its value as human-readable prose is a reasonable one when using Git without tooling on top of it, but not as valid when a tool like Gerrit or Phabricator is used on top of Git. In these cases, a proper commit message should expose its metadata in an unambiguously machine-parseable way first, and serve human consumers second.
Declining to write "Summary:" would work fine in most cases, but occasionally introduce parsing ambiguity where none previously existed. For example, the summary Reviewers: should accept names in uppercase would become a "Reviewers" field if written without a "Summary" header. T6050 is an example of an issue caused by parsing ambiguity and this change would tend to increase parsing ambiguity for a small improvement in aesthetics. (Messages have various parsing ambiguities today, but there aren't many clear steps we could take to reduce ambiguity without also reducing usability.)
We might make some effort to handle these cases better (e.g., do parsing in multiple phases) but messages like this are fundamentally ambiguous:
Let reviewers be specified in uppercase Reviewers: ...
This might be:
Let reviewers be specified in uppercase Reviewers: should allow usernames to be entered in any case. Currently, the field is case-sensitive.
Or it might be:
Let reviewers be specified in uppercase Reviewers: alincoln, htaft
And we have no way to know.
Comment Actions
I see your point.
Am I right then to assume then that given that the summary is non-ambigious, simply leaving out "Summary:" should work?
Since we (our team of phabricator-users) will realistically never get everyone else to use Differential, following a common commit message format is unfortunately more important than avoiding these ambigious situations, so I'm willing to accept the risks and keep a local patch.
How can I get DifferentialSummaryField.php to skip the header when rendering (I'm assuming that's the correct term for writing field-id + value) the field?
Is there some abstract function that I might overload in order to render the summary different only when it's passing through arc land?
Comment Actions
You'd have to modify the code; add a special case for "Summary" here, like we have an existing special case that prevents titles from being written as "Title: the title".
Comment Actions
@metellius did you succeed patching phabricator? If so, how did you do it exactly? I tried with
diff --git a/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php index 288a8c4..cc0f96e 100644 --- a/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php @@ -110,6 +110,8 @@ protected function execute(ConduitAPIRequest $request) { $key_title = id(new DifferentialTitleField())->getFieldKey(); $default_title = DifferentialTitleField::getDefaultTitle(); + $key_summary = id(new DifferentialSummaryField())->getFieldKey(); + $commit_message = array(); foreach ($field_list->getFields() as $key => $field) { $handles = array_select_keys($all_handles, $phids[$key]); @@ -127,17 +129,18 @@ protected function execute(ConduitAPIRequest $request) { } $is_title = ($key == $key_title); + $is_summary = ($key == $key_summary); if (!strlen($value)) { if ($is_title) { $commit_message[] = $default_title; } else { - if ($is_edit && $field->shouldAppearInCommitMessageTemplate()) { + if (!$is_summary && $is_edit && $field->shouldAppearInCommitMessageTemplate()) { $commit_message[] = $label.': '; } } } else { - if ($is_title) { + if ($is_title || $is_summary) { $commit_message[] = $value; } else { $value = str_replace(
but then arc diff would complain that
Commit message has errors:
- Invalid or missing field "Test Plan": You must provide a test plan.
Describe the actions you performed to verify the behavior of this
change.
You must resolve these errors to continue.what else do I need to patch to get rid of this superfluous Summary:-key in commit messages?
Comment Actions
It's been a long time since I looked at our custom patches, but I do believe this is the one that did it for us:
diff --git a/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php index 288a8c4..266dff0 100644 --- a/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php @@ -144,7 +144,9 @@ final class DifferentialGetCommitMessageConduitAPIMethod array("\r\n", "\r"), array("\n", "\n"), $value); - if (strpos($value, "\n") !== false || substr($value, 0, 2) === ' ') { + if (!$field->shouldIncludeLabelInCommitMessage()) { + $commit_message[] = "{$value}"; + } else if (strpos($value, "\n") !== false || substr($value, 0, 2) === ' ') { $commit_message[] = "{$label}:\n{$value}"; } else { $commit_message[] = "{$label}: {$value}"; diff --git a/src/applications/differential/customfield/DifferentialCustomField.php b/src/applications/differential/customfield/DifferentialCustomField.php index b4ef47c..17a0473 100644 --- a/src/applications/differential/customfield/DifferentialCustomField.php +++ b/src/applications/differential/customfield/DifferentialCustomField.php @@ -90,6 +90,9 @@ abstract class DifferentialCustomField return false; } + public function shouldIncludeLabelInCommitMessage() { + return true; + } /** * @task commitmessage diff --git a/src/applications/differential/customfield/DifferentialReviewedByField.php b/src/applications/differential/customfield/DifferentialReviewedByField.php index 6949bbb..e3b6980 100644 --- a/src/applications/differential/customfield/DifferentialReviewedByField.php +++ b/src/applications/differential/customfield/DifferentialReviewedByField.php @@ -15,6 +15,10 @@ final class DifferentialReviewedByField return pht('Reviewed-by'); } + public function shouldIncludeLabelInCommitMessage() { + return false; + } + public function getFieldDescription() { return pht('Records accepting reviewers in the durable message.'); } diff --git a/src/applications/differential/customfield/DifferentialSummaryField.php b/src/applications/differential/customfield/DifferentialSummaryField.php index 5052e23..6dce84d 100644 --- a/src/applications/differential/customfield/DifferentialSummaryField.php +++ b/src/applications/differential/customfield/DifferentialSummaryField.php @@ -138,6 +138,10 @@ final class DifferentialSummaryField return true; } + public function shouldIncludeLabelInCommitMessage() { + return false; + } + public function shouldOverwriteWhenCommitMessageIsEdited() { return true; }
Comment Actions
the part in the reviewed-by field is in relation to another commit that makes it look like "Reviewed-by: ..." instead of "Reviewed by\n..."
Comment Actions
Thanks! so the patch above is to be used on its own (i.e. without the one I posted)?
the part in the reviewed-by field is in relation to another commit that makes it look like "Reviewed-by: ..." instead of "Reviewed by\n..."
yeah, that violation of Git-convention has been annoying me as well, as well as generally using whitespaces rather than dashes in trailer-keys which confuses/breaks Git tooling :-(
Comment Actions
@metellius I tried your patch, but I hit the same issue; arc diff reports an Invalid or missing field "Test Plan"; I guess I need to tweak something in the commit msg parser... you didn't need that?
Comment Actions
I can't see any patches doing that, no. Maybe there's a setting somewhere, I do remember there being something about test plan in the differential config?