Re: [PATCH v3] send-email: validate charset name in 8bit encoding prompt

[Junio C Hamano <gitster@pobox.com>]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

> diff --git a/git-send-email.perl b/git-send-email.perl
> index cd4b316ddc..3230b80701 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -23,6 +23,7 @@
>  use Git::LoadCPAN::Error qw(:try);
>  use Git;
>  use Git::I18N;
> +use Encode qw(find_encoding);

I wonder how common is this module already installed on users'
systems (not asking "how widely available"---which is "can users
easily make it work?", but asking "would this work out of box with
what users already have?").

> +sub confirm_ask {
> +	my ($resp) = @_;
> +	my $term = term();
> +	return 0
> +		unless defined $term->IN and defined fileno($term->IN) and
> +		       defined $term->OUT and defined fileno($term->OUT);
> +	my $yesno = $term->readline(
> +		# TRANSLATORS: please keep [y/N] as is.
> +		sprintf(__("Are you sure you want to use <%s> [y/N]? "), $resp));
> +	return defined $yesno && $yesno =~ /y/i;
> +}

This is a bit incosistent with what "sub ask" (the only caller of
this sub) does, isn't it?  Before entering the loop that makes a
call into this, it does this:

        sub ask {
                my ($prompt, %arg) = @_;
                my $valid_re = $arg{valid_re};
                my $default = $arg{default};
                my $confirm_only = $arg{confirm_only};
                my $resp;
                my $i = 0;
                my $term = term();
                return defined $default ? $default : undef
                        unless defined $term->IN and defined fileno($term->IN) and
                               defined $term->OUT and defined fileno($term->OUT);

If $term is not usable for interactive prompt, it uses the default
setting.  But the new confirm_ask always says "no".

confirm_ask does its own "check term() to see it is usable" because
it is called from another code path which does not have its own
logic, but it may be a wrong abstraction to give uneven interface.
It would make it more clear what is going on if you just do the
interactive $term->readline() thing in "sub ask", instead of calling
"sub confirm_ask" that does tghe $term thing redundantly.

Can't the other confirm_ask() caller call a normal "sub ask"?  

I am not sure why we want to add a dedicated sub, just to ask "are
you sure you want to use X [y/N]? ".

> The intended flow is,
>
>         Declare which 8bit encoding to use [default: UTF-8]? foobar
>         warning: 'foobar' does not appear to be a valid charset name.
>         Are you sure you want to use <foobar> [y/N]?

It somehow looks uneven to have three lines, two of them
capitalizing their first word while the other one is all lowercase.
I wonder if this would be simpler?

    Declare which 8bit encoding to use [default: UTF-8]?  foobar<RET>
    Do you really mean 'foobar', not a valid charset name [y/N]?



So, taking all of the above together, perhaps:

 * Discard changes to "sub ask" and addition of "sub confirm_ask".

 * Tweak this part a bit to call ask().

> +	while(1) {

Style.  missing SP before "(".

> +		my $encoding = ask(__("Declare which 8bit encoding to use [default: UTF-8]? "),

Overly long line.

> +		valid_re => qr/^\S+$/,
> +		default  => "UTF-8");
> +		next unless defined $encoding;
> +		if (find_encoding($encoding)) {
> +			$auto_8bit_encoding = $encoding;
> +			last;
> +		}

> +		printf STDERR __("warning: '%s' does not appear to be a valid charset name.\n"), $encoding;
> +		if (confirm_ask($encoding)) {

Use ask() to ask 

    Do you really mean 'foobar', not a valid charset name [y/N]?

here, perhaps?

> +			$auto_8bit_encoding = $encoding;
> +			last;
> +		}
> +	}
>  }