I'm pretty hesitant to upstream this because I think your install is the only install using the existing option and I haven't seen more interest in this, but I've had to port the existing stuff forward once already (in connection with T2222) and am likely to need to generalize it for Audit (in connection with T4896), and make it interact correctly with planned changes to inline behavior (T1460). Making the feature more complicated is increasing the upstream cost for a feature that only your install uses.

In T1460, we also plan to introduce explicit "done" and "reply" states to inline comments, but have a different definition of "reply".

If you:

fix the correctness issue around drafts;
remove the configuration option (so this behavior is always enabled if you have inlines enabled);
are comfortable that we will change what "reply" means in the future when T1460 happens; and
do what you reasonably can about the query stuff (the hard query should get easier after T1460, so it's not hugely important that it get corrected)

...I guess we can upstream this. The config option is the biggest issue, but I think that's not contentious to remove.

src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
297–311	Do we need this option? Do you think it's useful to enable one the other option but not this one? At a minimum, this should mention that the option has no effect unless inlines are already enabled.
src/applications/differential/editor/DifferentialTransactionEditor.php
1307	Since this must be an array, prefer an `array` typehint.
1308	By convention, avoid foreach-by-ref, or explicitly unset the reference variable after exiting the loop. This pattern (binding references and not unsetting them) tends to lead to subtle bugs where reuse of an iterator later in a function has surprising, nonlocal side effects.
1314	Prefer class typehint.
1316–1319	This incorrectly selects draft inlines (which have not yet been submitted, and should not be visible to other users) in addition to visible inlines. Add a `transactionPHID IS NOT NULL` clause to avoid selecting them.
1316–1319	This suffers from the "N+1" query problem: https://secure.phabricator.com/book/phabcontrib/article/n_plus_one/ Specifically, if there are 100 inlines, we'll issue 100 queries. Although this probably isn't a huge problem since it's rare to have 100 inlines, it would be better to issue a single query.
1325–1326	This is another N+1 query. Instead, authors should be loaded in a single query. For objects where a real `Query` class is available (most objects, but not `DifferentialTransactionComment`), prefer it. The appropriate `Query` class in this case is `PhabricatorPeopleQuery`.
1328	Using `getRealName()` is unconventional: Phabricator identifies users with `getUsername()`.

This revision now requires changes to proceed.Aug 12 2014, 12:39 PM

Address review comments.

Harbormaster completed remote builds in B2206: Diff 24679.Aug 13 2014, 8:21 AM

Regarding your effort to maintain this, a few thoughts:

Thank you so much for doing it; no matter whether you take this in the end, I really appreciate your feedback and time; I understand the trade-off between features and maintenance headaches, so I fully appreciate if we have to keep our own fork.
That said, keeping our own fork often feels very very frustrating; I know that's not your problem, but I think that 99.9% of the time I've spent countless hours in syncing and resolving merge conflicts would have cost no time if the person writing the feature (-> you ;) would have changed the code with the refactoring (I love the refactorings, just not the merging ...); that would save so much of my time that you dropping me an email whenever there is a problem with this code so I can resolve this would be immediately prioritized for me, based on that I would lose so much if I have to keep our own fork
<<</me opens the bag and pulls out a carrot; the carrot looks somewhat translucent, so you can't really be sure whether it exists>>>: we would very much like not having to maintain our own instance, and we'd like to become paying customers (the countless hours I throw at maintaining our own GCE instance with this stuff are beyond reasonable); not sure whether you still aim at providing such a service though...
I think the feature might not be used very much until now because it's not feature complete yet; I think the comments in the diff actually make the whole email diff thingy much more useful, and if we add parsing in-line replies and adding them to comment hunks as replies, that would make it even more useful (it would mean I could just do reviews completely by email if I have enough context); but of course I would say that, so probably not a very convincing argument on its own :P

src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
297–311	Done.
src/applications/differential/editor/DifferentialTransactionEditor.php
1307	Done.
1308	Yea, that doesn't make any sense. /me shudders. Sorry for having left that in ...
1314	Done.
1316–1319	Draft bug: Yikes. Done.
1316–1319	N+1. Done, yea, that was bad. /me's first real LAMP project, so thanks for teaching :)
1325–1326	And done.
1328	Hm, I'd like to honor the user-address-format setting, but I don't think people care too much (hopefully). I'll go with getUserName and we'll see whether people scream at me.

Cool, this is in pretty good shape. One query construction safety thing and a couple of minor things.

src/applications/differential/editor/DifferentialTransactionEditor.php

1331

Slightly better as:

pht('%s wrote:', $user->getUserName())

...so it can be translated.

1369–1371

This should use qsprintf() to prevent possible SQL injection:

$table = new DifferentialTransactionComment();
$conn_r = $table->establishConnection('r');

$queries = array();
foreach ($stuff as $thing) {
  $queries[] = qsprintf(
    $conn_r,
    '(changesetID = %d AND (lineNumber IN (%Ld))',
    $id,
    $line_numbers);
}

Then, below, use:

$table->loadAllWhere(
  'transactionPHID IS NOT NULL AND (%Q)',
  implode(' OR ', $queries));

1381

Slightly simpler as mpull($all_comments, 'getAuthorPHID').

This revision now requires changes to proceed.Aug 14 2014, 4:30 PM

Address review comments
Merge to head

Harbormaster completed remote builds in B2243: Diff 24745.Aug 15 2014, 5:13 PM

klimek added inline comments.Aug 15 2014, 5:13 PM

src/applications/differential/editor/DifferentialTransactionEditor.php
1331	Done.
1369–1371	Done, thx again for the teaching effort :) Btw, I assume 'implode' is just more php'y than 'join'?
1381	So that would assume that 'withPHIDs' doesn't depend on the keys. I assume in php it's basically "unless otherwise stated, if you take a list of things you shouldn't depend on the keys in the array?". This seems a little fragile at a first glance, but probably just takes some getting used to... So, done :)

Cool. Thanks!

This revision is now accepted and ready to land.Aug 15 2014, 5:13 PM

Closed by commit rPeb3ed9bbc994 (authored by @klimek, committed by @epriestley).

On join/implode, they're totally equivalent, we just try to pick one and stick with it in this codebase. We should write a lint rule but it doesn't come up too often.

epriestley mentioned this in D15857: Refine inline style rendering in email.May 6 2016, 1:38 AM