git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings
Date: Tue, 01 Nov 2022 09:15:00 +0100	[thread overview]
Message-ID: <221101.86sfj33wmg.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <Y2DNS0W5vgk2Q3qJ@coredump.intra.peff.net>


On Tue, Nov 01 2022, Jeff King wrote:

> On Tue, Nov 01, 2022 at 04:29:31AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > It is unclear as to _why_, but under certain circumstances the warning
>> > about credentials being passed as part of the URL seems to be swallowed
>> > by the `git remote-https` helper in the Windows jobs of Git's CI builds.
>> 
>> ..this description dosen't match what the patch is doing, okey, so
>> there's a case where the remote helper swallows the warnings, i.e. we'll
>> emit fewer than we expected...
>
> 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?

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.

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

Which is what I'm trying to get across here, and why I'm a bit
confused. I.e. I thought you'd agree that we should test that we emit
exactly one warning if & when we've fixed the underlying issue.

That issue is already fixed for "die", so even if you want to loosen up
the test your [2] should only keep the first line removal/addition in
the first two hunks, and drop the 3rd one.

>> > @@ -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.

It should be "-ge 0", i.e. surely that "one warning" can get swallowed
too?

Now, I know that never happens, because we'll always get at least
one.

I'm just saying that as soon as you stop to think about that you must
also come to the conclusion that the "die" ones are OK as-is.

That's because the reason we always get at least one is the same as
we're always guaranteed to emit just one in the "die" case: The parent
process emits it, then dies.

>> And even if some of this was invoking that remote helper and we were
>> modifying it blindly, then presumably the helper swallowing it would
>> result in 0 some of the time, so "-ge 1" would be wrong.
>> 
>> (That's not the case, but it's why I think the patch doesn't make much
>> sense).
>
> 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.

1. http://lore.kernel.org/git/RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com
2. https://lore.kernel.org/git/Y2CD6GBl6ORqKsug@coredump.intra.peff.net/

  reply	other threads:[~2022-11-01  8:30 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-31 19:47 [PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests Johannes Schindelin via GitGitGadget
2022-10-31 19:47 ` [PATCH 1/2] t5516/t5601: avoid using `localhost` " Johannes Schindelin via GitGitGadget
2022-10-31 20:49   ` Ævar Arnfjörð Bjarmason
2022-10-31 23:20   ` Jeff King
2022-11-01  0:59     ` Taylor Blau
2022-11-01  2:28       ` Jeff King
2022-11-01  2:03     ` Jeff King
2022-11-01  2:25       ` Jeff King
2022-11-01  2:26         ` [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 Jeff King
2022-11-01  3:18           ` Ævar Arnfjörð Bjarmason
2022-11-01  7:32             ` Jeff King
2022-11-01 20:37               ` Taylor Blau
2022-11-01  2:26         ` [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings Jeff King
2022-11-01  3:29           ` Ævar Arnfjörð Bjarmason
2022-11-01  7:39             ` Jeff King
2022-11-01  8:15               ` Ævar Arnfjörð Bjarmason [this message]
2022-11-01  9:12                 ` Jeff King
2022-11-01 14:05                   ` Ævar Arnfjörð Bjarmason
2022-11-01  4:54           ` Junio C Hamano
2022-11-01  7:42             ` Jeff King
2022-11-01 20:50               ` Taylor Blau
2022-10-31 19:47 ` Johannes Schindelin via GitGitGadget
2022-10-31 23:22   ` Jeff King
2022-11-01  0:57     ` Taylor Blau
2022-11-01  2:27   ` Jeff King
2022-10-31 20:47 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Ævar Arnfjörð Bjarmason
2022-11-01  1:06   ` Taylor Blau
2022-11-01  2:32   ` Jeff King
2022-11-01  3:01     ` Ævar Arnfjörð Bjarmason
2022-11-01 20:54       ` Taylor Blau
2022-11-01 22:17         ` Ævar Arnfjörð Bjarmason
2022-11-02  0:53           ` Taylor Blau
2022-11-02  8:42         ` [PATCH v3 2/2] t5551: be less strict about the number of credential warnings Jeff King
2022-11-02  8:49           ` Eric Sunshine
2022-11-02  9:15             ` Jeff King
2022-11-02  9:31               ` Eric Sunshine
2022-11-02  9:18           ` Jeff King
2022-11-03  1:31             ` Taylor Blau
2022-11-01  9:35     ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Jeff King
2022-11-01 13:07       ` Ævar Arnfjörð Bjarmason
2022-11-01 21:00         ` Taylor Blau
2022-11-01 21:57           ` Ævar Arnfjörð Bjarmason
2022-11-02  8:19             ` Jeff King
2022-11-04  9:01               ` Ævar Arnfjörð Bjarmason
2022-11-04 13:16                 ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=221101.86sfj33wmg.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).