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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: Junio C Hamano <gitster@pobox.com>,
	Alban Gruin <alban.gruin@gmail.com>,
	Git List <git@vger.kernel.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script
Date: Fri, 3 Aug 2018 06:02:11 -0400	[thread overview]
Message-ID: <CAPig+cTyTHxwFvk3ZtOq3L7KEtEjKLKu6-RnLC-_NuL1Xzhqzw@mail.gmail.com> (raw)
In-Reply-To: <c7b8629d-7b93-2fbf-6793-0d566e86a229@talktalk.net>

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).

> 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/