Improvements for Perforce tests
cover Improvements for Perforce tests
Hi,
I was recently trying to figure out whether the Perforce tests are memory-leak free, but noticed that the tests don't run at all on my machine because I have a newer version of Perforce than our CI. And because the on-disk depot format has changed since r21, tests are broken.
This small series adapts tests to work with both r21 and r23, updates the version of Perforce used in CI and then ultimately marks all of the Perforce tests as memory-leak free.
Thanks!
Patrick
Patrick Steinhardt (3):
t98xx: fix Perforce tests with p4d r23 and newer
ci: update Perforce version to r23.2
t98xx: mark Perforce tests as memory-leak free
ci/install-dependencies.sh | 2 +- t/t9800-git-p4-basic.sh | 14 ++++++++++++-- t/t9801-git-p4-branch.sh | 1 + t/t9802-git-p4-filetype.sh | 16 +++++++++++++--- t/t9803-git-p4-shell-metachars.sh | 1 + t/t9804-git-p4-label.sh | 1 + t/t9805-git-p4-skip-submit-edit.sh | 1 + t/t9806-git-p4-options.sh | 1 + t/t9808-git-p4-chdir.sh | 1 + t/t9809-git-p4-client-view.sh | 1 + t/t9810-git-p4-rcs.sh | 1 + t/t9811-git-p4-label-import.sh | 1 + t/t9812-git-p4-wildcards.sh | 1 + t/t9813-git-p4-preserve-users.sh | 1 + t/t9814-git-p4-rename.sh | 1 + t/t9815-git-p4-submit-fail.sh | 1 + t/t9816-git-p4-locked.sh | 1 + t/t9817-git-p4-exclude.sh | 1 + t/t9818-git-p4-block.sh | 1 + t/t9819-git-p4-case-folding.sh | 1 + t/t9820-git-p4-editor-handling.sh | 1 + t/t9821-git-p4-path-variations.sh | 1 + t/t9822-git-p4-path-encoding.sh | 1 + t/t9823-git-p4-mock-lfs.sh | 1 + t/t9825-git-p4-handle-utf16-without-bom.sh | 19 ++++++++++++++++--- t/t9826-git-p4-keep-empty-commits.sh | 1 + t/t9827-git-p4-change-filetype.sh | 1 + t/t9828-git-p4-map-user.sh | 1 + t/t9829-git-p4-jobs.sh | 1 + t/t9830-git-p4-symlink-dir.sh | 1 + t/t9831-git-p4-triggers.sh | 1 + t/t9832-unshelve.sh | 1 + t/t9833-errors.sh | 1 + t/t9834-git-p4-file-dir-bug.sh | 1 + t/t9835-git-p4-metadata-encoding-python2.sh | 1 + t/t9836-git-p4-metadata-encoding-python3.sh | 1 + 36 files changed, 74 insertions(+), 9 deletions(-)
Signature · 1 line
2.46.0.rc1.dirty
PATCH 1/3 t98xx: fix Perforce tests with p4d r23 and newer
Some of the tests in t98xx modify the Perforce depot in ways that the tool wouldn't normally allow. This is done to test behaviour of git-p4 in certain edge cases that we have observed in the wild, but which should in theory not be possible.
Naturally, modifying the depot on disk directly is quite intimate with the tool and thus prone to breakage when Perforce updates the way that data is stored. And indeed, those tests are broken nowadays with r23 of Perforce. While a file revision was previously stored as plain file "depot/file,v", it is now stored in a directory "depot/file,d" with compression.
Adapt those tests to handle both old- and new-style depot layouts.
--- t/t9800-git-p4-basic.sh | 13 +++++++++++-- t/t9802-git-p4-filetype.sh | 15 ++++++++++++--- t/t9825-git-p4-handle-utf16-without-bom.sh | 18 +++++++++++++++--- 3 files changed, 38 insertions(+), 8 deletions(-)
t/t9800-git-p4-basic.sh +11 -2
@@ -297,8 +297,17 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' # p4 changes, files, or describe; just in p4 print. If P4CLIENT is unset, the # message will include "Librarian checkout". test_expect_success 'exit gracefully for p4 server errors' ' - test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && - mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden && + case "$(echo "$db"/depot/file1*)" in + *,v) + test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && + mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden;; + *,d) + path="$(echo "$db"/depot/file1,d/*.gz)" && + test_when_finished "mv \"$path\",hidden \"$path\"" && + mv "$path" "$path",hidden;; + *) + BUG "unhandled p4d layout";; + esac && test_when_finished cleanup_git && test_expect_code 1 git p4 clone --dest="$git" //depot@1 >out 2>err && test_grep "Error from p4 print" err
t/t9802-git-p4-filetype.sh +12 -3
@@ -301,9 +301,18 @@ test_expect_success SYMLINKS 'empty symlink target' ' # @@ # cd "$db/depot" && - sed "/@target1/{; s/target1/@/; n; d; }" \ - empty-symlink,v >empty-symlink,v.tmp && - mv empty-symlink,v.tmp empty-symlink,v + case "$(echo empty-symlink*)" in + empty-symlink,v) + sed "/@target1/{; s/target1/@/; n; d; }" \ + empty-symlink,v >empty-symlink,v.tmp && + mv empty-symlink,v.tmp empty-symlink,v;; + empty-symlink,d) + path="empty-symlink,d/$(ls empty-symlink,d/ | tail -n1)" && + rm "$path" && + gzip </dev/null >"$path";; + *) + BUG "unhandled p4d layout";; + esac ) && ( # Make sure symlink really is empty. Asking
t/t9825-git-p4-handle-utf16-without-bom.sh +15 -3
@@ -22,9 +22,21 @@ test_expect_success 'init depot with UTF-16 encoded file and artificially remove cd db && p4d -jc && # P4D automatically adds a BOM. Remove it here to make the file invalid. - sed -e "\$d" depot/file1,v >depot/file1,v.new && - mv depot/file1,v.new depot/file1,v && - printf "@$UTF16@" >>depot/file1,v && + case "$(echo depot/file1*)" in + depot/file1,v) + sed -e "\$d" depot/file1,v >depot/file1,v.new && + mv depot/file1,v.new depot/file1,v && + printf "@$UTF16@" >>depot/file1,v;; + depot/file1,d) + path="$(echo depot/file1,d/*.gz)" && + gunzip -c "$path" >"$path.unzipped" && + sed -e "\$d" "$path.unzipped" >"$path.new" && + printf "$UTF16" >>"$path.new" && + gzip -c "$path.new" >"$path" && + rm "$path.unzipped" "$path.new";; + *) + BUG "unhandled p4d layout";; + esac && p4d -jrF checkpoint.1 ) '
Signature · 1 line
2.46.0.rc1.dirty
Re: [PATCH 1/3] t98xx: fix Perforce tests with p4d r23 and newer
Quoted 4 lines · Patrick Steinhardt
Some of the tests in t98xx modify the Perforce depot in ways that the tool wouldn't normally allow. This is done to test behaviour of git-p4 in certain edge cases that we have observed in the wild, but which should in theory not be possible.
If in theory these edge cases being tested should not be possible, that sounds like a bug and maybe in newer versions of p4 that is no longer relevant? Does it make sense to even support these rather intimate test cases going forward? Maybe we could instead skip these tests for newer versions?
Quoted 6 lines
Naturally, modifying the depot on disk directly is quite intimate with the tool and thus prone to breakage when Perforce updates the way that data is stored. And indeed, those tests are broken nowadays with r23 of Perforce. While a file revision was previously stored as plain file "depot/file,v", it is now stored in a directory "depot/file,d" with compression.
s/plain/a plain/
This sounds like a bit of a maintenance headache, especially if there are not many eyes on it to begin with. I guess this ties in with other discussion from this thread about whether and of git-p4 should remain in the codebase.
Quoted 20 lines
Adapt those tests to handle both old- and new-style depot layouts.
Signed-off-by Patrick Steinhardt <[email protected]>--- t/t9800-git-p4-basic.sh | 13 +++++++++++-- t/t9802-git-p4-filetype.sh | 15 ++++++++++++--- t/t9825-git-p4-handle-utf16-without-bom.sh | 18 +++++++++++++++--- 3 files changed, 38 insertions(+), 8 deletions(-)t/t9800-git-p4-basic.sh +1
-2@@ -297,8 +297,17 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' # p4 changes, files, or describe; just in p4 print. If P4CLIENT is unset, the # message will include "Librarian checkout". test_expect_success 'exit gracefully for p4 server errors' ' - test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && - mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden && + case "$(echo "$db"/depot/file1*)" in
t/t9800-git-p4-basic.sh test_expect_success 'exit when p4 fails to produce marshaled output' ' · line 310
Quoted 10 lines
t/t9800-git-p4-basic.sh +10
-0+ *,v) + test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && + mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden;; + *,d) + path="$(echo "$db"/depot/file1,d/*.gz)" && + test_when_finished "mv \"$path\",hidden \"$path\"" && + mv "$path" "$path",hidden;; + *) + BUG "unhandled p4d layout";; + esac &&
I'm not familiar with Perforce, but the test looks like it is simply appending ",hidden" to the file name. I assume this to trigger some error.
We are simply extending the test to also perform the same rename if, instead of depot/file1,f, a newer version uses depot/file1,d.
Makes sense to me, but without surrounding context its rather difficult to understand that the "case" statement here pertains to different Perforce versions that may be used. It might be nice to have a comment explaining this.
Quoted 16 lines
t/t9800-git-p4-basic.sh +0
-0test_when_finished cleanup_git && test_expect_code 1 git p4 clone --dest="$git" //depot@1 >out 2>err && test_grep "Error from p4 print" errt/t9802-git-p4-filetype.sh +2
-3@@ -301,9 +301,18 @@ test_expect_success SYMLINKS 'empty symlink target' ' # @@ # cd "$db/depot" && - sed "/@target1/{; s/target1/@/; n; d; }" \ - empty-symlink,v >empty-symlink,v.tmp && - mv empty-symlink,v.tmp empty-symlink,v + case "$(echo empty-symlink*)" in + empty-symlink,v)
t/t9802-git-p4-filetype.sh test_expect_success SYMLINKS 'empty symlink target' ' · line 315
Quoted 10 lines
t/t9802-git-p4-filetype.sh +10
-0+ sed "/@target1/{; s/target1/@/; n; d; }" \ + empty-symlink,v >empty-symlink,v.tmp && + mv empty-symlink,v.tmp empty-symlink,v;; + empty-symlink,d) + path="empty-symlink,d/$(ls empty-symlink,d/ | tail -n1)" && + rm "$path" && + gzip </dev/null >"$path";; + *) + BUG "unhandled p4d layout";; + esac
Looks like for this test, for previous Perforce versions we were making a symlink point to nothing. For newer versions of Perforce, we seem to accomplish the same think by creating an empty compressed file.
Seems reasonable to me.
Quoted 19 lines
t/t9802-git-p4-filetype.sh +0
-0) && ( # Make sure symlink really is empty. Askingt/t9825-git-p4-handle-utf16-without-bom.sh +5
-3@@ -22,9 +22,21 @@ test_expect_success 'init depot with UTF-16 encoded file and artificially remove cd db && p4d -jc && # P4D automatically adds a BOM. Remove it here to make the file invalid. - sed -e "\$d" depot/file1,v >depot/file1,v.new && - mv depot/file1,v.new depot/file1,v && - printf "@$UTF16@" >>depot/file1,v && + case "$(echo depot/file1*)" in + depot/file1,v) + sed -e "\$d" depot/file1,v >depot/file1,v.new && + mv depot/file1,v.new depot/file1,v && + printf "@$UTF16@" >>depot/file1,v;;
t/t9825-git-p4-handle-utf16-without-bom.sh test_expect_success 'init depot with UTF-16 encoded file and artificially remove · line 39
Quoted 10 lines
t/t9825-git-p4-handle-utf16-without-bom.sh +10
-0+ depot/file1,d) + path="$(echo depot/file1,d/*.gz)" && + gunzip -c "$path" >"$path.unzipped" && + sed -e "\$d" "$path.unzipped" >"$path.new" && + printf "$UTF16" >>"$path.new" && + gzip -c "$path.new" >"$path" && + rm "$path.unzipped" "$path.new";; + *) + BUG "unhandled p4d layout";; + esac &&
Looks like the same deal here. For the new version we are modifying some file, but first have to uncompress it and then recompress it after it is modified. Again seems reasonable.
Thanks
-Justin
Quoted 6 lines
t/t9825-git-p4-handle-utf16-without-bom.sh +0
-0p4d -jrF checkpoint.1 ) 'Signature · 1 line
2.46.0.rc1.dirty
Re: [PATCH 1/3] t98xx: fix Perforce tests with p4d r23 and newer
Quoted 11 lines · Justin Tobler
Quoted 4 lines · Patrick Steinhardt
Some of the tests in t98xx modify the Perforce depot in ways that the tool wouldn't normally allow. This is done to test behaviour of git-p4 in certain edge cases that we have observed in the wild, but which should in theory not be possible.
If in theory these edge cases being tested should not be possible, that sounds like a bug and maybe in newer versions of p4 that is no longer relevant? Does it make sense to even support these rather intimate test cases going forward? Maybe we could instead skip these tests for newer versions?
I wouldn't feel comfortable skipping this without knowing anything about the underlying bug that had been tested in the past. If we removed it, then we'd be second guessing whether or not that bug is still possible. Seemingly, it wasn't possible to trigger it without bugs in the past either, so it feels like we'd be assuming too much.
Quoted 7 lines
Quoted 6 lines
Naturally, modifying the depot on disk directly is quite intimate with the tool and thus prone to breakage when Perforce updates the way that data is stored. And indeed, those tests are broken nowadays with r23 of Perforce. While a file revision was previously stored as plain file "depot/file,v", it is now stored in a directory "depot/file,d" with compression.
Quoted 6 lines
s/plain/a plain/
This sounds like a bit of a maintenance headache, especially if there are not many eyes on it to begin with. I guess this ties in with other discussion from this thread about whether and of git-p4 should remain in the codebase.
Oh, it certainly isn't great. Whether it is a headache remains to be seen. If we had to pile onto hacks like this in the future then I'd also vote to remove or skip such tests. But we didn't really have to do a lot of maintenance for the Perforce tests in the past either, so I rather assume that it's going to be okayish in the future, too.
Part of the reason is likely that git-p4 simply isn't well-maintained, and the less maintained something is the less churn you have. But for now I only see two ways out of it:
- Either we get somebody willing to adopt it, and in that case it will be their responsibility to handle such potential headaches. In other words, I'll leave it to them to decide what to do with those tests, also because they are going to be better equipped with knowledge around Perforce itself.
- Or we remove it, and then it doesn't matter much anyway.
All to say that I'm merely being pragmatic and punt the issue to the future.
Quoted 35 lines
Quoted 20 lines
Adapt those tests to handle both old- and new-style depot layouts.
Signed-off-by Patrick Steinhardt <[email protected]>--- t/t9800-git-p4-basic.sh | 13 +++++++++++-- t/t9802-git-p4-filetype.sh | 15 ++++++++++++--- t/t9825-git-p4-handle-utf16-without-bom.sh | 18 +++++++++++++++--- 3 files changed, 38 insertions(+), 8 deletions(-)t/t9800-git-p4-basic.sh +1
-2@@ -297,8 +297,17 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' # p4 changes, files, or describe; just in p4 print. If P4CLIENT is unset, the # message will include "Librarian checkout". test_expect_success 'exit gracefully for p4 server errors' ' - test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && - mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden && + case "$(echo "$db"/depot/file1*)" int/t9800-git-p4-basic.sh test_expect_success 'exit when p4 fails to produce marshaled output' ' · line 310
Quoted 10 lines
t/t9800-git-p4-basic.sh +10
-0+ *,v) + test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && + mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden;; + *,d) + path="$(echo "$db"/depot/file1,d/*.gz)" && + test_when_finished "mv \"$path\",hidden \"$path\"" && + mv "$path" "$path",hidden;; + *) + BUG "unhandled p4d layout";; + esac &&I'm not familiar with Perforce, but the test looks like it is simply appending ",hidden" to the file name. I assume this to trigger some error.
Quoted 7 lines
We are simply extending the test to also perform the same rename if, instead of
depot/file1,f, a newer version usesdepot/file1,d.Makes sense to me, but without surrounding context its rather difficult to understand that the "case" statement here pertains to different Perforce versions that may be used. It might be nice to have a comment explaining this.
Fair, I will add a comment to all three sites.
Patric
PATCH 2/3 ci: update Perforce version to r23.2
In reply to Patrick SteinhardtUpdate our Perforce version from r21.2 to r23.2. Note that the updated version is not the newest version. Instead, it is the last version where the way that Perforce is being distributed remains the same as in r21.2. Newer releases stopped distributing p4 and p4d executablesas well as the macOS archives directly and would thus require more work.
--- ci/install-dependencies.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ci/install-dependencies.sh +1 -1
@@ -7,7 +7,7 @@ begin_group "Install dependencies" -P4WHENCE=https://cdist2.perforce.com/perforce/r21.2 +P4WHENCE=https://cdist2.perforce.com/perforce/r23.2 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION JGITWHENCE=https://repo.eclipse.org/content/groups/releases//org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh
Signature · 1 line
2.46.0.rc1.dirty
Re: [PATCH 2/3] ci: update Perforce version to r23.2
Hi Patrick,
Quoted 5 lines · Patrick Steinhardt
Update our Perforce version from r21.2 to r23.2. Note that the updated version is not the newest version. Instead, it is the last version where the way that Perforce is being distributed remains the same as in r21.2. Newer releases stopped distributing p4 and p4d executablesas well as the macOS archives directly and would thus require more work.
An alternative would be to simply stop installing p4 in CI. I would actually be in favor of that, for multiple reasons:
- The pace of reviews and integration of git-p4 patches has slowed down over the couple of years. For example,
https://lore.kernel.org/git/20210510183638.156a6b1d@ado-tr/ has not seen any traction in over three years (most likely because we no longer have any active contributor with a vested interest in git-p4), and
https://github.com/gitgitgadget/git/pull/1028 and
https://github.com/gitgitgadget/git/pull/1070 have not even been submitted to the Git mailing list (most likely because of the hurdles to contribute).
- Over the years, it has been made harder and harder to install Perforce in CI. I spent a good deal of time trying to keep the Homebrew taps up to date (which was hard because Perforce kept replacing the archive behind that URL with newer versions, which always broke Homebrew's SHA check until it was adjusted accordingly).
- The git-p4 tests use quite a bit of time and electricity in all those CI builds. Therefore, it seems desirable to me to stop running these tests as part of the CI builds.
Ciao,
Johannes
Re: [PATCH 2/3] ci: update Perforce version to r23.2
Quoted 29 lines · Johannes Schindelin
Hi Patrick,
Quoted 5 lines · Patrick Steinhardt
Update our Perforce version from r21.2 to r23.2. Note that the updated version is not the newest version. Instead, it is the last version where the way that Perforce is being distributed remains the same as in r21.2. Newer releases stopped distributing p4 and p4d executablesas well as the macOS archives directly and would thus require more work.
An alternative would be to simply stop installing
p4in CI. I would actually be in favor of that, for multiple reasons:- The pace of reviews and integration of
git-p4patches has slowed down over the couple of years. For example,
https://lore.kernel.org/git/20210510183638.156a6b1d@ado-tr/ has not seen any traction in over three years (most likely because we no longer have any active contributor with a vested interest ingit-p4), and
https://github.com/gitgitgadget/git/pull/1028 and
https://github.com/gitgitgadget/git/pull/1070 have not even been submitted to the Git mailing list (most likely because of the hurdles to contribute).- Over the years, it has been made harder and harder to install Perforce in CI. I spent a good deal of time trying to keep the Homebrew taps up to date (which was hard because Perforce kept replacing the archive behind that URL with newer versions, which always broke Homebrew's SHA check until it was adjusted accordingly).
Quoted 3 lines
- The
git-p4tests use quite a bit of time and electricity in all those CI builds. Therefore, it seems desirable to me to stop running these tests as part of the CI builds.
I don't think that is a good idea. If we stop installing p4, the result is that _nobody_ will ever run the tests at all. The tests, and by extension git-p4 itself, would start to bitrot and we wouldn't notice any kind of regressions at all anymore.
If we want to consider going down that route, I'd rather say we should do it all or nothing: either we rip out git-p4 and the tests, or we leave both of them in. I couldn't care less about git-p4 itself, so I would not mind ripping it out altogether. But there may be users of this script out there that do care, so I don't want to make that decision unilaterally.
Patrick
Re: [PATCH 2/3] ci: update Perforce version to r23.2
Quoted 11 lines · Patrick Steinhardt
I don't think that is a good idea. If we stop installing p4, the result is that _nobody_ will ever run the tests at all. The tests, and by extension git-p4 itself, would start to bitrot and we wouldn't notice any kind of regressions at all anymore.
If we want to consider going down that route, I'd rather say we should do it all or nothing: either we rip out git-p4 and the tests, or we leave both of them in. I couldn't care less about git-p4 itself, so I would not mind ripping it out altogether. But there may be users of this script out there that do care, so I don't want to make that decision unilaterally.
Yup, I was actually interpreting Dscho's message as advocating the removal of "git p4". Such a move would certainly force people who do care about it to come out. It is up to them to volunteer to help maintaining "git p4".
This may be a good example to discuss "support policy" not on niche platforms but on niche features (Emily Cc'ed).
Thanks.
Re: [PATCH 2/3] ci: update Perforce version to r23.2
Quoted 14 lines · Junio C Hamano
Quoted 11 lines · Patrick Steinhardt
I don't think that is a good idea. If we stop installing p4, the result is that _nobody_ will ever run the tests at all. The tests, and by extension git-p4 itself, would start to bitrot and we wouldn't notice any kind of regressions at all anymore.
If we want to consider going down that route, I'd rather say we should do it all or nothing: either we rip out git-p4 and the tests, or we leave both of them in. I couldn't care less about git-p4 itself, so I would not mind ripping it out altogether. But there may be users of this script out there that do care, so I don't want to make that decision unilaterally.
Quoted 7 lines
Yup, I was actually interpreting Dscho's message as advocating the removal of "git p4". Such a move would certainly force people who do care about it to come out. It is up to them to volunteer to help maintaining "git p4".
This may be a good example to discuss "support policy" not on niche platforms but on niche features (Emily Cc'ed).
As said, I wouldn't mind dropping support for git p4 altogether. That is a much bigger discussion though, and I'm not sure whether we should just drop it at a "random" point in time without something like a grace period where people can chime in and express their wish to help out with the maintenance of it. In other words, we should probably deprecate it properly and announce our intent to deprecate it. Both our release notes and "Documentation/BreakingChanges.txt" could we viable ways to do that.
And while we haven't yet decided to rip out support for Perforce, I think that the proposed patch series is somewhat sensible. I honestly really only care about marking the patches as leak-free to help my bigger goal of making the whole test suite leak-free. The other patches that make the tests compatible with newer versions of Perforce aren't all that important, but at least they help to make those tests a bit more accessible to interested folks.
Patrick
Re: [PATCH 2/3] ci: update Perforce version to r23.2
In reply to Patrick SteinhardtQuoted 4 lines · Patrick Steinhardt
Update our Perforce version from r21.2 to r23.2. Note that the updated version is not the newest version. Instead, it is the last version where the way that Perforce is being distributed remains the same as in r21.2. Newer releases stopped distributing p4 and p4d executablesas well as the
s/executablesas/executables as/
Quoted 1 line
macOS archives directly and would thus require more work.
Out of curiousity, for Perforce is there a defined range of versions that the Git project supports? I guess I'm trying to figure if it even makes sense to support older version of Perforce in our tests.
Quoted 22 lines
Signed-off-by Patrick Steinhardt <[email protected]>--- ci/install-dependencies.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)ci/install-dependencies.sh +1
-1@@ -7,7 +7,7 @@ begin_group "Install dependencies" -P4WHENCE=https://cdist2.perforce.com/perforce/r21.2 +P4WHENCE=https://cdist2.perforce.com/perforce/r23.2 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION JGITWHENCE=https://repo.eclipse.org/content/groups/releases//org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.shSignature · 1 line
2.46.0.rc1.dirty
Re: [PATCH 2/3] ci: update Perforce version to r23.2
Quoted 6 lines · Justin Tobler
Quoted 4 lines · Patrick Steinhardt
Update our Perforce version from r21.2 to r23.2. Note that the updated version is not the newest version. Instead, it is the last version where the way that Perforce is being distributed remains the same as in r21.2. Newer releases stopped distributing p4 and p4d executablesas well as the
Quoted 7 lines
s/executablesas/executables as/
Quoted 1 line
macOS archives directly and would thus require more work.
Out of curiousity, for Perforce is there a defined range of versions that the Git project supports? I guess I'm trying to figure if it even makes sense to support older version of Perforce in our tests.
Not that I'd know of. Which is why I'm being doubly cautious to deprecate support for older versions :) Not that many people would care, but still, I don't want to make such decisions without having any clue at all about the surrounding Perforce ecosystem.
Patrick
PATCH 3/3 t98xx: mark Perforce tests as memory-leak free
In reply to Patrick SteinhardtAll the Perforce tests are free of memory leaks. This went unnoticed because most folks do not have p4 and p4d installed on their computers. Consequently, given that the prerequisites for running those tests aren't fulfilled, TEST_PASSES_SANITIZE_LEAK=check won't notice that those tests are indeed memory leak free.
Mark those tests accordingly.
--- t/t9800-git-p4-basic.sh | 1 + t/t9801-git-p4-branch.sh | 1 + t/t9802-git-p4-filetype.sh | 1 + t/t9803-git-p4-shell-metachars.sh | 1 + t/t9804-git-p4-label.sh | 1 + t/t9805-git-p4-skip-submit-edit.sh | 1 + t/t9806-git-p4-options.sh | 1 + t/t9808-git-p4-chdir.sh | 1 + t/t9809-git-p4-client-view.sh | 1 + t/t9810-git-p4-rcs.sh | 1 + t/t9811-git-p4-label-import.sh | 1 + t/t9812-git-p4-wildcards.sh | 1 + t/t9813-git-p4-preserve-users.sh | 1 + t/t9814-git-p4-rename.sh | 1 + t/t9815-git-p4-submit-fail.sh | 1 + t/t9816-git-p4-locked.sh | 1 + t/t9817-git-p4-exclude.sh | 1 + t/t9818-git-p4-block.sh | 1 + t/t9819-git-p4-case-folding.sh | 1 + t/t9820-git-p4-editor-handling.sh | 1 + t/t9821-git-p4-path-variations.sh | 1 + t/t9822-git-p4-path-encoding.sh | 1 + t/t9823-git-p4-mock-lfs.sh | 1 + t/t9825-git-p4-handle-utf16-without-bom.sh | 1 + t/t9826-git-p4-keep-empty-commits.sh | 1 + t/t9827-git-p4-change-filetype.sh | 1 + t/t9828-git-p4-map-user.sh | 1 + t/t9829-git-p4-jobs.sh | 1 + t/t9830-git-p4-symlink-dir.sh | 1 + t/t9831-git-p4-triggers.sh | 1 + t/t9832-unshelve.sh | 1 + t/t9833-errors.sh | 1 + t/t9834-git-p4-file-dir-bug.sh | 1 + t/t9835-git-p4-metadata-encoding-python2.sh | 1 + t/t9836-git-p4-metadata-encoding-python3.sh | 1 + 35 files changed, 35 insertions(+)
t/t9800-git-p4-basic.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git p4 tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9801-git-p4-branch.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git p4 tests for p4 branches' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9802-git-p4-filetype.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 filetype tests' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9803-git-p4-shell-metachars.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 transparency to shell metachars in filenames' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9804-git-p4-label.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 label tests' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9805-git-p4-skip-submit-edit.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 skipSubmitEdit config variables' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9806-git-p4-options.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git p4 options' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9808-git-p4-chdir.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 relative chdir' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9809-git-p4-client-view.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 client view' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9810-git-p4-rcs.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 rcs keywords' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh CP1252="\223\224"
t/t9811-git-p4-label-import.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git p4 label tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9812-git-p4-wildcards.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 wildcards' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9813-git-p4-preserve-users.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 preserve users' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9814-git-p4-rename.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 rename' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9815-git-p4-submit-fail.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 submit failure handling' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9816-git-p4-locked.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 locked file behavior' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9817-git-p4-exclude.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 tests for excluded paths during clone and sync' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9818-git-p4-block.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 fetching changes in multiple blocks' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9819-git-p4-case-folding.sh +1 -0
@@ -2,6 +2,7 @@ test_description='interaction with P4 case-folding' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh if test_have_prereq CASE_INSENSITIVE_FS
t/t9820-git-p4-editor-handling.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 handling of EDITOR' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9821-git-p4-path-variations.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories with path case variations' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d with case folding enabled' '
t/t9822-git-p4-path-encoding.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories with non ASCII paths' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh UTF8_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
t/t9823-git-p4-mock-lfs.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories and store files in Mock LFS' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_file_is_not_in_mock_lfs () {
t/t9825-git-p4-handle-utf16-without-bom.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 handling of UTF-16 files without BOM' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh UTF16="\227\000\227\000"
t/t9826-git-p4-keep-empty-commits.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories and keep empty commits' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9827-git-p4-change-filetype.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 support for file type change' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9828-git-p4-map-user.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories and map users' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9829-git-p4-jobs.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 retrieve job info' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9830-git-p4-symlink-dir.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 symlinked directories' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9831-git-p4-triggers.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 with server triggers' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9832-unshelve.sh +1 -0
@@ -6,6 +6,7 @@ last_shelved_change () { test_description='git p4 unshelve' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9833-errors.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 errors' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9834-git-p4-file-dir-bug.sh +1 -0
@@ -6,6 +6,7 @@ This test creates files and directories with the same name in perforce and checks that git-p4 recovers from the error at the same time as the perforce repository.' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9835-git-p4-metadata-encoding-python2.sh +1 -0
@@ -6,6 +6,7 @@ This test checks that the import process handles inconsistent text encoding in p4 metadata (author names, commit messages, etc) without failing, and produces maximally sane output in git.' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh python_target_version='2'
t/t9836-git-p4-metadata-encoding-python3.sh +1 -0
@@ -6,6 +6,7 @@ This test checks that the import process handles inconsistent text encoding in p4 metadata (author names, commit messages, etc) without failing, and produces maximally sane output in git.' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh python_target_version='3'
Signature · 1 line
2.46.0.rc1.dirty
Re: [PATCH 3/3] t98xx: mark Perforce tests as memory-leak free
Quoted 7 lines · Patrick Steinhardt
All the Perforce tests are free of memory leaks. This went unnoticed because most folks do not have p4 and p4d installed on their computers. Consequently, given that the prerequisites for running those tests aren't fulfilled,
TEST_PASSES_SANITIZE_LEAK=checkwon't notice that those tests are indeed memory leak free.Mark those tests accordingly.
Nice to have more tests marked off the list! To me, it makes sense to go ahead and mark these tests as leak free even if we end up getting rid of them in the future. Thanks
Quoted 2 lines
Signed-off-by Patrick Steinhardt <[email protected]>
v2 cover Improvements for Perforce tests
In reply to Patrick SteinhardtHi,
this is the second version of my patch series that improves Perforce tests a bit. As with the first version, the main intent is to mark all tests as leak free. The other two patches polish the Perforce tests such that they work on my machine, which has a more recent version of Perforce than what we have been testing with in CI.
As discussed, despite these patches I consider Perforce to be basically unmaintained, and my patches do not change that. But given that I don't think we can rip out git-p4(1) just so without any kind of announcement up front, I still think that these patches are worth it to help my own cause, namely plugging all memory leaks hit by tests in Git.
Changes compared to v1:
- Fix some typos.
- Add in-code comments explaining what the compatibility case statements are about.
Thanks!
Patrick
Patrick Steinhardt (3):
t98xx: fix Perforce tests with p4d r23 and newer
ci: update Perforce version to r23.2
t98xx: mark Perforce tests as memory-leak free
ci/install-dependencies.sh | 2 +- t/t9800-git-p4-basic.sh | 17 +++++++++++++-- t/t9801-git-p4-branch.sh | 1 + t/t9802-git-p4-filetype.sh | 19 ++++++++++++++--- t/t9803-git-p4-shell-metachars.sh | 1 + t/t9804-git-p4-label.sh | 1 + t/t9805-git-p4-skip-submit-edit.sh | 1 + t/t9806-git-p4-options.sh | 1 + t/t9808-git-p4-chdir.sh | 1 + t/t9809-git-p4-client-view.sh | 1 + t/t9810-git-p4-rcs.sh | 1 + t/t9811-git-p4-label-import.sh | 1 + t/t9812-git-p4-wildcards.sh | 1 + t/t9813-git-p4-preserve-users.sh | 1 + t/t9814-git-p4-rename.sh | 1 + t/t9815-git-p4-submit-fail.sh | 1 + t/t9816-git-p4-locked.sh | 1 + t/t9817-git-p4-exclude.sh | 1 + t/t9818-git-p4-block.sh | 1 + t/t9819-git-p4-case-folding.sh | 1 + t/t9820-git-p4-editor-handling.sh | 1 + t/t9821-git-p4-path-variations.sh | 1 + t/t9822-git-p4-path-encoding.sh | 1 + t/t9823-git-p4-mock-lfs.sh | 1 + t/t9825-git-p4-handle-utf16-without-bom.sh | 23 ++++++++++++++++++--- t/t9826-git-p4-keep-empty-commits.sh | 1 + t/t9827-git-p4-change-filetype.sh | 1 + t/t9828-git-p4-map-user.sh | 1 + t/t9829-git-p4-jobs.sh | 1 + t/t9830-git-p4-symlink-dir.sh | 1 + t/t9831-git-p4-triggers.sh | 1 + t/t9832-unshelve.sh | 1 + t/t9833-errors.sh | 1 + t/t9834-git-p4-file-dir-bug.sh | 1 + t/t9835-git-p4-metadata-encoding-python2.sh | 1 + t/t9836-git-p4-metadata-encoding-python3.sh | 1 + 36 files changed, 84 insertions(+), 9 deletions(-)
Range-diff against v1:
1: f40f62f257 ! 1: 7f06de7e6b t98xx: fix Perforce tests with p4d r23 and newer
@@ Commit message
Naturally, modifying the depot on disk directly is quite intimate with the tool and thus prone to breakage when Perforce updates the way that data is stored. And indeed, those tests are broken nowadays with r23 of
- Perforce. While a file revision was previously stored as plain file
+ Perforce. While a file revision was previously stored as a plain file
"depot/file,v", it is now stored in a directory "depot/file,d" with compression.
@@ t/t9800-git-p4-basic.sh: test_expect_success 'exit when p4 fails to produce mars
test_expect_success 'exit gracefully for p4 server errors' '
- test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" &&
- mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden &&
++ # Note that newer Perforce versions started to store files
++ # compressed in directories. The case statement handles both
++ # old and new layout.
+ case "$(echo "$db"/depot/file1*)" in
+ *,v)
+ test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" &&
@@ t/t9800-git-p4-basic.sh: test_expect_success 'exit when p4 fails to produce mars
## t/t9802-git-p4-filetype.sh ##
@@ t/t9802-git-p4-filetype.sh: test_expect_success SYMLINKS 'empty symlink target' '
+ # text
# @@
#
++ # Note that newer Perforce versions started to store files
++ # compressed in directories. The case statement handles both
++ # old and new layout.
cd "$db/depot" &&
- sed "/@target1/{; s/target1/@/; n; d; }" \
- empty-symlink,v >empty-symlink,v.tmp &&
@@ t/t9825-git-p4-handle-utf16-without-bom.sh: test_expect_success 'init depot with
- sed -e "\$d" depot/file1,v >depot/file1,v.new &&
- mv depot/file1,v.new depot/file1,v &&
- printf "@$UTF16@" >>depot/file1,v &&
++ #
++ # Note that newer Perforce versions started to store files
++ # compressed in directories. The case statement handles both
++ # old and new layout.
+ case "$(echo depot/file1*)" in
+ depot/file1,v)
+ sed -e "\$d" depot/file1,v >depot/file1,v.new &&
2: ee5d836b77 ! 2: f41473d991 ci: update Perforce version to r23.2
@@ Commit message
Update our Perforce version from r21.2 to r23.2. Note that the updated version is not the newest version. Instead, it is the last version where the way that Perforce is being distributed remains the same as in r21.2.
- Newer releases stopped distributing p4 and p4d executablesas well as the
- macOS archives directly and would thus require more work.
+ Newer releases stopped distributing p4 and p4d executables as well as
+ the macOS archives directly and would thus require more work.
3: d0a80ba403 = 3: bf03405db9 t98xx: mark Perforce tests as memory-leak free
base-commit: d19b6cd2dd72dc811f19df4b32c7ed223256c3ee
Signature · 1 line
2.46.0.dirty
PATCH v2 1/3 t98xx: fix Perforce tests with p4d r23 and newer
Some of the tests in t98xx modify the Perforce depot in ways that the tool wouldn't normally allow. This is done to test behaviour of git-p4 in certain edge cases that we have observed in the wild, but which should in theory not be possible.
Naturally, modifying the depot on disk directly is quite intimate with the tool and thus prone to breakage when Perforce updates the way that data is stored. And indeed, those tests are broken nowadays with r23 of Perforce. While a file revision was previously stored as a plain file "depot/file,v", it is now stored in a directory "depot/file,d" with compression.
Adapt those tests to handle both old- and new-style depot layouts.
--- t/t9800-git-p4-basic.sh | 16 ++++++++++++++-- t/t9802-git-p4-filetype.sh | 18 +++++++++++++++--- t/t9825-git-p4-handle-utf16-without-bom.sh | 22 +++++++++++++++++++--- 3 files changed, 48 insertions(+), 8 deletions(-)
t/t9800-git-p4-basic.sh +14 -2
@@ -297,8 +297,20 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' # p4 changes, files, or describe; just in p4 print. If P4CLIENT is unset, the # message will include "Librarian checkout". test_expect_success 'exit gracefully for p4 server errors' ' - test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && - mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden && + # Note that newer Perforce versions started to store files + # compressed in directories. The case statement handles both + # old and new layout. + case "$(echo "$db"/depot/file1*)" in + *,v) + test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && + mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden;; + *,d) + path="$(echo "$db"/depot/file1,d/*.gz)" && + test_when_finished "mv \"$path\",hidden \"$path\"" && + mv "$path" "$path",hidden;; + *) + BUG "unhandled p4d layout";; + esac && test_when_finished cleanup_git && test_expect_code 1 git p4 clone --dest="$git" //depot@1 >out 2>err && test_grep "Error from p4 print" err
t/t9802-git-p4-filetype.sh +15 -3
@@ -300,10 +300,22 @@ test_expect_success SYMLINKS 'empty symlink target' ' # text # @@ # + # Note that newer Perforce versions started to store files + # compressed in directories. The case statement handles both + # old and new layout. cd "$db/depot" && - sed "/@target1/{; s/target1/@/; n; d; }" \ - empty-symlink,v >empty-symlink,v.tmp && - mv empty-symlink,v.tmp empty-symlink,v + case "$(echo empty-symlink*)" in + empty-symlink,v) + sed "/@target1/{; s/target1/@/; n; d; }" \ + empty-symlink,v >empty-symlink,v.tmp && + mv empty-symlink,v.tmp empty-symlink,v;; + empty-symlink,d) + path="empty-symlink,d/$(ls empty-symlink,d/ | tail -n1)" && + rm "$path" && + gzip </dev/null >"$path";; + *) + BUG "unhandled p4d layout";; + esac ) && ( # Make sure symlink really is empty. Asking
t/t9825-git-p4-handle-utf16-without-bom.sh +19 -3
@@ -22,9 +22,25 @@ test_expect_success 'init depot with UTF-16 encoded file and artificially remove cd db && p4d -jc && # P4D automatically adds a BOM. Remove it here to make the file invalid. - sed -e "\$d" depot/file1,v >depot/file1,v.new && - mv depot/file1,v.new depot/file1,v && - printf "@$UTF16@" >>depot/file1,v && + # + # Note that newer Perforce versions started to store files + # compressed in directories. The case statement handles both + # old and new layout. + case "$(echo depot/file1*)" in + depot/file1,v) + sed -e "\$d" depot/file1,v >depot/file1,v.new && + mv depot/file1,v.new depot/file1,v && + printf "@$UTF16@" >>depot/file1,v;; + depot/file1,d) + path="$(echo depot/file1,d/*.gz)" && + gunzip -c "$path" >"$path.unzipped" && + sed -e "\$d" "$path.unzipped" >"$path.new" && + printf "$UTF16" >>"$path.new" && + gzip -c "$path.new" >"$path" && + rm "$path.unzipped" "$path.new";; + *) + BUG "unhandled p4d layout";; + esac && p4d -jrF checkpoint.1 ) '
Signature · 1 line
2.46.0.dirty
PATCH v2 2/3 ci: update Perforce version to r23.2
In reply to Patrick SteinhardtUpdate our Perforce version from r21.2 to r23.2. Note that the updated version is not the newest version. Instead, it is the last version where the way that Perforce is being distributed remains the same as in r21.2. Newer releases stopped distributing p4 and p4d executables as well as the macOS archives directly and would thus require more work.
--- ci/install-dependencies.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ci/install-dependencies.sh +1 -1
@@ -7,7 +7,7 @@ begin_group "Install dependencies" -P4WHENCE=https://cdist2.perforce.com/perforce/r21.2 +P4WHENCE=https://cdist2.perforce.com/perforce/r23.2 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION JGITWHENCE=https://repo.eclipse.org/content/groups/releases//org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh
Signature · 1 line
2.46.0.dirty
PATCH v2 3/3 t98xx: mark Perforce tests as memory-leak free
In reply to Patrick SteinhardtAll the Perforce tests are free of memory leaks. This went unnoticed because most folks do not have p4 and p4d installed on their computers. Consequently, given that the prerequisites for running those tests aren't fulfilled, TEST_PASSES_SANITIZE_LEAK=check won't notice that those tests are indeed memory leak free.
Mark those tests accordingly.
--- t/t9800-git-p4-basic.sh | 1 + t/t9801-git-p4-branch.sh | 1 + t/t9802-git-p4-filetype.sh | 1 + t/t9803-git-p4-shell-metachars.sh | 1 + t/t9804-git-p4-label.sh | 1 + t/t9805-git-p4-skip-submit-edit.sh | 1 + t/t9806-git-p4-options.sh | 1 + t/t9808-git-p4-chdir.sh | 1 + t/t9809-git-p4-client-view.sh | 1 + t/t9810-git-p4-rcs.sh | 1 + t/t9811-git-p4-label-import.sh | 1 + t/t9812-git-p4-wildcards.sh | 1 + t/t9813-git-p4-preserve-users.sh | 1 + t/t9814-git-p4-rename.sh | 1 + t/t9815-git-p4-submit-fail.sh | 1 + t/t9816-git-p4-locked.sh | 1 + t/t9817-git-p4-exclude.sh | 1 + t/t9818-git-p4-block.sh | 1 + t/t9819-git-p4-case-folding.sh | 1 + t/t9820-git-p4-editor-handling.sh | 1 + t/t9821-git-p4-path-variations.sh | 1 + t/t9822-git-p4-path-encoding.sh | 1 + t/t9823-git-p4-mock-lfs.sh | 1 + t/t9825-git-p4-handle-utf16-without-bom.sh | 1 + t/t9826-git-p4-keep-empty-commits.sh | 1 + t/t9827-git-p4-change-filetype.sh | 1 + t/t9828-git-p4-map-user.sh | 1 + t/t9829-git-p4-jobs.sh | 1 + t/t9830-git-p4-symlink-dir.sh | 1 + t/t9831-git-p4-triggers.sh | 1 + t/t9832-unshelve.sh | 1 + t/t9833-errors.sh | 1 + t/t9834-git-p4-file-dir-bug.sh | 1 + t/t9835-git-p4-metadata-encoding-python2.sh | 1 + t/t9836-git-p4-metadata-encoding-python3.sh | 1 + 35 files changed, 35 insertions(+)
t/t9800-git-p4-basic.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git p4 tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9801-git-p4-branch.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git p4 tests for p4 branches' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9802-git-p4-filetype.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 filetype tests' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9803-git-p4-shell-metachars.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 transparency to shell metachars in filenames' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9804-git-p4-label.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 label tests' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9805-git-p4-skip-submit-edit.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 skipSubmitEdit config variables' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9806-git-p4-options.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git p4 options' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9808-git-p4-chdir.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 relative chdir' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9809-git-p4-client-view.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 client view' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9810-git-p4-rcs.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 rcs keywords' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh CP1252="\223\224"
t/t9811-git-p4-label-import.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git p4 label tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9812-git-p4-wildcards.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 wildcards' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9813-git-p4-preserve-users.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 preserve users' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9814-git-p4-rename.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 rename' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9815-git-p4-submit-fail.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 submit failure handling' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9816-git-p4-locked.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 locked file behavior' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9817-git-p4-exclude.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 tests for excluded paths during clone and sync' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9818-git-p4-block.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 fetching changes in multiple blocks' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9819-git-p4-case-folding.sh +1 -0
@@ -2,6 +2,7 @@ test_description='interaction with P4 case-folding' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh if test_have_prereq CASE_INSENSITIVE_FS
t/t9820-git-p4-editor-handling.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 handling of EDITOR' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9821-git-p4-path-variations.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories with path case variations' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d with case folding enabled' '
t/t9822-git-p4-path-encoding.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories with non ASCII paths' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh UTF8_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
t/t9823-git-p4-mock-lfs.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories and store files in Mock LFS' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_file_is_not_in_mock_lfs () {
t/t9825-git-p4-handle-utf16-without-bom.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 handling of UTF-16 files without BOM' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh UTF16="\227\000\227\000"
t/t9826-git-p4-keep-empty-commits.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories and keep empty commits' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9827-git-p4-change-filetype.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 support for file type change' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9828-git-p4-map-user.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Clone repositories and map users' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9829-git-p4-jobs.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 retrieve job info' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9830-git-p4-symlink-dir.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 symlinked directories' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9831-git-p4-triggers.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 with server triggers' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9832-unshelve.sh +1 -0
@@ -6,6 +6,7 @@ last_shelved_change () { test_description='git p4 unshelve' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9833-errors.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git p4 errors' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9834-git-p4-file-dir-bug.sh +1 -0
@@ -6,6 +6,7 @@ This test creates files and directories with the same name in perforce and checks that git-p4 recovers from the error at the same time as the perforce repository.' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh test_expect_success 'start p4d' '
t/t9835-git-p4-metadata-encoding-python2.sh +1 -0
@@ -6,6 +6,7 @@ This test checks that the import process handles inconsistent text encoding in p4 metadata (author names, commit messages, etc) without failing, and produces maximally sane output in git.' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh python_target_version='2'
t/t9836-git-p4-metadata-encoding-python3.sh +1 -0
@@ -6,6 +6,7 @@ This test checks that the import process handles inconsistent text encoding in p4 metadata (author names, commit messages, etc) without failing, and produces maximally sane output in git.' +TEST_PASSES_SANITIZE_LEAK=true . ./lib-git-p4.sh python_target_version='3'
Signature · 1 line
2.46.0.dirty
Quoted 8 lines · Patrick Steinhardt
Hi,
this is the second version of my patch series that improves Perforce tests a bit. As with the first version, the main intent is to mark all tests as leak free. The other two patches polish the Perforce tests such that they work on my machine, which has a more recent version of Perforce than what we have been testing with in CI.
Quoted 5 lines
As discussed, despite these patches I consider Perforce to be basically unmaintained, and my patches do not change that. But given that I don't think we can rip out git-p4(1) just so without any kind of announcement up front, I still think that these patches are worth it to help my own cause, namely plugging all memory leaks hit by tests in Git.
Agreed that proposing a removal of git-p4(1) is more of a separate issue here. For now, these patches make sense so we can get closer to making the tests memory leak free. :)
Quoted 11 lines
Changes compared to v1:
- Fix some typos.
- Add in-code comments explaining what the compatibility case statements are about.
Thanks!
Patrick
From looking at the range diff, this version looks good to me. Thanks
-Justin
Quoted 1 line · Justin Tobler
From looking at the range diff, this version looks good to me. Thanks
Thanks, both. Perhaps I should mark this for 'next' by now.