Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script
- From
- Phillip Wood <phillip.wood@talktalk.net>
- Date
- Aug 7, 2018, 13:54 UTC
- Message-ID
- <753b860e-c922-fd70-9d2d-0581976a6f37@talktalk.net>
- In-Reply-To
- <CAPig+cQkY1_qDu=fdbA0gVPH88B47bM5BpgbYY8ivsj1Y0QHTw@mail.gmail.com>
Hi Eric
On 07/08/18 11:23, Eric Sunshine wrote:
Show 24 quoted lines
> On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
>> - Reverted the implementation to v2 with more robust detection of the
>> missing "'" on the last line of the author script based on a
>> suggestion by Eric. This means that this series needs to progress
>> closely with Eric's series of fixes or the fallback code will never
>> be called.
>
> Thanks for working on this. I haven't read the patch closely yet, but
> one thing caught my attention as I ran my eye over it...
>
>> +static int quoting_is_broken(const char *s, size_t n)
>> +{
>> + /* Skip any empty lines in case the file was hand edited */
>> + while (n > 0 && s[--n] == '\n')
>> + ; /* empty */
>> + if (n > 0 && s[n] != '\'')
>> + return 1;
>
> To be "technically correct", I think the condition in the 'if'
> statement should be ">=". It should never happen in practice that the
> entire content of the file is a single character followed by zero or
> more newlines, but using the proper condition ">=" would save future
> readers of this code a "huh?" moment.
> I'm not sure it is that simple. If the script consists solely of a single quote then we should return 1, if it is a single character that is not "'" then it should return 0. Currently it returns 0 in both those cases so is technically broken when the script is "'". If it used ">=" instead then I think it would return 0 when it should return 1 and vice versa. As you say this shouldn't happen in practice.
Best Wishes
Phillip