Re: [PATCH 1/3] t98xx: fix Perforce tests with p4d r23 and newer
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Jul 30, 2024, 22:41 UTC
- Message-ID
- <aw2i5dz2tflrosz3enetxelnal6utk4g7kq7yos3jyssloysng@j2nqbxotdujg>
- In-Reply-To
- <f40f62f257de38c3a38e942eb9ca1d2109c2b059.1721740612.git.ps@pks.im>
On 24/07/23 04:05PM, Patrick Steinhardt wrote:
> 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?
Show 6 quoted 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.
Show 30 quoted lines
> Adapt those tests to handle both old- and new-style depot layouts. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > 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(-) > > diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh > index 53af8e34ac..4e95622670 100755 > --- a/t/t9800-git-p4-basic.sh > +++ b/t/t9800-git-p4-basic.sh > @@ -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 &&
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.
Show 26 quoted lines
> 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
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> index bb236cd2b5..557e11b16c 100755
> --- a/t/t9802-git-p4-filetype.sh
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -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";;
> + esacLooks 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.
Show 29 quoted lines
> ) && > ( > # Make sure symlink really is empty. Asking > diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh > index f049ff8229..8e34f72198 100755 > --- a/t/t9825-git-p4-handle-utf16-without-bom.sh > +++ b/t/t9825-git-p4-handle-utf16-without-bom.sh > @@ -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 &&
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
Show 6 quoted lines
> p4d -jrF checkpoint.1 > ) > ' > -- > 2.46.0.rc1.dirty >