From: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
Date: Sat, 28 Feb 2026 08:36:15 GMT
Subject: Re: [PATCH v3] send-email: validate charset name in 8bit encoding prompt
Message-ID: <20260228083803.238503-1-shreyanshpaliwalcmsmn@gmail.com>
In-Reply-To: <xmqq8qcf2vk8.fsf@gitster.g>

[...]
> > +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]?
>

Actually that makes sense, because if we need to add a special warning
in between the two prompts (what I was aiming for), either we need to
modify ask() to add the warning into the flow, or we had to seperate the
confirm_ask because we have to change the flow in any case, but if we
drop the additional warning, and instead warn/confirm together in the
second prompt we dont need this abstraction.

>
>
> 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.
>

my bad. will fix.

> > +		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?
>

Understood. I am hoping now this doesn't need any additional
abstraction as Ben suggested.
Sorry for the delay in response to the review.
I will send a reroll.

Thanks,
Shreyansh