Summary: Ref T13276. This edge is pointed the wrong way. Point it the right way.
Test Plan: Will verify production works better.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20490
Summary:
Depends on D20469. Ref T13276. See PHI1159. See PHI953. See PHI901.
Allow Herald to detect when "arc land" would (or did) warn users about failed or ongoing builds. This respects the "Warn on Landing" build plan behavior.
To accomplish this:
- When we close a revision, set a "wrong build state" flag if it lands in the wrong build state.
- If the revision is closed when we hit Herald, look for the flag.
- If not (common for push rules, can happen for commit rules if we race against the revision update worker), hit Harbormaster ourselves and check the current state.
Test Plan:
- Wrote a "Require Green" rule.
- Ran it against various commits with various build states (good, not good).
- Fiddled with "Warn on Landing" and saw the effect in rule evaluation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20470
Summary:
Depends on D20468. Ref T13276. See PHI1008.
When a commit or revision "reverts <hash>", and that's the hash of a commit which has a revision, also write a "reverts" edge to the revision.
Test Plan:
Created "reverts X" objects for:
- a revision;
- a commit;
- a commit with a revision (both got marked properly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20469
Summary:
Depends on D20467. Ref T13277. Currently, the "MessageParserWorker" writes this property on commits, then Herald and Audit both read it.
Make them share code so this property has one writer and one reader. This property isn't great, but at least now the badness is hidden.
Currently, we can't just use edges because they may not have been written yet. I am likely to just do this, soon:
- Just write the edges (in "MessageParserWorker").
- Hide the edges from mail.
However, we'll sort-of lose the "revisionMatchData" explanation thing if I do that. Maybe this is fine? But when commits match because hashes match, it legitimately isn't obvious.
For now, just reduce the amount of harm/badness here.
Test Plan:
- Ran `bin/repository reparse --publish ...`.
- Ran a Herald "Audit" rule using the "Accepted Differential revision" field.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20468
Summary:
Depends on D20465. Ref T13277. Currently, when a commit is unpublished, we put a single line about it on the "Edit Commit" page. This is pretty much impossible to find.
Move it to the main page. This treatment is more big/bold than I'd probably like to end up, but we should probably overshoot on the explanatory text until users get used to this behavior.
Also, allow searching for only published / unpublished commits.
Test Plan: {F6395705}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20466
Summary:
Depends on D20464. Ref T13277. Broadly:
- Move all the "should publish X" and "why aren't we publishing X" stuff to a separate class (`PhabricatorRepositoryPublisher`).
- Rename things to be more consistent with modern terminology ("Publish", "Permanent Refs").
Test Plan:
This could use some trial-by-fire on `secure`, but:
- Grepped for all symbols.
- Viewed various commits.
- Reparsed commits.
- Here's a commit with an explanation:
{F6394569}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20465
Summary: Depends on D20463. Ref T13277. This flag was added some time before 2015 and I don't think I've ever used it. Just get rid of it.
Test Plan: Grepped for `force-autoclose`, `forceAutoclose`, `AUTOCLOSE_FORCED`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20464
Summary:
Depends on D20462. Ref T13276. Currently, the "Message" parser also updates related tasks and revisions when a commit is published.
For PHI1165, which ran into a race with message parsing, I originally believed we needed to separate this logic and lock + yield to avoid the race. D20462 provides what is probably a better approach for avoiding the race.
Still, I think separating these "update related revisions" and "updated related tasks" chunks into separate workers is a net improvement. There may still be some value in doing lock + yield in the future to deal with other issues, and when we occasionally run into problems with pulling a diff out of the repository to update the revision (usually because the diff is too big) this isolates the problem better and allows the commit to import.
I think the only thing to watch out for here is that Herald may now run before the revision and commit are attached to one another. This is fine for all current Herald rules, we just need to be mindful in implementing new rules.
Test Plan: Used `bin/repository reparse --message` on various commits, including commits that close revisions and close tasks.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20463
Summary: Depends on D20457. Ref T13276. Kill all remaining callers to this method and delete it.
Test Plan:
- Grepped for `loadIDsByCommitPHIDs`.
- Viewed blame again to make sure I didn't break it.
- Viewed "History" view for commits with revisions.
- Viewed "Graph" view for commits with revisions.
- Viewed "Merged Commits" table for commits with revisions.
- Viewed "Compare" table for commits with revisions.
- Viewed "Repository" main page history table for commits with revisions.
- Grepped for `linkRevision`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20458
Summary:
Ref T13276. Differential has a pre-edge "TABLE_COMMIT" with about a half-dozen weird callers I'd like to get rid of.
Move blame to use edges instead. (Bonus: this makes blame respect edge edits in the UI.)
Since there are some more callers to clean up this code may move into some "RelatedObjectQueryThing" class or something, but I'm taking it one step at a time for now.
Test Plan: {F6394106}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20457
Summary:
Ref T13280. In D20421, I changed our observe strategy to `git fetch <uri>` in all cases.
This doesn't work in an ancient, non-bare repository if `master` is checked out and `master` is also fetch: `git` refuses to overwrite the local ref unless we pass `--update-head-ok`. Pass this flag.
Also, remove some code which examines branches and tags in a special way for non-bare working copies. The old `git fetch <origin>` code without explicit revsets meant that `refs/remotes/orgin/heads/xyz` got updated instead of `refs/heads/xyz`. We now update our local refs in all cases (bare and non-bare) so we can throw away this special casing.
Test Plan:
- Replaced a modern bare working copy with a non-bare working copy by explicitly using `git clone` without `--bare`.
- Ran `bin/repository update`, hit the `--update-head-ok` error. Applied the patch, got a clean update.
- Used the "repository.branchquery" API method...
- ...with "contains" to trigger the "git branch" case. Got identical results after removing the special casing.
- ...without "contains" to trigger the "low level ref" case. Got identical results after removing the special casing.
- Grepped for `isWorkingCopyBare()`. The only remaining callsites deal with hook paths, and genuinely need to be specialized.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13280
Differential Revision: https://secure.phabricator.com/D20450
Summary:
Depends on D20433. Ref T13277. Since "Autoclose" no longer exists, update the documentation.
Currently, this documentation focuses a lot on troubleshooting because users historically had a lot of trouble with figuring out why things were or were not autoclosing. I haven't seen any real confusion about this in years, so I suspect we may have improved the import pipeline and/or UI to make this less of a problem.
It's also possible that this document "fixed" the problem, but usually I expect a documentation fix to not affect the frequency of reports, just make them easier to resolve, so I doubt it.
If unclear things remain //and// documentation really did fix it, maybe we can fix the issues. Or we can just put the troubleshooting documentation back.
Test Plan: Read documentation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20434
Summary:
Depends on D20432. Ref T13277. Fixes T12967. Removes some "Track Only" hints and warns that the feature is deprecated in favor of "Permanent Refs" and "Fetch Only".
(This "fixes" T12967 by mooting it.)
Test Plan: Viewed "branches" sectino of the manage UI, edited "braches" section of a repository.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277, T12967
Differential Revision: https://secure.phabricator.com/D20433
Summary:
Depends on D20426. Ref T13277. The new behavior is to fire Herald only once a commit becomes reachable from a permanent ref (previously, an "Autoclose" branch).
That means that every "Commit" Herald rule implicitly has this field as a condition, and it no longer does anything.
Test Plan: Wrote a Herald rule, saw this as an option in the "Deprecated" section.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20427
Summary:
Depends on D20424. Ref T13277. Now that the "Actions" panel only has one item ("Publishing"), just move it to the "Basics" panel.
Update the UI to show active/publishing status more clearly and relate them to one another and importing state.
Test Plan: {F6378087}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20425
Summary:
Depends on D20423. Ref T13277. Repositories currently have separate toggles for "Autoclose" and "Publishing".
Merge the "Autoclose" toggle into the "Publishing" toggle. I'm unaware of any valid use case for enabling one but not the other.
(This doesn't fix all the documentation, yet.)
Test Plan: Edited a repository, saw only one publishing option.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20424
Summary:
Depends on D20422. Ref T13277. Currently, "track only", "publish", and "autoclose" are three separate ideas. I'd like to generally merge them into a more natural idea called "permanent refs".
Since "Autoclose" effectively now controls both "autoclose" and "publish", rename it.
This doesn't rename all the methods or internals, and the documentation needs an update, but it renames most of the UI-facing stuff.
(You also can only specify branches as "Permanent Refs" today, but we may let you specify tags and other arbitrary refs in the future.)
Test Plan: Grepped, poked around the UI, saw UI show "Permanent" / "Permanent Refs" more often and "Autoclose" less.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20423
Summary:
Depends on D20421. Ref T13277. I'd generally like to move away from "Track Only".
Some of the use cases for "Track Only" (or adjacent to "Track Only") are better resolved with "Fetch Rules" -- basically, rules to fetch only some subset of refs from the observed remote.
Add configurable "Fetch Rules" for Git repositories. For example, if you only want to fetch `master`, you can now speify:
```
refs/heads/master
```
If you only want to fetch branches and tags, you can use:
```
refs/heads/*
refs/tags/*
```
In theory, this is slightly less powerful in the general case than "Track Only", but gives us better behavior in some cases (e.g., when the remote has 50K random temporary branches). In practice, I think this and a better "Autoclose Only" will let us move away from "Track Only", get default behavior which is better aligned with what users actually expect, and dodge all the "track tags/refs" questions.
Test Plan: Configured repositories with "Fetch Refs" rules, used `bin/repository pull --verbose --trace ...` to run pulls, saw expected pull/fetch behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20422
Summary:
Depends on D20418. Ref T13277. Fixes T11314.
Currently, when you push commits to some arbitrary ref or tag (like `refs/pull/123` on GitHub, `refs/tags/phabricator/diff/123` on Phabricator, or `refs/changes/whatever` on Gerrit), we do not "autoclose" related objects. This means that we don't process `Ref T123` to create links to tasks, and don't process `Differential Revision: xyz` to close revisions.
However, we //do// still publish these commits. "Publish" means: trigger audits, publish feed stories, and run Herald rules.
- Stop publishing these commits.
- In the UI, show these commits as "Not Permanent" with a note that they are "Not [on any permanent branch]."
These commits will publish and autoclose if they ever become reachable from an "autoclose" ref (most commonly, if they are later merged to "master").
Test Plan:
- Pushed a commit to `refs/tags/quack`.
- Before: got a feed story.
- After: no feed story, UI shows commit as "Not Permanent".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277, T11314
Differential Revision: https://secure.phabricator.com/D20419
Summary:
Depends on D20381. Ref T8093. This makes minor improvements to the protocol proxy to handle cases where we add, remove, or replace refs and may need to move the "capabilities" section.
Rather than invoking a callback on every ref: parse the whole ref list into a data structure, mutate it if necessary (in a future diff), then dump it back into wire format.
This allows us to shift the capabilities data (which needs to be coupled with the first ref) around if we modify the first ref, and reorder the reflist alphabetically like git does.
When the server has no refs, Git sends no capabilities data. This is easy to emulate, just surprising.
Test Plan:
Tested the cases not covered by D20381:
- Fetching where the fetch actually fetches data.
- `ls-remote` when we hide the first ref (capabilities data properly moves to the first visible ref).
- `ls-remote` when the remote is empty (we just drop the capabilities frame completely).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8093
Differential Revision: https://secure.phabricator.com/D20436
Summary:
Depends on D20380. Ref T8093. When prototypes are enabled, inject a (hopefully?) no-op proxy into the Git wire protocol.
This proxy decodes "git upload-pack" and allows the list of references to be rewritten, in a similar way to how we already proxy the Subversion protocol to rewrite URIs and proxy the Mercurial protocol to distinguish between read and write operations.
The piece we care about comes at the beginning, and looks like this:
```
<frame-length><ref-hash> <ref-name>\0<server-capabilities>\n
<frame-length><ref-hash> <ref-name>\n
<frame-length><ref-hash> <ref-name>\n
...
<0000>
```
We can add, remove, or modify this section to make it appear that the server has different refs than the refs that exist on disk.
Things I have tried:
- `git ls-remote`
- `git ls-remote` where the server hides some refs.
- `git fetch` where the fetch is a no-op.
Things I have not tried:
- `git fetch` where the fetch is not a no-op.
- Tricking things into doing protocol v2. Or: I tried this, I wasn't successful. In v2, additional "\0" tricks are used to hide data in the capabilities, I think?
- `git ls-remote` where we rewrite/hide the first ref in the list, and need to move the capabilities frame elsewhere.
- `git ls-remote` where the server has no refs at all, or we remove every ref.
So the "interesting" piece of this works, but it almost certainly needs some cleanup to survive interaction with the real world.
Test Plan: See above.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8093
Differential Revision: https://secure.phabricator.com/D20381
Summary:
Ref T8093. Support dumping the protocol bytes to a side channel logfile, as a precursor to parsing the protocol and rewriting protocol frames to virtualize refs.
The protocol itself is mostly ASCII text so the raw protocol bytes are pretty comprehensible.
Test Plan:
{F6363221}
{F6363222}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8093
Differential Revision: https://secure.phabricator.com/D20380
Summary:
Depends on D20419. Ref T13277. Fixes T8936. Fixes T9383. Fixes T12300. When you push arbitrary refs to Phabricator, the push log currently complains if those refs are not tags or branches.
Upstream Git now features "notes", and there's no reason to prevent writes to arbitrary refs, particularly beause we plan to start using them soon (see T13278).
Allow these writes as affecting raw refs.
Test Plan:
- Pushed an arbitrary ref.
- Pushed some Git notes.
- Wrote a Herald ref rule, saw "ref" in the dropdown.
{F6376492}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277, T8936, T9383, T12300
Differential Revision: https://secure.phabricator.com/D20420
Summary:
Depends on D20412. See PHI1147.
- Index the targets of "Add Reviewer", "Add Blocking Reviewer", "Add Auditor", "Add Subscriber", and "Remove Subscriber" Herald rules. My major goal is to get Owners packages. This will also hit projects/users, but we just don't read those edges (for now, at least).
- Add a "Related Herald Rules" panel to Owners Package pages.
- Add a migration to reindex Herald rules for the recent build plan stuff and this, now that such a migration is easy to write.
Test Plan:
Ran migration, verified all rules reindexed.
{F6372695}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20413
Summary:
Fixes T13270. In Diffusion, the "Code" tab is linked in a weird way that isn't consistent with the other tabs.
Particularly, if you navigate to `x/y/z/` and toggle between the "Branches" and "History" tabs (or other tabs), you keep your path. If you click "Code", you lose your path.
Instead, retain the path, so you can navigate somewhere and then toggle to/from the "Code" tab to get different views of the same path.
Test Plan: Browed into a repository, clicked "History", clicked "Code", ended up back in the place I started.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13270
Differential Revision: https://secure.phabricator.com/D20323
Summary:
Depends on D20291. Ref T13259. Move all the simple cases (where paging depends only on the partial object and does not depend on keys) to a simple wrapper.
This leaves a smaller set of more complex cases where we care about external data or which keys were requested that I'll convert in followups.
Test Plan: Poked at things, but a lot of stuff is still broken until everything is converted.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20292
Summary:
See PHI1123. The key on this table is `<resource, type, code>` but we currently query for only `<type, code>`. This can't use the key.
Constrain the query to the resource we expect (the repository) so it can use the key.
Test Plan: Pushed files using LFS. See PHI1123 for more, likely.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20261
Summary:
Ref T13121. When you connect to a host with SSH, don't already know the host key, and don't have strict host key checking, it prints "Permanently adding host X to known hosts". This is super un-useful.
In a perfect world, we'd probably always have strict host key checking, but this is a significant barrier to configuration/setup and I think not hugely important (MITM attacks against SSH hosts are hard/rare and probably not hugely valuable). I'd imagine a more realistic long term approach is likely optional host key checking.
For now, try using `LogLevel=ERROR` instead of `LogLevel=quiet` to suppress this error. This should be strictly better (since at least some messages we want to see are ERROR or better), although it may not be perfect (there may be other INFO messages we would still like to see).
Test Plan:
- Ran `ssh -o LogLevel=... -o 'StrictHostKeyChecking=no' -o 'UserKnownHostsFile=/dev/null'` with bad credentials, for "ERROR", "quiet", and default ("INFO") log levels.
- With `INFO`, got a warning about adding the key, then an error about bad credentials (bad: don't want the key warning).
- With `quiet`, got nothing (bad: we want the credential error).
- With `ERROR`, got no warning but did get an error (good!).
Not sure this always gives us exactly what we want, but it seems like an improvement over "quiet".
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13121
Differential Revision: https://secure.phabricator.com/D20240
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary: Ref T13250. See D20149. In a number of cases, we use `setQueryParams()` immediately after URI construction. To simplify this slightly, let the constructor take parameters, similar to `HTTPSFuture`.
Test Plan: See inlines.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20151
Summary: See D20136. This method is sort of inherently bad because it is destructive for some inputs (`x=1&x=2`) and had "PHP-flavored" behavior for other inputs (`x[]=1&x[]=2`). Move to explicit `...AsMap` and `...AsPairList` methods.
Test Plan: Bit of an adventure, see inlines in a minute.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20141
Summary:
Ref T13244. See PHI1055. (Earlier, see D20091 and PHI1047.) Previously, we expanded the Owners package autoreview rules from "Yes/No" to several "Review (Blocking) If Non-Owner Author Not Subscribed via Package" kinds of rules. The sky didn't fall and this feature didn't turn into "Herald-in-Owners", so I'm comfortable doing something similar to the "Audit" rules.
PHI1055 is a request for a way to configure slightly different audit behavior, and expanding the options seems like a good approach to satisfy the use case.
Prepare to add more options by moving everything into a class that defines all the behavior of different states, and converting the "0/1" boolean column to a text column.
Test Plan:
- Created several packages, some with and some without auditing.
- Inspected database for: package state; and associated transactions.
- Ran the migrations.
- Inspected database to confirm that state and transactions migrated correctly.
- Reviewed transaction logs.
- Created and edited packages and audit state.
- Viewed the "Package List" element in Diffusion.
- Pulled package information with `owners.search`, got sensible results.
- Edited package audit status with `owners.edit`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20124
Summary:
Ref T13244. See PHI1057. Currently, if you're a member of a lot of projects/packages, you can end up with a very large `commit.authorPHID IN (...)` clause in part of the "Active Audits" query, since your `alice` token in "Responsible Users: alice" expands into every package and project you can audit on behalf of.
It's impossible for a commit to be authored by anything but a user, and evidence in PHI1057 suggests this giant `IN (...)` list can prevent MySQL from making effective utilization of the `<authorPHID, auditStatus, ...>` key on the table.
Prefilter the list of PHIDs to only PHIDs which can possibly author a commit.
(We'll also eventually need to convert the `authorPHIDs` into `identityPHIDs` anyway, for T12164, and this moves us slightly toward that.)
Test Plan: Loaded "Active Audits" before and after change, saw a more streamlined and sensible `authorPHID IN (...)` clause afterwards.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20129
Summary:
Fixes T13192. See PHI1015. When you deactivate a repository, we currently stop serving it.
This creates a problem for intracluster sync, since new nodes can't sync it. If nothing else, this means that if you "ship of theseus" your cluster and turn nodes over one at a time, you will eventually lose the entire repository. Since that's clearly a bad outcome, support sync.
Test Plan:
Testing this requires a "real" cluster, so I mostly used `secure`.
I deactivated rGITTEST and ran this on `secure002`:
```
./bin/repository thaw --demote secure002.phacility.net --force GITTEST && ./bin/repository update GITTEST
```
Before the patch, this failed:
```
[2019-01-31 19:40:37] EXCEPTION: (CommandException) Command failed with error #128!
COMMAND
git fetch --prune -- 'ssh://172.30.0.64:22/diffusion/GITTEST/' '+refs/*:refs/*'
STDOUT
(empty)
STDERR
Warning: Permanently added '172.30.0.64' (RSA) to the list of known hosts.
phabricator-ssh-exec: This repository ("rGITTEST") is not available over SSH.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
```
After applying (a similar patch to) this patch to `secure001`, the sync worked.
I'll repeat this test with the actual patch once this deploys to `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13192
Differential Revision: https://secure.phabricator.com/D20077
Summary: Now that we have a nice function for this, use it to simplify some code.
Test Plan: Ran through the Duo enroll workflow to make sure signing still works.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20053
Summary:
Ref T13242. See <https://discourse.phabricator-community.org/t/out-of-range-value-for-column-deviceversion/2218>.
The synchronization log column is `uint32?` and `-1` doesn't go into that column.
Since we're only using `-1` for convenience to cheat through `$max_version > $this_version` checks, use `null` instead and make the checks more explicit.
Test Plan: Reproducing this is a bit tricky and I cheated fairly heavily to force the code down this pathway without actually building a multi-device cluster, but I did reproduce the original exception, apply the patch, and observe that it fixed things.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20047
Summary:
In ~2012, the first of these options was added because someone who hates dogs and works at Asana also hated `[Differential]` in the subject line. The use case there was actually //removing// the text, not changing it, but I made the prefix editable since it seemed like slightly less of a one-off.
These options are among the dumbest and most useless config options we have and very rarely used, see T11760. A very small number of instances have configured one of these options.
Newer applications stopped providing these options and no one has complained.
You can get the same effect with `translation.override`. Although I'm not sure we'll keep that around forever, it's a reasonable replacement today. I'll call out an example in the changelog to help installs that want to preserve this option.
If we did want to provide this, it should just be in {nav Applications > Settings} for each application, but I think it's wildly-low-value and "hack via translations" or "local patch" are entirely reasonable if you really want to change these strings.
Test Plan: Grepped for `subject-prefix`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19993
Summary:
See PHI1014. We may not have Identities if you race the import pipeline, or in some other cases which are more "bug" / "missing migration"-flavored.
Load the commit data so we can fall back to it if we don't have identities.
Test Plan:
- Wiped out all my identities with `UPDATE ... SET authorIdentityPHID = NULL WHERE ...`.
- Before change: blame fataled with `Attempting to access attached data on PhabricatorRepositoryCommit (via getCommitData()), but the data is not actually attached.`.
- After change: blame falls back gracefully.
- Restored identities with `bin/repository rebuild-identities`, checked blame again.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19958
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary:
Ref T13217. See <https://discourse.phabricator-community.org/t/unsafe-raw-string-warnings-while-importing-git-commits/2191>.
Hunt down and fix two more `qsprintf()` things.
I just converted the "performance optimization" into a normal, safe call since we're dealing with far less SVN stuff nowadays and the actual issue has been lost in the mists of time. If it resurfaces, we can take another look.
Test Plan: Imported some commits, no longer saw these warnings in the daemon logs.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19869
Summary:
Ref T13222. See PHI995. Before making a change to inline rendering, consolidate this code for generating the "alice added inlines comments." and "alice marked X inlines as done." transactions.
Both Differential and Diffusion have four very similar chunks of code. Merge them into shared methods and reduce code duplication across the methods.
(In the next change, I plan to hide the "done" story when the mark affects your own inline, since users marking their own inlines as "done" is generally not very interesting or useful.)
Test Plan: As author and reviewer/auditor, added inlines in Differential and Diffusion. As author, marked own and others inlines as done and undone. Got sensible transaction rendering and persistence of "Done".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19858
Summary: Depends on D19856. Ref T13222. See D19829. Make access to "Track Only" slightly cleaner and more consistent..
Test Plan: Set, edited, and removed "Track Only" settings for a repository. Saw sensible persistence and display behaviors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19857
Summary:
Ref T13222. See D19829. We're inconsistent about using `getDetail()/setDetail()` to do some ad-hoc reads. Put this stuff in proper accessor methods.
Also a couple of text fixes from D19850.
Test Plan: Set, edited, and removed autoclose branches from a repository. Got sensible persistence and rendering behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19856
Summary:
See <https://discourse.phabricator-community.org/t/cannot-accept-commits-in-audit/2166/>.
In D19842, I changed `PhabricatorEditField->shouldGenerateTransactionsFromComment()`.
- Previously, it bailed on `getIsConduitOnly()`.
- After the patch, it bails on a missing `getCommentActionLabel()`.
The old code was actually wrong, and it was previously possible to apply possibly-invalid actions in some cases (or, at least, sneak them through this layer: they would only actually apply if not validated properly).
In practice, it let a different bug through: we sometimes loaded commits without loading their audit authority, so testing whether the viewer could "Accept" the commit or not (or take some other actions like "Raise Concern") would always fail and throw an exception: "Trying to access data not attached to this object..."
Fixing the insufficiently-strict transaction generation code exposed the "authority not attached" bug, which caused some actions to fail to generate transactions.
This appeared in the UI as either an unhelpful error ("You can't post an empty comment") or an action with no effect. The unhelpful error was because we show that error if you aren't taking any //other// actions, and we wouldn't generate an "Accept" action because of the interaction of these bugs, so the code thought you were just posting an empty comment.
Test Plan: Without leaving comments, accepted and rejected commits. No more error messages, and actions took effect.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: stephan.senkbeil, hskiba
Differential Revision: https://secure.phabricator.com/D19845
