I think it's T4768: Phabricator silently overwrites concurrent changes (no conflict detection)?

Oh god, that's it. This has been around since 2014? :(

Actually.... Rereading, maaaaaaaybe it's the same? It would have to depend
on how the back end actually managed those revisions. If phriction treats a
wiki update the same way diffusion treats a differential update then yeah,
this is our bug.

This is not T4768.

Can you reproduce the issue on this install?

The behavior you describe is not how Differential works or ever has worked, so I'm either misunderstanding or something else is afoot.

In particular, inlines are bound to the exact diff they are left on ("line 17 of changeset 28942 on diff 1842"), not a mutable location like "line 17 on file x/y.c on the most recent diff of revision 293". For example, you can go make comments on older diffs, which would not be possible if we didn't bind them to specific changesets.

If you can reproduce the issue on this install I think that should make it more clear where things are going awry or what I'm misunderstanding in your report.

Sure, let me see if I can do that, I'm heading in to the office now.

So I just want to add a little bit more context here:

[jane@polonium ~ ]$ arc --version
arcanist 30513ae74a1546f824b6dfa5781af3b2766d262c (7 Sep 2015)
libphutil d82ea11b139dbb4b6f9cc37ad3220f8e1f490a95 (5 Sep 2015)

I can't seem to find the phabricator version we are using from the settings (but I am not an admin user or anything). I will try to create the issue here, but I would need a repo and to configure arc to talk to it and I am not sure I can do that. Let me dig a little?

You can probably use rGITTEST, but just copy/pasting some random diffs into the web UI is likely sufficient in this case too (no special behavior around inlines should get triggered). You're recent enough locally that you shouldn't run into any compatibility issues.

yep, doing so now.

so let's start with this revision. to reproduce, i would need to create a diff on that, and then comment on it, and while i had a session with those comments "in the air" in the web, have that diff updated, and then submit my comments.

make sense?

i am not sure it works if all of those things happen with me as the sole actor.

I am having trouble configuring arcanist to work against this phabricator vs my "home" phabricator. Otherwise I am ready to do this.

Maybe it's easier if I try to follow your instructions. Here's what I did:

I cd'd into libphutil/ and replaced the README.md with a file with 20 lines of "A" with a couple of random "A" words. I ran git diff HEAD to produce this local diff:

