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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 3, 2018, 07:59 UTC
Message-ID: <CAPig+cQaaHfkR8jrXb-+PB4bSZbKLQzRkZNo+DNfz92hKO=04Q@mail.gmail.com>
In-Reply-To: <xmqqk1p83cig.fsf@gitster-ct.c.googlers.com>

On Thu, Aug 2, 2018 at 1:27 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

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

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.

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/