* [PATCH] sequencer.c: terminate the last line of author-script properly @ 2018-07-12 11:18 Akinori MUSHA 2018-07-12 17:22 ` Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 27+ messages in thread From: Akinori MUSHA @ 2018-07-12 11:18 UTC (permalink / raw) To: git; +Cc: Junio C Hamano It looks like write_author_script() intends to write out a file in Bourne shell syntax, but it doesn't put a closing single quote on the last line. This patch makes .git/rebase-merge/author-script actually parsable by sh(1) by adding a single quote and a linefeed to terminate the line properly. Signed-off-by: Akinori MUSHA <knu@idaemons.org> --- sequencer.c | 1 + t/t3404-rebase-interactive.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/sequencer.c b/sequencer.c index 4034c0461..5f32b6df1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -651,6 +651,7 @@ static int write_author_script(const char *message) strbuf_addch(&buf, *(message++)); else strbuf_addf(&buf, "'\\\\%c'", *(message++)); + strbuf_addstr(&buf, "'\n"); res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); strbuf_release(&buf); return res; diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 352a52e59..345b103eb 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' ' test_line_count = 6 actual ' +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' + test_when_finished "git rebase --abort ||:" && + git checkout master && + set_fake_editor && + FAKE_LINES="edit 1" git rebase -i HEAD^ && + test -f .git/rebase-merge/author-script && + unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && + eval "$(cat .git/rebase-merge/author-script)" && + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" +' + test_expect_success 'rebase -i with the exec command' ' git checkout master && ( -- 2.18.0 -- Akinori MUSHA / https://akinori.org/ ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-12 11:18 [PATCH] sequencer.c: terminate the last line of author-script properly Akinori MUSHA @ 2018-07-12 17:22 ` Junio C Hamano 2018-07-18 9:45 ` Phillip Wood 2018-07-12 20:13 ` Junio C Hamano ` (2 subsequent siblings) 3 siblings, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2018-07-12 17:22 UTC (permalink / raw) To: Akinori MUSHA; +Cc: git "Akinori MUSHA" <knu@iDaemons.org> writes: > It looks like write_author_script() intends to write out a file in > Bourne shell syntax, but it doesn't put a closing single quote on the > last line. s/closing single quote/& and the terminating newline/? > > This patch makes .git/rebase-merge/author-script actually parsable by > sh(1) by adding a single quote and a linefeed to terminate the line > properly. Sounds good. I wonder why this breakage was left unnoticed for a long time, though. It's not like writing and reading the author-script from C code was done first in the "rebase -i" and friends that are users of the sequencer machinery (I think we had code to do so in "git am" that was rewritten in C first). Do we have a similar issue over there as well? If not, perhaps if we reused the existing code that was not broken, we wouldn't have seen this breakage on the sequencer side? Thanks. > > Signed-off-by: Akinori MUSHA <knu@idaemons.org> > --- > sequencer.c | 1 + > t/t3404-rebase-interactive.sh | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/sequencer.c b/sequencer.c > index 4034c0461..5f32b6df1 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -651,6 +651,7 @@ static int write_author_script(const char *message) > strbuf_addch(&buf, *(message++)); > else > strbuf_addf(&buf, "'\\\\%c'", *(message++)); > + strbuf_addstr(&buf, "'\n"); > res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); > strbuf_release(&buf); > return res; > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 352a52e59..345b103eb 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' ' > test_line_count = 6 actual > ' > > +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' > + test_when_finished "git rebase --abort ||:" && > + git checkout master && > + set_fake_editor && > + FAKE_LINES="edit 1" git rebase -i HEAD^ && > + test -f .git/rebase-merge/author-script && > + unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > + eval "$(cat .git/rebase-merge/author-script)" && > + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" > +' > + > test_expect_success 'rebase -i with the exec command' ' > git checkout master && > ( > -- > 2.18.0 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-12 17:22 ` Junio C Hamano @ 2018-07-18 9:45 ` Phillip Wood 2018-07-18 13:46 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Phillip Wood 2018-07-18 17:17 ` [PATCH] sequencer.c: terminate the last line of author-script properly Junio C Hamano 0 siblings, 2 replies; 27+ messages in thread From: Phillip Wood @ 2018-07-18 9:45 UTC (permalink / raw) To: Junio C Hamano, Akinori MUSHA; +Cc: git Hi Junio On 12/07/18 18:22, Junio C Hamano wrote: > > "Akinori MUSHA" <knu@iDaemons.org> writes: > >> It looks like write_author_script() intends to write out a file in >> Bourne shell syntax, but it doesn't put a closing single quote on the >> last line. > > s/closing single quote/& and the terminating newline/? > >> >> This patch makes .git/rebase-merge/author-script actually parsable by >> sh(1) by adding a single quote and a linefeed to terminate the line >> properly. > > Sounds good. > > I wonder why this breakage was left unnoticed for a long time, > though. It's not like writing and reading the author-script from C > code was done first in the "rebase -i" and friends that are users of > the sequencer machinery The only consumer of a faulty author script written by the sequencer is read_env_script() in sequencer.c which doesn't worry about checking that quotes are paired. The shell versions of rebase use the code in git-sh-setup.sh to create the author script which does write valid posix shell. > (I think we had code to do so in "git am" > that was rewritten in C first). The code in builtin/am.c doesn't try to write valid posix shell (if one assumes it is the only consumer of the author script then it doesn't need to) which results in simpler code, but external scripts cannot safely eval it anymore. > Do we have a similar issue over > there as well? If not, perhaps if we reused the existing code that > was not broken, we wouldn't have seen this breakage on the sequencer > side? Best Wishes Phillip > Thanks. > >> >> Signed-off-by: Akinori MUSHA <knu@idaemons.org> >> --- >> sequencer.c | 1 + >> t/t3404-rebase-interactive.sh | 13 +++++++++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/sequencer.c b/sequencer.c >> index 4034c0461..5f32b6df1 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -651,6 +651,7 @@ static int write_author_script(const char *message) >> strbuf_addch(&buf, *(message++)); >> else >> strbuf_addf(&buf, "'\\\\%c'", *(message++)); >> + strbuf_addstr(&buf, "'\n"); >> res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); >> strbuf_release(&buf); >> return res; >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh >> index 352a52e59..345b103eb 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' ' >> test_line_count = 6 actual >> ' >> >> +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' >> + test_when_finished "git rebase --abort ||:" && >> + git checkout master && >> + set_fake_editor && >> + FAKE_LINES="edit 1" git rebase -i HEAD^ && >> + test -f .git/rebase-merge/author-script && >> + unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && >> + eval "$(cat .git/rebase-merge/author-script)" && >> + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && >> + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && >> + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" >> +' >> + >> test_expect_success 'rebase -i with the exec command' ' >> git checkout master && >> ( >> -- >> 2.18.0 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly 2018-07-18 9:45 ` Phillip Wood @ 2018-07-18 13:46 ` Phillip Wood 2018-07-18 15:55 ` [RFC PATCH] sequencer: fix quoting in write_author_script Phillip Wood 2018-07-18 17:24 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Junio C Hamano 2018-07-18 17:17 ` [PATCH] sequencer.c: terminate the last line of author-script properly Junio C Hamano 1 sibling, 2 replies; 27+ messages in thread From: Phillip Wood @ 2018-07-18 13:46 UTC (permalink / raw) To: Junio C Hamano, Akinori MUSHA; +Cc: git On 18/07/18 10:45, Phillip Wood wrote: > > Hi Junio > > On 12/07/18 18:22, Junio C Hamano wrote: >> >> "Akinori MUSHA" <knu@iDaemons.org> writes: >> >>> It looks like write_author_script() intends to write out a file in >>> Bourne shell syntax, but it doesn't put a closing single quote on the >>> last line. >> >> s/closing single quote/& and the terminating newline/? >> >>> >>> This patch makes .git/rebase-merge/author-script actually parsable by >>> sh(1) by adding a single quote and a linefeed to terminate the line >>> properly. >> >> Sounds good. >> >> I wonder why this breakage was left unnoticed for a long time, >> though. It's not like writing and reading the author-script from C >> code was done first in the "rebase -i" and friends that are users of >> the sequencer machinery > > The only consumer of a faulty author script written by the sequencer is > read_env_script() in sequencer.c which doesn't worry about checking that > quotes are paired. That's not quite true anymore, recently another consumer read_author_ident() was added which uses sq_dequote() instead of custom code. Looking more closely at write_author_script() the quoting of single quotes is buggy they are escaped as \\' instead of \'. The code in read_env_script() expects the buggy quoting so it works. I just tried the code path that uses sq_dequote() by doing rebase --root with an author containing a single quote and that worked but I'm not sure why. The shell versions of rebase use the code in > git-sh-setup.sh to create the author script which does write valid posix > shell. > >> (I think we had code to do so in "git am" >> that was rewritten in C first). > > The code in builtin/am.c doesn't try to write valid posix shell (if one > assumes it is the only consumer of the author script then it doesn't > need to) Though it will break if git gets upgraded while am is stopped for a conflict resolution. Given that the code has been around for a while that may not be much of a concern now which results in simpler code, but external scripts cannot > safely eval it anymore. > >> Do we have a similar issue over >> there as well? If not, perhaps if we reused the existing code that >> was not broken, we wouldn't have seen this breakage on the sequencer >> side? > > Best Wishes > > Phillip > >> Thanks. >> >>> >>> Signed-off-by: Akinori MUSHA <knu@idaemons.org> >>> --- >>> sequencer.c | 1 + >>> t/t3404-rebase-interactive.sh | 13 +++++++++++++ >>> 2 files changed, 14 insertions(+) >>> >>> diff --git a/sequencer.c b/sequencer.c >>> index 4034c0461..5f32b6df1 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -651,6 +651,7 @@ static int write_author_script(const char *message) >>> strbuf_addch(&buf, *(message++)); >>> else >>> strbuf_addf(&buf, "'\\\\%c'", *(message++)) >>> + strbuf_addstr(&buf, "'\n"); >>> res = write_message(buf.buf, buf.len, >>> rebase_path_author_script(), 1); >>> strbuf_release(&buf); >>> return res; >>> diff --git a/t/t3404-rebase-interactive.sh >>> b/t/t3404-rebase-interactive.sh >>> index 352a52e59..345b103eb 100755 >>> --- a/t/t3404-rebase-interactive.sh >>> +++ b/t/t3404-rebase-interactive.sh >>> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' ' >>> test_line_count = 6 actual >>> ' >>> +test_expect_success 'rebase -i writes out >>> .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' >>> + test_when_finished "git rebase --abort ||:" && >>> + git checkout master && >>> + set_fake_editor && >>> + FAKE_LINES="edit 1" git rebase -i HEAD^ && >>> + test -f .git/rebase-merge/author-script && >>> + unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && >>> + eval "$(cat .git/rebase-merge/author-script)" && >>> + test "$(git show --quiet --pretty=format:%an)" = >>> "$GIT_AUTHOR_NAME" && >>> + test "$(git show --quiet --pretty=format:%ae)" = >>> "$GIT_AUTHOR_EMAIL" && >>> + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = >>> "$GIT_AUTHOR_DATE" >>> +' >>> + >>> test_expect_success 'rebase -i with the exec command' ' >>> git checkout master && >>> ( >>> -- >>> 2.18.0 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH] sequencer: fix quoting in write_author_script 2018-07-18 13:46 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Phillip Wood @ 2018-07-18 15:55 ` Phillip Wood 2018-07-24 15:31 ` Junio C Hamano 2018-07-26 12:33 ` Johannes Schindelin 2018-07-18 17:24 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Junio C Hamano 1 sibling, 2 replies; 27+ messages in thread From: Phillip Wood @ 2018-07-18 15:55 UTC (permalink / raw) To: Git Mailing List Cc: Johannes Schindelin, Junio C Hamano, Akinori MUSHA, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> Single quotes should be escaped as \' not \\'. Note that this only affects authors that contain a single quote and then only external scripts that read the author script and users whose git is upgraded from the shell version of rebase -i while rebase was stopped. This is because the parsing in read_env_script() expected the broken version and for some reason sq_dequote() called by read_author_ident() seems to handle the broken quoting correctly. Ideally write_author_script() would be rewritten to use split_ident_line() and sq_quote_buf() but this commit just fixes the immediate bug. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- This is untested, unfortuantely I don't have really have time to write a test or follow this up at the moment, if someone else want to run with it then please do. sequencer.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index 5354d4d51e..0b78d1f100 100644 --- a/sequencer.c +++ b/sequencer.c @@ -638,21 +638,21 @@ static int write_author_script(const char *message) else if (*message != '\'') strbuf_addch(&buf, *(message++)); else - strbuf_addf(&buf, "'\\\\%c'", *(message++)); + strbuf_addf(&buf, "'\\%c'", *(message++)); strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='"); while (*message && *message != '\n' && *message != '\r') if (skip_prefix(message, "> ", &message)) break; else if (*message != '\'') strbuf_addch(&buf, *(message++)); else - strbuf_addf(&buf, "'\\\\%c'", *(message++)); + strbuf_addf(&buf, "'\\%c'", *(message++)); strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@"); while (*message && *message != '\n' && *message != '\r') if (*message != '\'') strbuf_addch(&buf, *(message++)); else - strbuf_addf(&buf, "'\\\\%c'", *(message++)); + strbuf_addf(&buf, "'\\%c'", *(message++)); res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); strbuf_release(&buf); return res; @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env) { struct strbuf script = STRBUF_INIT; int i, count = 0; - char *p, *p2; + const char *p2; + char *p; if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) return -1; for (p = script.buf; *p; p++) - if (skip_prefix(p, "'\\\\''", (const char **)&p2)) + /* + * write_author_script() used to escape "'" incorrectly as + * "'\\\\''" rather than "'\\''" so we check for the correct + * version the incorrect version in case git was upgraded while + * rebase was stopped. + */ + if (skip_prefix(p, "'\\''", &p2) || + skip_prefix(p, "'\\\\''", &p2)) strbuf_splice(&script, p - script.buf, p2 - p, "'", 1); else if (*p == '\'') strbuf_splice(&script, p-- - script.buf, 1, "", 0); -- 2.18.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] sequencer: fix quoting in write_author_script 2018-07-18 15:55 ` [RFC PATCH] sequencer: fix quoting in write_author_script Phillip Wood @ 2018-07-24 15:31 ` Junio C Hamano 2018-07-26 12:33 ` Johannes Schindelin 1 sibling, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2018-07-24 15:31 UTC (permalink / raw) To: Johannes Schindelin, Phillip Wood Cc: Git Mailing List, Akinori MUSHA, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Single quotes should be escaped as \' not \\'. Note that this only > affects authors that contain a single quote and then only external > scripts that read the author script and users whose git is upgraded from > the shell version of rebase -i while rebase was stopped. This is because > the parsing in read_env_script() expected the broken version and for > some reason sq_dequote() called by read_author_ident() seems to handle > the broken quoting correctly. > > Ideally write_author_script() would be rewritten to use > split_ident_line() and sq_quote_buf() but this commit just fixes the > immediate bug. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > > This is untested, unfortuantely I don't have really have time to write a test or > follow this up at the moment, if someone else want to run with it then please > do. Any follow-up on this? Dscho, I do agree with Phillip that the author-script this code produces does not quote single quotes correctly (if we consider that the author-script is meant to be compatible with shell script version and what "git am" uses, that is), and Phillip's fix looks like a reasonable first step going forward (the next step would remove the transitional band-aid from the reading code, also added by this patch). Thanks. > > sequencer.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 5354d4d51e..0b78d1f100 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -638,21 +638,21 @@ static int write_author_script(const char *message) > else if (*message != '\'') > strbuf_addch(&buf, *(message++)); > else > - strbuf_addf(&buf, "'\\\\%c'", *(message++)); > + strbuf_addf(&buf, "'\\%c'", *(message++)); > strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='"); > while (*message && *message != '\n' && *message != '\r') > if (skip_prefix(message, "> ", &message)) > break; > else if (*message != '\'') > strbuf_addch(&buf, *(message++)); > else > - strbuf_addf(&buf, "'\\\\%c'", *(message++)); > + strbuf_addf(&buf, "'\\%c'", *(message++)); > strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@"); > while (*message && *message != '\n' && *message != '\r') > if (*message != '\'') > strbuf_addch(&buf, *(message++)); > else > - strbuf_addf(&buf, "'\\\\%c'", *(message++)); > + strbuf_addf(&buf, "'\\%c'", *(message++)); > res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); > strbuf_release(&buf); > return res; > @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env) > { > struct strbuf script = STRBUF_INIT; > int i, count = 0; > - char *p, *p2; > + const char *p2; > + char *p; > > if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) > return -1; > > for (p = script.buf; *p; p++) > - if (skip_prefix(p, "'\\\\''", (const char **)&p2)) > + /* > + * write_author_script() used to escape "'" incorrectly as > + * "'\\\\''" rather than "'\\''" so we check for the correct > + * version the incorrect version in case git was upgraded while > + * rebase was stopped. > + */ > + if (skip_prefix(p, "'\\''", &p2) || > + skip_prefix(p, "'\\\\''", &p2)) > strbuf_splice(&script, p - script.buf, p2 - p, "'", 1); > else if (*p == '\'') > strbuf_splice(&script, p-- - script.buf, 1, "", 0); ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] sequencer: fix quoting in write_author_script 2018-07-18 15:55 ` [RFC PATCH] sequencer: fix quoting in write_author_script Phillip Wood 2018-07-24 15:31 ` Junio C Hamano @ 2018-07-26 12:33 ` Johannes Schindelin 2018-07-27 10:36 ` Phillip Wood 1 sibling, 1 reply; 27+ messages in thread From: Johannes Schindelin @ 2018-07-26 12:33 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Junio C Hamano, Akinori MUSHA Hi Phillip, On Wed, 18 Jul 2018, Phillip Wood wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Single quotes should be escaped as \' not \\'. Note that this only > affects authors that contain a single quote and then only external > scripts that read the author script and users whose git is upgraded from > the shell version of rebase -i while rebase was stopped. This is because > the parsing in read_env_script() expected the broken version and for > some reason sq_dequote() called by read_author_ident() seems to handle > the broken quoting correctly. > > Ideally write_author_script() would be rewritten to use > split_ident_line() and sq_quote_buf() but this commit just fixes the > immediate bug. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- Good catch. > This is untested, unfortuantely I don't have really have time to write a test or > follow this up at the moment, if someone else want to run with it then please > do. I modified the test that was added by Akinori. As it was added very early, and as there is still a test case *after* Akinori's that compares a hard-coded SHA-1, I refrained from using `test_commit` (which would change that SHA-1). See below. > diff --git a/sequencer.c b/sequencer.c > index 5354d4d51e..0b78d1f100 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -638,21 +638,21 @@ static int write_author_script(const char *message) > else if (*message != '\'') > strbuf_addch(&buf, *(message++)); > else > - strbuf_addf(&buf, "'\\\\%c'", *(message++)); > + strbuf_addf(&buf, "'\\%c'", *(message++)); > strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='"); > while (*message && *message != '\n' && *message != '\r') > if (skip_prefix(message, "> ", &message)) > break; > else if (*message != '\'') > strbuf_addch(&buf, *(message++)); > else > - strbuf_addf(&buf, "'\\\\%c'", *(message++)); > + strbuf_addf(&buf, "'\\%c'", *(message++)); > strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@"); > while (*message && *message != '\n' && *message != '\r') > if (*message != '\'') > strbuf_addch(&buf, *(message++)); > else > - strbuf_addf(&buf, "'\\\\%c'", *(message++)); > + strbuf_addf(&buf, "'\\%c'", *(message++)); > res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); I resolved the merge conflict with Akinori's patch. FWIW I pushed all of this, including the fixup to Junio's fixup to the `fix-t3404-author-script-test` branch at https://github.com/dscho/git. > strbuf_release(&buf); > return res; > @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env) > { > struct strbuf script = STRBUF_INIT; > int i, count = 0; > - char *p, *p2; > + const char *p2; > + char *p; > > if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) > return -1; > > for (p = script.buf; *p; p++) > - if (skip_prefix(p, "'\\\\''", (const char **)&p2)) > + /* > + * write_author_script() used to escape "'" incorrectly as > + * "'\\\\''" rather than "'\\''" so we check for the correct > + * version the incorrect version in case git was upgraded while > + * rebase was stopped. > + */ > + if (skip_prefix(p, "'\\''", &p2) || > + skip_prefix(p, "'\\\\''", &p2)) I think in this form, it is possibly unsafe because it assumes that the new code cannot generate output that would trigger that same code path. Although I have to admit that I did not give this a great deal of thought. In any case, if you have to think long and hard about some fix, it might be better to go with something that is easier to reason about. So how about this: we already know that the code is buggy, Akinori fixed the bug, where the author-script missed its trailing single-quote. We can use this as a tell-tale for *this* bug. Assuming that Junio will advance both your and Akinori's fix in close proximity. Again, this is pushed to the `fix-t3404-author-script-test` branch at https://github.com/dscho/git; My fixup on top of your patch looks like this (feel free to drop the sq_bug part and only keep the test part): -- snipsnap -- diff --git a/sequencer.c b/sequencer.c index 46c0b3e720f..7abe78dc78e 100644 --- a/sequencer.c +++ b/sequencer.c @@ -573,13 +573,14 @@ static int write_author_script(const char *message) static int read_env_script(struct argv_array *env) { struct strbuf script = STRBUF_INIT; - int i, count = 0; + int i, count = 0, sq_bug; const char *p2; char *p; if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) return -1; + sq_bug = script.len && script.buf[script.len - 1] != '\''; for (p = script.buf; *p; p++) /* * write_author_script() used to escape "'" incorrectly as @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env) * version the incorrect version in case git was upgraded while * rebase was stopped. */ - if (skip_prefix(p, "'\\''", &p2) || - skip_prefix(p, "'\\\\''", &p2)) + if (sq_bug && skip_prefix(p, "'\\\\''", &p2)) + strbuf_splice(&script, p - script.buf, p2 - p, "'", 1); + else if (skip_prefix(p, "'\\''", &p2)) strbuf_splice(&script, p - script.buf, p2 - p, "'", 1); else if (*p == '\'') strbuf_splice(&script, p-- - script.buf, 1, "", 0); diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 97f0b4bf881..dd726ff4dc4 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -75,16 +75,18 @@ test_expect_success 'rebase --keep-empty' ' test_line_count = 6 actual ' +SQ="'" test_expect_success 'rebase -i writes correct author-script' ' test_when_finished "test_might_fail git rebase --abort" && - git checkout master && + git checkout -b author-with-sq master && + GIT_AUTHOR_NAME="Auth O$SQ R" git commit --allow-empty -m with-sq && set_fake_editor && - FAKE_LINES="edit 1" git rebase -i HEAD^ && + FAKE_LINES="edit 1" git rebase -ki HEAD^ && ( sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && . .git/rebase-merge/author-script && test "$(git show -s --date=raw --format=%an,%ae,@%ad)" = \ - "$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE" + "Auth O$SQ R,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE" ) ' @@ -1347,7 +1349,6 @@ test_expect_success 'editor saves as CR/LF' ' ) ' -SQ="'" test_expect_success 'rebase -i --gpg-sign=<key-id>' ' set_fake_editor && FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \ ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] sequencer: fix quoting in write_author_script 2018-07-26 12:33 ` Johannes Schindelin @ 2018-07-27 10:36 ` Phillip Wood 2018-07-27 12:37 ` Johannes Schindelin 0 siblings, 1 reply; 27+ messages in thread From: Phillip Wood @ 2018-07-27 10:36 UTC (permalink / raw) To: Johannes Schindelin, Phillip Wood Cc: Git Mailing List, Junio C Hamano, Akinori MUSHA Hi Johannes On 26/07/18 13:33, Johannes Schindelin wrote: > Hi Phillip, > > On Wed, 18 Jul 2018, Phillip Wood wrote: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> Single quotes should be escaped as \' not \\'. Note that this only >> affects authors that contain a single quote and then only external >> scripts that read the author script and users whose git is upgraded from >> the shell version of rebase -i while rebase was stopped. This is because >> the parsing in read_env_script() expected the broken version and for >> some reason sq_dequote() called by read_author_ident() seems to handle >> the broken quoting correctly. >> >> Ideally write_author_script() would be rewritten to use >> split_ident_line() and sq_quote_buf() but this commit just fixes the >> immediate bug. >> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- > > Good catch. > >> This is untested, unfortuantely I don't have really have time to write a test or >> follow this up at the moment, if someone else want to run with it then please >> do. > > I modified the test that was added by Akinori. As it was added very early, > and as there is still a test case *after* Akinori's that compares a > hard-coded SHA-1, I refrained from using `test_commit` (which would change > that SHA-1). See below. Thanks for adding a test, that sounds like sensible approach, however having thought about it I wonder if we should just be writing a plain text file (e.g rebase-merge/author-data) and fixing the reader to read that if it exists and only then fall back to reading the legacy rebase-merge/author-script with a fix to correctly handle the script written by the shell version - what do you think? The author-script really should be just an implementation detail. If anyone really wants to read it they can still do 'read -r l' and split the lines with ${l%%=*} and ${l#*=} >> diff --git a/sequencer.c b/sequencer.c >> index 5354d4d51e..0b78d1f100 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -638,21 +638,21 @@ static int write_author_script(const char *message) >> else if (*message != '\'') >> strbuf_addch(&buf, *(message++)); >> else >> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); >> + strbuf_addf(&buf, "'\\%c'", *(message++)); >> strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='"); >> while (*message && *message != '\n' && *message != '\r') >> if (skip_prefix(message, "> ", &message)) >> break; >> else if (*message != '\'') >> strbuf_addch(&buf, *(message++)); >> else >> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); >> + strbuf_addf(&buf, "'\\%c'", *(message++)); >> strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@"); >> while (*message && *message != '\n' && *message != '\r') >> if (*message != '\'') >> strbuf_addch(&buf, *(message++)); >> else >> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); >> + strbuf_addf(&buf, "'\\%c'", *(message++)); >> res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); > > I resolved the merge conflict with Akinori's patch. FWIW I pushed all of > this, including the fixup to Junio's fixup to the > `fix-t3404-author-script-test` branch at https://github.com/dscho/git. > >> strbuf_release(&buf); >> return res; >> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env) >> { >> struct strbuf script = STRBUF_INIT; >> int i, count = 0; >> - char *p, *p2; >> + const char *p2; >> + char *p; >> >> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) >> return -1; >> >> for (p = script.buf; *p; p++) >> - if (skip_prefix(p, "'\\\\''", (const char **)&p2)) >> + /* >> + * write_author_script() used to escape "'" incorrectly as >> + * "'\\\\''" rather than "'\\''" so we check for the correct >> + * version the incorrect version in case git was upgraded while >> + * rebase was stopped. >> + */ >> + if (skip_prefix(p, "'\\''", &p2) || >> + skip_prefix(p, "'\\\\''", &p2)) > > I think in this form, it is possibly unsafe because it assumes that the > new code cannot generate output that would trigger that same code path. > Although I have to admit that I did not give this a great deal of thought. Hm, I not sure that it can. If the Author begins \\' then this will be written as the C string "'\\\\'\\''...". If \\' comes at the end then I think this will be written as "\\\\'\\'''", in the middle of the name it will be "\\\\'\\''..." > In any case, if you have to think long and hard about some fix, it might > be better to go with something that is easier to reason about. So how > about this: we already know that the code is buggy, Akinori fixed the bug, > where the author-script missed its trailing single-quote. We can use this > as a tell-tale for *this* bug. Assuming that Junio will advance both your > and Akinori's fix in close proximity. That sounds like a good approach > Again, this is pushed to the `fix-t3404-author-script-test` branch at > https://github.com/dscho/git; My fixup on top of your patch looks like > this (feel free to drop the sq_bug part and only keep the test part): > > -- snipsnap -- > diff --git a/sequencer.c b/sequencer.c > index 46c0b3e720f..7abe78dc78e 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -573,13 +573,14 @@ static int write_author_script(const char *message) > static int read_env_script(struct argv_array *env) > { > struct strbuf script = STRBUF_INIT; > - int i, count = 0; > + int i, count = 0, sq_bug; > const char *p2; > char *p; > > if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) > return -1; > > + sq_bug = script.len && script.buf[script.len - 1] != '\''; > for (p = script.buf; *p; p++) > /* > * write_author_script() used to escape "'" incorrectly as > @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env) > * version the incorrect version in case git was upgraded while > * rebase was stopped. > */ We probably want the change the comment slightly to explain sq_bug > - if (skip_prefix(p, "'\\''", &p2) || > - skip_prefix(p, "'\\\\''", &p2)) > + if (sq_bug && skip_prefix(p, "'\\\\''", &p2)) > + strbuf_splice(&script, p - script.buf, p2 - p, "'", 1); > + else if (skip_prefix(p, "'\\''", &p2)) > strbuf_splice(&script, p - script.buf, p2 - p, "'", 1); > else if (*p == '\'') > strbuf_splice(&script, p-- - script.buf, 1, "", 0); > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 97f0b4bf881..dd726ff4dc4 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -75,16 +75,18 @@ test_expect_success 'rebase --keep-empty' ' > test_line_count = 6 actual > ' > > +SQ="'" > test_expect_success 'rebase -i writes correct author-script' ' > test_when_finished "test_might_fail git rebase --abort" && > - git checkout master && > + git checkout -b author-with-sq master && > + GIT_AUTHOR_NAME="Auth O$SQ R" git commit --allow-empty -m with-sq && > set_fake_editor && > - FAKE_LINES="edit 1" git rebase -i HEAD^ && > + FAKE_LINES="edit 1" git rebase -ki HEAD^ && > ( > sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > . .git/rebase-merge/author-script && > test "$(git show -s --date=raw --format=%an,%ae,@%ad)" = \ > - "$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE" > + "Auth O$SQ R,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE" > ) > ' Testing all three variables together is nice. Best Wishes Phillip > @@ -1347,7 +1349,6 @@ test_expect_success 'editor saves as CR/LF' ' > ) > ' > > -SQ="'" > test_expect_success 'rebase -i --gpg-sign=<key-id>' ' > set_fake_editor && > FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \ > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] sequencer: fix quoting in write_author_script 2018-07-27 10:36 ` Phillip Wood @ 2018-07-27 12:37 ` Johannes Schindelin 2018-07-30 9:35 ` Phillip Wood 0 siblings, 1 reply; 27+ messages in thread From: Johannes Schindelin @ 2018-07-27 12:37 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Junio C Hamano, Akinori MUSHA Hi Phillip, Junio and Akinori, I just noticed that t3404 is broken without my patches (but with Junio's fixup), on Windows, macOS and Linux. (See log at the end.) On Fri, 27 Jul 2018, Phillip Wood wrote: > On 26/07/18 13:33, Johannes Schindelin wrote: > > > > On Wed, 18 Jul 2018, Phillip Wood wrote: > > > >> Single quotes should be escaped as \' not \\'. Note that this only > >> affects authors that contain a single quote and then only external > >> scripts that read the author script and users whose git is upgraded from > >> the shell version of rebase -i while rebase was stopped. This is because > >> the parsing in read_env_script() expected the broken version and for > >> some reason sq_dequote() called by read_author_ident() seems to handle > >> the broken quoting correctly. > >> > >> Ideally write_author_script() would be rewritten to use > >> split_ident_line() and sq_quote_buf() but this commit just fixes the > >> immediate bug. > > > >> This is untested, unfortuantely I don't have really have time to write a test or > >> follow this up at the moment, if someone else want to run with it then please > >> do. > > > > I modified the test that was added by Akinori. As it was added very early, > > and as there is still a test case *after* Akinori's that compares a > > hard-coded SHA-1, I refrained from using `test_commit` (which would change > > that SHA-1). See below. > > Thanks for adding a test, that sounds like sensible approach, however > having thought about it I wonder if we should just be writing a plain > text file (e.g rebase-merge/author-data) and fixing the reader to read > that if it exists and only then fall back to reading the legacy > rebase-merge/author-script with a fix to correctly handle the script > written by the shell version - what do you think? The author-script > really should be just an implementation detail. If anyone really wants > to read it they can still do 'read -r l' and split the lines with > ${l%%=*} and ${l#*=} In contrast to `git am`, there *is* a use case where power users might have come to rely on the presence of the .git/rebase-merge/author-script file *and* its nature as a shell script snippet: we purposefully allow scripting `rebase -i`. So I don't think that we can declare the file and its format as implementation detail, even if the idea is very, very tempting. > >> diff --git a/sequencer.c b/sequencer.c > >> index 5354d4d51e..0b78d1f100 100644 > >> --- a/sequencer.c > >> +++ b/sequencer.c > >> @@ -638,21 +638,21 @@ static int write_author_script(const char *message) > >> else if (*message != '\'') > >> strbuf_addch(&buf, *(message++)); > >> else > >> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); > >> + strbuf_addf(&buf, "'\\%c'", *(message++)); > >> strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='"); > >> while (*message && *message != '\n' && *message != '\r') > >> if (skip_prefix(message, "> ", &message)) > >> break; > >> else if (*message != '\'') > >> strbuf_addch(&buf, *(message++)); > >> else > >> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); > >> + strbuf_addf(&buf, "'\\%c'", *(message++)); > >> strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@"); > >> while (*message && *message != '\n' && *message != '\r') > >> if (*message != '\'') > >> strbuf_addch(&buf, *(message++)); > >> else > >> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); > >> + strbuf_addf(&buf, "'\\%c'", *(message++)); > >> res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); > > > > I resolved the merge conflict with Akinori's patch. FWIW I pushed all of > > this, including the fixup to Junio's fixup to the > > `fix-t3404-author-script-test` branch at https://github.com/dscho/git. > > > >> strbuf_release(&buf); > >> return res; > >> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env) > >> { > >> struct strbuf script = STRBUF_INIT; > >> int i, count = 0; > >> - char *p, *p2; > >> + const char *p2; > >> + char *p; > >> > >> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) > >> return -1; > >> > >> for (p = script.buf; *p; p++) > >> - if (skip_prefix(p, "'\\\\''", (const char **)&p2)) > >> + /* > >> + * write_author_script() used to escape "'" incorrectly as > >> + * "'\\\\''" rather than "'\\''" so we check for the correct > >> + * version the incorrect version in case git was upgraded while > >> + * rebase was stopped. > >> + */ > >> + if (skip_prefix(p, "'\\''", &p2) || > >> + skip_prefix(p, "'\\\\''", &p2)) > > > > I think in this form, it is possibly unsafe because it assumes that the > > new code cannot generate output that would trigger that same code path. > > Although I have to admit that I did not give this a great deal of thought. > > Hm, I not sure that it can. If the Author begins \\' then this will be > written as the C string "'\\\\'\\''...". If \\' comes at the end then I > think this will be written as "\\\\'\\'''", in the middle of the name it > will be "\\\\'\\''..." Yes, that matches my gut feeling... but... > > In any case, if you have to think long and hard about some fix, it might > > be better to go with something that is easier to reason about. So how > > about this: we already know that the code is buggy, Akinori fixed the bug, > > where the author-script missed its trailing single-quote. We can use this > > as a tell-tale for *this* bug. Assuming that Junio will advance both your > > and Akinori's fix in close proximity. > > That sounds like a good approach I am glad that you agree to this. I am a big fan of this age-old wisdom that goes somewhat like this: some code is so simple that there is no space for obvious bugs, and some code is so complicated that there is no space for obvious bugs. In this context, I would use the modified version: some code is so easy to reason about that there is no obvious flaw, and some other code is so difficult to reason about that there is no obvious flaw. Besides, using the sq_bug version can serve as a reminder to "pay down the technical debt" in the future. > > Again, this is pushed to the `fix-t3404-author-script-test` branch at > > https://github.com/dscho/git; My fixup on top of your patch looks like > > this (feel free to drop the sq_bug part and only keep the test part): > > > > -- snipsnap -- > > diff --git a/sequencer.c b/sequencer.c > > index 46c0b3e720f..7abe78dc78e 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -573,13 +573,14 @@ static int write_author_script(const char *message) > > static int read_env_script(struct argv_array *env) > > { > > struct strbuf script = STRBUF_INIT; > > - int i, count = 0; > > + int i, count = 0, sq_bug; > > const char *p2; > > char *p; > > > > if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) > > return -1; > > > > + sq_bug = script.len && script.buf[script.len - 1] != '\''; > > for (p = script.buf; *p; p++) > > /* > > * write_author_script() used to escape "'" incorrectly as > > @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env) > > * version the incorrect version in case git was upgraded while > > * rebase was stopped. > > */ > > We probably want the change the comment slightly to explain sq_bug True. Can you give it a shot? If not, I will try to remember some time mid next week. Here the promised log of t3404 with -i -v -x (on macOS, but Linux and Windows shows equivalent failures, and the way I read it, the problem is simply that the test was introduced in the middle of t3404 and subsequent test cases' assumptions are no longer met): -- snipsnap -- 2018-07-27T11:53:04.2450890Z ok 2 - rebase --keep-empty 2018-07-27T11:53:04.2464690Z 2018-07-27T11:53:04.2489730Z expecting success: 2018-07-27T11:53:04.2513840Z test_when_finished "git rebase --abort ||:" && 2018-07-27T11:53:04.2536980Z git checkout master && 2018-07-27T11:53:04.2563050Z set_fake_editor && 2018-07-27T11:53:04.2590300Z FAKE_LINES="edit 1" git rebase -i HEAD^ && 2018-07-27T11:53:04.2614900Z test -f .git/rebase-merge/author-script && 2018-07-27T11:53:04.2639740Z ( 2018-07-27T11:53:04.2665320Z sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && 2018-07-27T11:53:04.2688410Z eval "$(cat .git/rebase-merge/author-script)" && 2018-07-27T11:53:04.2710930Z test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && 2018-07-27T11:53:04.2734740Z test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && 2018-07-27T11:53:04.2756700Z test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" 2018-07-27T11:53:04.2778060Z ) 2018-07-27T11:53:04.2788910Z 2018-07-27T11:53:04.2810410Z ++ test_when_finished 'git rebase --abort ||:' 2018-07-27T11:53:04.2831430Z ++ test 0 = 0 2018-07-27T11:53:04.2852830Z ++ test_cleanup='{ git rebase --abort ||: 2018-07-27T11:53:04.2874430Z } && (exit "$eval_ret"); eval_ret=$?; :' 2018-07-27T11:53:04.2892720Z ++ git checkout master 2018-07-27T11:53:04.2911560Z Switched to branch 'master' 2018-07-27T11:53:04.2929490Z ++ set_fake_editor 2018-07-27T11:53:04.2947690Z ++ write_script fake-editor.sh 2018-07-27T11:53:04.2966040Z ++ echo '#!/bin/sh' 2018-07-27T11:53:04.2983780Z ++ cat 2018-07-27T11:53:04.3001950Z ++ chmod +x fake-editor.sh 2018-07-27T11:53:04.3019770Z +++ pwd 2018-07-27T11:53:04.3038530Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh' 2018-07-27T11:53:04.3057950Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh' 2018-07-27T11:53:04.3076510Z ++ export FAKE_EDITOR 2018-07-27T11:53:04.3094750Z ++ EDITOR='"$FAKE_EDITOR"' 2018-07-27T11:53:04.3112610Z ++ export EDITOR 2018-07-27T11:53:04.3130670Z ++ FAKE_LINES='edit 1' 2018-07-27T11:53:04.3153990Z ++ git rebase -i 'HEAD^' 2018-07-27T11:53:04.4674460Z rebase -i script before editing: 2018-07-27T11:53:04.4698420Z pick 8f99a4f E 2018-07-27T11:53:04.4710290Z 2018-07-27T11:53:04.4792780Z rebase -i script after editing: 2018-07-27T11:53:04.4817630Z edit 8f99a4f E 2018-07-27T11:53:04.5025190Z Rebasing (1/1) 2018-07-27T11:53:04.5046400Z Stopped at 8f99a4f... E 2018-07-27T11:53:04.5065680Z You can amend the commit now, with 2018-07-27T11:53:04.5074810Z 2018-07-27T11:53:04.5092850Z git commit --amend 2018-07-27T11:53:04.5101800Z 2018-07-27T11:53:04.5119900Z Once you are satisfied with your changes, run 2018-07-27T11:53:04.5129030Z 2018-07-27T11:53:04.5147390Z git rebase --continue 2018-07-27T11:53:04.5165610Z ++ test -f .git/rebase-merge/author-script 2018-07-27T11:53:04.5183820Z ++ sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE 2018-07-27T11:53:04.5201610Z ++ unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE 2018-07-27T11:53:04.5220730Z ++ return 0 2018-07-27T11:53:04.5244590Z +++ cat .git/rebase-merge/author-script 2018-07-27T11:53:04.5269130Z ++ eval 'GIT_AUTHOR_NAME='\''A U Thor'\'' 2018-07-27T11:53:04.5294440Z GIT_AUTHOR_EMAIL='\''author@example.com'\'' 2018-07-27T11:53:04.5319550Z GIT_AUTHOR_DATE='\''@1112912233 -0700'\''' 2018-07-27T11:53:04.5345580Z +++ GIT_AUTHOR_NAME='A U Thor' 2018-07-27T11:53:04.5365960Z +++ GIT_AUTHOR_EMAIL=author@example.com 2018-07-27T11:53:04.5384150Z +++ GIT_AUTHOR_DATE='@1112912233 -0700' 2018-07-27T11:53:04.5402250Z +++ git show --quiet --pretty=format:%an 2018-07-27T11:53:04.5420400Z ++ test 'A U Thor' = 'A U Thor' 2018-07-27T11:53:04.5438410Z +++ git show --quiet --pretty=format:%ae 2018-07-27T11:53:04.5459570Z ++ test author@example.com = author@example.com 2018-07-27T11:53:04.5477950Z +++ git show --quiet --date=raw --pretty=format:@%ad 2018-07-27T11:53:04.5496180Z ++ test '@1112912233 -0700' = '@1112912233 -0700' 2018-07-27T11:53:04.5514390Z ++ git rebase --abort 2018-07-27T11:53:04.6924920Z ++ exit 0 2018-07-27T11:53:04.6950890Z ++ eval_ret=0 2018-07-27T11:53:04.6973900Z ++ : 2018-07-27T11:53:04.6998480Z ok 3 - rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse 2018-07-27T11:53:04.7009400Z 2018-07-27T11:53:04.7030290Z expecting success: 2018-07-27T11:53:04.7050880Z set_fake_editor && 2018-07-27T11:53:04.7072520Z test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 && 2018-07-27T11:53:04.7092910Z test_i18ncmp expect actual 2018-07-27T11:53:04.7102960Z 2018-07-27T11:53:04.7123150Z ++ set_fake_editor 2018-07-27T11:53:04.7144080Z ++ write_script fake-editor.sh 2018-07-27T11:53:04.7164970Z ++ echo '#!/bin/sh' 2018-07-27T11:53:04.7185360Z ++ cat 2018-07-27T11:53:04.7212950Z ++ chmod +x fake-editor.sh 2018-07-27T11:53:04.7236720Z +++ pwd 2018-07-27T11:53:04.7263500Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh' 2018-07-27T11:53:04.7291390Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh' 2018-07-27T11:53:04.7314090Z ++ export FAKE_EDITOR 2018-07-27T11:53:04.7338290Z ++ EDITOR='"$FAKE_EDITOR"' 2018-07-27T11:53:04.7362710Z ++ export EDITOR 2018-07-27T11:53:04.7386370Z ++ test_must_fail env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^' 2018-07-27T11:53:04.7412410Z ++ case "$1" in 2018-07-27T11:53:04.7436170Z ++ _test_ok= 2018-07-27T11:53:04.7455340Z ++ env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^' 2018-07-27T11:53:04.9560670Z ++ exit_code=0 2018-07-27T11:53:04.9581700Z ++ test 0 -eq 0 2018-07-27T11:53:04.9603200Z ++ list_contains '' success 2018-07-27T11:53:04.9628730Z ++ case ",$1," in 2018-07-27T11:53:04.9652120Z ++ return 1 2018-07-27T11:53:04.9677740Z ++ echo 'test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^' 2018-07-27T11:53:04.9704590Z test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^ 2018-07-27T11:53:04.9730750Z ++ return 1 2018-07-27T11:53:04.9754580Z error: last command exited with $?=1 2018-07-27T11:53:04.9779100Z not ok 4 - rebase -i with empty HEAD 2018-07-27T11:53:04.9802670Z # 2018-07-27T11:53:04.9826330Z # set_fake_editor && 2018-07-27T11:53:04.9851320Z # test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 && 2018-07-27T11:53:04.9875680Z # test_i18ncmp expect actual 2018-07-27T11:53:04.9899170Z # 2018-07-27T11:53:04.9923310Z make[1]: *** [t3404-rebase-interactive.sh] Error 1 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [RFC PATCH] sequencer: fix quoting in write_author_script 2018-07-27 12:37 ` Johannes Schindelin @ 2018-07-30 9:35 ` Phillip Wood 0 siblings, 0 replies; 27+ messages in thread From: Phillip Wood @ 2018-07-30 9:35 UTC (permalink / raw) To: Johannes Schindelin, Phillip Wood Cc: Git Mailing List, Junio C Hamano, Akinori MUSHA On 27/07/18 13:37, Johannes Schindelin wrote: > Hi Phillip, Junio and Akinori, > > I just noticed that t3404 is broken without my patches (but with Junio's > fixup), on Windows, macOS and Linux. (See log at the end.) > > On Fri, 27 Jul 2018, Phillip Wood wrote: > >> On 26/07/18 13:33, Johannes Schindelin wrote: >>> >>> On Wed, 18 Jul 2018, Phillip Wood wrote: >>> >>>> Single quotes should be escaped as \' not \\'. Note that this only >>>> affects authors that contain a single quote and then only external >>>> scripts that read the author script and users whose git is upgraded from >>>> the shell version of rebase -i while rebase was stopped. This is because >>>> the parsing in read_env_script() expected the broken version and for >>>> some reason sq_dequote() called by read_author_ident() seems to handle >>>> the broken quoting correctly. >>>> >>>> Ideally write_author_script() would be rewritten to use >>>> split_ident_line() and sq_quote_buf() but this commit just fixes the >>>> immediate bug. >>> >>>> This is untested, unfortuantely I don't have really have time to write a test or >>>> follow this up at the moment, if someone else want to run with it then please >>>> do. >>> >>> I modified the test that was added by Akinori. As it was added very early, >>> and as there is still a test case *after* Akinori's that compares a >>> hard-coded SHA-1, I refrained from using `test_commit` (which would change >>> that SHA-1). See below. >> >> Thanks for adding a test, that sounds like sensible approach, however >> having thought about it I wonder if we should just be writing a plain >> text file (e.g rebase-merge/author-data) and fixing the reader to read >> that if it exists and only then fall back to reading the legacy >> rebase-merge/author-script with a fix to correctly handle the script >> written by the shell version - what do you think? The author-script >> really should be just an implementation detail. If anyone really wants >> to read it they can still do 'read -r l' and split the lines with >> ${l%%=*} and ${l#*=} > > In contrast to `git am`, there *is* a use case where power users might > have come to rely on the presence of the .git/rebase-merge/author-script > file *and* its nature as a shell script snippet: we purposefully allow > scripting `rebase -i`. > > So I don't think that we can declare the file and its format as > implementation detail, even if the idea is very, very tempting. > >>>> diff --git a/sequencer.c b/sequencer.c >>>> index 5354d4d51e..0b78d1f100 100644 >>>> --- a/sequencer.c >>>> +++ b/sequencer.c >>>> @@ -638,21 +638,21 @@ static int write_author_script(const char *message) >>>> else if (*message != '\'') >>>> strbuf_addch(&buf, *(message++)); >>>> else >>>> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); >>>> + strbuf_addf(&buf, "'\\%c'", *(message++)); >>>> strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='"); >>>> while (*message && *message != '\n' && *message != '\r') >>>> if (skip_prefix(message, "> ", &message)) >>>> break; >>>> else if (*message != '\'') >>>> strbuf_addch(&buf, *(message++)); >>>> else >>>> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); >>>> + strbuf_addf(&buf, "'\\%c'", *(message++)); >>>> strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@"); >>>> while (*message && *message != '\n' && *message != '\r') >>>> if (*message != '\'') >>>> strbuf_addch(&buf, *(message++)); >>>> else >>>> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); >>>> + strbuf_addf(&buf, "'\\%c'", *(message++)); >>>> res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); >>> >>> I resolved the merge conflict with Akinori's patch. FWIW I pushed all of >>> this, including the fixup to Junio's fixup to the >>> `fix-t3404-author-script-test` branch at https://github.com/dscho/git. >>> >>>> strbuf_release(&buf); >>>> return res; >>>> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env) >>>> { >>>> struct strbuf script = STRBUF_INIT; >>>> int i, count = 0; >>>> - char *p, *p2; >>>> + const char *p2; >>>> + char *p; >>>> >>>> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) >>>> return -1; >>>> >>>> for (p = script.buf; *p; p++) >>>> - if (skip_prefix(p, "'\\\\''", (const char **)&p2)) >>>> + /* >>>> + * write_author_script() used to escape "'" incorrectly as >>>> + * "'\\\\''" rather than "'\\''" so we check for the correct >>>> + * version the incorrect version in case git was upgraded while >>>> + * rebase was stopped. >>>> + */ >>>> + if (skip_prefix(p, "'\\''", &p2) || >>>> + skip_prefix(p, "'\\\\''", &p2)) >>> >>> I think in this form, it is possibly unsafe because it assumes that the >>> new code cannot generate output that would trigger that same code path. >>> Although I have to admit that I did not give this a great deal of thought. >> >> Hm, I not sure that it can. If the Author begins \\' then this will be >> written as the C string "'\\\\'\\''...". If \\' comes at the end then I >> think this will be written as "\\\\'\\'''", in the middle of the name it >> will be "\\\\'\\''..." > > Yes, that matches my gut feeling... but... > >>> In any case, if you have to think long and hard about some fix, it might >>> be better to go with something that is easier to reason about. So how >>> about this: we already know that the code is buggy, Akinori fixed the bug, >>> where the author-script missed its trailing single-quote. We can use this >>> as a tell-tale for *this* bug. Assuming that Junio will advance both your >>> and Akinori's fix in close proximity. >> >> That sounds like a good approach > > I am glad that you agree to this. > > I am a big fan of this age-old wisdom > that goes somewhat like this: some code is so simple that there is no > space for obvious bugs, and some code is so complicated that there is no > space for obvious bugs. In this context, I would use the modified > version: some code is so easy to reason about that there is no obvious > flaw, and some other code is so difficult to reason about that there is > no obvious flaw. > > Besides, using the sq_bug version can serve as a reminder to "pay down > the technical debt" in the future. > >>> Again, this is pushed to the `fix-t3404-author-script-test` branch at >>> https://github.com/dscho/git; My fixup on top of your patch looks like >>> this (feel free to drop the sq_bug part and only keep the test part): >>> >>> -- snipsnap -- >>> diff --git a/sequencer.c b/sequencer.c >>> index 46c0b3e720f..7abe78dc78e 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -573,13 +573,14 @@ static int write_author_script(const char *message) >>> static int read_env_script(struct argv_array *env) >>> { >>> struct strbuf script = STRBUF_INIT; >>> - int i, count = 0; >>> + int i, count = 0, sq_bug; >>> const char *p2; >>> char *p; >>> >>> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) >>> return -1; >>> >>> + sq_bug = script.len && script.buf[script.len - 1] != '\''; >>> for (p = script.buf; *p; p++) >>> /* >>> * write_author_script() used to escape "'" incorrectly as >>> @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env) >>> * version the incorrect version in case git was upgraded while >>> * rebase was stopped. >>> */ >> >> We probably want the change the comment slightly to explain sq_bug > > True. Can you give it a shot? If not, I will try to remember some time mid > next week. Sure, I'll send something out tomorrow or Wednesday. I think we should alter the test to check that there is no empty line at the end of the author-script file as that will break the test for the missing "'". > > Here the promised log of t3404 with -i -v -x (on macOS, but Linux and > Windows shows equivalent failures, and the way I read it, the problem is > simply that the test was introduced in the middle of t3404 and > subsequent test cases' assumptions are no longer met): > > -- snipsnap -- > 2018-07-27T11:53:04.2450890Z ok 2 - rebase --keep-empty > 2018-07-27T11:53:04.2464690Z > 2018-07-27T11:53:04.2489730Z expecting success: > 2018-07-27T11:53:04.2513840Z test_when_finished "git rebase --abort ||:" && > 2018-07-27T11:53:04.2536980Z git checkout master && > 2018-07-27T11:53:04.2563050Z set_fake_editor && > 2018-07-27T11:53:04.2590300Z FAKE_LINES="edit 1" git rebase -i HEAD^ && > 2018-07-27T11:53:04.2614900Z test -f .git/rebase-merge/author-script && > 2018-07-27T11:53:04.2639740Z ( > 2018-07-27T11:53:04.2665320Z sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > 2018-07-27T11:53:04.2688410Z eval "$(cat .git/rebase-merge/author-script)" && > 2018-07-27T11:53:04.2710930Z test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > 2018-07-27T11:53:04.2734740Z test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > 2018-07-27T11:53:04.2756700Z test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" > 2018-07-27T11:53:04.2778060Z ) > 2018-07-27T11:53:04.2788910Z > 2018-07-27T11:53:04.2810410Z ++ test_when_finished 'git rebase --abort ||:' > 2018-07-27T11:53:04.2831430Z ++ test 0 = 0 > 2018-07-27T11:53:04.2852830Z ++ test_cleanup='{ git rebase --abort ||: > 2018-07-27T11:53:04.2874430Z } && (exit "$eval_ret"); eval_ret=$?; :' > 2018-07-27T11:53:04.2892720Z ++ git checkout master > 2018-07-27T11:53:04.2911560Z Switched to branch 'master' > 2018-07-27T11:53:04.2929490Z ++ set_fake_editor > 2018-07-27T11:53:04.2947690Z ++ write_script fake-editor.sh > 2018-07-27T11:53:04.2966040Z ++ echo '#!/bin/sh' > 2018-07-27T11:53:04.2983780Z ++ cat > 2018-07-27T11:53:04.3001950Z ++ chmod +x fake-editor.sh > 2018-07-27T11:53:04.3019770Z +++ pwd > 2018-07-27T11:53:04.3038530Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh' > 2018-07-27T11:53:04.3057950Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh' > 2018-07-27T11:53:04.3076510Z ++ export FAKE_EDITOR > 2018-07-27T11:53:04.3094750Z ++ EDITOR='"$FAKE_EDITOR"' > 2018-07-27T11:53:04.3112610Z ++ export EDITOR > 2018-07-27T11:53:04.3130670Z ++ FAKE_LINES='edit 1' > 2018-07-27T11:53:04.3153990Z ++ git rebase -i 'HEAD^' > 2018-07-27T11:53:04.4674460Z rebase -i script before editing: > 2018-07-27T11:53:04.4698420Z pick 8f99a4f E > 2018-07-27T11:53:04.4710290Z > 2018-07-27T11:53:04.4792780Z rebase -i script after editing: > 2018-07-27T11:53:04.4817630Z edit 8f99a4f E > 2018-07-27T11:53:04.5025190Z Rebasing (1/1) > 2018-07-27T11:53:04.5046400Z Stopped at 8f99a4f... E > 2018-07-27T11:53:04.5065680Z You can amend the commit now, with > 2018-07-27T11:53:04.5074810Z > 2018-07-27T11:53:04.5092850Z git commit --amend > 2018-07-27T11:53:04.5101800Z > 2018-07-27T11:53:04.5119900Z Once you are satisfied with your changes, run > 2018-07-27T11:53:04.5129030Z > 2018-07-27T11:53:04.5147390Z git rebase --continue > 2018-07-27T11:53:04.5165610Z ++ test -f .git/rebase-merge/author-script > 2018-07-27T11:53:04.5183820Z ++ sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE > 2018-07-27T11:53:04.5201610Z ++ unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE > 2018-07-27T11:53:04.5220730Z ++ return 0 > 2018-07-27T11:53:04.5244590Z +++ cat .git/rebase-merge/author-script > 2018-07-27T11:53:04.5269130Z ++ eval 'GIT_AUTHOR_NAME='\''A U Thor'\'' > 2018-07-27T11:53:04.5294440Z GIT_AUTHOR_EMAIL='\''author@example.com'\'' > 2018-07-27T11:53:04.5319550Z GIT_AUTHOR_DATE='\''@1112912233 -0700'\''' > 2018-07-27T11:53:04.5345580Z +++ GIT_AUTHOR_NAME='A U Thor' > 2018-07-27T11:53:04.5365960Z +++ GIT_AUTHOR_EMAIL=author@example.com > 2018-07-27T11:53:04.5384150Z +++ GIT_AUTHOR_DATE='@1112912233 -0700' > 2018-07-27T11:53:04.5402250Z +++ git show --quiet --pretty=format:%an > 2018-07-27T11:53:04.5420400Z ++ test 'A U Thor' = 'A U Thor' > 2018-07-27T11:53:04.5438410Z +++ git show --quiet --pretty=format:%ae > 2018-07-27T11:53:04.5459570Z ++ test author@example.com = author@example.com > 2018-07-27T11:53:04.5477950Z +++ git show --quiet --date=raw --pretty=format:@%ad > 2018-07-27T11:53:04.5496180Z ++ test '@1112912233 -0700' = '@1112912233 -0700' > 2018-07-27T11:53:04.5514390Z ++ git rebase --abort > 2018-07-27T11:53:04.6924920Z ++ exit 0 > 2018-07-27T11:53:04.6950890Z ++ eval_ret=0 > 2018-07-27T11:53:04.6973900Z ++ : > 2018-07-27T11:53:04.6998480Z ok 3 - rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse > 2018-07-27T11:53:04.7009400Z > 2018-07-27T11:53:04.7030290Z expecting success: > 2018-07-27T11:53:04.7050880Z set_fake_editor && > 2018-07-27T11:53:04.7072520Z test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 && > 2018-07-27T11:53:04.7092910Z test_i18ncmp expect actual > 2018-07-27T11:53:04.7102960Z > 2018-07-27T11:53:04.7123150Z ++ set_fake_editor > 2018-07-27T11:53:04.7144080Z ++ write_script fake-editor.sh > 2018-07-27T11:53:04.7164970Z ++ echo '#!/bin/sh' > 2018-07-27T11:53:04.7185360Z ++ cat > 2018-07-27T11:53:04.7212950Z ++ chmod +x fake-editor.sh > 2018-07-27T11:53:04.7236720Z +++ pwd > 2018-07-27T11:53:04.7263500Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh' > 2018-07-27T11:53:04.7291390Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh' > 2018-07-27T11:53:04.7314090Z ++ export FAKE_EDITOR > 2018-07-27T11:53:04.7338290Z ++ EDITOR='"$FAKE_EDITOR"' > 2018-07-27T11:53:04.7362710Z ++ export EDITOR > 2018-07-27T11:53:04.7386370Z ++ test_must_fail env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^' > 2018-07-27T11:53:04.7412410Z ++ case "$1" in > 2018-07-27T11:53:04.7436170Z ++ _test_ok= > 2018-07-27T11:53:04.7455340Z ++ env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^' > 2018-07-27T11:53:04.9560670Z ++ exit_code=0 > 2018-07-27T11:53:04.9581700Z ++ test 0 -eq 0 > 2018-07-27T11:53:04.9603200Z ++ list_contains '' success > 2018-07-27T11:53:04.9628730Z ++ case ",$1," in > 2018-07-27T11:53:04.9652120Z ++ return 1 > 2018-07-27T11:53:04.9677740Z ++ echo 'test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^' > 2018-07-27T11:53:04.9704590Z test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^ > 2018-07-27T11:53:04.9730750Z ++ return 1 > 2018-07-27T11:53:04.9754580Z error: last command exited with $?=1 > 2018-07-27T11:53:04.9779100Z not ok 4 - rebase -i with empty HEAD > 2018-07-27T11:53:04.9802670Z # > 2018-07-27T11:53:04.9826330Z # set_fake_editor && > 2018-07-27T11:53:04.9851320Z # test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 && > 2018-07-27T11:53:04.9875680Z # test_i18ncmp expect actual > 2018-07-27T11:53:04.9899170Z # > 2018-07-27T11:53:04.9923310Z make[1]: *** [t3404-rebase-interactive.sh] Error 1 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly 2018-07-18 13:46 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Phillip Wood 2018-07-18 15:55 ` [RFC PATCH] sequencer: fix quoting in write_author_script Phillip Wood @ 2018-07-18 17:24 ` Junio C Hamano 1 sibling, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2018-07-18 17:24 UTC (permalink / raw) To: Phillip Wood; +Cc: Akinori MUSHA, git Phillip Wood <phillip.wood@talktalk.net> writes: >> The only consumer of a faulty author script written by the sequencer >> is read_env_script() in sequencer.c which doesn't worry about >> checking that quotes are paired. > > That's not quite true anymore, recently another consumer > read_author_ident() was added which uses sq_dequote() instead of > custom code. Looking more closely at write_author_script() the quoting > of single quotes is buggy they are escaped as \\' instead of \'. That's embarrassing re-invention (instead of reuse) with additional bug X-<. It seems that all of that blame to d87d48b2 ("sequencer: learn about the special "fake root commit" handling", 2018-05-04). We should fix both broken writer and readers that compensate for breakage in the writer, I guess. Sigh... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-18 9:45 ` Phillip Wood 2018-07-18 13:46 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Phillip Wood @ 2018-07-18 17:17 ` Junio C Hamano 2018-07-19 9:20 ` Phillip Wood 1 sibling, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2018-07-18 17:17 UTC (permalink / raw) To: Phillip Wood; +Cc: Akinori MUSHA, git Phillip Wood <phillip.wood@talktalk.net> writes: >> (I think we had code to do so in "git am" >> that was rewritten in C first). > > The code in builtin/am.c doesn't try to write valid posix shell (if > one assumes it is the only consumer of the author script then it > doesn't need to) which results in simpler code, but external scripts > cannot safely eval it anymore. Are you sure about that? If so we probably should see if we can fix the writer, and better yet, if we can share code with the writer discussed here, as presumably we are fixing it in this thread. But I do not see how builtin/am.c::write_author_script() would produce something that would not eval correctly. sq_quote_buf() was introduced specifically to write correct string for shell's consumption. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-18 17:17 ` [PATCH] sequencer.c: terminate the last line of author-script properly Junio C Hamano @ 2018-07-19 9:20 ` Phillip Wood 2018-07-26 12:39 ` Johannes Schindelin 0 siblings, 1 reply; 27+ messages in thread From: Phillip Wood @ 2018-07-19 9:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Akinori MUSHA, git Hi Junio On 18/07/18 18:17, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >>> (I think we had code to do so in "git am" >>> that was rewritten in C first). >> >> The code in builtin/am.c doesn't try to write valid posix shell (if >> one assumes it is the only consumer of the author script then it >> doesn't need to) which results in simpler code, but external scripts >> cannot safely eval it anymore. > > Are you sure about that? If so we probably should see if we can fix> the writer, and better yet, if we can share code with the writer > discussed here, as presumably we are fixing it in this thread. > > But I do not see how builtin/am.c::write_author_script() would > produce something that would not eval correctly. sq_quote_buf() was > introduced specifically to write correct string for shell's > consumption. You're right, I'm not sure how I missed the calls to sq_quote_buf() yesterday, sharing the am code with the sequencer would clean things up nicely. Best Wishes Phillip ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-19 9:20 ` Phillip Wood @ 2018-07-26 12:39 ` Johannes Schindelin 2018-07-26 17:53 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Johannes Schindelin @ 2018-07-26 12:39 UTC (permalink / raw) To: phillip.wood; +Cc: Junio C Hamano, Akinori MUSHA, git Hi Phillip, On Thu, 19 Jul 2018, Phillip Wood wrote: > On 18/07/18 18:17, Junio C Hamano wrote: > > Phillip Wood <phillip.wood@talktalk.net> writes: > > > >>> (I think we had code to do so in "git am" > >>> that was rewritten in C first). > >> > >> The code in builtin/am.c doesn't try to write valid posix shell (if > >> one assumes it is the only consumer of the author script then it > >> doesn't need to) which results in simpler code, but external scripts > >> cannot safely eval it anymore. > > > > Are you sure about that? If so we probably should see if we can fix> the writer, and better yet, if we can share code with the writer > > discussed here, as presumably we are fixing it in this thread. > > > > But I do not see how builtin/am.c::write_author_script() would > > produce something that would not eval correctly. sq_quote_buf() was > > introduced specifically to write correct string for shell's > > consumption. > > You're right, I'm not sure how I missed the calls to sq_quote_buf() > yesterday, sharing the am code with the sequencer would clean things up > nicely. No, actually Phillip was right. The `author-script` file written by `git-am` was always an implementation detail, and as there was no (intended) way to call shell scripts while running `git-am`, the only shell script to intentionally use `author-script` was `git-am` itself. Ever since `git-am` is a builtin, the `author-script` file format could be changed, because it is an implementation detail, no more nor less, and I think it *should* be changed, too. We're spending useless cycles on quoting and dequoting, when writing a NUL-separated list of var=value pairs would be totally sufficient to our ends. Ciao, Dscho ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-26 12:39 ` Johannes Schindelin @ 2018-07-26 17:53 ` Junio C Hamano 0 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2018-07-26 17:53 UTC (permalink / raw) To: Johannes Schindelin; +Cc: phillip.wood, Akinori MUSHA, git Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> You're right, I'm not sure how I missed the calls to sq_quote_buf() >> yesterday, sharing the am code with the sequencer would clean things up >> nicely. > > No, actually Phillip was right. The `author-script` file written by > `git-am` was always an implementation detail, and as there was no > (intended) way to call shell scripts while running `git-am`, the only > shell script to intentionally use `author-script` was `git-am` itself. Well the thing is that you did not write "am". When I wrote "am", I did so with a deliberate design decision to keep the author-script in the same format so that it can be read by shell. You are behaving as if you made a silent decision to improve the author-script by designing a better micro-format that deviates from what shells read by doubling the backslash quoting and losing the single quote at the end of the line for only the last one, and your justification is that it does not matter how broken the new micro-format is because it is an implementation detail nobody should care. And worse yet, you did that improvement without telling anybody else why the new format is better. That's just silly. Just like everybody else, you are sometimes wrong and you sometimes make mistakes. The rest of time you are not wrong and your design decisions are not mistaken, but trying to defend an obvious mistake like this one with silly excuses is an easy way to lose credibility. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-12 11:18 [PATCH] sequencer.c: terminate the last line of author-script properly Akinori MUSHA 2018-07-12 17:22 ` Junio C Hamano @ 2018-07-12 20:13 ` Junio C Hamano 2018-07-12 20:16 ` Eric Sunshine 2018-07-12 20:49 ` Junio C Hamano 2018-07-18 9:25 ` Phillip Wood 2018-07-18 13:50 ` Phillip Wood 3 siblings, 2 replies; 27+ messages in thread From: Junio C Hamano @ 2018-07-12 20:13 UTC (permalink / raw) To: Akinori MUSHA; +Cc: git "Akinori MUSHA" <knu@iDaemons.org> writes: > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 352a52e59..345b103eb 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' ' > test_line_count = 6 actual > ' > > +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' > + test_when_finished "git rebase --abort ||:" && > + git checkout master && > + set_fake_editor && > + FAKE_LINES="edit 1" git rebase -i HEAD^ && > + test -f .git/rebase-merge/author-script && > + unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && Is this "unset" safe? Some POSIX compliant shells barf if you unset a variable that is not set, so the answer to my question is yes only if we know these three variables are always set. > + eval "$(cat .git/rebase-merge/author-script)" && > + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" Oh, actually it is even worse than that. What if author-script is bogus, like in the version before your patch fixes the code? We do not restore the AUTHOR_NAME/EMAIL/DATE after this test_expect_success fails. How does that, i.e. missing some variable, affect execution of later steps in this same test script? I _think_ the right and safe way to fix taht is to do something like this: test -f .git/rebase-merge/author-script && ( safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... && eval "$(cat .git/rebase-merge/author-script)" && test ... && test ... && test ... ) That way, we won't have to worry about GIT_AUTHOR_* variables getting modified and affecting the tests that come later in the script. > +' > + > test_expect_success 'rebase -i with the exec command' ' > git checkout master && > ( > -- > 2.18.0 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-12 20:13 ` Junio C Hamano @ 2018-07-12 20:16 ` Eric Sunshine 2018-07-12 20:23 ` Junio C Hamano 2018-07-17 23:25 ` Junio C Hamano 2018-07-12 20:49 ` Junio C Hamano 1 sibling, 2 replies; 27+ messages in thread From: Eric Sunshine @ 2018-07-12 20:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: knu, Git List On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano <gitster@pobox.com> wrote: > I _think_ the right and safe way to fix taht is to do something like > this: > > test -f .git/rebase-merge/author-script && > ( > safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... && s/safe_unset/sane_unset/ > eval "$(cat .git/rebase-merge/author-script)" && > test ... && > test ... && > test ... > ) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-12 20:16 ` Eric Sunshine @ 2018-07-12 20:23 ` Junio C Hamano 2018-07-17 23:25 ` Junio C Hamano 1 sibling, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2018-07-12 20:23 UTC (permalink / raw) To: Eric Sunshine; +Cc: Akinori MUSHA, Git List Yup ;-) On Thu, Jul 12, 2018 at 1:16 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: > On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano <gitster@pobox.com> wrote: >> I _think_ the right and safe way to fix taht is to do something like >> this: >> >> test -f .git/rebase-merge/author-script && >> ( >> safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... && > > s/safe_unset/sane_unset/ > >> eval "$(cat .git/rebase-merge/author-script)" && >> test ... && >> test ... && >> test ... >> ) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-12 20:16 ` Eric Sunshine 2018-07-12 20:23 ` Junio C Hamano @ 2018-07-17 23:25 ` Junio C Hamano 2018-07-18 6:23 ` Akinori MUSHA 2018-07-26 12:07 ` Johannes Schindelin 1 sibling, 2 replies; 27+ messages in thread From: Junio C Hamano @ 2018-07-17 23:25 UTC (permalink / raw) To: knu; +Cc: Eric Sunshine, Git List I'll squash the following in (which I have been carrying in 'pu' for the past few days) unless I hear otherwise soonish to correct the issues raised during the review. Thanks. t/t3404-rebase-interactive.sh | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 2d189da2f1..b0cef509ab 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed set_fake_editor && FAKE_LINES="edit 1" git rebase -i HEAD^ && test -f .git/rebase-merge/author-script && - unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && - eval "$(cat .git/rebase-merge/author-script)" && - test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && - test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && - test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" + ( + sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && + eval "$(cat .git/rebase-merge/author-script)" && + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" + ) ' test_expect_success 'rebase -i with the exec command' ' -- 2.18.0-129-ge3331758f1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-17 23:25 ` Junio C Hamano @ 2018-07-18 6:23 ` Akinori MUSHA 2018-07-26 12:07 ` Johannes Schindelin 1 sibling, 0 replies; 27+ messages in thread From: Akinori MUSHA @ 2018-07-18 6:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Sunshine, Git List That's perfectly fine with me. I just thought each test case would run in a separate shell process and that's why I chose not to use a subshell for the last lines. I've learned a lot from feedback from you all. Thanks! On Wed, 18 Jul 2018 08:25:22 +0900, Junio C Hamano wrote: > > I'll squash the following in (which I have been carrying in 'pu' for > the past few days) unless I hear otherwise soonish to correct the > issues raised during the review. > > Thanks. > > t/t3404-rebase-interactive.sh | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 2d189da2f1..b0cef509ab 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed > set_fake_editor && > FAKE_LINES="edit 1" git rebase -i HEAD^ && > test -f .git/rebase-merge/author-script && > - unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > - eval "$(cat .git/rebase-merge/author-script)" && > - test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > - test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > - test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" > + ( > + sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > + eval "$(cat .git/rebase-merge/author-script)" && > + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" > + ) > ' > > test_expect_success 'rebase -i with the exec command' ' > -- > 2.18.0-129-ge3331758f1 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-17 23:25 ` Junio C Hamano 2018-07-18 6:23 ` Akinori MUSHA @ 2018-07-26 12:07 ` Johannes Schindelin 2018-07-26 17:44 ` Junio C Hamano 1 sibling, 1 reply; 27+ messages in thread From: Johannes Schindelin @ 2018-07-26 12:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: knu, Eric Sunshine, Git List Hi Junio, On Tue, 17 Jul 2018, Junio C Hamano wrote: > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 2d189da2f1..b0cef509ab 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed You missed a very long line here. > set_fake_editor && > FAKE_LINES="edit 1" git rebase -i HEAD^ && > test -f .git/rebase-merge/author-script && Why do we need this, if we already have an `eval` later on? > - unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > - eval "$(cat .git/rebase-merge/author-script)" && > - test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > - test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > - test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" > + ( > + sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > + eval "$(cat .git/rebase-merge/author-script)" && Why not . .git/rebase-merge/author-script instead? Less roundabout, easier to read, I think. > + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && How is this even working without `-s`? *clicketyclick* Ah, --quiet does this. Wait. `git show --quiet` is not even documented. All of those lines are too long, though. I am surprised you did not catch that. Besides, this would be more compact, less repetitive, *and* more readable as test "$(git show -s --date=raw --format=%an,%ae,@%ad)" = \ "$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE" t3404-rebase-interactive.sh already takes 8 minutes (last I checked, anyway) to run on a *fast* machine. There is absolutely no need to introduce even more spawning, not when it is so easily avoided. > + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" It is a shame that we cannot use %at directly here. > + ) > ' > > test_expect_success 'rebase -i with the exec command' ' Note: this is not a criticism of the original patch. It is a criticism of the review which could really have been better. I also saw that the test_when_finished uses a shell construct that shell script aficionados might like, but these days it is a lot better to use `test_might_fail` instead. Let's do this, then. So here goes, the clean-up patch on top of your 843654e435e (why does it have to be so darned tedious to get from a mail to the corresponding commit in `pu`), in all its glory: -- snipsnap -- diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index b0cef509ab7..97f0b4bf881 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -75,18 +75,16 @@ test_expect_success 'rebase --keep-empty' ' test_line_count = 6 actual ' -test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' - test_when_finished "git rebase --abort ||:" && +test_expect_success 'rebase -i writes correct author-script' ' + test_when_finished "test_might_fail git rebase --abort" && git checkout master && set_fake_editor && FAKE_LINES="edit 1" git rebase -i HEAD^ && - test -f .git/rebase-merge/author-script && ( sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && - eval "$(cat .git/rebase-merge/author-script)" && - test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && - test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && - test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" + . .git/rebase-merge/author-script && + test "$(git show -s --date=raw --format=%an,%ae,@%ad)" = \ + "$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE" ) ' ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-26 12:07 ` Johannes Schindelin @ 2018-07-26 17:44 ` Junio C Hamano 2018-07-27 15:49 ` Johannes Schindelin 0 siblings, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2018-07-26 17:44 UTC (permalink / raw) To: Johannes Schindelin; +Cc: knu, Eric Sunshine, Git List Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Junio, > > On Tue, 17 Jul 2018, Junio C Hamano wrote: > >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh >> index 2d189da2f1..b0cef509ab 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed > > You missed a very long line here. > >> set_fake_editor && >> FAKE_LINES="edit 1" git rebase -i HEAD^ && >> test -f .git/rebase-merge/author-script && > > Why do we need this, if we already have an `eval` later on? You are commenting on a wrong version. Comment on the original. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-26 17:44 ` Junio C Hamano @ 2018-07-27 15:49 ` Johannes Schindelin 0 siblings, 0 replies; 27+ messages in thread From: Johannes Schindelin @ 2018-07-27 15:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: knu, Eric Sunshine, Git List Hi Junio, On Thu, 26 Jul 2018, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > Hi Junio, > > > > On Tue, 17 Jul 2018, Junio C Hamano wrote: > > > >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > >> index 2d189da2f1..b0cef509ab 100755 > >> --- a/t/t3404-rebase-interactive.sh > >> +++ b/t/t3404-rebase-interactive.sh > >> @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed > > > > You missed a very long line here. > > > >> set_fake_editor && > >> FAKE_LINES="edit 1" git rebase -i HEAD^ && > >> test -f .git/rebase-merge/author-script && > > > > Why do we need this, if we already have an `eval` later on? > > You are commenting on a wrong version. Comment on the original. Sorry, no time. Take this review, or leave it. Ciao, Dscho ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-12 20:13 ` Junio C Hamano 2018-07-12 20:16 ` Eric Sunshine @ 2018-07-12 20:49 ` Junio C Hamano 1 sibling, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2018-07-12 20:49 UTC (permalink / raw) To: Akinori MUSHA; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > I _think_ the right and safe way to fix taht is to do something like > this: > > test -f .git/rebase-merge/author-script && > ( > safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... && > eval "$(cat .git/rebase-merge/author-script)" && > test ... && > test ... && > test ... > ) > > That way, we won't have to worry about GIT_AUTHOR_* variables > getting modified and affecting the tests that come later in the > script. It turns out that the use of subshell is *essential* for this test, as GIT_AUTHOR_* variables are exported and must remain so. unsetting and reading back may allows us to ensure that shell variables have the expected value, but then they are no longer exported, which will mean later tests will use whatever random author ident the person or the 'bot who is running the tests, not the one expected to be used by the test author(s). For tonight's pushout, I'll queue this on top. -- >8 -- From: Junio C Hamano <gitster@pobox.com> Date: Thu, 12 Jul 2018 13:23:02 -0700 Subject: [PATCH] SQUASH??? --- t/t3404-rebase-interactive.sh | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 2d189da2f1..b0cef509ab 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed set_fake_editor && FAKE_LINES="edit 1" git rebase -i HEAD^ && test -f .git/rebase-merge/author-script && - unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && - eval "$(cat .git/rebase-merge/author-script)" && - test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && - test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && - test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" + ( + sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && + eval "$(cat .git/rebase-merge/author-script)" && + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" + ) ' test_expect_success 'rebase -i with the exec command' ' -- 2.18.0-129-ge3331758f1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-12 11:18 [PATCH] sequencer.c: terminate the last line of author-script properly Akinori MUSHA 2018-07-12 17:22 ` Junio C Hamano 2018-07-12 20:13 ` Junio C Hamano @ 2018-07-18 9:25 ` Phillip Wood 2018-07-18 13:50 ` Phillip Wood 3 siblings, 0 replies; 27+ messages in thread From: Phillip Wood @ 2018-07-18 9:25 UTC (permalink / raw) To: Akinori MUSHA, git; +Cc: Junio C Hamano Hi Akinori On 12/07/18 12:18, Akinori MUSHA wrote: > > It looks like write_author_script() intends to write out a file in > Bourne shell syntax, but it doesn't put a closing single quote on the > last line. > > This patch makes .git/rebase-merge/author-script actually parsable by > sh(1) by adding a single quote and a linefeed to terminate the line > properly. > > Signed-off-by: Akinori MUSHA <knu@idaemons.org> > --- > sequencer.c | 1 + > t/t3404-rebase-interactive.sh | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/sequencer.c b/sequencer.c > index 4034c0461..5f32b6df1 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -651,6 +651,7 @@ static int write_author_script(const char *message) > strbuf_addch(&buf, *(message++)); > else > strbuf_addf(&buf, "'\\\\%c'", *(message++)); > + strbuf_addstr(&buf, "'\n"); > res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); The third parameter here means that write_message() will append a new line (you can check this by looking at the file that's created) so strictly speaking we only need to add "'" to the end of the message, alternatively it would be more obvious to keep adding "'\n" and change 1 to 0 in the call to write_message() Best Wishes Phillip > strbuf_release(&buf); > return res; > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 352a52e59..345b103eb 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' ' > test_line_count = 6 actual > ' > > +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' > + test_when_finished "git rebase --abort ||:" && > + git checkout master && > + set_fake_editor && > + FAKE_LINES="edit 1" git rebase -i HEAD^ && > + test -f .git/rebase-merge/author-script && > + unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > + eval "$(cat .git/rebase-merge/author-script)" && > + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" > +' > + > test_expect_success 'rebase -i with the exec command' ' > git checkout master && > ( > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-script properly 2018-07-12 11:18 [PATCH] sequencer.c: terminate the last line of author-script properly Akinori MUSHA ` (2 preceding siblings ...) 2018-07-18 9:25 ` Phillip Wood @ 2018-07-18 13:50 ` Phillip Wood 2018-07-18 13:58 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Phillip Wood 3 siblings, 1 reply; 27+ messages in thread From: Phillip Wood @ 2018-07-18 13:50 UTC (permalink / raw) To: Akinori MUSHA, git; +Cc: Junio C Hamano Hi Akinori On 12/07/18 12:18, Akinori MUSHA wrote: > > It looks like write_author_script() intends to write out a file in > Bourne shell syntax, but it doesn't put a closing single quote on the > last line. > > This patch makes .git/rebase-merge/author-script actually parsable by > sh(1) by adding a single quote and a linefeed to terminate the line > properly. > > Signed-off-by: Akinori MUSHA <knu@idaemons.org> > --- > sequencer.c | 1 + > t/t3404-rebase-interactive.sh | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/sequencer.c b/sequencer.c > index 4034c0461..5f32b6df1 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -651,6 +651,7 @@ static int write_author_script(const char *message) > strbuf_addch(&buf, *(message++)); > else > strbuf_addf(&buf, "'\\\\%c'", *(message++)); > + strbuf_addstr(&buf, "'\n"); > res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); > strbuf_release(&buf); > return res; > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 352a52e59..345b103eb 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' ' > test_line_count = 6 actual > ' > > +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' > + test_when_finished "git rebase --abort ||:" && > + git checkout master && > + set_fake_editor && > + FAKE_LINES="edit 1" git rebase -i HEAD^ && > + test -f .git/rebase-merge/author-script && > + unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > + eval "$(cat .git/rebase-merge/author-script)" && > + test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > + test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" > +' Have you checked that this test fails without your fix being applied? I just ran sh -c 'eval "$(cat .git/rebase-merge/author-script)"; echo "$GIT_AUTHOR_NAME"' while a rebase was stopped for an edit and it worked despite the fact that there is a missing quote at the end of the GIT_AUTHOR_DATE in the author script file. Best Wishes Phillip > test_expect_success 'rebase -i with the exec command' ' > git checkout master && > ( > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly 2018-07-18 13:50 ` Phillip Wood @ 2018-07-18 13:58 ` Phillip Wood 0 siblings, 0 replies; 27+ messages in thread From: Phillip Wood @ 2018-07-18 13:58 UTC (permalink / raw) To: Akinori MUSHA, git; +Cc: Junio C Hamano On 18/07/18 14:50, Phillip Wood wrote: > > Hi Akinori > On 12/07/18 12:18, Akinori MUSHA wrote: >> >> It looks like write_author_script() intends to write out a file in >> Bourne shell syntax, but it doesn't put a closing single quote on the >> last line. >> >> This patch makes .git/rebase-merge/author-script actually parsable by >> sh(1) by adding a single quote and a linefeed to terminate the line >> properly. >> >> Signed-off-by: Akinori MUSHA <knu@idaemons.org> >> --- >> sequencer.c | 1 + >> t/t3404-rebase-interactive.sh | 13 +++++++++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/sequencer.c b/sequencer.c >> index 4034c0461..5f32b6df1 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -651,6 +651,7 @@ static int write_author_script(const char *message) >> strbuf_addch(&buf, *(message++)); >> else >> strbuf_addf(&buf, "'\\\\%c'", *(message++)); >> + strbuf_addstr(&buf, "'\n"); >> res = write_message(buf.buf, buf.len, >> rebase_path_author_script(), 1); >> strbuf_release(&buf); >> return res; >> diff --git a/t/t3404-rebase-interactive.sh >> b/t/t3404-rebase-interactive.sh >> index 352a52e59..345b103eb 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' ' >> test_line_count = 6 actual >> ' >> +test_expect_success 'rebase -i writes out >> .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' >> + test_when_finished "git rebase --abort ||:" && >> + git checkout master && >> + set_fake_editor && >> + FAKE_LINES="edit 1" git rebase -i HEAD^ && >> + test -f .git/rebase-merge/author-script && >> + unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && >> + eval "$(cat .git/rebase-merge/author-script)" && >> + test "$(git show --quiet --pretty=format:%an)" = >> "$GIT_AUTHOR_NAME" && >> + test "$(git show --quiet --pretty=format:%ae)" = >> "$GIT_AUTHOR_EMAIL" && >> + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = >> "$GIT_AUTHOR_DATE" >> +' > > Have you checked that this test fails without your fix being applied? I > just ran > > sh -c 'eval "$(cat .git/rebase-merge/author-script)"; echo > "$GIT_AUTHOR_NAME"' > > while a rebase was stopped for an edit and it worked despite the fact > that there is a missing quote at the end of the GIT_AUTHOR_DATE in the > author script file. > > Best Wishes > > Phillip Please ignore that, I messed up, there was a closing "'" as I was using the shell version by a mistake. Sorry for the noise Phillip >> test_expect_success 'rebase -i with the exec command' ' >> git checkout master && >> ( >> > ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-07-30 9:35 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-12 11:18 [PATCH] sequencer.c: terminate the last line of author-script properly Akinori MUSHA 2018-07-12 17:22 ` Junio C Hamano 2018-07-18 9:45 ` Phillip Wood 2018-07-18 13:46 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Phillip Wood 2018-07-18 15:55 ` [RFC PATCH] sequencer: fix quoting in write_author_script Phillip Wood 2018-07-24 15:31 ` Junio C Hamano 2018-07-26 12:33 ` Johannes Schindelin 2018-07-27 10:36 ` Phillip Wood 2018-07-27 12:37 ` Johannes Schindelin 2018-07-30 9:35 ` Phillip Wood 2018-07-18 17:24 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Junio C Hamano 2018-07-18 17:17 ` [PATCH] sequencer.c: terminate the last line of author-script properly Junio C Hamano 2018-07-19 9:20 ` Phillip Wood 2018-07-26 12:39 ` Johannes Schindelin 2018-07-26 17:53 ` Junio C Hamano 2018-07-12 20:13 ` Junio C Hamano 2018-07-12 20:16 ` Eric Sunshine 2018-07-12 20:23 ` Junio C Hamano 2018-07-17 23:25 ` Junio C Hamano 2018-07-18 6:23 ` Akinori MUSHA 2018-07-26 12:07 ` Johannes Schindelin 2018-07-26 17:44 ` Junio C Hamano 2018-07-27 15:49 ` Johannes Schindelin 2018-07-12 20:49 ` Junio C Hamano 2018-07-18 9:25 ` Phillip Wood 2018-07-18 13:50 ` Phillip Wood 2018-07-18 13:58 ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Phillip Wood
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).