From: Junio C Hamano <hidden> Date: 2018-08-02 17:27:10
Phillip Wood [off-list ref] writes:
From: Phillip Wood <redacted>
Single quotes should be escaped as \' not \\'. The bad quoting breaks
the interactive version of 'rebase --root' (which is used when there is
no '--onto' even if the user does not specify --interactive) for authors
that contain "'" as sq_dequote() called read_author_ident() errors out
on the bad quoting.
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. Let's see how much we need to bend over backwards
to do this "transition" thing.
Ideally rebase and am would share the same code for reading and
writing the author script, but this commit just fixes the immediate
bug.
@@ -880,7 +880,7 @@ init_basic_state () {mkdir-p"$state_dir"||die"$(eval_gettext"Could not create temporary \$state_dir")"rm-f"$(gitrev-parse--git-pathREBASE_HEAD)"-:>"$state_dir"/interactive||die"$(gettext"Could not mark as interactive")"+echo1>"$state_dir"/interactive||die"$(gettext"Could not mark as interactive")"
This impacts the work Alban is doing, which at the end removes this
script altogether.
+/*
+ * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE line with
+ * a "'" and also escaped "'" incorrectly as "'\\\\''" rather than "'\\''". Fix
I think the comment here (for both the wrong and the right versions)
is easier to read if you wrote the string as literal without C, i.e.
The string is "'\\''" but as a string literal in C it is expressed
as "'\\\\''".
+static int fix_bad_author_script(struct strbuf *script)
+{
+ const char *next;
+ size_t off = 0;
+
+ while ((next = strstr(script->buf + off, "'\\\\''"))) {
This looks brittle.
We need assurance that the first "'\\''" we see on the line came
from the attempt by the broken writer to write out a single "'", and
not from anything else. The broken writer places its own "'"
immediately after GIT_AUTHOR_NAME= (just like the corrected one
does) before moving on to the end-user payload. Can the single
quote at the beginning of the substring you are looking for be that
one? If the end user's payload began with two backslashes, that
would have produced a result that matches the first three bytes of
the substring you are looking for. But there is no way for the
end-user payload to make the next two bytes "''"---any byte other
than a sq would result in a sq added to the result, and a byte that
is a sq would give one sq followed by a bs.
OK, so this is probably doing the right thing, as long as we know we
are reading from the old and broken writer. 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.
Thanks.
From: Eric Sunshine <hidden> Date: 2018-08-03 07:59:53
On Thu, Aug 2, 2018 at 1:27 PM Junio C Hamano [off-list ref] wrote:
Phillip Wood [off-list ref] writes:
quoted
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/
Dear Eric and Junio
On 03/08/18 08:59, Eric Sunshine wrote:
On Thu, Aug 2, 2018 at 1:27 PM Junio C Hamano [off-list ref] wrote:
quoted
Phillip Wood [off-list ref] writes:
quoted
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
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
From: Eric Sunshine <hidden> Date: 2018-08-03 10:02:26
On Fri, Aug 3, 2018 at 5:33 AM Phillip Wood [off-list ref] 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/
On Fri, Aug 3, 2018 at 5:33 AM Phillip Wood [off-list ref] wrote:
quoted
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
quoted
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/