From: Phillip Wood <phillip.wood@talktalk.net>
Date: Fri, 03 Aug 2018 14:12:49 GMT
Subject: Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script
Message-ID: <455fafb5-3c92-4348-0c2c-0a4ab62cf2ce@talktalk.net>
In-Reply-To: <CAPig+cTyTHxwFvk3ZtOq3L7KEtEjKLKu6-RnLC-_NuL1Xzhqzw@mail.gmail.com>

Hi Eric
On 03/08/18 11:02, Eric Sunshine wrote:
> On Fri, Aug 3, 2018 at 5:33 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
>> If there isn't some backward compatibility then if git gets upgraded
>> while rebase is stopped then the author data will be silently corrupted
>> if it contains "'". read_author_ident() will error out but that is only
>> used for the root commit. read_env_script() which is used for normal
>> picks will not dequote the badly quoted value correctly and will not
>> return an error. It is unlikely but possible, I'll leave it to Junio to
>> decide if it is worth it
> 
> If I understand correctly, the approach you implemented earlier[1]
> (perhaps coupled with the more robust detection suggested here[2])
> would be sufficient to handle this backward compatibility concern.
> While it may not be as pretty or generalized as the current patch, it
> involves far less machinery, thus is less likely to harbor its own
> bugs. The earlier version is also much more self-contained, which
> makes it easier to drop at some point when backward compatibility is
> no longer a concern (if ever).

Yes I think the earlier approach with the more robust detection you 
suggested is probably a good compromise. Junio does that sound good to you?

Best Wishes

Phillip

>> There is a precedent for adding backwards compatibility 84df4560ed
>> ("rebase: extract code for writing basic state", 2011-02-06) though it
>> is much simpler.
> 
> Indeed, it is much simpler, adding a one-liner 'else' case to an
> 'if-then' for backward compatibility. Your earlier implementation[1]
> was pretty much the equivalent, just adding an extra one-liner arm to
> an 'if-then' statement.
> 
> The bug fix itself is important, and, while I do favor the cleaner
> approach of not worrying about backward compatibility for this fairly
> unlikely case, your earlier version seems a better compromise between
> having no backward compatibility and the much more heavyweight version
> implemented here.
> 
> Anyhow, I'm fine with whatever Junio decides.
> 
> [1]: https://public-inbox.org/git/20180731111532.9358-3-phillip.wood@talktalk.net/
> [2]: https://public-inbox.org/git/CAPig+cTttbV2FjnoS_SZtwh2J4wwzsbK+48BAbt1cV0utynYzw@mail.gmail.com/
>