This also seems a little hard to believe to me, at least in terms of being a general flaw with this workflow:

the auditor may spend a significant amount of time before even realizing they've already looked at the changes.

You're not wrong ;). I'm mostly referring here to a scenario where a significant enough time has passed where the auditor might not remember the individual commits (months), so the list of merged commits doesn't provide too much value. The user then would have to click on each one to see if they reviewed them already, and then still try to figure out if htere are any additional changes.

It seems like T3917 is asking for content similarity search (or something similar) as you mention, although falls short of automatic exclusion rules. I don't think this case is covered by T3917, but I'll file another bug, which hopefully will be more descriptive, and I'll let you be the judge of that.

I'm not sure that's the same issue. Broadly, T3917 seems to describe a situation where multiple commits with the same content have different identity due to rebases or cherry-picks. This situation has to do with the exact same commit being re-audited due to merge. I guess it technically possible to review individual commits, then merge them into another branch while also making additional changes, but I would guess that's generally not the case and there doesn't seem to be a lot of use in re-auditing all the changes that have been audited individually, but now as a whole. So in a nutshell what t his is asking for is to only trigger an (automatic) audit on merge commits if:

• audit doesn't exist for all commits being merged
• there is a non-empty diff between the top of the merged branch and the top of the merge destination. I.e. there are additional changes made during the merge.

But I still kind of think that if you generated a link to your own content you should be able to navigate to it, so it seems reasonable that either:

• remote URI that start with pre-configured base URI are considered valid
• "View Raw File" generates a local URI relative to base URI to avoid this check

I guess I can phrase that differently, then.
Consider that this services as an exception and the associated error message is extremely confusing:

• It specifies a URI that is valid and is not external to my server and calls it both remote and invalid, which is odd
• The URI that is surfaced is a long and very internal-looking one, clearly generated by Phabricator itself
• There is no explanation given as to why this is considered invalid.

Looks like that fix affects Differential, but not Diffusion? After reading through that bug, I can totally reproduce this on demand in Audit
I have a test instance here that reproduces this problem (both actively able to reproduce and has commits in the database that are exhibiting the behavior already): https://test-kdauzhhtarbi.phacility.com/rLIBdde2f74f2793f0216f0d76618ed335a9c802cfec, but I reproduced it reliably on 3 separate instances now.

I'm unable to reproduce on a test install right now, but then again, I stopped reproducing on my install after rebooting the server. It looks like something happens eventually to cause this to break and I start getting these broken comments.
Looks like every time I get a broken one it doesn't have a transaction PHID associated with it and all comments submitted together have the same wrong associated commitPHID.

oh neat!

This is, by the way, on a brand new install that's not even a month old.
I tried to reproduce it on another install that is older and I'm not having any luck.

This actually is a lot worse than I originally thought.
All inline comments that are made on an audit are actually applied to the audit I was previously commenting on (like an off by 1 error). I've isolated the following things:

