On 10/11/23 09:41, Bagas Sanjaya wrote:
> On Mon, Sep 25, 2023 at 12:17:48PM -0400, Jeff King wrote:
>> On Mon, Sep 25, 2023 at 10:48:29AM -0400, Todd Zullinger wrote:
>>
>>> From the peanut gallery... could the presence or lack of the
>>> Email::Valid perl module be a factor?
>> Ah, thanks! The thought of differing modules even occurred to me, since
>> I know we have a few optimistic dependencies, but when I looked I didn't
>> manage to find that one (somehow I thought Mail::Address was the
>> interesting one here; I think I might be getting senile).
>>
>> With Email::Valid installed, I can reproduce with just (in git.git, but
>> I think it would work in any repo):
>>
>> $ echo "exit 0" >.git/hooks/sendemail-validate
>> $ chmod +x .git/hooks/sendemail-validate
>> $ git send-email --dry-run -1 --to=foo@xxxxxxxxxxx,bar@xxxxxxxxxxx
>> error: unable to extract a valid address from: foo@xxxxxxxxxxx,bar@xxxxxxxxxxx
>>
>> Disabling the hook with "chmod -x" makes the problem go away (and this
>> is with current "master", hence the more readable error message).
>>
>> I think the issue is that a8022c5f7b ends up in extract_valid_address()
>> via this call stack:
>>
>> $ = main::extract_valid_address('foo@xxxxxxxxxxx,bar@xxxxxxxxxxx') called from file '/home/peff/compile/git/git-send-email' line 1161
>> $ = main::extract_valid_address_or_die('foo@xxxxxxxxxxx,bar@xxxxxxxxxxx') called from file '/home/peff/compile/git/git-send-email' line 2087
>> @ = main::unique_email_list('foo@xxxxxxxxxxx,bar@xxxxxxxxxxx') called from file '/home/peff/compile/git/git-send-email' line 1507
>> @ = main::gen_header() called from file '/home/peff/compile/git/git-send-email' line 2113
>> . = main::validate_patch('/tmp/WfoPQSKCUa/0001-The-twelfth-batch.patch', 'auto') called from file '/home/peff/compile/git/git-send-email' line 815
>>
>> whereas prior to that commit, we hit it later:
>>
>> $ = main::extract_valid_address('foo@xxxxxxxxxxx') called from file '/home/peff/compile/git/git-send-email' line 1166
>> @ = main::validate_address('foo@xxxxxxxxxxx') called from file '/home/peff/compile/git/git-send-email' line 1189
>> @ = main::validate_address_list('foo@xxxxxxxxxxx', 'bar@xxxxxxxxxxx') called from file '/home/peff/compile/git/git-send-email' line 1348
>> @ = main::process_address_list('foo@xxxxxxxxxxx,bar@xxxxxxxxxxx') called from file '/home/peff/compile/git/git-send-email' line 1091
>>
>> So the issue is the call to gen_header() added in validate_patch(). We
>> won't yet have processed the address lists by that point. We can move
>> those calls up, but it requires moving a bit of extra code, too (like
>> the parts prompting for the "to" list if it isn't filled in).
>>
>> Possibly the validation checks need to be moved down, if they want to
>> see a more complete view of the emails. But now we're doing more work
>> (like asking the user to write the cover letter!) before we do
>> validation, which is probably bad.
>>
>> So I dunno. Maybe gen_header() should be lazily doing this
>> process_address_list() stuff? I'm not very familiar with the send-email
>> code, so I'm not sure what secondary effects that could have.
>>
> Michael, did you look into this since you authored the culprit?
>
I tried to repro the issue previously but didn't have luck (even with Email::Valid installed). I decided to try again today and realised I forgot to make the test sendemail-validate hook executable before. Now that I can repro it, I can look further.
The fix may not be easy for the reasons that Jeff King states. There is a lot of implicit order in the code because there are several places where code exists outside of any function, which has function definitions scattered through it. My change attempted to clean a portion of that up and encapsulated it into a reusable function for generating header information so that sendemail-validate could see it. I'll keep digging into it.
