I think the big trick on this is going to be making inline comments work somewhat-correctly in the extreme case of octopus merges.

Does upgrading past b21cd24341c6552a3fbd21305ca682a161129a6b resolve it?

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.

As an adversarial actor, I can use the proposed rule to avoid audit of dangerous changes like this:

The proposed heuristic (different author and committer) feels pretty shaky, since if you rebase some of your own commits they'll have the same author and committer. I would guess this is somewhat-common in mis-rebases from master?

My merge of T6557 here was questionable, but I think this is a case where the audits are basically correct and the remedy is better cleanup tools.

Sounds good, thanks.

On re-reading T3917 I agree it has approximately nothing to do with this, but see also T5338, where I think I go through a flow like this and it doesn't trigger an audit.

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.

Per above, not sure how to reproduce this.

I don't believe this report, but here goes:

I'm having some difficulty figuring out exactly what this and related reports are describing.

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.

I'm going to merge this into T3917, which I think is the broadest description of this issue.

I'm going to close this as "Wontfix". Although adding custom fields is something we may still do, I no longer expect to "rebuild" on top of that infrastructure.

D17221 adds an "Auditors" field to Herald.

making the old version of arc continue working as it did, if trivial, with new versions of the backend

based on the fact that 6 month old code worked fine until a backend upgrade last week, I'm guessing the breakage happened because of server-side changes

goes away after upgrading both to latest

No. This task covers documenting this syntax.

Is the information from above (T5132#69200) also available in the documentation in this comprehensive form?

After a quick look, ModularTransactions doesn't currently expose a shouldHideFormMail() hook and I may want to do something a little more tailored here, so fixing this isn't a one-liner.

This does not appear to be moving forward as a valid bug report which we can accept upstream.

For context, this is a side effect of the move to EditEngine in T11114.

Righty; I didn't want to create any extra noise. New ticket, ho.

Thanks, D17218 should fix this.

If you'd like us to look at it, please file a new issue following Contributing Bug Reports.

arcanist: ade25facfdf22aed1c1e20fed3e58e60c0be3c2b
libphutil: 9d85dfab0f532d50c2343719e92d574a4827341b
phabricator: 2d4eb460abb2ba34a944962c7cfea5741e099ff9

What branch you checkout - master or stable ?
Never mind, I found what you've done, sorry.

Wow.
With that charge you can retire at that moment ... ridiculous.
I'm pretty sure in lab clean env from scratch all will works flawlessly. The point is in working environment.
Should be some guide where to dig for a problem.
Anyway will try with new clean install and migration of real data.
And will inform my boss that my month salary some guys take it for an hour :)

The form still does nothing useful, but it's not a default any more

Closing per lack of feedback. If you figure out how to reproduce the bug, please let us know. Mostly, this looks like someone else (admin) changed the permissions on the application.

See also T3472.

Alright, then this is it. I think a visual indication would be helpful. But apart from that, this can be closed.

I can't reproduce this, so we can't move forward. Next steps:

I can not reproduce this. Here's what I did:

Fixed by D17188.

Closing for lack of feedback.

So, is it possible that the email field is shown when there is already an existing phabricator account with the same email address?