On Wed, Sep 22, 2021 at 01:51:39PM -0700, Junio C Hamano wrote: > >> Neither pattern of the above two will not match the HTTP/2 one, so > >> the first one would report "there is no leakage of Auth with a > >> caplital letter"; the second one may see only one pre-upgrade Auth > >> with a capital letter, but as long as it does find one, it should be > >> happy, no? > >> > >> I am a bit puzzled how the test gets confused. > > > > The first one matches nothing, because the HTTP/2 one which fails to > > redact has a lower-case "A". The second one _does_ match, because ... > > I thought we were talking about the original case sensitive test > getting confused when testing the software that is fixed, > i.e. HTTP/2 lowercase "authorization" line properly redacted. No, sorry. I meant: before the fix, even if we were running HTTP/2, the test does not detect the bug. And thus it is hard to realize that the fix is indeed making the bug go away. :) > > I get (with some extraneous headers omitted): > > ... > > => Send header: GET /auth/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > > => Send header: Authorization: Basic <redacted> > > So, this is what we see in HTTP/1.1 (with capitalization). And then > ... > > > ... > > => Send header: POST /auth/smart/repo.git/git-upload-pack HTTP/2 > > => Send header: authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA== > > this one, once the redaction code is fixed by applying this patch, > would show that we redacted it, too, no? Correct. But the test, without switching to "grep -i", does not realize that. > With or without the fix in the code, I agree that neither of the two > "grep" patterns without "grep -i" change will match this line. So > the end result is that the test finds no unredacted line, and one > redacted one (instead of two). > > I agree that it is *not* testing what we want to test, and if you > said so, I wouldn't have been puzzled. I just wanted to know if > there is something _else_ (other than "gee, we are not testing the > HTTP/2 case at all") going on that I failed to read in your > "... gets confused". No, I think we are on the same page now. Do you want me to take another stab at writing the commit message to clarify things (i.e., do we think it's badly written, or was it just mis-interpreted)? -Peff
