On Wed, Jun 05, 2024 at 09:45:32AM -0700, Aaron Plattner wrote: > > And in that case you really want to retain the "query" parts of the > > credential after the reject. In this toy example you could just move the > > url-to-cred parsing into the loop, but in the real world it's often more > > complicated. > > > > Arguably even the original code is a bit questionable for this, because > > we don't know if the username came from a helper or from the user, or if > > it was part of the original URL (e.g., "https://user@xxxxxxxxxxx/"; > > should prompt only for the password). But it feels like this hunk is > > making it worse. > > The comment above credential_reject() mentions that it is "readying the > credential for another call to `credential_fill`" which does imply that you > can use it again right away without having to fill in the protocol / host / > path fields. So you're probably right that this should remain the way it > was. Heh, OK. I was the one who wrote that comment originally, which I guess is why it was in the back of my mind. ;) As I said, clearing "username" is a little questionable there. But it also gives the user a chance to update the field, so maybe it's not so bad. There might be other fields in the same boat, but I think you'd really have to think about each one. I'm content to leave the code as it is for now, and if somebody comes up with a case where reject+fill doesn't behave as they expect, we can think about it further. > > The rest of the patch made sense to me, though. As would using > > credential_clear_secrets() here to replace the equivalent lines. > > That's certainly fine with me. Using credential_clear_secrets() to just > replace those two lines would definitely keep the original behavior of this > code. > > I'll send a v3 patch to do that. Great, thanks! -Peff
