Page MenuHomePhabricator

Include full link to Maniphest tasks in commit messages
Open, Needs TriagePublic

Description

As an example, Here's a commit to a public project which is internally managed with Phabricator, but publicly available on GitHub:

https://github.com/paperg/terraform-fleet/commit/4815ad799adb9016157e2f3095da459a82f9ccd3

Update state appropriately when unit does not exist

Summary:
If a unit that existed in the past no longer exists, we must inform Terraform
of that fact so that it can plan to re-create it.

Ref T39.

Test Plan:
Deployed the test phabricator deployment I'm working on. Deleted the units with
fleetctl behind Terraform's back. Ran `terraform plan`, and saw that it was
planning to re-create the units I deleted.

Reviewers: kian, #developers

Reviewed By: kian, #developers

Maniphest Tasks: T39

Differential Revision: https://phabricator.paperg.com/D30

Note how Phabricator inserts a full link to the differential revision. This cool, because if I'm viewing the commit message in GitHub, or on the console with git log, I can just click it and go to differential.

However, "Maniphest Tasks" is not a link. To view this I have to copy it, open up a browser window, navigate to Phabricator, and paste the ticket number.

Making that a link would not only make some of my own work easier, but it would make it stupid easy for my coworkers to go from "overheard Phil talk about fabricator" to "Actually looked at Phabricator."

Event Timeline

bitglue raised the priority of this task from to Needs Triage.
bitglue updated the task description. (Show Details)
bitglue added a project: Maniphest.
bitglue added a subscriber: bitglue.
joshuaspence added a subscriber: joshuaspence.

I think this is purely an Arcanist thing.

One minor note, but we've also seen feedback in the other direction, where users want to drop the URL.

chad added a subscriber: chad.Mar 3 2015, 8:16 PM

There is a task somewhere to convert full URLs in Remarkup back to Tokens. That would be a blocker for this.

chad added a comment.Mar 3 2015, 8:37 PM

Seems inconsistent either way we go though, since Differential is a URL and Maniphest is just the Task ID.

chad added a comment.Mar 3 2015, 8:38 PM

If Task ID is already a token in Summary, it probably is fine to just have the URL in Maniphest Tasks. ?

My take on T5378 is that links don't get the same helpful context as tokens, such as:

  • strikethrough when they refer to a closed task, etc
  • a hover popup giving a summary of the linked thing

If links had those same features, I'm not sure anyone would complain about "Manifest Tasks" being a URL. I could see a case against expanding every token into a URL (that could be very verbose), but that's not what I'm proposing.

epriestley added a comment.EditedMar 4 2015, 6:48 PM

T6030 might be one way forward here, since I think there are (well, maybe) reasonable use cases for wanting a full URL or for wanting only a reference.

In particular, the problems that the full URL originally solved (allowing you to import a repository from another install without closing all your revisions) were eventually mooted by the introduction of mentions, object mentions, etc., and I think there's a strong argument against always writing full URLs for everything. I feel like it would be very awkward if I wrote this in my summary:

I talked to @chad and he said...

...and arc turned it into this in the commit log:

I talked to https://secure.phabricator.com/p/chad/ and he said...

Basically, we have to resolve the ambiguity issue without using full URLs, because using full URLs for all user and object mentions, "Closes", "Ref", etc., would be ugly and gross (and we do have reasonable approaches to this problem that don't require using full URLs).

So I'd sort of like to take "Differential Revision" back to just doing D123, and then maybe add a full URL option to the fields if we still think it's useful. I think https://secure.phabriator.com/p/bitglue/ is the only user I've heard express interest in it, though, and I worry a little that the motivation (onboarding other users) may be shorter-lived than the commits themselves (e.g., if everyone converts at some point, I'd expect someone to be like "why are these things using ugly full URLs" and then there to be some small level of regret about how messy the log is forever).

Some discussion of the "other repositories" issue in T6516.

Instead of full URLs, couldn't the daemons determine whether or not to create transactions/mentions based on phabricator.uri from the project's .arcconfig file?

I think you are misunderstanding my proposal.

I am not suggesting that all tokens get converted into URLs.

I am suggesting that only the "Maniphest Tasks:" field be a URL. Indeed, making everything a URL would be horrible.

Onboarding users is not the motivation.

The motivation is integration with tools which aren't Phabricator (git console, source code search tools, test/build/deployment tools, mailing list, IRC...) which I use on a daily basis. Phabricator is really great, but it will never be the only tool I need. It's a happy accident that URLs are almost universally understood by programs and humans alike.

Because the goal is integration with other tools which know nothing about Phabricator, I don't see any option other than a URL. Yes, you can make Phabricator smarter so that URLs aren't necessary, but that doesn't help all the other tools in the world. Yes, a URL less than ideal, and it contains a lot of redundant information, and some of that information might change some day, but not everything can be as smart as Phabricator. I don't think making "Maniphest Tasks:" a URL would be very messy, and the benefits outweigh the costs. My reasoning:

  • "Maniphest Tasks:" is already on a line by itself, so a URL there doesn't introduce any discontinuity in a paragraph of prose.
  • It's below the meat of the commit message so it adds minimal distraction. It's at the end, right where you'd expect a "for more information, see..." link to be.
  • It's already automatically generated, so making it a URL doesn't constitute any mangling of user input.
  • Of all the objects in Phabricator associated with a commit, the task is the first thing I'll look at if reading the commit message isn't sufficient.
  • If someday the tasks are migrated to some other location, we have HTTP 301 for that.
  • Even without a working HTTP redirect, a broken URL isn't less usable than no URL at all. Anyone who knows where the new task tracker is will copy just the task number from the URL and paste it in the new tracker.
  • If the broken URLs bother you that much, there's git filter-branch. A great thing about URLs is they are amenable to regex manipulation with a low possibility of false matches.

As for removing the Differential URL and switching it back to just a token, I'm a huge -1 on that. As it stands, there's at least 1 URL in every commit someone can click on, and then the task is just 1 click away from that. With 0 URLs in the commit message, then the process becomes not 1 or 2 clicks, but:

  • know that Phabricator even exists, and how to access it. For an open source project this is a blocker for most people. I can mention Phabricator in a README, but no one reads those.
  • copy the ticket number
  • switch to the web browser or open a new tab
  • navigate to phabricator
  • paste the ticket number

This is currently what we do with our commits that reference JIRA issues, and it's a tremendous pain in the ass, even though I know how to access JIRA and I have it open most of the time. It would be really ironic if this task results in the differential URL being removed, because I was prompted to file this task as I was writing some guidelines on good commit messages, and I was going to write "And a great thing about Phabricator is that it puts URLs in the commit messages so you can always easily get to the issue tracker, unlike JIRA" when I realized that Phabricator only somewhat does that.

bitglue renamed this task from Include full link to Manifest tasks in commit messages to Include full link to Maniphest tasks in commit messages.Mar 4 2015, 8:33 PM
bitglue updated the task description. (Show Details)

Actually I guess this (storage format in commit history) is largely unrelated to that (display format in web UI).

urzds added a subscriber: urzds.Jul 12 2017, 11:11 AM
scp added a subscriber: scp.Jun 27 2018, 3:34 PM
epriestley edited projects, added Diffusion; removed Maniphest.Feb 20 2019, 11:29 PM