Page MenuHomePhabricator

[feature request] Tag commits
Closed, ResolvedPublic

Description

First of all, I'd like to say that our company has been using Phabricator since January and it has been really great. It really facilitates the conversation surrounding code and commits. I'd say that we primarily use the post-commit audit feature, and use pre-commit code review on larger/heavier problems. I have two cups of phabricator for breakfast, personally. Today I thought of a feature request:

What: Add ability to tag commits. For example, devs John and Adam are working on the new, hot 3D scrolling view to their app. When crafting commits, they could add a line to the commit 'Tags: 3d-scroll' akin to "Auditors: John". Additionally, when reviewing commits, a user could tag the commit as an option from the dropdown. Somewhere in the web interface, you could view all commits by tag.

Why: I think Phabricator's greatest strength is to be able to have a conversation and simultaneously document our code. It allows for our code to be improved and also surface information that may not be obvious just from the lines of code. After a commit has been accepted, it sort of just disappears, though. I think on top of accepting a commit, it would be cool to tag the commit for organization purposes. Now, not only is code being accepted, but it is being categorized and organized.

Impetus: Today, a dev pushed 80+ commits to the repository. The commits were part of a month-long secret/local feature branch. As I saw the 80 commits sitting in the queue waiting to be audited. I realize other devs on the team are going to start auditing the code and conversation pertaining the feature branch could very easily pass me by and disappear into the history feed. You can search for commits, etc, so it is not that it really disappears, but realistically, it is going to be very hard to find those commits again. Being able to tag commits would allow for all of those commits to be easily referenced again in the future.

Brad

Revisions and Commits

rP Phabricator
D10877
Restricted Differential Revision

Related Objects

StatusAssignedTask
Resolvedbtrahan
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley

Event Timeline

This dovetails with these tasks:

  • {T793}
  • {T390}

Generally, we plan to enrich projects a bit and make the tags vs other-things distinction more explicit. Projects are essentially tags right now, but we plan to make this more natural and straightforward in cases where they're really used as just tags, while also making the more powerful in other cases where they're used as more-than-tags.

Differential is responsible for parsing all the commit field stuff and currently doesn't support a "Projects:/Tags:" field, but T793 will add one. One open question here is what alternate representations of projects we should support. Two candidates:

  • Possibly, we should interpret [something] in titles as a tag (e.g., [feature request] here).
  • Possibly, we should add a short project/tag reference syntax, like we have with @user, Dnnn, rXnnn, etc. The obvious choice is probably #project, but this isn't ideal when entering things from the CLI, since "#" is a comment string in editors and commands. Facebook also uses #123 for their internal task tool, although those IDs are numeric so there isn't much of a real conflict.

Diffusion already supports tagging commits with projects, but only after the fact from the web UI, which is pretty inconvenient.

Upshot: this is totally reasonable, but probably some distance away on the roadmap because it overlaps with a lot of other planned work.

Also, this is a really well-written feature request. Thanks for taking the time to explain the context and reasoning!

Great, this is good to know. I didn't really spend much time browsing previous feature requests, ideas, so I did not realize there had been similar ideas already.

As for how tags are implemented, I don't have much preference. [TAG_NAME] square brackets in the title would be reasonable. @ or # also sounds good to me. I defer to the Phabricator gurus on this one. The only thing I would note is that we generally follow some guidelines on our git commit title length, and try to cap it at 50 chars. Forcing the tag remarkup to be in the title within square brackets could be throw a wrench into our existing workflow.

We definitely won't require [project] in titles (from T3190, it looks like #project in bodies will be the defacto syntax), but it seems like people tend to naturally use [project] in titles too so we could additionally parse it in titles and probably get it right almost all of the time.

Specifically, the implementation will probably look like:

  • [project] in titles implies project;
  • #project in body fields implies project;
  • Project: project, Projects: project, Tag: project or Tags: project imply project.
epriestley changed the visibility from "All Users" to "Public (No Login Required)".Jan 28 2014, 4:08 PM

Specifically, the implementation will probably look like:

  • [project] in titles implies project;
  • #project in body fields implies project;
  • Project: project, Projects: project, Tag: project or Tags: project imply project.

So I think remaining work is make sure commits in Diffusion have some "project" web UI and then that the various remarkup parsing n the quote above works correctly?

I think it's:

  • PhabricatorRepositoryCommit does not implement PhabricatorProjectInterface. I think it stores edges correctly, just doesn't implement the interface. Once the interface is implemented, "Projects" should show up in the UI (maybe we need a setObject() on the PropertyListView or something).
  • The #projects in bodies should already work after your Audit + ApplicationTransactions work, I believe -- it's just not easily visible in the UI right now.
  • The "Edit Commit" workflow might not be using transactions correctly. Don't remember if we updated it or not.
  • We aren't parsing stuff out of titles anywhere else and [project] doesn't necessarily seem common enough to justify in this case? I think we should either do this everywhere or nowhere, and favor shelving it for now.
  • Message parsing in Diffusion is sort of a mess. We have other reasons to fix it up eventually, but I'd probably just punt all of the special field stuff for now? That is, #project in bodies feels like it gets us 95% of the way there in terms of getting things tagged from the CLI, to me.

So: implement PhabricatorProjectInterface, get the UI, make sure "Edit Commit" is transactional, call this done for now? Unless you think [project] or Projects: xyz are significantly valuable.

Op woensdag 12 november 2014 heeft joshuaspence (Joshua Spence) <
noreply@phabricator.com> het volgende geschreven: