threads / patch / 61828

patch, 3 partsImprovements for Perforce tests

Subject: [PATCH 0/3] Improvements for Perforce tests

## tl;dr

19 messages between Jul 23, 2024 and Jul 31, 2024. Diffs are folded; open one to read it.

replies: 18people: 4as markdown or json

Patrick Steinhardt· Jul 23, 2024, 14:05 UTC · lore
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(-)
-- 
2.46.0.rc1.dirty
Patrick Steinhardt· Jul 23, 2024, 14:05 UTC · re: Patrick Steinhardt · lore

[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.
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(-)
Show changes to 3 files +38 −8

t/t9800-git-p4-basic.sh, t/t9802-git-p4-filetype.sh, t/t9825-git-p4-handle-utf16-without-bom.sh

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 &&
 	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
 	) &&
 	(
 		# 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 &&
 		p4d -jrF checkpoint.1
 	)
 '

-- 
2.46.0.rc1.dirty
Justin Tobler· Jul 30, 2024, 22:41 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 1/3] t98xx: fix Perforce tests with p4d r23 and newer

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";;
> +		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.
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
>
Patrick Steinhardt· Jul 31, 2024, 10:28 UTC · re: Justin Tobler · lore

Re: [PATCH 1/3] t98xx: fix Perforce tests with p4d r23 and newer

On Tue, Jul 30, 2024 at 05:41:18PM -0500, Justin Tobler wrote:
Show 11 quoted lines
> 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?

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.

Show 13 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.

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.

Show 42 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.
Fair, I will add a comment to all three sites.
Patric
Patrick Steinhardt· Jul 23, 2024, 14:05 UTC · re: Patrick Steinhardt · lore

[PATCH 2/3] ci: update Perforce version to r23.2

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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +1 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 6ec0f85972..b59fd7c1fd 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -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
 

-- 
2.46.0.rc1.dirty
Johannes Schindelin· Jul 24, 2024, 08:39 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 2/3] ci: update Perforce version to r23.2

Hi Patrick,
On Tue, 23 Jul 2024, Patrick Steinhardt wrote:
Show 5 quoted lines
> 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

Patrick Steinhardt· Jul 24, 2024, 09:01 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/3] ci: update Perforce version to r23.2

On Wed, Jul 24, 2024 at 10:39:54AM +0200, Johannes Schindelin wrote:
Show 32 quoted lines
> Hi Patrick,
> 
> On Tue, 23 Jul 2024, Patrick Steinhardt wrote:
> 
> > 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.

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
Junio C Hamano· Jul 24, 2024, 16:10 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 2/3] ci: update Perforce version to r23.2

Patrick Steinhardt <ps@pks.im> writes:
Show 11 quoted lines
> 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.
Patrick Steinhardt· Jul 30, 2024, 06:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] ci: update Perforce version to r23.2

On Wed, Jul 24, 2024 at 09:10:54AM -0700, Junio C Hamano wrote:
Show 21 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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).

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
Justin Tobler· Jul 30, 2024, 22:48 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 2/3] ci: update Perforce version to r23.2

On 24/07/23 04:05PM, Patrick Steinhardt wrote:
> 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/
> 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.

Show 22 quoted lines
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  ci/install-dependencies.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 6ec0f85972..b59fd7c1fd 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -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
>  
> -- 
> 2.46.0.rc1.dirty
>
Patrick Steinhardt· Jul 31, 2024, 10:15 UTC · re: Justin Tobler · lore

Re: [PATCH 2/3] ci: update Perforce version to r23.2

On Tue, Jul 30, 2024 at 05:48:38PM -0500, Justin Tobler wrote:
Show 13 quoted lines
> On 24/07/23 04:05PM, Patrick Steinhardt wrote:
> > 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/
> 
> > 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
Patrick Steinhardt· Jul 23, 2024, 14:05 UTC · re: Patrick Steinhardt · lore