• This happens across repositories (i.e. if I have multiple repositories, commenting on one commit and then switching to another repository's commit and commenting on it will produce a comment on the first commit)
• This happens for all users
• This happens regardless of audit status - i.e. commits with audit requested exhibit this as much as commits that don't require audit.
• Submitting an empty comment first thing before adding any content to a commit works around this problem - subsequently added commits appear as expected.
• The broken state can be easily detected by unavailability of the "hover state" of the overall commit field - pressing "z" does nothing

Thanks for the analysis and the change. It makes a lot of sense to me now and, to be fair, I wouldn't have even seen the exception had the repository finished importing, which is what the fix does.
Thanks again.

Oh wow! Thanks for looking at this on a Sunday morning!
If this is expected then I think there are a couple of things that are happening that could make it better:

1. It looks like an uncaught exception, which seems like a crash and definitely says "unexpected"
2. It gives no explanation of what's really going on here. All there is is Commit "191140" has been deleted: it is no longer reachable from any ref., but it's unclear what 191140 is, and it doesn't look like a valid commit, so at a glance, it looks like something isn't getting parsed right. It is also happening while parsing a commit that I can't find in my local copy, but it exists in Phabricator's local copy. An explanation on how that could happen would be useful, too.
3. Most importantly, it gives no way to get out of this state. There are now commits stuck in "Importing" state permanently, which leaves the entire repository in an unfinished state. Providing the user with a facility to resolve this (and pointing them to it in the error message) would go a long way here, even if it's something simple and brute force, like ignoring this commit, or marking the whole repository as imported (which is what I ended up doing).

Thanks for fixing it so quickly!

eventually the dead ones appear to fall off the edge, but in the meantime UI doesn't give me a warning that my daemons are not actually running.

Ok, I just installed a new instance of the server in another VM.
Steps to reproduce, as far as I can tell, are:

Nice! Thanks.

Never noticed that before? Is this specific for Audit?

This is our expected use of Flags. They already exist on most objects, and users can build/query/sort them as they like for whatever reason.

Yes, that totally makes sense to me (and, in fact, that's what I suggested by teammates do), but even an ability to use Herald to automatically flag an audit based on comment activity would go a long way here.

Some more info I dug up:

• There are 2 commits that appear to have flipped comments, i.e. the comments that appear on audit1 actually apply to audit2 and vice versa.
• These commits are sequential and were pushed simultaneously (i.e. the author made multiple commits on master locally before git pushing them all)
• In both cases only comments made by the author appear out of place.

I was picturing this as more of a tool to help the author keep track of things that may require attention rather than an auditor to verify that something is done.
This also allows for a workflow where a comment is made after the audit is closed that is not worthy of reopening the audit (and potentially by a 3rd party). And it could be both "I'm late to the party and I have input" kind of scenario and "I'm an intern and I have a question about this change 3 years later"

That almost works. The problem is that as soon as the audit is accepted it fall off of everyone's radar and there is no visibility into the comments an auditor may have made.

Oh, thanks! I'm not sure how I didn't find that one...

See T10376 for a related issue with diagnostics.

In T10274#157022, @epriestley wrote:

Where's the other request for this?

I was referring to T9973

The motivation was this scenario:

You can land onto a remote branch with a local copy of the branch which isn't really that branch but is just some other branch which you've named the same thing as the other branch.

We can't do that if we actually write onto Release1. The original task or tasks are linked from T9657. The new behavior generally remedies a large range of scenarios where users were confused by arc land writing to Release1 when they did not expect it to.

I think that makes sense for arc land, and as odd a workflow as I find it, I understand the need to support it, but arc land --hold implies modification of local repository, no? Previously arc land --hold was a legitimate workflow that put the user's repository in a very predictable and easy to understand state. Now the end result feels like some process got interrupted in the middle and is really not useful for anything other than pushing. It even kind of looks like a repository that is left in the middle of a failed rebase, with detached heads and unmerged branches. It also leaves garbage branches behind that the user now needs to go clean up manually, and since the branches do not appear merged into trunk, it needs to be a force delete, which is always a little bit of a scary operation, especially for people not experienced with git.
>

See also T8227.

I'm very much with you on that one. That's why I'm suggesting assuming a certain intent behind using arc land --hold and Doing The Right Thing(TM).

You can use arc alias to define a new command which runs arc land --hold --onto X && git branch -f X HEAD or similar.

Yeah, but that requires us to specify the branch to land onto, which is something arc already knows from .arcconfig.

Yes, that will work.

thanks

and you beat me to it.

Sorry, I accidentally submitted in the middle of typing :)

Sure. Is there a task tracking this work so I know what to expect?

I belive in our case it may have been a case of bad SSL config.
It no longer reproduces on our setup.

T8131 is kinda related here.

Maybe collapsing older comments when you get past some threshold similar to what is done in the section of the review and providing a per-thread "show older comments" control or something similar would make sense here?

Maybe collapsing older comments when you get past some threshold similar to what is done in the section of the review and providing a per-thread "show older comments" control or something similar would make sense here?

In the image above, I (anton) am the owner of both "Viewing Pane" and "Header Display" packages.

That doesn't seem to do it. I still get "Partially Audited".

Other than bin/audit delete, is there any other way for clearing this (preferably automatic)?

Also Collaborator, advertises porting comments forward specifically as one of their key features in their demos.

In T7447#100156, @btrahan wrote:

I think it might be helpful to provide examples of how this works in these other tools, particularly the failure cases epriestley outlined. In my minds eye the times we would fail to port it forward are many and would ultimately yield more confusion than the current tool.

Unless this is a very recent addition, ReviewBoard doesn't port comments forward, as far as I know. What they do provide is a separate comment digest section where inline comments are threaded, so you can continue the conversation that started on a particular diff even after a new diff is uploaded. I personally find it kind of awkward because you basically end up with your inline comments separated from code and you have 2 separate views of your review. But pretty much everyone else on my team has specifically pointed out as a feature they need the most. So much so that the decision was made to still use ReviewBoard for large reviews that are expected to have iterations.

For us the small reviews like you guys mostly have don't present a problem. Where we get into issues is when we start developing more comprehensive features and reviews start taking a form of a discussion. Our policy is to have 2 reviewers sign off on the review, so we commonly get 3 way conversations in reviews going and we get into issues where multiple iterations are needed AND a discussion continues. For example, if I submit a review and some minor issues come up that I can easily fix, but also a larger discussion starts on one of the changed areas, fixing those small issues and submitting a new diff essentially makes it impossible to follow the (usually more important) longer discussion because it is now broken up across multiple revisions. This is the single most common complaint I hear from my team - there's actually no way to see the entire flow of the conversation.

Fair enough. I got a new certificate that doesn't require a passphrase and it's working as expected now, except now I'm seeing T7435

administrator@ubuntu:/phabricator/phabricator$ ll /var/tmp/
total 12
drwxrwxrwt 3 root root 4096 Feb 27 18:32 ./
drwxr-xr-x 14 root root 4096 Oct 24 18:31 ../
drwxrwxr-x 4 phd-user phd-user 4096 Oct 23 14:46 phd/

Just an idea :). Thanks!

