Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing
Rasmus Villemoes writes: > So, it seems you're ok with this tightening, but some comment on the > non-interactive use case should be made in the commit log? Or am I > misunderstanding? I do not think we need any immediate action on this step. I was just wondering if we want two classes of "I am not running you interactively, so assume I said 'yes' when you need to ask me any confirmation on X and Y" and "I am not running you interactively, so assume I said 'no' for safety when you need to ask me any confirmation on Z" supported in the future. Lines with both @ and <> fall into the first class, while lines with only <> fall into the second camp, I would guess.
Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing
On 2018-10-11 08:06, Junio C Hamano wrote: > Rasmus Villemoes writes: > >> I considered that (and also had a version where I simply insisted on a @ >> being present), but that means the user no longer would get prompted >> about the cases where the address was just slightly obfuscated, e.g. the >> >> Cc: John Doe >> >> cases, which would be a regression, I guess. So I do want to pass such >> cases through, and have them be dealt with when process_address_list >> gets called. > > We are only tightening with this patch, and we were passing any > random things through with the original code anyway, so without > [PATCH 3/3], this step must be making it only better, but I have to > wonder one thing. > > You keep saying "get prompted" but are we sure we always stop and > ask (and preferrably---fail and abort when the end user is not > available at the terminal to interact) when we have such a > questionable address? > I dunno. I guess I've never considered non-interactive use of send-email. But the ask() in validate_address does have default q[uit], which I suppose gets used if stdin is /dev/null? I did do an experiment adding a bunch of the random odd patterns found in kernel commit messages to see how send-email reacted before/after this, and the only things that got filtered away (i.e., no longer prompted about) were things where the user probably couldn't easily fix it anyway. In the cases where there was a "Cc: stable" that might be fixed to the proper sta...@vger.kernel.org, the logic in extract_valid_address simply saw that as a local address, so we didn't use to be prompted, but simply sent to stable@localhost. Now we simply don't pass that through. So, for non-interactive use, I guess the effect of this patch is to allow more cases to complete succesfully, since we filter away (some) cases where extract_valid_address would cause us to prompt (and thus quit). So, it seems you're ok with this tightening, but some comment on the non-interactive use case should be made in the commit log? Or am I misunderstanding? Thanks, Rasmus
Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing
Rasmus Villemoes writes: > I considered that (and also had a version where I simply insisted on a @ > being present), but that means the user no longer would get prompted > about the cases where the address was just slightly obfuscated, e.g. the > > Cc: John Doe > > cases, which would be a regression, I guess. So I do want to pass such > cases through, and have them be dealt with when process_address_list > gets called. We are only tightening with this patch, and we were passing any random things through with the original code anyway, so without [PATCH 3/3], this step must be making it only better, but I have to wonder one thing. You keep saying "get prompted" but are we sure we always stop and ask (and preferrably---fail and abort when the end user is not available at the terminal to interact) when we have such a questionable address?
Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing
On 2018-10-10 14:57, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Oct 10 2018, Rasmus Villemoes wrote:
>
>> +if ($c !~ /.+@.+|<.+>/) {
>> +printf("(body) Ignoring %s from line '%s'\n",
>> +$what, $_) unless $quiet;
>> +next;
>> +}
>> push @cc, $c;
>> printf(__("(body) Adding cc: %s from line '%s'\n"),
>> $c, $_) unless $quiet;
>
> There's a extract_valid_address() function in git-send-email already,
> shouldn't this be:
>
> if (!extract_valid_address($c)) {
> [...]
>
> Or is there a good reason not to use that function in this case?
>
I considered that (and also had a version where I simply insisted on a @
being present), but that means the user no longer would get prompted
about the cases where the address was just slightly obfuscated, e.g. the
Cc: John Doe
cases, which would be a regression, I guess. So I do want to pass such
cases through, and have them be dealt with when process_address_list
gets called.
So this is just a rather minimal and simple heuristic, which should
still be able to handle the vast majority of cases correctly, and at
least almost never exclude anything that might have a chance of becoming
a real address.
Rasmus
Re: [PATCH 2/3] send-email: only consider lines containing @ or <> for automatic Cc'ing
On Wed, Oct 10 2018, Rasmus Villemoes wrote:
> + if ($c !~ /.+@.+|<.+>/) {
> + printf("(body) Ignoring %s from line '%s'\n",
> + $what, $_) unless $quiet;
> + next;
> + }
> push @cc, $c;
> printf(__("(body) Adding cc: %s from line '%s'\n"),
> $c, $_) unless $quiet;
There's a extract_valid_address() function in git-send-email already,
shouldn't this be:
if (!extract_valid_address($c)) {
[...]
Or is there a good reason not to use that function in this case?
