For some reason, the merge of rETMH2512bac8def0 closed T123823 even though there's nothing about the patch that reasonably indicates that that should happen.
A mere mention of a task shouldn't be sufficient to close it; for that matter, even a "Bug:" footer doesn't necessarily indicate that the patch completely resolves a task such that nothing further is needed.
Probably the fix T123823 text on the commit message, when it landed in diffusion triggered the close. This seems the usual workflow on diffusion and github. See also T116528: Prevent imported commits from upstream phabricator to close random tasks
Yes, that is the reason. "Fix" or fixes" closes tasks automaticaly. That's autoclose. So there are two possibilitys:
With our current practices, I'd expect there to be a "Fixes: Txxxx" footer that works like "Bug" plus closes the task when merged if we wanted to do this. We basically never mention a task being fixed in running text in the comment (we use the "Bug" footer), so I wouldn't be surprised if every example of this auto-closing turns out to be a false positive. Is there a way to search for these auto-closings to check that?
That's not to say we might not change our practices when we move away from Gerrit, but in the current setup it's completely unexpected.
Not a duplicate of T55387. That's asking for a "Fixes" footer of some sort, this is complaining that Phab will close a task if you happen to say "doesn't fix Txxxx" or "until we fix Txxxx" in the message body (among other things).
The issues you raise here have already been addressed in T55387 by the powers that be.
I don't think they were (unless you count "we don't care" as addressing). The two issues that were brought up there were that it is easy to unintentionally close tasks, especially given the flexibility Phabricator applies to the 'fixes' keyword (ex. T55387#1312852) and that due to the same flexibility, this is hopelessly Phabricator-specific and we our submit message special instructions to have a format that can be easily used by any tool.
We can customize which words trigger the autoclose behavior but we cannot change the general syntax. So we could make it so that "fix Txxx" doesn't trigger autoclose, but we cannot change it to "Fix: Txxx" and we cannot force the trigger to be in the footer of the commit message.
Now we're moving to Differential, we could embrass this syntax to be able to write the task information as a natural language in the commit.
Lorem ipsum dolor The component lorem weren't efficient as ipsum dolor sit amet nunc abuntur. This change eum solet recusabo id, mollis audiam neglegentur ei mei. This fixes Txxx.
A complementary Maniphest Tasks: Txxx will be added by default by Phabricator to the commit message.
So we have the automatic metadata add and we also have an autoclose feature. With the benefit to have plain English task information in the text.
I have seen examples of this before where a bug gets auto-closed but the expected fixes didn't work and then the ticket was already closed without having had a "verify" step and had to be found and reopened. Potentially this encourages to ignore verifying after a _claim_ that something is fixed . In other bug trackers there are sometimes 2 steps in the workflow because of this, first "resolved" where somebody claims they fixed it, but the last step after that is "verified" where the bug reporter or somebody else confirms it's actually fixed.
It's still possible to avoid the autoclose using Ref. Txxxx instead of Fixes Txxxxx. We should invite the commit authors to consider the workflow and how the change will be babysit or is really resolved by a commit, to use Fixes with care.
For example, when a config change is merged during SWAT, we already have a workflow warranting the verification.
I don't know if such a two-step resolution status is overkill here but phabricator definitely supports such a setup. The list of statuses is configurable and multiple arbitrary 'open' and 'closed' states can be defined.
@Dereckson You can use "this solves Txxx" too, that's not a problem. I think we should just update the commit message guidelines, so we don't need to do complicated things.
No, unfortunately Phabricator does not allow natural language in the commit message. That is the specific problem this task was opened for as Phabricator read the sentence "Temporarily move this back while we fix T123823" as "This commit fixes T123823".
So developers cannot use the syntax they know if they used Git since August 2005 (or any other Unix utility that is inspired by more than 40 years of best practice), and they cannot use the language spoken by about a billion people, but they must use a syntax that is only useful for them if they want to work for WMF, Facebook or Bloomberg.
For everybody else, it is just another tiny bit of frustration when contributing to MediaWiki.
No, that syntax is mandatory in a message body primarily intended to be read by humans.
That argument is no longer valid. I already updated our phabricator settings and removed 'fix' from the list of prefix words. So now 'while we fix Tnnn' will not have any special meaning to phabricator.