When we get a 400 error but we didn't prompt the user, we should prompt the user for credentials before giving up.
<rdar://problem/79313850>
Created attachment 431385 [details] Patch
Comment on attachment 431385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431385&action=review > Tools/ChangeLog:10 > + (credentials): We should attempt to retrieve credentials, even if they Why?
Does getting credentials prompt? Because if so we should figure out a better way to know their required than always fetching them
Comment on attachment 431385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431385&action=review > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:100 > + if authenticated is None and not auth and response.status_code - (response.status_code % 100) == 400: Is `response.status_code - (response.status_code % 100) == 400` equivalent to `response.status_code // 100 == 4`?
(In reply to dewei_zhu from comment #5) > Comment on attachment 431385 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=431385&action=review > > > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:100 > > + if authenticated is None and not auth and response.status_code - (response.status_code % 100) == 400: > > Is `response.status_code - (response.status_code % 100) == 400` equivalent > to `response.status_code // 100 == 4`? Yes. Do you prefer the `response.status_code // 100 == 4` version? No particular reason I used one over the other
Comment on attachment 431385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431385&action=review >> Tools/ChangeLog:10 >> + (credentials): We should attempt to retrieve credentials, even if they > > Why? Because retrieving credentials is a cheap operation. If they aren't there, we won't prompt. But we should be retrieving them (or at least trying to) even if they aren't required.
(In reply to Stephanie Lewis from comment #4) > Does getting credentials prompt? Because if so we should figure out a > better way to know their required than always fetching them No, getting credentials does not, generally prompt. You might get a prompt for keychain if a particular credential is available, but not accessible to Python. That case should be pretty rare though, because in most cases, it's Python that put the credential there in the first place. The only case where you will get a command line prompt is the one where we don't have a credential and we need one. Right now "needing" one means that Python specifically says "hey, I need a credential to do this thing". After this patch, "needing" one means either Python specifically asked, or we got a 400 error on something.
Comment on attachment 431385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431385&action=review >>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:100 >>> + if authenticated is None and not auth and response.status_code - (response.status_code % 100) == 400: >> >> Is `response.status_code - (response.status_code % 100) == 400` equivalent to `response.status_code // 100 == 4`? > > Yes. Do you prefer the `response.status_code // 100 == 4` version? No particular reason I used one over the other // looks cleaner to me. Is this script Python 3 only?
(In reply to Alexey Proskuryakov from comment #9) > Comment on attachment 431385 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=431385&action=review > > >>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:100 > >>> + if authenticated is None and not auth and response.status_code - (response.status_code % 100) == 400: > >> > >> Is `response.status_code - (response.status_code % 100) == 400` equivalent to `response.status_code // 100 == 4`? > > > > Yes. Do you prefer the `response.status_code // 100 == 4` version? No particular reason I used one over the other > > // looks cleaner to me. Is this script Python 3 only? Not yet, buildbot still runs it as Python 2 in a few places
Created attachment 431442 [details] Patch
Comment on attachment 431442 [details] Patch r=me
I still don't understand why we need credentials if we say we don't need them. And your commit log should probably explain that too
(In reply to Stephanie Lewis from comment #13) > I still don't understand why we need credentials if we say we don't need > them. And your commit log should probably explain that too It's more in the case that we don't know if we need them. Consider the case where we are grabbing a commit from a repository. We don't know in advance if that repository is private or public. If the repository is public, we don't need credentials. If it's private, we do. However, if we have credentials available (as in, we can access those credentials without prompting the user) it's better that we use the credentials. This is for two reasons: First, if we happen to be accessing a private repo, we saved ourselves a request. Second, though, is that if we're accessing a public repo with a credentialed request, that request does not count against the rate limit for your current IP address.
Created attachment 431458 [details] Patch for landing
Committed r278890 (238832@main): <https://commits.webkit.org/238832@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431458 [details].