git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

From
Jeff King <peff@peff.net>
Date
Nov 1, 2022, 09:12 UTC
Message-ID
<Y2Di/5Hgp6IdSuuk@coredump.intra.peff.net>
In-Reply-To
<221101.86sfj33wmg.gmgdl@evledraar.gmail.com>
On Tue, Nov 01, 2022 at 09:15:00AM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 14 quoted lines
> > So I really didn't revisit this commit much at all, and was just trying
> > to save Dscho (or Taylor) the work of having to rebase it, if we go with
> > my patch 1.
> >
> > IMHO it is OK enough as it is, but if I were writing it from scratch I
> > probably would have given the rationale that the tests are insiting on a
> > dumb, sub-optimal behavior. And flakes or inconsistencies aside, they
> > should be asserting only the presence or absence of the message. And
> > probably would have left each at "grep" and dropped the test_line_count
> > totally.
> 
> Do you mean that even if we fix the bug and consistently emit one and
> only one such message you'd like to have the tests not assert that
> that's the case?

No, I wouldn't mind it, if that is a bug we've fixed. I just mean that the tests as written never wanted to say "3 is the absolute right number of times to write this message". They only put "3" there because it made things pass.

> I do think that UX is important enough to test for, particularly if
> we've had a bug related to that that we've fixed. I.e. if something in
> the direction of my [1] goes in.

Sure, I don't mind at all a test for it. In the short-term, if you want a test that fails, I'd prefer it be separate so that we can test the useful existing behavior that _does_ work. If the multiple-messages bug is fixed, I don't mind folding them together into a single test that passes.

Show 7 quoted lines
> > It is not even clear to me that the remote-https is the one being
> > swallowed (at least, I have not seen an argument or evidence that this
> > is so; it does seem plausible).
> 
> It is the case, the only ones that are going to be duplicated are the
> "warn" ones, because for "die" we'll die right away in the parent
> process.

Right, I understand why "die" produces only one. My question was when we produce 2 on Windows (sometimes?) but 3 elsewhere, are we sure it is the one from remote-https that is eaten, or could it ever be one of the others?

In a sense we do not need to worry about "why is it sometimes eaten" if the bug is fixed to produce only the one message. But it may point to a separate and interesting problem (e.g., is stderr from remote-https not reliable?).

Show 18 quoted lines
> >> > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '
> >> >  	test_must_fail git -c transfer.credentialsInUrl=die \
> >> >  		push $url_userpass 2>err &&
> >> >  	grep "fatal: $message" err >warnings &&
> >> > -	test_line_count = 1 warnings
> >> > +	test_line_count -ge 1 warnings
> >> >  '
> >> 
> >> ...but then why are we modifying these codepaths that have nothing to do
> >> with invoking the remote helper, i.e. where we die early before we get
> >> to that?
> >
> > If you're arguing that we should only do s/= 3/-ge 1/ for the test that
> > is flaking, I could buy that.
> 
> I'm saying that if we're doing a handwaivy-fix and saying "sometimes the
> message gets swallowed" and fixing this blindly without checking how it
> works, then changing "= 1" to "-ge 1" doesn't make sense.

Right, I'm fine with that (I perhaps should have said something stronger than "I could buy that"). As I said, I was mostly just rebasing Dscho's patch and I think it was good enough in the sense that it was hand-waving away the whole "there may be more than one" problem.

But I do agree that we'll never see more in the "die" cases, and there is no need to change them.

Show 12 quoted lines
> > I thought the point is that the outer program calling the helper would
> > consistently produce the error, always yielding at least one instance.
> > The helper one is generally "extra" and undesired.
> 
> Yes, exactly. Which is what my fix[1] the root cause addresses.
> 
> Anyway, I'm just trying to help here. If you/Johannes/others want to go
> with the "hotfix" as-is that's fine my me.
> 
> I just don't see what the hurry is, it's been this way for two releases,
> if it's flaky that's been the case for months, I'd think we could just
> fix the root cause.

It recently bit me twice, so maybe I am giving it more urgency than it deserves (or maybe something changed in CI to make it more likely).

I do think it would be nice to fix it. I don't love your patch for the reasons I replied there (not your fault; it's inherently a crappy and complicated problem). In the meantime, I'd like to see CI fixed, as it is wasting developer's time. And that's why I called Dscho's loosening "good enough". It is hopefully a temporary state anyway.

But I would be just as happy to see a similar patch which just changed the 2/3 lines to "-ge 1" (or just a straight grep).

-Peff
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 17 of 45 in “t5516/t5601: avoid using localhost for failing HTTPS requests”
  1. 0/2 t5516/t5601: avoid using localhost for failing HTTPS requestsJohannes Schindelin via GitGitGadget, Oct 31, 2022
  2. 1/2 t5516/t5601: avoid using `localhost` for failing HTTPS requestsJohannes Schindelin via GitGitGadget, Oct 31, 2022
  3. Ævar Arnfjörð BjarmasonOct 31, 2022
  4. Jeff KingOct 31, 2022
  5. Taylor BlauNov 1, 2022
  6. Jeff KingNov 1, 2022
  7. Jeff KingNov 1, 2022
  8. Jeff KingNov 1, 2022
  9. 1/2 t5516: move plaintext-password tests from t5601 and t5516Jeff King, Nov 1, 2022
  10. Ævar Arnfjörð BjarmasonNov 1, 2022
  11. Jeff KingNov 1, 2022
  12. Taylor BlauNov 1, 2022
  13. 2/2 t5516/t5601: be less strict about the number of credential warningsJeff King, Nov 1, 2022
  14. Ævar Arnfjörð BjarmasonNov 1, 2022
  15. Jeff KingNov 1, 2022
  16. Ævar Arnfjörð BjarmasonNov 1, 2022
  17. Jeff KingNov 1, 2022
  18. Ævar Arnfjörð BjarmasonNov 1, 2022
  19. Junio C HamanoNov 1, 2022
  20. Jeff KingNov 1, 2022
  21. Taylor BlauNov 1, 2022
  22. 2/2 t5516/t5601: be less strict about the number of credential warningsJohannes Schindelin via GitGitGadget, Oct 31, 2022
  23. Jeff KingOct 31, 2022
  24. Taylor BlauNov 1, 2022
  25. Jeff KingNov 1, 2022
  26. fetch: stop emitting duplicate transfer.credentialsInUrl=warn warningsÆvar Arnfjörð Bjarmason, Oct 31, 2022
  27. Taylor BlauNov 1, 2022
  28. Jeff KingNov 1, 2022
  29. Ævar Arnfjörð BjarmasonNov 1, 2022
  30. Taylor BlauNov 1, 2022
  31. Ævar Arnfjörð BjarmasonNov 1, 2022
  32. Taylor BlauNov 2, 2022
  33. 2/2 t5551: be less strict about the number of credential warningsJeff King, Nov 2, 2022
  34. Eric SunshineNov 2, 2022
  35. Jeff KingNov 2, 2022
  36. Eric SunshineNov 2, 2022
  37. Jeff KingNov 2, 2022
  38. Taylor BlauNov 3, 2022
  39. Jeff KingNov 1, 2022
  40. Ævar Arnfjörð BjarmasonNov 1, 2022
  41. Taylor BlauNov 1, 2022
  42. Ævar Arnfjörð BjarmasonNov 1, 2022
  43. Jeff KingNov 2, 2022
  44. Ævar Arnfjörð BjarmasonNov 4, 2022
  45. Jeff KingNov 4, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.