Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Aug 3, 2018, 09:33 UTC
Message-ID: <c7b8629d-7b93-2fbf-6793-0d566e86a229@talktalk.net>
In-Reply-To: <CAPig+cQaaHfkR8jrXb-+PB4bSZbKLQzRkZNo+DNfz92hKO=04Q@mail.gmail.com>

Dear Eric and Junio On 03/08/18 08:59, Eric Sunshine wrote:

Show 27 quoted lines

> On Thu, Aug 2, 2018 at 1:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Phillip Wood <phillip.wood@talktalk.net> writes:
>>> For other interactive rebases this only affects external scripts that
>>> read the author script and users whose git is upgraded from the shell
>>> version of rebase -i while rebase was stopped when the author contains
>>> "'". This is because the parsing in read_env_script() expected the
>>> broken quoting.
>>
>> I wasn't following the discussion, but is it the general consensus
>> that reading the broken a-i file is a requirement for the new code?
>> Not an objection phrased as a question.
>>
>> I do not think it is worth worrying about the "upgrade while rebase
>> was in progress" case, if it involves much more code than necessary
>> without its support, especially if the only thing the user needs to
>> do recover from such a situation is to say "rebase --abort" and then
>> to retry the same rebase with the fixed version that was installed
>> in the meantime. [...]
>>
>> [...] It still does look
>> unnecessarily ugly and over-engineered to have this (and the
>> "version" reading code), though, at least to me, but perhaps it is
>> just me.
> 
> It's not just you. I also questioned[1] if such backward compatibility
> was needed, and had concerns[2] about a version file being heavyweight
> and over-engineered.

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

Show 8 quoted lines

> 
> This is a lot of new code (possibly harboring its own bugs) for a
> situation unlikely to arise, and which becomes ever more unlikely as
> time passes. Also, unlike long-lived (years or decades) resources,
> such as a repository or pack file, for instance, for which a version
> number makes sense, this file is very short-lived, which makes it even
> more difficult to justify adding this much machinery for something so
> unlikely to arise in practice.

There is a precedent for adding backwards compatibility 84df4560ed ("rebase: extract code for writing basic state", 2011-02-06) though it is much simpler. Part of the commit message reads

    Note that non-interactive rebase stores the sha1 of the
    original head in a file called orig-head, while interactive
    rebase stores it in a file called head. Change this by
    writing to orig-head in both cases. When reading, try to read
    from orig-head. If that fails, read from head instead. This
    protects users who upgraded git while they had an ongoing
    interactive rebase, while still making it possible to remove
    the code that reads from head at some point in the future.

Best Wishes

Phillip

Show 11 quoted lines

> The overall aim of this series to fix these bugs is laudable, but I
> would be happy to see this one reduced to just a "bug fix" patch
> without all the backward-compatibility machinery (and wouldn't mind
> seeing patch 1/2 simplified[3], as well).
> 
> Thanks.
> 
> [1]: https://public-inbox.org/git/CAPig+cR5VHP8muo5_A_9t7OPZam8O_uPb0nd73B15Ye92n+p7Q@mail.gmail.com/
> [2]: https://public-inbox.org/git/CAPig+cTttbV2FjnoS_SZtwh2J4wwzsbK+48BAbt1cV0utynYzw@mail.gmail.com/
> [3]: https://public-inbox.org/git/CAPig+cSZ3Zm=qFcvGjyj_uStn=JMAYuskMa0O_2yxkKjaRWTSg@mail.gmail.com/
>