That’s totally understandable. I was under the impression that you discourage local forks though. Seeing that all of my requests seem to converge onto T1460, if I wanted to help out with that how would I go about it?

Put this in the wrong task, sorry :).

In T6873#90197, @anton.vladimirov wrote:

Here’s an example. There are 2 conversations and some one-offs going on here, but trying to follow what is being discussed is already fairly difficult. This is a mild example - it fit on one screen.

Here’s an example. There are 2 conversations and some one-offs going on here, but trying to follow what is being discussed is already fairly difficult. This is a mild example - it fit on one screen.

Sounds like this article might be helpful -- https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/

No doubt! And I personally agree, but the team has been doing it this way for the better part of a decade, and trying to convince a group of people that they need to change the way they do things to fit the new better tool we installed is basically a non-starter :). Just getting them to try switching to a different tool was a challenge.

Sure, but the problem my team is having is actually the opposite of “these inline comments are a queue of things to get done”. Differential actually works really well for us when we’re converging a feature and doing a bunch of small targeted changes, but things tend to fall apart for larger reviews that take some online discussion and multiple iterations, like rewriting a component, for example. Unfortunately, my team tends to not incrementally iterate when doing large changes and we often get reviews in 100s of lines of code. I realize that this isn’t the best practice, but this is what they’re used to doing and Review Board supported this habit.

The closest I could find by reading through this stuff is T5654, which funnily enough is exactly what I was asking for in T6873.
There is a mention of this in that task, but that's about it. It's not really obvious that fixing either T1460 or T5654 will also address this. T1460 seems like it has grown into a monster that will address all inline comment issues/concerns/suggestions, though.

I agree about that "new" thing.
Do you guys have plans to persist threaded view in code itself through rev updates?

This would be especially useful in conjunction with T6874

I just realized that it would be even cooler if we could extend arc land with our own custom extra processing so that arc land is all I need to do and it will do the right thing depending on the project/branch I'm in.

In T731#32693, @epriestley wrote:

Note that part of the reason this is a low priority is that everyone who wants it has subtly different requirements -- the original requestor wanted "all reviewers", you want "at least two reviewers", another requestor suggested "a reviewer from project A and a reviewer from project B", and the earliest version of this request was at Facebook and amounted to a less extreme version of the symbolic expression language implemented in D4697.