[PATCH 3/3] t98xx: mark Perforce tests as memory-leak free

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=check` won't notice that those tests are indeed memory leak free.

Mark those tests accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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(+)
Show changes to 35 files +35 −0

t/t9800-git-p4-basic.sh, t/t9801-git-p4-branch.sh, t/t9802-git-p4-filetype.sh, t/t9803-git-p4-shell-metachars.sh, t/t9804-git-p4-label.sh, t/t9805-git-p4-skip-submit-edit.sh, t/t9806-git-p4-options.sh, t/t9808-git-p4-chdir.sh, t/t9809-git-p4-client-view.sh, t/t9810-git-p4-rcs.sh, t/t9811-git-p4-label-import.sh, t/t9812-git-p4-wildcards.sh, t/t9813-git-p4-preserve-users.sh, t/t9814-git-p4-rename.sh, t/t9815-git-p4-submit-fail.sh, t/t9816-git-p4-locked.sh, t/t9817-git-p4-exclude.sh, t/t9818-git-p4-block.sh, t/t9819-git-p4-case-folding.sh, t/t9820-git-p4-editor-handling.sh, t/t9821-git-p4-path-variations.sh, t/t9822-git-p4-path-encoding.sh, t/t9823-git-p4-mock-lfs.sh, t/t9825-git-p4-handle-utf16-without-bom.sh, t/t9826-git-p4-keep-empty-commits.sh, t/t9827-git-p4-change-filetype.sh, t/t9828-git-p4-map-user.sh, t/t9829-git-p4-jobs.sh, t/t9830-git-p4-symlink-dir.sh, t/t9831-git-p4-triggers.sh, t/t9832-unshelve.sh, t/t9833-errors.sh, t/t9834-git-p4-file-dir-bug.sh, t/t9835-git-p4-metadata-encoding-python2.sh, t/t9836-git-p4-metadata-encoding-python3.sh

diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 4e95622670..00b5bdd727 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -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' '
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index c598011635..cdbfacc727 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -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' '
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 557e11b16c..4f8cb4a768 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 filetype tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh
index 2913277013..ab7fe16266 100755
--- a/t/t9803-git-p4-shell-metachars.sh
+++ b/t/t9803-git-p4-shell-metachars.sh
@@ -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' '
diff --git a/t/t9804-git-p4-label.sh b/t/t9804-git-p4-label.sh
index 3236457106..c8963fd398 100755
--- a/t/t9804-git-p4-label.sh
+++ b/t/t9804-git-p4-label.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 label tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index 90ef647db7..72dce3d2b4 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh
@@ -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' '
diff --git a/t/t9806-git-p4-options.sh b/t/t9806-git-p4-options.sh
index c26d297433..e4ce44ebf3 100755
--- a/t/t9806-git-p4-options.sh
+++ b/t/t9806-git-p4-options.sh
@@ -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' '
diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index 58a9b3b71e..342f7f3d4a 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 relative chdir'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 9c9710d8c7..f33fdea889 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 client view'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index 5fe83315ec..15e32c9f35 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 rcs keywords'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 CP1252="\223\224"
diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5ac5383fb7..52a4b0af81 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -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' '
diff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh
index 254a7c2446..46aa5fd56c 100755
--- a/t/t9812-git-p4-wildcards.sh
+++ b/t/t9812-git-p4-wildcards.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 wildcards'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index fd018c87a8..0efea28da2 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 preserve users'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 2a9838f37f..00df6ebd3b 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 rename'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9815-git-p4-submit-fail.sh b/t/t9815-git-p4-submit-fail.sh
index c766fd159f..92ef9d8c24 100755
--- a/t/t9815-git-p4-submit-fail.sh
+++ b/t/t9815-git-p4-submit-fail.sh
@@ -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' '
diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
index 5e904ac80d..e687fbc25f 100755
--- a/t/t9816-git-p4-locked.sh
+++ b/t/t9816-git-p4-locked.sh
@@ -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' '
diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index ec3d937c6a..3deb334fed 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -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' '
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index de591d875c..091bb72bdb 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -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' '
diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
index b4d93f0c17..985be20357 100755
--- a/t/t9819-git-p4-case-folding.sh
+++ b/t/t9819-git-p4-case-folding.sh
@@ -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
diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh
index fa1bba1dd9..48e4dfb95c 100755
--- a/t/t9820-git-p4-editor-handling.sh
+++ b/t/t9820-git-p4-editor-handling.sh
@@ -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' '
diff --git a/t/t9821-git-p4-path-variations.sh b/t/t9821-git-p4-path-variations.sh
index ef80f1690b..49691c53da 100755
--- a/t/t9821-git-p4-path-variations.sh
+++ b/t/t9821-git-p4-path-variations.sh
@@ -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' '
diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
index 572d395498..e62ed49f51 100755
--- a/t/t9822-git-p4-path-encoding.sh
+++ b/t/t9822-git-p4-path-encoding.sh
@@ -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"
diff --git a/t/t9823-git-p4-mock-lfs.sh b/t/t9823-git-p4-mock-lfs.sh
index 88b76dc4d6..98a40d8af3 100755
--- a/t/t9823-git-p4-mock-lfs.sh
+++ b/t/t9823-git-p4-mock-lfs.sh
@@ -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 () {
diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh
index 8e34f72198..4d1d4fa5f3 100755
--- a/t/t9825-git-p4-handle-utf16-without-bom.sh
+++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
@@ -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"
diff --git a/t/t9826-git-p4-keep-empty-commits.sh b/t/t9826-git-p4-keep-empty-commits.sh
index fd64afe064..54083f842e 100755
--- a/t/t9826-git-p4-keep-empty-commits.sh
+++ b/t/t9826-git-p4-keep-empty-commits.sh
@@ -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' '
diff --git a/t/t9827-git-p4-change-filetype.sh b/t/t9827-git-p4-change-filetype.sh
index d3670bd7a2..3476ea2fd3 100755
--- a/t/t9827-git-p4-change-filetype.sh
+++ b/t/t9827-git-p4-change-filetype.sh
@@ -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' '
diff --git a/t/t9828-git-p4-map-user.sh b/t/t9828-git-p4-map-user.sh
index ca6c2942bd..7c8f9e3930 100755
--- a/t/t9828-git-p4-map-user.sh
+++ b/t/t9828-git-p4-map-user.sh
@@ -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' '
diff --git a/t/t9829-git-p4-jobs.sh b/t/t9829-git-p4-jobs.sh
index 88cfb1fcd3..3fc0948d9c 100755
--- a/t/t9829-git-p4-jobs.sh
+++ b/t/t9829-git-p4-jobs.sh
@@ -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' '
diff --git a/t/t9830-git-p4-symlink-dir.sh b/t/t9830-git-p4-symlink-dir.sh
index 3fb6960c18..02561a7f0e 100755
--- a/t/t9830-git-p4-symlink-dir.sh
+++ b/t/t9830-git-p4-symlink-dir.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 symlinked directories'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh
index ff6c0352e6..f287f41e37 100755
--- a/t/t9831-git-p4-triggers.sh
+++ b/t/t9831-git-p4-triggers.sh
@@ -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' '
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 6b3cb0414a..a266775408 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -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' '
diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh
index e22369ccdf..da1d30c142 100755
--- a/t/t9833-errors.sh
+++ b/t/t9833-errors.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 errors'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh
index dac67e89d7..565870fc74 100755
--- a/t/t9834-git-p4-file-dir-bug.sh
+++ b/t/t9834-git-p4-file-dir-bug.sh
@@ -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' '
diff --git a/t/t9835-git-p4-metadata-encoding-python2.sh b/t/t9835-git-p4-metadata-encoding-python2.sh
index 036bf79c66..ad20ffdede 100755
--- a/t/t9835-git-p4-metadata-encoding-python2.sh
+++ b/t/t9835-git-p4-metadata-encoding-python2.sh
@@ -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'
diff --git a/t/t9836-git-p4-metadata-encoding-python3.sh b/t/t9836-git-p4-metadata-encoding-python3.sh
index 63350dc4b5..71ae763399 100755
--- a/t/t9836-git-p4-metadata-encoding-python3.sh
+++ b/t/t9836-git-p4-metadata-encoding-python3.sh
@@ -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'

-- 
2.46.0.rc1.dirty
Justin Tobler· Jul 30, 2024, 22:54 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 3/3] t98xx: mark Perforce tests as memory-leak free

On 24/07/23 04:05PM, Patrick Steinhardt wrote:
Show 7 quoted lines
> 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=check` won'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

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
Patrick Steinhardt· Jul 31, 2024, 10:37 UTC · re: Patrick Steinhardt · lore

