Sven Strickroth <sven.strickroth@xxxxxxxxxxxxxxx> writes:
> Signed-off-by: Sven Strickroth <email@xxxxxxxxxx>
> ---
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.
> 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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html