Differential Email only contains file basename for inline comments

The email sent with inline comments explicitly sets the path to be just the filename.

With reviews full of changes to multiple distinct files all with the same name (eg Makefile, init.py, etc) this is very confusing for the reader of the email.

Can this either be made configurable or maybe just changed to show the full filename ?

1 Like

I don’t consider this to be a bug: the application is making an intentional tradeoff to make email more readable in cases where file paths are long (which I believe is fairly common) in exchange for making email a little less precise in cases where multiple files with the same name have inline comments (which I believe is fairly uncommon).

I doubt Phabricator will ever have a setting to control this behavior. See https://secure.phabricator.com/T8227 for discussion.

We could imagine behavior which minimizes the downsides of this tradeoff without requiring a setting: for example, the code could:

  • build a list of all paths which will be referenced in the mail;
  • for each non-unique entry on the list, add one more directory of context.

So a unique example.c would still appear as example.c in the email, but a non-unique Makefile would appear as subthing/Makefile.

However, I’m not aware of any customer requests for this sort of improvement, so I don’t currently have any plans to build it.

In general, users viewing email are always one click away from the actual revision and a lot of information and features which are available on the revision page can never be made available in email (like expanding context or replying to a comment), so I’m not trying to make email have every possible feature or affordance it could. The primary goal of email is to help users answer the question “Should I take some kind of action in response to this change?”, and detailed inspection of each inline is usually not necessary to make that determination.

There are some exceptional cases (most commonly, viewing this email from a phone which can not access Phabricator because of a VPN) but the framing of these cases begins with “My company chooses to use a VPN which prevents most employees from accessing important systems most of the time…” and I think that’s the more valuable problem to attack in these cases.

I like your “un-ambiguation” idea. Please count me as a customer requesting this.

If you’re a customer (i.e., you pay Phacility for hosting or support), please file a ticket via your support channel (you can just reference this thread).

The suggestion of including more of the path when there are duplicates is interesting and would help us most of the time. Most of our codereviews this doesn’t actually mater but we sometimes have ones that touch a lot of files where the last directory component and the file component are the same, eg amd64/Makefile, sparcv9/Makefile.

Wouldn’t it be simpler code and more consistent behaviour to pick a maximum line length and always include as many path components that fit ?

This is certainly something we can maintain a local patch for it if isn’t likely to be changed.

Thanks for considering it anyway.