Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516

From: Jeff King <peff@peff.net>
Date: Nov 1, 2022, 07:32 UTC
Message-ID: <Y2DLdsM2CYY3Z2Sc@coredump.intra.peff.net>
In-Reply-To: <221101.865yfz5pjp.gmgdl@evledraar.gmail.com>

On Tue, Nov 01, 2022 at 04:18:32AM +0100, Ævar Arnfjörð Bjarmason wrote:

> Did you consider just using git://; on the WIP branch I linked to where
> I fixed these tests quite a bit already I left them in their own file in
> anticipation of having to test that (but didn't finish that yet). I.e.:

No, I didn't really consider that. Mostly because I was trying to stick with the original intent of 6dcbdc0d66 that created them.

If we toss that out, then in theory that widens the options. And in some ways it's nice to use git://, because it has fewer dependencies and so is run on more platforms. But it strikes me as a pretty unrealistic test, just because credentials in git:// URLs make no sense and are not something anybody would do.

As you note, since the error comes from remote.c, it would still trigger. But it's a bit more "white box" testing than I think makes sense here. I prefer the original tests' method of trying to create plausible real-world scenarios and seeing how they behave (and I think my patch even improves that, since having an actual server on the other end is the usual case).

-Peff