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

From: Patrick Steinhardt <ps@pks.im>
Date: Jul 31, 2024, 10:28 UTC
Message-ID: <ZqoRvMRp5BZ2B6dA@tanuki>
In-Reply-To: <aw2i5dz2tflrosz3enetxelnal6utk4g7kq7yos3jyssloysng@j2nqbxotdujg>

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