Differential D21686 Diff 51666 src/repository/state/ArcanistRepositoryLocalState.php

Changeset View

Standalone View

src/repository/state/ArcanistRepositoryLocalState.php

Show First 20 Lines • Show All 186 Lines • ▼ Show 20 Lines	abstract class ArcanistRepositoryLocalState
final public function getRestoreCommandsForDisplay() {		final public function getRestoreCommandsForDisplay() {
return $this->newRestoreCommandsForDisplay();		return $this->newRestoreCommandsForDisplay();
}		}

protected function canStashChanges() {		protected function canStashChanges() {
return false;		return false;
}		}

		/**
		* Stash uncommitted changes temporarily. Use {@method:restoreStash()} to
		* bring these changes back.
		*
		* Note that saving and restoring changes may not behave as expected if used
		* in a non-stack manner, i.e. proper use involves only restoring stashes in
		* the reverse order they were saved.
		*
		* @return wild A reference object that refers to the changes which were
		* saved. When restoring changes this should be passed to
		* {@method:restoreStash()}.
		*/
protected function saveStash() {		protected function saveStash() {
throw new PhutilMethodNotImplementedException();		throw new PhutilMethodNotImplementedException();
}		}

		/**
		* Restores changes that were previously stashed by {@method:saveStash()}.
		*
		* @param wild A reference object referring to which previously stashed
		* changes to restore, from invoking {@method:saveStash()}.
		epriestleyUnsubmitted Not Done Inline Actions This is neither here nor there, but my imagined "correct" use of this API is to treat the "$ref" as opaque and always return the "$ref" you were handed under every VCS. Git happens to ignore it today but may not in some future version of the API, i.e. this use of the API would be "incorrect" but is arguably somewhat-supported by the comments: $ref = $git->saveStash(); unset($ref); // Just throw the ref away, Git always returns true. // ... $git->restoreStash(true); // Just pass true, Git doesn't actually use this. I think this is really a bigger issue ("who is the audience for comments?") and I don't feel strongly about this, but I suspect if I was convinced that comments were worth getting right and tried to develop a guide for them, I'd probably end up with guidance like "don't put implementation details in a doc comment far away from the implementation?" and maybe "instead, put them inline next to the implementation" perhaps with a side of "annotate them in such-and-such a way and the documentation generator will extract them". epriestley: This is neither here nor there, but my imagined "correct" use of this API is to treat the…
		cspeckmimAuthorUnsubmitted Done Inline Actions I agree with `$ref` being opaque and will update the comments here. At the time I think was more keen to validate my own understanding by writing it out without second-guessing whether it made sense to put the implementation-level details at this abstract level. cspeckmim: I agree with `$ref` being opaque and will update the comments here. At the time I think was…
		cspeckmimAuthorUnsubmitted Done Inline Actions Ah though one point about this is that it does impact how Git repositories interract with this API. Someone following this API currently might assume that the following is valid, however it turns out to work properly with Mercurial but not in Git. save -> ref1 save -> ref2 restore(ref1) restore(ref2) It might not cause an error but the behavior would be unexpected. I think that was my original intention behind the first note on `saveStash()`. I'll clean up some of the details about what specifically is being returned but I think it would be useful to retain some notice about Git's behavior at least until (or if) the implementation supports that. cspeckmim: Ah though one point about this is that it does impact how Git repositories interract with this…
		cspeckmimAuthorUnsubmitted Done Inline Actions I think I'll make a new diff which updates `GitBinaryAnalyzer` to check for this better stash usage, and if not uses an internal stack to ensure that we don't accidentally run a restore out of order cspeckmim: I think I'll make a new diff which updates `GitBinaryAnalyzer` to check for this better stash…
		*/
protected function restoreStash($ref) {		protected function restoreStash($ref) {
throw new PhutilMethodNotImplementedException();		throw new PhutilMethodNotImplementedException();
}		}

protected function discardStash($ref) {		protected function discardStash($ref) {
throw new PhutilMethodNotImplementedException();		throw new PhutilMethodNotImplementedException();
}		}

▲ Show 20 Lines • Show All 53 Lines • Show Last 20 Lines