[PATCH v2 0/3] Improvements for Perforce tests

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.

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.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
3:  d0a80ba403 = 3:  bf03405db9 t98xx: mark Perforce tests as memory-leak free
base-commit: d19b6cd2dd72dc811f19df4b32c7ed223256c3ee
-- 
2.46.0.dirty
Patrick Steinhardt· Jul 31, 2024, 10:37 UTC · re: Patrick Steinhardt · lore

[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.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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(-)
Show changes to 3 files +48 −8

t/t9800-git-p4-basic.sh, t/t9802-git-p4-filetype.sh, t/t9825-git-p4-handle-utf16-without-bom.sh

diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 53af8e34ac..0816763e46 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -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
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index bb236cd2b5..df01a5d338 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -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
diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh
index f049ff8229..6a60b32349 100755
--- a/t/t9825-git-p4-handle-utf16-without-bom.sh
+++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
@@ -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
 	)
 '

-- 
2.46.0.dirty
Patrick Steinhardt· Jul 31, 2024, 10:37 UTC · re: Patrick Steinhardt · lore

[PATCH v2 2/3] ci: update Perforce version to r23.2

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 executables as well as the macOS archives directly and would thus require more work.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +1 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 6ec0f85972..b59fd7c1fd 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -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
 

-- 
2.46.0.dirty
Patrick Steinhardt· Jul 31, 2024, 10:37 UTC · re: Patrick Steinhardt · lore

