Page MenuHomePhabricator

Make callsigns optional in arcanist
ClosedPublic

Authored by avivey on Mar 15 2016, 12:40 AM.

Details

Summary

Remove couple of references to callsigns:

  • arc which now prints repository name
  • getShouldAmend() can now use new format of commit name

a quick git-grep looks like the remaining references are all about repository.callsign config.

Ref T4245

Test Plan
  • arc which on a repository with no callsign
  • trigger requireCleanWorkingCopy(), see both "Do you want to amend this change" and "Do you want to create a new commit" prompts.
  • fire this diff with new code.

Diff Detail

Repository
rARC Arcanist
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avivey updated this revision to Diff 37293.Mar 15 2016, 12:40 AM
avivey retitled this revision from to Make callsigns optional in arcanist.
avivey updated this object.
avivey edited the test plan for this revision. (Show Details)
avivey added a reviewer: epriestley.
avivey updated this object.Mar 15 2016, 12:41 AM
avivey edited the test plan for this revision. (Show Details)
avivey edited edge metadata.
epriestley accepted this revision.Mar 15 2016, 2:12 AM
epriestley edited edge metadata.
epriestley added inline comments.
src/workflow/ArcanistWorkflow.php
2029

This is technically a backward compatibility break (the older looksoon method didn't have this parameter), but probably fine since looksoon is optional anyway.

This revision is now accepted and ready to land.Mar 15 2016, 2:12 AM
This revision was automatically updated to reflect the committed changes.
jparise added inline comments.
src/workflow/ArcanistWorkflow.php
1699–1700

Could we restore the protected getRepositoryCallsign() method? Without it, there doesn't appear to be a way for (custom) workflows to access the callsign value (because getRepositoryInformation() is private). The callsign is used by the owners.query conduit method, for example.

(I know about owners.search, which doesn't use the callsign, but that is still an unstable API.)

Despite the status, owners.search is probably more stable than owners.query at this point. Is there any technical reason you can't switch to owners.search?

The deprecated/unstable statuses don't do a great job of describing the actual state of the world right now, although there's some value in not having 15 statuses. But the internal state is something like this:

  • owners.query: Deprecated, just not marked as deprecated yet.
  • *.search: Mostly stable, but possibly still subject to minor changes because the underlying infrastructure is so sweeping and not yet adopted everywhere. These changes are unlikely to break backward compatibility unless your API wrapper fails explicitly when new fields appear in the response, but it's possible that, say, the format for some common attachment like subscribers or something is goofy. If you use part of the API and it actually works properly, it's probably safe, I just didn't want to over-promise these as fully stable since I don't want to commit to a broken subscribers on 30 different *.search methods or whatever if it turns out I did something dumb or overlooked something.

I can properly mark owners.query deprecated, at least.

Despite the status, owners.search is probably more stable than owners.query at this point. Is there any technical reason you can't switch to owners.search?

I don't think there's anything blocking that transition, although I haven't attempted it yet. I'll give that a look later today.

Are there any other non-deprecated conduit APIs with callsign parameters? The main thing I wanted to flag here is this change breaks (or at least hurts) compatibility with them.

I also made the "unstable" warnings a little better-aligned with reality in D15769 (less scary presentation, less absolute language).

Because callsigns are already optional, any method which relies on them was endangered previously by T4245: it's possible to create repositories today that methods like owners.query can not interact with. From inspection of conduit.query, I think this list of methods is:

  • diffusion.getrecentcommitsbypath (possibly has zero callers in existence anywhere, not apparently called in the upstream)
  • owners.query (here; obsoleted by owners.search)
  • repository.create (planned obsoletion by diffusion.repository.create)
  • repository.query (optional, planned obsoletion by diffusion.repository.search)
  • differential.query (minor, planned eventual obsoletion by differential.revision.search).