Re: [PATCH RESEND] hooks: add sendemail-validate-series

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 5, 2023, 21:49 UTC
Message-ID: <xmqqwn2qt2x3.fsf@gitster.g>
In-Reply-To: <CROOKNR29PDV.1WIGA6219L1C6@ringo>

"Robin Jarry" <robin@jarry.cc> writes:

Show 5 quoted lines

> Just a thought. Instead of adding another hook, wouldn't it be better to
> add an option --validate-series (git config sendemail.validateSeries)
> that would change the behaviour of the sendemail-validate hook? Calling
> it with all patch files as command line arguments only once instead of
> once per file.

I do not see much upside in doing so. Instead of having to write a new validate-series hook script to store it under a new name, users can update an existing validate-patch hook script to take a batch of patches in one go. Then the user now has to set a new configuration variable. Because the expected way to feed the hook script is *not* per invocation but depends solely on how the script is written, a command line option would not make much sense. Not having to rename the updated validate-patch script to validate-series might be a small win, but I do not think it is a compelling reason to take that approach.

Show 8 quoted lines

> I know I was concerned with the max size of the command line args but is
> there really a chance that we hit that maximum? On my system, it is
> 2097152 bytes. Even with a 1000 patches series with 1000 bytes
> filenames, we wouldn't hit the limit.
>
> This way, we support any crap filename that the user may send and we
> don't add a new hook which basically does the same thing than the
> existing one.

Between "we may exceed command line argument limit" (which by the way is way lower on certain systems than what you expect, IIRC) and "the user may throw us a file with LF in its name", I'd find it simpler to punt on the latter and tell them "if it hurts, don't do it". As long as the limitation is clearly documented, I'd say it is OK.

Thanks.