[PATCH v2 3/3] t98xx: mark Perforce tests as memory-leak free

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=check` won't notice that those tests are indeed memory leak free.

Mark those tests accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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(+)
Show changes to 35 files +35 −0

t/t9800-git-p4-basic.sh, t/t9801-git-p4-branch.sh, t/t9802-git-p4-filetype.sh, t/t9803-git-p4-shell-metachars.sh, t/t9804-git-p4-label.sh, t/t9805-git-p4-skip-submit-edit.sh, t/t9806-git-p4-options.sh, t/t9808-git-p4-chdir.sh, t/t9809-git-p4-client-view.sh, t/t9810-git-p4-rcs.sh, t/t9811-git-p4-label-import.sh, t/t9812-git-p4-wildcards.sh, t/t9813-git-p4-preserve-users.sh, t/t9814-git-p4-rename.sh, t/t9815-git-p4-submit-fail.sh, t/t9816-git-p4-locked.sh, t/t9817-git-p4-exclude.sh, t/t9818-git-p4-block.sh, t/t9819-git-p4-case-folding.sh, t/t9820-git-p4-editor-handling.sh, t/t9821-git-p4-path-variations.sh, t/t9822-git-p4-path-encoding.sh, t/t9823-git-p4-mock-lfs.sh, t/t9825-git-p4-handle-utf16-without-bom.sh, t/t9826-git-p4-keep-empty-commits.sh, t/t9827-git-p4-change-filetype.sh, t/t9828-git-p4-map-user.sh, t/t9829-git-p4-jobs.sh, t/t9830-git-p4-symlink-dir.sh, t/t9831-git-p4-triggers.sh, t/t9832-unshelve.sh, t/t9833-errors.sh, t/t9834-git-p4-file-dir-bug.sh, t/t9835-git-p4-metadata-encoding-python2.sh, t/t9836-git-p4-metadata-encoding-python3.sh

diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 0816763e46..3e6dfce248 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -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' '
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index c598011635..cdbfacc727 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -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' '
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index df01a5d338..1bc48305b0 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 filetype tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh
index 2913277013..ab7fe16266 100755
--- a/t/t9803-git-p4-shell-metachars.sh
+++ b/t/t9803-git-p4-shell-metachars.sh
@@ -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' '
diff --git a/t/t9804-git-p4-label.sh b/t/t9804-git-p4-label.sh
index 3236457106..c8963fd398 100755
--- a/t/t9804-git-p4-label.sh
+++ b/t/t9804-git-p4-label.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 label tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index 90ef647db7..72dce3d2b4 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh
@@ -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' '
diff --git a/t/t9806-git-p4-options.sh b/t/t9806-git-p4-options.sh
index c26d297433..e4ce44ebf3 100755
--- a/t/t9806-git-p4-options.sh
+++ b/t/t9806-git-p4-options.sh
@@ -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' '
diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index 58a9b3b71e..342f7f3d4a 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 relative chdir'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 9c9710d8c7..f33fdea889 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 client view'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index 5fe83315ec..15e32c9f35 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 rcs keywords'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 CP1252="\223\224"
diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 5ac5383fb7..52a4b0af81 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -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' '
diff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh
index 254a7c2446..46aa5fd56c 100755
--- a/t/t9812-git-p4-wildcards.sh
+++ b/t/t9812-git-p4-wildcards.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 wildcards'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index fd018c87a8..0efea28da2 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 preserve users'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 2a9838f37f..00df6ebd3b 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 rename'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9815-git-p4-submit-fail.sh b/t/t9815-git-p4-submit-fail.sh
index c766fd159f..92ef9d8c24 100755
--- a/t/t9815-git-p4-submit-fail.sh
+++ b/t/t9815-git-p4-submit-fail.sh
@@ -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' '
diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
index 5e904ac80d..e687fbc25f 100755
--- a/t/t9816-git-p4-locked.sh
+++ b/t/t9816-git-p4-locked.sh
@@ -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' '
diff --git a/t/t9817-git-p4-exclude.sh b/t/t9817-git-p4-exclude.sh
index ec3d937c6a..3deb334fed 100755
--- a/t/t9817-git-p4-exclude.sh
+++ b/t/t9817-git-p4-exclude.sh
@@ -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' '
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index de591d875c..091bb72bdb 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -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' '
diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
index b4d93f0c17..985be20357 100755
--- a/t/t9819-git-p4-case-folding.sh
+++ b/t/t9819-git-p4-case-folding.sh
@@ -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
diff --git a/t/t9820-git-p4-editor-handling.sh b/t/t9820-git-p4-editor-handling.sh
index fa1bba1dd9..48e4dfb95c 100755
--- a/t/t9820-git-p4-editor-handling.sh
+++ b/t/t9820-git-p4-editor-handling.sh
@@ -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' '
diff --git a/t/t9821-git-p4-path-variations.sh b/t/t9821-git-p4-path-variations.sh
index ef80f1690b..49691c53da 100755
--- a/t/t9821-git-p4-path-variations.sh
+++ b/t/t9821-git-p4-path-variations.sh
@@ -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' '
diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
index 572d395498..e62ed49f51 100755
--- a/t/t9822-git-p4-path-encoding.sh
+++ b/t/t9822-git-p4-path-encoding.sh
@@ -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"
diff --git a/t/t9823-git-p4-mock-lfs.sh b/t/t9823-git-p4-mock-lfs.sh
index 88b76dc4d6..98a40d8af3 100755
--- a/t/t9823-git-p4-mock-lfs.sh
+++ b/t/t9823-git-p4-mock-lfs.sh
@@ -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 () {
diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh
index 6a60b32349..d0b86537dd 100755
--- a/t/t9825-git-p4-handle-utf16-without-bom.sh
+++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
@@ -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"
diff --git a/t/t9826-git-p4-keep-empty-commits.sh b/t/t9826-git-p4-keep-empty-commits.sh
index fd64afe064..54083f842e 100755
--- a/t/t9826-git-p4-keep-empty-commits.sh
+++ b/t/t9826-git-p4-keep-empty-commits.sh
@@ -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' '
diff --git a/t/t9827-git-p4-change-filetype.sh b/t/t9827-git-p4-change-filetype.sh
index d3670bd7a2..3476ea2fd3 100755
--- a/t/t9827-git-p4-change-filetype.sh
+++ b/t/t9827-git-p4-change-filetype.sh
@@ -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' '
diff --git a/t/t9828-git-p4-map-user.sh b/t/t9828-git-p4-map-user.sh
index ca6c2942bd..7c8f9e3930 100755
--- a/t/t9828-git-p4-map-user.sh
+++ b/t/t9828-git-p4-map-user.sh
@@ -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' '
diff --git a/t/t9829-git-p4-jobs.sh b/t/t9829-git-p4-jobs.sh
index 88cfb1fcd3..3fc0948d9c 100755
--- a/t/t9829-git-p4-jobs.sh
+++ b/t/t9829-git-p4-jobs.sh
@@ -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' '
diff --git a/t/t9830-git-p4-symlink-dir.sh b/t/t9830-git-p4-symlink-dir.sh
index 3fb6960c18..02561a7f0e 100755
--- a/t/t9830-git-p4-symlink-dir.sh
+++ b/t/t9830-git-p4-symlink-dir.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 symlinked directories'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9831-git-p4-triggers.sh b/t/t9831-git-p4-triggers.sh
index ff6c0352e6..f287f41e37 100755
--- a/t/t9831-git-p4-triggers.sh
+++ b/t/t9831-git-p4-triggers.sh
@@ -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' '
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 6b3cb0414a..a266775408 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -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' '
diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh
index e22369ccdf..da1d30c142 100755
--- a/t/t9833-errors.sh
+++ b/t/t9833-errors.sh
@@ -2,6 +2,7 @@
 
 test_description='git p4 errors'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-git-p4.sh
 
 test_expect_success 'start p4d' '
diff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh
index dac67e89d7..565870fc74 100755
--- a/t/t9834-git-p4-file-dir-bug.sh
+++ b/t/t9834-git-p4-file-dir-bug.sh
@@ -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' '
diff --git a/t/t9835-git-p4-metadata-encoding-python2.sh b/t/t9835-git-p4-metadata-encoding-python2.sh
index 036bf79c66..ad20ffdede 100755
--- a/t/t9835-git-p4-metadata-encoding-python2.sh
+++ b/t/t9835-git-p4-metadata-encoding-python2.sh
@@ -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'
diff --git a/t/t9836-git-p4-metadata-encoding-python3.sh b/t/t9836-git-p4-metadata-encoding-python3.sh
index 63350dc4b5..71ae763399 100755
--- a/t/t9836-git-p4-metadata-encoding-python3.sh
+++ b/t/t9836-git-p4-metadata-encoding-python3.sh
@@ -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'

-- 
2.46.0.dirty
Justin Tobler· Jul 31, 2024, 20:50 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 0/3] Improvements for Perforce tests

On 24/07/31 12:37PM, Patrick Steinhardt wrote:
Show 13 quoted lines
> 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.
> 
> 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. :)

Show 11 quoted 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
Junio C Hamano· Jul 31, 2024, 22:32 UTC · re: Justin Tobler · lore

Re: [PATCH v2 0/3] Improvements for Perforce tests

Justin Tobler <jltobler@gmail.com> writes:
> From looking at the range diff, this version looks good to me. Thanks
Thanks, both.  Perhaps I should mark this for 'next' by now.

← back to recent threads