Re: [PATCH 5/5] make askpass_prompt a global prompt method for asking users

Sven Strickroth [off-list ref] writes:

Signed-off-by: Sven Strickroth <redacted>
---

I think the end result of having a prompt function that can be used with
or without echoing like Peff's git_prompt() series does is a very good
thing. But then we just should introduce "prompt()" from day one of the
series, instead of introducing a half-featured one "askpass_prompt()" and
then later renaming the callers like this patch does.

It may a good idea to take the stepwise approach like this series does,
but in that case, the proposed log message must explain what the new
"prompt()" function is and does. It is derived from askpass_prompt() and
apparently it does more than its ancestor, but what are differences
between the two?

For example, it is totally unclear why these two are equivalent without
any explanation in the commit log message.

-	$choice = Git->askpass_prompt("Certificate unknown. " . $options);
-	if (!defined $choice) {
-		print STDERR $options;
-		STDERR->flush;
-		$choice = lc(substr(<STDIN> || 'R', 0, 1));
-	}
+	$choice = substr(Git->prompt("Certificate unknown. " . $options) || 'R', 0, 1);

or this:

-	my $filename = Git->askpass_prompt("Client certificate filename:");
-	if (!defined $filename) {
-		print STDERR "Client certificate filename: ";
-		STDERR->flush;
-		chomp($filename = <STDIN>);
-	}
-	$cred->cert_file($filename);
+	$cred->cert_file(Git->prompt("Client certificate filename: "));

I *suspect* the difference is that you discarded that "return false at the
end to let the caller do whatever they want" found in patch 1/5 and have
the fallback inside the prompt() funtion now. And if that is the primary
difference between the old "askpass_prompt()" and the new "prompt()", I
tend to think that the series should be restructured to use the "prompt()"
semantics from the beginning. No reason to start with a known-to-be-wrong
way to do a thing and then fix it in a series that is new to the codebase.

Thanks.