diff --git a/README.md b/README.md
index 6f775e2..3b3e28f 100644
--- a/README.md
+++ b/README.md
@@ -1,65 +1,20 @@
-`libphutil` is a collection of utility classes and functions for PHP. Some
-features of the library include:
-
-**libphutil Library System**
-A system for organizing, loading and introspecting PHP classes and functions.
-Uses static analysis to generate, validate and update library contents and
-includes. Based on Facebook's similar `flib` system.
-
-**Futures**
-Futures (also known as "promises") are objects which act as placeholders for
-some future result of computation. They let you express parallel and
-asynchronous execution with a natural syntax. There are three provided
-concrete `Future` implementations: `ExecFuture` for executing system commands,
-`HTTPFuture` for making HTTP requests, and `QueryFuture` for executing database
-queries.
-
-**Filesystem**
-The builtin PHP filesystem functions return error codes and emit warnings. It is
-tedious to check these consistently. The `Filesystem` class provides a simple
-API for common filesystem operations that throws exceptions on failure.
-
-**xsprintf**
-This module allows you to build `sprintf()`-style functions that have arbitrary
-conversions. This is particularly useful for escaping data correctly. Three
-concrete implementations are provided:
-
-  - `csprintf()`: safely escape data for system commands
-  - `jsprintf()`: safely escape data for Javascript
-  - `qsprintf()`: safely escape data for MySQL
-
-**AAST/PHPAST**
-An abstract, abstract syntax tree which can make it easier to perform simple
-static analysis, and a concrete AST for PHP.
-
-**Remarkup**
-A Markdown-like lightweight markup language. Remarkup's syntax is defined by
-parser plugins and fairly easy to extend and configure.
-
-**Daemons**
-Enables running PHP scripts as stable, long-lived daemons.
-
-**Utilities**
-A handful of solid utility functions.
-
-`libphutil` is used by
- - [Phabricator](https://secure.phabricator.com/diffusion/P/)
- - [Arcanist](https://secure.phabricator.com/diffusion/ARC/)
- - [Diviner](https://secure.phabricator.com/book/phabricator/article/diviner/)
-
-
-----------
-
-
-**BUG REPORTS**
-
-Please update `libphutil` to **HEAD** before filing bug reports. Follow our [bug reporting guide](https://secure.phabricator.com/book/phabcontrib/article/bug_reports/) for complete instructions.
-
-
-**PULL REQUESTS**
-
-We do not accept pull requests through GitHub. If you would like to contribute code, please read our [Contributor's Guide](https://secure.phabricator.com/book/phabcontrib/article/contributing_code/) for more information.
-
-**LICENSE**
-
-`libphutil` is released under the Apache 2.0 license except as otherwise noted.
+A
+A
+A
+A
+A
+A
+A
+A
+A
+Apple
+A
+A
+Albatross
+A
+A
+A
+A
+A
+A
+A

I copy-pasted the results into this page:

https://secure.phabricator.com/differential/diff/create/

I hit "Create Diff", then created a new revision and filled in some dummy data, producing D14332.

I clicked on the "Apple" line and created an inline comment. My UI looked like this:

I did not submit this comment.

I went back to the CLI and replaced the file with a lot of "B", with a couple of rnadom "B" words. I ran git diff HEAD to produce this local diff:

diff --git a/README.md b/README.md
index 6f775e2..afbc48e 100644
--- a/README.md
+++ b/README.md
@@ -1,65 +1,28 @@
-`libphutil` is a collection of utility classes and functions for PHP. Some
-features of the library include:
+B
+B
+B
+B
+B
+B
+B
+B
+B
+Banana
+B
+B
+B
+B
+B
+B
+B
+B
+B
+B
+B
+Boat
+B
+B
+B
+B
+B
 
-**libphutil Library System**
-A system for organizing, loading and introspecting PHP classes and functions.
-Uses static analysis to generate, validate and update library contents and
-includes. Based on Facebook's similar `flib` system.
-
-**Futures**
-Futures (also known as "promises") are objects which act as placeholders for
-some future result of computation. They let you express parallel and
-asynchronous execution with a natural syntax. There are three provided
-concrete `Future` implementations: `ExecFuture` for executing system commands,
-`HTTPFuture` for making HTTP requests, and `QueryFuture` for executing database
-queries.
-
-**Filesystem**
-The builtin PHP filesystem functions return error codes and emit warnings. It is
-tedious to check these consistently. The `Filesystem` class provides a simple
-API for common filesystem operations that throws exceptions on failure.
-
-**xsprintf**
-This module allows you to build `sprintf()`-style functions that have arbitrary
-conversions. This is particularly useful for escaping data correctly. Three
-concrete implementations are provided:
-
-  - `csprintf()`: safely escape data for system commands
-  - `jsprintf()`: safely escape data for Javascript
-  - `qsprintf()`: safely escape data for MySQL
-
-**AAST/PHPAST**
-An abstract, abstract syntax tree which can make it easier to perform simple
-static analysis, and a concrete AST for PHP.
-
-**Remarkup**
-A Markdown-like lightweight markup language. Remarkup's syntax is defined by
-parser plugins and fairly easy to extend and configure.
-
-**Daemons**
-Enables running PHP scripts as stable, long-lived daemons.
-
-**Utilities**
-A handful of solid utility functions.
-
-`libphutil` is used by
- - [Phabricator](https://secure.phabricator.com/diffusion/P/)
- - [Arcanist](https://secure.phabricator.com/diffusion/ARC/)
- - [Diviner](https://secure.phabricator.com/book/phabricator/article/diviner/)
-
-
-----------
-
-
-**BUG REPORTS**
-
-Please update `libphutil` to **HEAD** before filing bug reports. Follow our [bug reporting guide](https://secure.phabricator.com/book/phabcontrib/article/bug_reports/) for complete instructions.
-
-
-**PULL REQUESTS**
-
-We do not accept pull requests through GitHub. If you would like to contribute code, please read our [Contributor's Guide](https://secure.phabricator.com/book/phabcontrib/article/contributing_code/) for more information.
-
-**LICENSE**
-
-`libphutil` is released under the Apache 2.0 license except as otherwise noted.

I copy-pasted it into the web UI, then used "Update revision..." to update the existing revision.

I switched back to my first window and submitted my inline with a comment.

I now see this:

Is this the same thing you're seeing, at least approximately? And you expected your comment to not be present at all?

Yes, that is exactly what I was expecting to see. I had just now gotten a diff created. :)

The comments were pinned to line numbers, and when the diff was updated, the line numbers changed, and so the comments became meaningless. This was really a problem because the diff changed substantially and the comments were important to those lines, rather than the specific files.

Oh, wait, re-reading your comment. No, I can't share the exact screenshots because it's our source, and that's proprietary. But when I commented on line 58 of the diff, and then the diff was updated as I submitted, that line actually became line 78 or whatever, and so the comment is still there on 58 or whichever, and makes no sense anymore, so the "review" of my comments on that diff are now entirely confusing and not helpful.

Okay, I think what you're seeing is an inline which has been ported forward. This is a feature, and an expected behavior. Here's an example of what it looks like.

Note that the normal inline (on the current diff) is colorful, while the ported inline (from a different diff) is transparent and ghostly. The ported inline also has a button to jump to the original location of the inline (here, "View on previous revision"). If you click that, it should take you back to the previous diff and to the original context of the inline.

You can disable this behavior in Settings → Diff Preferences → Show Older Inlines if you don't want inlines to port.

T7447 discusses why it's impossible to port inlines "correctly" (to the exact same line a human would) in all cases, and why we err heavily on the side of porting them even if the new diff is very different (roughly, it makes sure an important inline isn't forgotten about).

Does that explain what you're seeing?

So for example, if i were to go make some changes to D14333 and wait to submit those changes, and you were to comment on it, those comments would be largely meaningless after.

In T9628#141693, @epriestley wrote:

Okay, I think what you're seeing is an inline which has been ported forward. This is a feature, and an expected behavior. Here's an example of what it looks like.

Oh good grief, yeah, that is exactly it.

You can disable this behavior in Settings → Diff Preferences → Show Older Inlines if you don't want inlines to port.

Thank you.

T7447 discusses why it's impossible to port inlines "correctly" (to the exact same line a human would) in all cases, and why we err heavily on the side of porting them even if the new diff is very different (roughly, it makes sure an important inline isn't forgotten about).

Yeah, seriously. It occurs to me this is a Really Super Hard Problem to solve and I wasn't sure how phabricator was going to get around that except to tell the person submitting comments to hold their horses and maybe re-submit after re-reviewing. This is good enough for me.

Does that explain what you're seeing?

Yep. Thank you. We can consider this resolved. Thanks for helping me out on a Sunday.

This feature is always going to be imperfect, but my personal experience, at least, is that something like 80-90% of inlines port somewhere reasonable (the code is still the same, or nearly the same), and I understand about half of the rest of them from context, and can click the "Rewind" button for the remaining few to see what the author was talking about if I don't remember. There are also cases where we can improve the behavior without needing to solve any hard AI problems, see T7870 for the ones we know about (it's possible that whatever you hit here is a case we could improve).

We also make an explicit decision to be very, very aggressive about porting things forward. Other software like GitHub doesn't do this (GitHub is extremely conservative about porting -- some discussion buried in T7447) but I think being aggressive is the better behavior overall. It seems bad to me to train users that comments port forward, but then occasionally not port a "This is really dangerous! Don't do it!!!" comment forward because there was some unrelated whitespace change and the code is no longer identical.

My previous employer was a 100%-on-github shop. This employer is a nearly-100%-on-phabricator shop. I will tell you I am entirely a phabricator convert. I've been using github and singing its praises for years and the last six months I've been using phabricator have been eye-opening. I love this software. :)

Cool, glad to hear it! This is, at the very least, something we could document better. I don't think the documentation currently mentions that inlines are even a thing, much less describes the porting behavior or the rationale for it.

No, this was surprising, and surprises are bad. If it could alert the user
and say "whoa there, it looks like this diff was updated while you were
commenting," that would be good -- but this was literally the exact minute.
My coworker and I were submitting within seconds of eachother. This is
definitely an edge case.