Change Details

See PHI1506. When you `arc land`, we invoke `arc close-revision --finalize` to close the revision after pushing it. `arc close-revision --finalize` bails out without doing anything if: (A) the repository is known to Phabricator; or (B) the revision is not "Accepted". Almost all `arc land` workflows hit case (A), where the repository is tracked on the server and `arc` can figure out which tracked repository the working copy corresponds to (that is, `arc which` reports a matching repository). In this case, Phabricator closes the revision on the server side with "alice closed this revision by committing rXYZaabbccdd." This is the preferred workflow and overwhelmingly the most common workflow. However, if the repository isn't tracked or we fail the lookup, we may also declined to close the revision if the state is not currently "Accepted". This approximately traces back to D1104, although it looks like the sequence of actions was: - The server changed to prevent closing revisions from a state other than "Accepted". - This broke `arc land` on non-Accepted revisions during `arc close-revision --finalize` (which was `arc mark-committed --finalize` at the time). - D1104 fixed this by just skipping the close step if it wouldn't work. In the same flow, the revision //is// closed if things are handled on the server, with a "closed from the wrong state" warning since D18808. Confusion can arise from this discrepancy between the behavior of the server-side flow and the client-side flow. Although ideal behavior is probably that the server and client do the same thing, I'm hesitant about supporting an "invalid" API transition to "Closed", at least with this as the only justification. It's hard to put the "invalid transitions are legal now" cat back in the bag, and would be somewhat tricky to even write the client logic to deal with this gracefully (since the server may be running older code and not accept the transaction). There's probably very little peril here, but just printing a message explaining that the step is being skipped, at least for now, is even less perilous.

See PHI1506. When you `arc land`, we invoke `arc close-revision --finalize` to close the revision after pushing it. `arc close-revision --finalize` bails out without doing anything if: (A) the repository is known to Phabricator; or (B) the revision is not "Accepted". Almost all `arc land` workflows hit case (A), where the repository is tracked on the server and `arc` can figure out which tracked repository the working copy corresponds to (that is, `arc which` reports a matching repository). In this case, Phabricator closes the revision on the server side with "alice closed this revision by committing rXYZaabbccdd." This is the preferred workflow and overwhelmingly the most common workflow. However, if the repository isn't tracked or we fail the lookup, we may also decline to close the revision if the state is not currently "Accepted". This approximately traces back to D1104, although it looks like the sequence of actions was: - The server changed to prevent closing revisions from a state other than "Accepted". - This broke `arc land` on non-Accepted revisions during `arc close-revision --finalize` (which was `arc mark-committed --finalize` at the time). - D1104 fixed this by just skipping the close step if it wouldn't work. In the same flow, the revision //is// closed if things are handled on the server, with a "closed from the wrong state" warning since D18808. Confusion can arise from this discrepancy between the behavior of the server-side flow and the client-side flow. Although ideal behavior is probably that the server and client do the same thing, I'm hesitant about supporting an "invalid" API transition to "Closed", at least with this as the only justification. It's hard to put the "invalid transitions are legal now" cat back in the bag, and would be somewhat tricky to even write the client logic to deal with this gracefully (since the server may be running older code and not accept the transaction). There's probably very little peril here, but just printing a message explaining that the step is being skipped, at least for now, is even less perilous.

See PHI1506. When you `arc land`, we invoke `arc close-revision --finalize` to close the revision after pushing it. `arc close-revision --finalize` bails out without doing anything if: (A) the repository is known to Phabricator; or (B) the revision is not "Accepted". Almost all `arc land` workflows hit case (A), where the repository is tracked on the server and `arc` can figure out which tracked repository the working copy corresponds to (that is, `arc which` reports a matching repository). In this case, Phabricator closes the revision on the server side with "alice closed this revision by committing rXYZaabbccdd." This is the preferred workflow and overwhelmingly the most common workflow. However, if the repository isn't tracked or we fail the lookup, we may also declined to close the revision if the state is not currently "Accepted". This approximately traces back to D1104, although it looks like the sequence of actions was: - The server changed to prevent closing revisions from a state other than "Accepted". - This broke `arc land` on non-Accepted revisions during `arc close-revision --finalize` (which was `arc mark-committed --finalize` at the time). - D1104 fixed this by just skipping the close step if it wouldn't work. In the same flow, the revision //is// closed if things are handled on the server, with a "closed from the wrong state" warning since D18808. Confusion can arise from this discrepancy between the behavior of the server-side flow and the client-side flow. Although ideal behavior is probably that the server and client do the same thing, I'm hesitant about supporting an "invalid" API transition to "Closed", at least with this as the only justification. It's hard to put the "invalid transitions are legal now" cat back in the bag, and would be somewhat tricky to even write the client logic to deal with this gracefully (since the server may be running older code and not accept the transaction). There's probably very little peril here, but just printing a message explaining that the step is being skipped, at least for now, is even less perilous.