(In reply to comment #0)

However, it doesn't mark it as Resolved: Fixed when it's merged. I think
this is because tagging it with a Bug number does not always mean the bug is
always fixed.

I always assumed not marking the bug as resolved/fixed was a feature to prevent false positives. Determining whether a bug is resolved by a particular changeset can be difficult, even for experienced humans.

"Bug: 12345" would remain available, for when the patch set was just one step
towards fixing the bug.

I can't say I'm a huge fan of the idea of having two very similar keywords. I think it will create confusion. But meh.

(In reply to comment #1)

(In reply to comment #0)

However, it doesn't mark it as Resolved: Fixed when it's merged. I think
this is because tagging it with a Bug number does not always mean the bug is
always fixed.

I always assumed not marking the bug as resolved/fixed was a feature to
prevent
false positives. Determining whether a bug is resolved by a particular
changeset can be difficult, even for experienced humans.

I fully agree.

"Bug: 12345" would remain available, for when the patch set was just one step
towards fixing the bug.

I can't say I'm a huge fan of the idea of having two very similar keywords. I
think it will create confusion.

I fully agree here as well.

(In reply to comment #1)

Determining whether a bug is resolved by a particular
changeset can be difficult, even for experienced humans.

I fully agree.

And that's why I'm suggesting humans explicitly write "Fixes-Bug" in their commit message, when applicable.

I can't say I'm a huge fan of the idea of having two very similar keywords. I
think it will create confusion.

I fully agree here as well.

I don't think it's that confusing. We're talking about two keywords, and the one that marks it fixed has the word "Fixes" in the message.

However, we could rename Bug: to Related-Bug: to make it even clearer.

(In reply to comment #3)

And that's why I'm suggesting humans explicitly write "Fixes-Bug" in their
commit message, when applicable.

Right. I understand the function and utility of the proposed keyword.

I don't think it's that confusing. We're talking about two keywords, and the
one that marks it fixed has the word "Fixes" in the message.

I see this as a design issue, though. With the introduction of a second bug-related keyword, the immediate question becomes: which do I use? Do I use both? Do I use just one? What if it fixes a bug, but for some reason I don't want the bug to be auto-marked as fixed?

And this design issue reverberates onto other people who might be trying to use the keyword(s). Does everyone now have to account for both when running reports?

However, we could rename Bug: to Related-Bug: to make it even clearer.

Sure, a lot of things could be done. I'm just not sure what issue is trying to be solved here. I don't think there's any significant efficiency gain here. People really interested in auto-marked as fixed could surely set up a script of their own that stalks their merged commits, though doing so would still be a bad idea for the reasons mentioned above, in my opinion.

The primary use case is for bug reporters, not developers: gerrit-bot being able to tell you that code that /fixes/ the bug has been pushed to gerrit (or merged) is much more interesting that "some related code that may be a million miles from fixing this bug might have changed state, maybe".

(In reply to comment #4)

I see this as a design issue, though. With the introduction of a second
bug-related keyword, the immediate question becomes: which do I use? Do I use
both? Do I use just one? What if it fixes a bug, but for some reason I don't
want the bug to be auto-marked as fixed?

You use Fixes-Bug if it it fixes the bug. You use Bug otherwise. I can't think of any scenario you would tag it Fixes-Bug but not want it marked fixed. In that unlikely case, you would use Bug.

And this design issue reverberates onto other people who might be trying to
use the keyword(s). Does everyone now have to account for both when running
reports?

I don't think there are any reports on bug status built on parsing commit message.

Sure, a lot of things could be done. I'm just not sure what issue is trying
to be solved here. I don't think there's any significant efficiency gain here.

I disagree. I frequently have to go and manually change fixed bugs from PATCH_TO_REVIEW to FIXED, both things I have fixed and things other people have fixed. I'm obviously not the only one.

People really interested in auto-marked as fixed could surely set up a script
of their own that stalks their merged commits, though doing so would still
be a bad idea for the reasons mentioned above, in my opinion.

It makes no sense for everyone to do it separately.

This is really very common functionality for integrations between bug trackers and version control.

(In reply to comment #5)

The primary use case is for bug reporters, not developers: gerrit-bot being
able to tell you that code that /fixes/ the bug has been pushed to gerrit (or
merged) is much more interesting that "some related code that may be a
million miles from fixing this bug might have changed state, maybe".

And you want to deprive humans the opportunity to enlighten (and excite!) the masses with the good news? ;-)

(In reply to comment #6)

This is really very common functionality for integrations between bug
trackers and version control.

[[WP:OKAY]]. I do some peripheral work (not much in Gerrit, but I'm active in Bugzilla and somehow I'm now maintaining Gerrit reports) and I think this change wouldn't be a great idea to implement. However, I don't think it's worth arguing about either way: I don't like the idea and I explained why. I'll leave it at that. :-)

• Bug 39402 has been marked as a duplicate of this bug. ***

a) +1.

b) Opportunity for bikeshedding: OpenStack uses "Closes-Bug", "Partial-Bug" and "Related-Bug" (cf. https://wiki.openstack.org/wiki/GitCommitMessages#Including_external_references). Personally, I prefer "Fixes-Bug" because it is unambiguous how the bug should be closed ("FIXED").

Glancing over https://gerrit.googlesource.com/plugins/hooks-bugzilla/+/master/src/main/resources/Documentation/config.md and the mention of "Fixes-Issue" there, this seems to be fairly straightforward to set up. Can someone give an estimate what is actually needed to implement this?

Maybe instead of closing the task it should move it to the "Done" / "For sign-off" column on the workboard of the main(?!) project? Then QA/Product can do manual approval, as we do in e.g. Editing and I believe is also done in Multimedia, Mobile Apps and other teams?

Maniphest already closes tasks when the commit says "Fixes Tnnn" or "Resolves Tnnn" and you can add a reference to a task by mentioning the task in your commit like this: "Refs T###" and maniphest will add a comment to the task like this: "committer added a commit ***"

So should we stop doing "Bug: T1234" and instead do "Fixes T1234" in gerrit?

note to the above^ that's only if that repo is currently in diffusion I believe

Aha. Oh well.

Is there any reason not to include all of our repos in diffusion?

To me diffusion integration is at least 20% of the value of phabricator.

In T55387#786264, @mmodell wrote:

Is there any reason not to include all of our repos in diffusion?

That's Gitblit-Deprecate for the read-only mirrors (T616), and #Code-Review for the useful stuff (getting notified that a patch was added only once the patch is merged and synced downstream to the read-only mirror in Diffusion wouldn't be valuable).

@Chad and I plan to drive the first forward relatively quickly in the coming few weeks, but #Code-Review is going to take a bunch of time to get right.

In T55387#786222, @Jdforrester-WMF wrote:

Maybe instead of closing the task it should move it to the "Done" / "For sign-off" column on the workboard of the main(?!) project? Then QA/Product can do manual approval, as we do in e.g. Editing and I believe is also done in Multimedia, Mobile Apps and other teams?

This task is about a "Fixes-Bug:" footer. It means: "This change fixes this bug."

In T55387#786536, @scfc wrote:

In T55387#786222, @Jdforrester-WMF wrote:

Maybe instead of closing the task it should move it to the "Done" / "For sign-off" column on the workboard of the main(?!) project? Then QA/Product can do manual approval, as we do in e.g. Editing and I believe is also done in Multimedia, Mobile Apps and other teams?

This task is about a "Fixes-Bug:" footer. It means: "This change fixes this bug."

So we should mark as "Declined" given we now have Tasks and don't have Bugs? Maybe we should think about the ambition on terms of outcomes. :-)

In T55387#786545, @Jdforrester-WMF wrote:

[…]
So we should mark as "Declined" given we now have Tasks and don't have Bugs? Maybe we should think about the ambition on terms of outcomes. :-)

The "ambition" was and is to increase productivity (for volunteer contributors). Given that this bug (or "task") was created two years ago and itself is an offspring of the discussion in T19322 (five and a half years ago), the "outcome" will in no case be something to be proud of.

In T55387#786222, @Jdforrester-WMF wrote:

Maybe instead of closing the task it should move it to the "Done" / "For sign-off" column on the workboard of the main(?!) project? Then QA/Product can do manual approval, as we do in e.g. Editing and I believe is also done in Multimedia, Mobile Apps and other teams?

If this is necessary (the alternative is to watch for bugs to be marked Fixed, do sign-off then, and reopen them if they're not actually fixed), I suggest a different keyword should be used:

E.g.:

For-Signoff: T123

I would prefer that be a separate task. I think there are many projects that would use the feature here (i.e. Fixes-Task) for at least some bugs. Even for e.g. Flow which does have a signoff process, it's not used for low-level bugs e.g. fatals where it's obvious the behavior is now correct, or for certain kinds of tech debt.

Also, there really is no main project AFAICT, which is problematic for that workflow.

In T55387#786264, @mmodell wrote:

Is there any reason not to include all of our repos in diffusion?

I think it should be fine. However, in order to get notifications before merge (not required for this task, but required to preserve existing patch-need-review functionality), you would need to put both Fixes T123 and Bug: T123. That in turn means you'd get the "gerrit change merged" and the automatic Resolved message, which might be slightly annoying, but still less work than the status quo.

In T55387#786264, @mmodell wrote:

Is there any reason not to include all of our repos in diffusion?

Nope and I agree so much of this comes free, as is intended, with diffusion adoption.

Ref diffusion adoption, please check T75983: Process to request the read-only mirror of a non-gerrit code repository in Diffusion

This would be more useful if it would close the bug only after the code has been deployed everywhere. Closed tickets for which the fix is not yet live on Wikipedia are a major source of confusion.

In T55387#820171, @Tgr wrote:

This would be more useful if it would close the bug only after the code has been deployed everywhere. Closed tickets for which the fix is not yet live on Wikipedia are a major source of confusion.

I think the convention on most teams is that Resolved/Fixed means fixed on master.

Deployment is a separate issue. Some extensions aren't deployed by WMF at all, and even the WMF has two separate release branches at any given time. We probably don't want to say it's only fixed when on Wikipedia, given that it will make it to other WMF sites earlier (e.g. MediaWiki.org) and still other sites (e.g. third-party wikis using tarballs) much later (though conceivably third-party sites can also deploy faster than us, that's probably rare).

Also, this would be much harder to implement, since some automatic system would have to be able to update all the appropriate tasks when the Wikipedia branch switches over.

In T55387#787041, @Mattflaschen wrote:

However, in order to get notifications before merge (not required for this task, but required to preserve existing patch-need-review functionality), you would need to put both Fixes T123 and Bug: T123.

See an example at https://phabricator.wikimedia.org/T69971#1015726

In T55387#820172, @Tgr wrote:

This would be more useful if it would close the bug only after the code has been deployed everywhere. Closed tickets for which the fix is not yet live on Wikipedia are a major source of confusion.

I think this is a job for the deployment tooling. We have some tentative plans to build a deployment dashboard which provides an overview of which fixes are deployed, and where. There could definitely be a tie-in with phabricator but I'm not sure it should involve task status changes.

This is in Gerrit-Migration project, ostensibly making it a blocker. I'm going to remove that project thus removing it from the list of blockers because implementing a new feature shouldn't block the migration to a new tool.

Phabricator already closes tasks when you add "Fixes Txxx" to the commit message.

Should this be marked as resolved? It already works and you can do it today, just merge a change that includes "Fixes T55387" in the commit message, doesn't even have to be on a line by it's self.

@Dzahn ran into this Phabricator behaviour on T93645 where the commit messages in 9743ea4423641a8be83bc397a5deda39f27e5157:

apt: indentation fixes

T93645

Change-Id: Iefdb89699e5f1c8606ae9595f1784774fcdac6de

and 3577c5f9f7591c51c3162ac3b6c5b0669b36afd4:

datasets: indentation fixes

T93645

Change-Id: I214048b9aedf93560600f19269b96c95fd174545

(incorrectly) closed the mentioned task. I much prefer a solution where a) I can't accidentally close a task and b) I am not forced to use natural language in an unnatural way.

a) That sounds like a bug in the parser
b) phabricator actually allows you to use a lot of different forms of the fixes Txxx pattern - for example, you can write Resolves bug Txxx as fixed

See https://secure.phabricator.com/T5132 for details on these features.

@scfc: Reported upstream. https://secure.phabricator.com/T8318

To quote https://secure.phabricator.com/T5132#69200:

Close Related Task

Summary: Close a task when pushing a commit with Fixes T123 in the summary.

Close a related task with syntax specified in maniphest.statuses. Each task status can specify prefixes and suffixes. These are used in the form <prefix> <optional noun> Txxx [optional more Txxx] <optional suffix>. For example, Closes T123 or Closes T123 as Wontfix.

By default, the prefixes are: closed, closes, close, fix, fixes, fixed, resolve, resolves, resolved, wontfix, wontfixes, wontfixed, invalidate, invalidates, invalidated, spite, spites, spited.

By default, the suffixes are: out of spite, as spite, as invalid, as wontfix, as resolved, as fixed.

The prefix (and, optionally, the suffix) select which status the task will be closed with. For example, Invalidates T123 closes T123 as Invalid. When a phrase has both a prefix and a suffix, the suffix wins. For example, Invalidates T123 as wontfix closes T123 as Wontfix. The intent is to allow common prefixes like "closes" to be bound to a generic closed status while still allowing the relatively natural construction "closes X as invalid" to work as expected.

You can change these in maniphest.statuses. They are case-insensitive.

You can optionally use a noun after the prefix. The supported nouns are: task, tasks, issue, issues, bug, bugs. These are hard-coded and case insensitive. These also work with the "Ref" syntax. For example, Fixes bugs T123, T124.

These phrases work in commits. In Differential, they are recognized, but just work like "Ref". The intent is to allow you to write Fixes T123 in a commit message, have it reviewed, and then have the task automatically close on commit.

I can remember "Fixes-Task: T12345", but I am not able to memorize the above, let alone apply it mentally to see if my draft commit message would indeed trigger the intended behaviour (or trigger unwanted behaviour). This task's description points out: "[…] marking the bug as fixed without ambiguity." To me that is important.

I agree that's quite hard to memorise as well as an unwanted wall of text addition for https://www.mediawiki.org/wiki/Gerrit/Commit_message_guidelines

I don't understand the problem with "fixes Txxx", however, at any rate, I don't see a problem with "Fixes-task: Tnnn" either.

@scfc:
Why do you think that "Fixes-task: Tnnn" is better than "fixes task Tnnn" - I don't understand the preference for formal fields with colons over free-form words.

In T55387#1324494, @mmodell wrote:

Why do you think that "Fixes-task: Tnnn" is better than "fixes task Tnnn" - I don't understand the preference for formal fields with colons over free-form words.

Explicit over implicit. Structured data is always easier to parse and use than unstructured data. Any tool that would want to detect whether something fixes a bug (aka ForrestBot) would need to re-implement Phabricator's parsing logic, but if we used a consistent, standardized method, there would be no need.

If those tools need to run post-merge, they should be able to read the 'Daemons' actions posted to the task, rather than re-implementing the parsing logic.

In T55387#1332931, @Mattflaschen wrote:

If those tools need to run post-merge, they should be able to read the 'Daemons' actions posted to the task, rather than re-implementing the parsing logic.

That doesn't make sense, git has a standardized format for metadata, why are we making up our own? The parsing logic is "Field{colon} value". My tool shouldn't need to be Phabricator specific so I have to re-write it when we migrate to the next code review tool in 3 years, it should just use plain git.

Surely you could configure Phabricator to use "Fixes-Task:" as a valid
'prefix' for closing a task.

In T55387#1324494, @mmodell wrote:

Why do you think that "Fixes-task: Tnnn" is better than "fixes task Tnnn" - I don't understand the preference for formal fields with colons over free-form words.

I found the example given in T55387#1312852 rather convincing.

In T55387#1332938, @Legoktm wrote:

My tool shouldn't need to be Phabricator specific so I have to re-write it when we migrate to the next code review tool in 3 years, it should just use plain git.

Also, we want the next tool to be backwards compatible. We still handle patches which were written in the Bugzilla era, and isn't it nice that Bug: 12345 just works, without Phabricator needing to consult old-bugzilla in the background?