Re: [PATCH 2/5] switch to central prompt method

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2011, 20:47 UTC
Message-ID: <7vpqf968sr.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4EF9ECEF.6020403@tu-clausthal.de>

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

It would be better to have this and the previous patch squashed into one commit, for three reasons:

 - It is far easier to review the patch that way, because it will show the
   old way to get the password from the user as the removed code and new
   way to do so as the added helper function, enabling reviewers to
   compare the two in a single review session, to see if the change keeps
   the feature bug-to-bug compatible, introduces any regression, and/or
   offers improvements over existing code.

 - If there were a bug in the implementation of askpass_prompt method
   introduced in patch 1 without being used, and the calling codepath
   introduced in patch 2 is bug-free (i.e. it correctly follows the
   calling convention of the new method, only the implementation of the
   method is buggy), bisection will still point at patch 2 that dumped the
   old proven working way and started using the buggy new implementation.

   Of course, it is possible that patch 1 perfectly implements the new
   method and a bug exists in the way the caller introduced in patch 2
   calls the method and in such a case bisection will correctly point out
   that the caller is at fault, but the point of this refactoring is to
   make it harder for callers to make such mistakes.

 - As we can see above the three-dash line, even the author of the series
   could not come up with any justification why the proposed change is a
   good thing in the proposed commit log message for this patch alone (or
   the previous patch alone for that matter). Combining these patches
   together would make it clearer why it may be a good thing, which would
   make it easier to come up with a better log message.

Show 23 quoted lines

>  git-svn.perl |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index eeb83d3..25d5da7 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -4415,13 +4415,8 @@ sub username {
>
>  sub _read_password {
>  	my ($prompt, $realm) = @_;
> -	my $password = '';
> -	if (exists $ENV{GIT_ASKPASS}) {
> -		open(PH, "-|", $ENV{GIT_ASKPASS}, $prompt);
> -		$password = <PH>;
> -		$password =~ s/[\012\015]//; # \n\r
> -		close(PH);
> -	} else {
> +	my $password = Git->askpass_prompt($prompt);;
> +	if (!defined $password) {
>  		print STDERR $prompt;
>  		STDERR->flush;
>  		require Term::ReadKey;