Repository navigation
Fix what the last review before 2.0 found - #263
Merged
Merged
Conversation
The configuration of a repository may name a command in core.fsmonitor, which git runs to read the working tree, and even to diff a commit: getUntrackedFiles(), the diffs of the working copy and Commit::getDiff() ran it. CliDriver sets core.fsmonitor back to false, on every command. The doc said safe.directory applies: it doesn't, as CliDriver runs git with --git-dir, which git trusts, whoever owns the repository. Its hooks and the filters of its .gitattributes still run, like with git: the doc says to only open a repository whose .git directory is trusted.
LibGit2Driver checks whether the URL of a clone is a bundle with is_file(), which hands a URL to the stream wrapper of its scheme: for "ftp://user:password@host/...", PHP connected to the host and sent the credentials, before libgit2 saw the URL. With "ssh://" or "git://", PHP warned that no wrapper exists, which error handlers turn into an exception whose trace held the URL. Only a local path, "file://" included, is looked at, and the URL is a #[\SensitiveParameter].
An "fd::7" URL makes git talk through file descriptors it inherits from PHP: an open file, the socket of a database... isValidRepository() of such a URL, which an application may get from its users, made git wait on a socket of the application until it timed out, an hour by default. Both drivers refuse it with an InvalidArgumentException, like an "ext::" URL.
The policy promised 12 months of support to the previous major release, but 1.x has no branch, and gets no fix for the security issues 2.0 fixes. It now says only 2.x gets security fixes, and keeps the promise for the next major releases.
…ased WorkingCopy::merge() came with #248, after 1.6.0: no release has its array of arguments of git merge ([], ["--no-ff"] or ["--ff-only"]), which 2.0 deprecated in favor of FastForward. 2.0 would have shipped a deprecation of its first day, and a FastForward|array argument for all of 2.x. merge() takes a FastForward only. The deprecation, its section of UPGRADE-2.0.md and its line in the CHANGELOG go.
A negative offset or limit gave wrong data, different with each driver: withLimit(-1) counted -1 commits, and iterated the whole history with CliDriver, but one commit with LibGit2Driver. withOffset(-5) counted more commits than it iterated. Log, Repository::getLog() and Revision::getLog() throw an InvalidArgumentException for one. A limit of 0 gives no commit, with both drivers.
Hooks threw an InvalidArgumentException for a hook that doesn't exist in get(), but a RuntimeException in remove(), where the UPGRADE announced a RuntimeException for both. remove() throws an InvalidArgumentException too, which is still the LogicException 1.x threw. getSizeInBytes() let PHP's UnexpectedValueException out for a directory it can't read, or which git deletes while it counts, like the quarantine of a push. It walks the git directory itself: a directory git deleted counts for nothing, and one it can't read throws a RuntimeException of gitlib, like a file.
git reads "@" as HEAD since 1.8.5, alone or followed by "~", "^" or
":". libgit2 only reads it alone, and Revision::getCommit() always adds
"^{commit}": getRevision('@'), getLog('@~2..@'), getDiff('@~1', '@'),
checkout('@') or merge('@') threw a ReferenceNotFoundException with
LibGit2Driver, but not with CliDriver.
LibGit2Driver reads it as HEAD before libgit2 parses the revision, and
checkout('@') leaves HEAD attached, like checkout('HEAD') and git since
2.45.
A log without revisions is the one of every reference and HEAD, like "git log --all". LibGit2Driver only walked from the references: with a detached HEAD on a commit no reference has, like after the checkout of a hash, the log lacked commits, and count() was wrong, without error.
Commit::getIncludingBranches() and Repository::getHead() read the branches from git, then looked them up in the references gitlib read before: a branch another process created or checked out since then made them throw a ReferenceNotFoundException, which a long-running worker meets. getIncludingBranches() also threw for every commit of a regular refs/remotes/origin/HEAD, which a mirror leaves, and which the references leave out. Both read the references again for a branch they don't know. getIncludingBranches() then leaves out a branch still unknown: the default branch of a remote, or one deleted meanwhile.
After a merge whose sides kept a file under two names, git blames a
commit under both, and "git blame -p" repeats its "previous" and
"filename" lines on each of its groups of lines: the parser only read
them on the first one, and threw ('Expected "\t", got "filename b"').
LibGit2Driver read it.
Old commits, imported from other tools, have authors and committers git wouldn't write anymore: "Name<email>", no date, an offset of 5 digits, no name nor email. git and libgit2 read them, but CliDriver required "Name <email> timestamp +hhmm" exactly, and one such commit made the whole log throw. ParserBase::parseSignature() reads them like libgit2, which LibGit2Driver reads them with: the email between the last "<" and ">", the name before it, and the date and offset optional, the epoch in UTC without them. Tags read their tagger with it too.
PushReference::isCreate() and isDelete() named what Diff\File names isCreation() and isDeletion(), whose isDelete() 2.0 deprecates for this: one concept, two names. PushReference gets isCreation() and isDeletion(), and isCreate() and isDelete() are deprecated. The argument of its constructor holding the full name of the reference is named $fullname, like getFullname().
gitlib required symfony/process ^6.4 || ^7.4 || ^8.1, but the versions before 6.4.33 and 7.4.5 escape the arguments wrongly on Windows (CVE-2024-51736, CVE-2026-24739). Recent Composer versions refuse to install them, so the job of the lowest dependencies tested 6.4.33, not the 6.4.0 gitlib declared. It requires ^6.4.33 || ^7.4.5 || ^8.1.
The doc invites to implement DriverInterface and RepositoryDriverInterface, in tests for instance, without saying whether 2.x may add methods to them: any new method would have broken every implementation. Both interfaces say gitlib may add methods in a minor version: the promise of backward compatibility covers the code calling them, not the drivers implementing them. The parsers of Driver\Cli\Parser, which only CliDriver uses, are internal.
- UPGRADE-2.0.md says an unknown revision throws a ReferenceNotFoundException, which a catch of RuntimeException doesn't catch, where getDiff(), getBlame() or checkout() threw a ProcessException; that getRawDiff() returns the output of "git diff --raw -z --patch", or nothing with LibGit2Driver; which parameters are renamed, for named arguments; and how ParserBase changes. Its examples import the classes they use. - doc/diff.md says what getRawDiff() returns. - The example of getHead() in doc/repository.md checks the class of HEAD, which PHPStan understands. - The differences of LibGit2Driver list the blame, which may attribute some lines to another commit, and ignores blame.ignoreRevsFile. - The doc says a password in a git:// URL, which takes no credentials, isn't hidden: git quotes such a URL without its scheme.
The entries of the work of #254 had no link, or the one of #253, and some were wrong for 2.0: - reftable repositories open with CliDriver only; - run() doesn't keep its "exact original behavior", but still returns the standard output; - Repository::cloneTo() read like a replacement of Admin, and $reload like an argument of ReferenceBag::update() and Branch::isMergedTo(), which are deprecated themselves; - the fix of #239 for the "debug" option set to false is gone: 2.0 refuses that option. The header stays "Unreleased", until the release date is known.
…y URL isValidRepository() and isValidRepositoryAndBranch() ran ls-remote in the working directory of PHP, which may be a repository, like the checkout of an application: git read its configuration, its rewrites of URLs for instance, and an empty URL named it, which was then valid with CliDriver, not with LibGit2Driver. git 1.8.4 can't even run in a linked worktree, which made them return false. ls-remote runs in the temporary directory, and both drivers refuse an empty URL with an InvalidArgumentException.
TagData held the object a tag points to, its type and the name of the tag, which nothing reads: the reference gives the object already. A driver implemented outside gitlib had to fill them anyway. TagData keeps the tagger, the message and the signature.
- A revision starting with a dash names the revision, without the
"^{commit}" gitlib adds: Revision::getCommit() checks it first.
- A git:// URL whose host starts with a dash names the command of
core.gitProxy, not ssh only.
- getIncludingBranches(false, false) says what to do, without "a
least".
- A timeout of one second is "1 second", and a timeout git reaches
while it starts is a ProcessTimedOutException, instead of a
DriverException holding the command line.
- A single unknown option of Repository is "Unknown option".
- getLastModification() of a path no commit changes names the path.
- The ProcessException of a merge in conflict has the first CONFLICT
line git writes on its output, instead of nothing.
Eleven deprecated methods ran in no test: File::addChange(), isChangeMode(), isDelete() and getAnchor(), FileChange::getCount(), Diff::setRepository(), PushReference::getReference() and getRevision(), Commit::getFixedShortHash(), Log::setOffset() and Repository::shell(). Nothing checked the message of the deprecation of CommitReference either. Nothing fixed their behavior until 3.0: each gets a legacy test, CommitReference in a process of its own, as Submodule declares its alias once loaded, and shell() through a command of ~/git-shell-commands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A last review of
mainbefore tagging 2.0: four checks in parallel (security and the CLI driver,LibGit2Driver, the API and the doc, the tests and the packaging) went over both previous audits, item by item, with both drivers and git 1.8.4. Every item of them is fixed or decided. This PR fixes what the review found, one commit each, with its tests and its doc.Security
core.fsmonitor, and say gitlib trusts the repository it opens. The configuration of a repository may name a command incore.fsmonitor, which git ran to read the working tree, and even to diff a commit.CliDriversets it back tofalse. The doc saidsafe.directoryapplies: it doesn't, asCliDriverpasses--git-dir. Its hooks and filters still run, like with git: only open a repository whose.gityou trust.LibGit2Driverchecked whether a clone URL is a bundle withis_file(), which hands a URL to the stream wrapper of its scheme: forftp://user:password@host/..., PHP connected to the host and sent the credentials.fd::URLs. git talks through the file descriptors it inherits from PHP, like the socket of a database, and waited on it until it timed out, an hour by default.symfony/processwithout advisories:^6.4.33 || ^7.4.5 || ^8.1(CVE-2024-51736, CVE-2026-24739, on Windows).API, before it is frozen
git mergeofWorkingCopy::merge(): Add WorkingCopy::merge() to support git merge #248 never was in a release, and 2.0 would have shipped their deprecation, and aFastForward|arrayargument for all of 2.x.Hooks::remove()of a missing hook throws anInvalidArgumentException, likeget(), andgetSizeInBytes()no longer lets PHP'sUnexpectedValueExceptionout.PushReference::isCreation()andisDeletion(), likeDiff\File;isCreate()andisDelete()are deprecated.Bugs
LibGit2Driverreads@as HEAD, followed by~,^or:too:getRevision('@~1'),getLog('@~2..@'),checkout('@')threw.LibGit2Driverwalks from HEAD too in a log of every reference: with a detached HEAD on a commit no reference has, commits were missing, without error.getIncludingBranches()andgetHead()threw after a write of another process, andgetIncludingBranches()for every commit of a regularrefs/remotes/origin/HEAD, as a mirror leaves it.CliDriver, after a merge whose sides kept a file under two names.Name<email>, or without date, made the whole log throw withCliDriver.Doc
ReferenceNotFoundException, which acatch (RuntimeException)doesn't catch;getRawDiff(); the renamed parameters, for named arguments;ParserBase; the imports of the examples; the example ofgetHead(); the blame ofLibGit2Driver; a password in agit://URL.Added after the first review
ls-remoteout of the working directory of PHP, and refuse an empty URL: git read the configuration of the repository PHP runs in, like its rewrites of URLs, and an empty URL named it.TagDatanothing reads: the object, its type and the name of the tag.^{commit}gitlib adds,core.gitProxyfor agit://URL, "1 second", a timeout while git starts, a single unknown option, the path ofgetLastModification(), and theCONFLICTline of a merge.CommitReference, ran in no test.Checked locally