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?
> 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.
> Adapt those tests to handle both old- and new-style depot layouts.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> 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.
> 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";;
> + 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.
> ) &&
> (
> # 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
> p4d -jrF checkpoint.1
> )
> '
> --
> 2.46.0.rc1.dirty
>
