sequencer.c: terminate the last line of author-script properly
PATCH 1/1 sequencer.c: terminate the last line of author-script properly
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.
--- sequencer.c | 1 + t/t3404-rebase-interactive.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+)
sequencer.c +1 -0
@@ -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;
t/t3404-rebase-interactive.sh +13 -0
@@ -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 && (
Signature · 5 lines
2.18.0 -- Akinori MUSHA / https://akinori.org/
Quoted 3 lines · Akinori MUSHA
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/?
Quoted 4 lines
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.
Quoted 45 lines
Signed-off-by Akinori MUSHA <[email protected]>--- sequencer.c | 1 + t/t3404-rebase-interactive.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+)sequencer.c +1
-0@@ -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;t/t3404-rebase-interactive.sh +13
-0@@ -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 && (Signature · 1 line
2.18.0
Hi Junio
Quoted 14 lines · Junio C Hamano
Quoted 3 lines · Akinori MUSHA
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/?
Quoted 4 lines
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.
Quoted 6 lines
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.
Quoted 2 lines
(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.
Quoted 4 lines
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
Quoted 47 lines
Thanks.
Quoted 45 lines
Signed-off-by Akinori MUSHA <[email protected]>--- sequencer.c | 1 + t/t3404-rebase-interactive.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+)sequencer.c +1
-0@@ -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;t/t3404-rebase-interactive.sh +13
-0@@ -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 && (Signature · 1 line
2.18.0
Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly
Quoted 18 lines · Phillip Wood
Hi Junio
Quoted 14 lines · Junio C Hamano
Quoted 3 lines · Akinori MUSHA
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/?
Quoted 4 lines
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.
Quoted 8 lines
Quoted 6 lines
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()insequencer.cwhich 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
Quoted 6 lines
git-sh-setup.shto create the author script which does write valid posix shell.Quoted 2 lines
(I think we had code to do so in "git am"
that was rewritten in C first).The code in
builtin/am.cdoesn'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
Quoted 61 lines
safely eval it anymore.
Quoted 4 lines
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
Quoted 49 lines
Thanks.
Quoted 47 lines
Signed-off-by Akinori MUSHA <[email protected]>---
sequencer.c | 1 +t/t3404-rebase-interactive.sh| 13 +++++++++++++2 files changed, 14 insertions(+)sequencer.c +0
-0@@ -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;t/t3404-rebase-interactive.sh +0
-0@@ -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 &&
+ unsetGIT_AUTHOR_NAMEGIT_AUTHOR_EMAILGIT_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 &&
(Signature · 1 line
2.18.0
RFC PATCH 1/1 sequencer: fix quoting in write_author_script
From: Phillip Wood <[email protected]>
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.
sequencer.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
sequencer.c +13 -5
@@ -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 );
Signature · 1 line
2.18.0
Re: [RFC PATCH] sequencer: fix quoting in write_author_script
Quoted 14 lines · Phillip Wood
From: Phillip Wood <[email protected]>
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 reasonsq_dequote()called byread_author_ident()seems to handle the broken quoting correctly.Ideally
write_author_script()would be rewritten to usesplit_ident_line()andsq_quote_buf()but this commit just fixes the immediate bug.
Quoted 6 lines
Signed-off-by Phillip Wood <[email protected]>---
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.
Quoted 57 lines
sequencer.c| 18 +++++++++++++-----1 file changed, 13 insertions(+), 5 deletions(-)sequencer.c +13
-5@@ -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 );
Re: [RFC PATCH] sequencer: fix quoting in write_author_script
In reply to Phillip WoodHi Phillip,
Quoted 10 lines · Phillip Wood
From: Phillip Wood <[email protected]>
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 reasonsq_dequote()called byread_author_ident()seems to handle the broken quoting correctly.
Quoted 6 lines
Ideally
write_author_script()would be rewritten to usesplit_ident_line()andsq_quote_buf()but this commit just fixes the immediate bug.Signed-off-by Phillip Wood <[email protected]>---
Good catch.
Quoted 3 lines
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.
Quoted 17 lines
sequencer.c +1
-1@@ -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
sequencer.c static int write_author_script(const char *message) · line 656
Quoted 10 lines
sequencer.c +2
-2-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.
Quoted 13 lines
sequencer.c +2
-1strbuf_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 ;
sequencer.c static int read_env_script(struct argv_array *env) · line 683
Quoted 10 lines
sequencer.c +8
-1for (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 --
sequencer.c +5 -3
@@ -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 );
t/t3404-rebase-interactive.sh +5 -4
@@ -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^ \
Re: [RFC PATCH] sequencer: fix quoting in write_author_script
Hi Johannes
Quoted 23 lines · Johannes Schindelin
Hi Phillip,
Quoted 10 lines · Phillip Wood
From: Phillip Wood <[email protected]>
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 reasonsq_dequote()called byread_author_ident()seems to handle the broken quoting correctly.Quoted 6 lines
Ideally
write_author_script()would be rewritten to usesplit_ident_line()andsq_quote_buf()but this commit just fixes the immediate bug.Signed-off-by Phillip Wood <[email protected]>---
Good catch.
Quoted 8 lines
Quoted 3 lines
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#*=}
Quoted 45 lines
Quoted 17 lines
sequencer.c +1
-1@@ -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 sequencer.c static int write_author_script(const char *message) · line 656
Quoted 10 lines
sequencer.c +2
-2-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-testbranch at https://github.com/dscho/git.Quoted 13 lines
sequencer.c +2
-1strbuf_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 ;
Quoted 14 lines
sequencer.c static int read_env_script(struct argv_array *env) · line 683
Quoted 10 lines
sequencer.c +8
-1for (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 "\\\\'\\''..."
Quoted 6 lines
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
Quoted 26 lines
Again, this is pushed to the
fix-t3404-author-script-testbranch at
https://github.com/dscho/git; My fixup on top of your patch looks like this (feel free to drop thesq_bugpart and only keep the test part):-- snipsnap --
sequencer.c +2
-1@@ -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
sequencer.c static int read_env_script(struct argv_array *env) · line 590
Quoted 3 lines
sequencer.c +0
-0@@ -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
Quoted 23 lines
sequencer.c +3
-2-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 );t/t3404-rebase-interactive.sh +3
-1@@ -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 &&
t/t3404-rebase-interactive.sh test_expect_success 'rebase --keep-empty' ' · line 91
Quoted 10 lines
t/t3404-rebase-interactive.sh +2
-2- 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
Quoted 9 lines
t/t3404-rebase-interactive.sh +0
-1@@ -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^ \
Re: [RFC PATCH] sequencer: fix quoting in write_author_script
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.)
Quoted 25 lines · Phillip Wood
Quoted 15 lines · Johannes Schindelin
Quoted 11 lines · Phillip Wood
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 reasonsq_dequote()called byread_author_ident()seems to handle the broken quoting correctly.Ideally
write_author_script()would be rewritten to usesplit_ident_line()andsq_quote_buf()but this commit just fixes the immediate bug.Quoted 8 lines
Quoted 3 lines
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.
Quoted 9 lines
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.
Quoted 60 lines
Quoted 45 lines
Quoted 17 lines
sequencer.c +1
-1@@ -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 sequencer.c static int write_author_script(const char *message) · line 656
Quoted 10 lines
sequencer.c +2
-2-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-testbranch at https://github.com/dscho/git.Quoted 13 lines
sequencer.c +2
-1strbuf_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 ;Quoted 14 lines
sequencer.c static int read_env_script(struct argv_array *env) · line 683
Quoted 10 lines
sequencer.c +8
-1for (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.
Quoted 4 lines
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...
Quoted 8 lines
Quoted 6 lines
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.
Quoted 21 lines
Quoted 21 lines
Again, this is pushed to the
fix-t3404-author-script-testbranch at
https://github.com/dscho/git; My fixup on top of your patch looks like this (feel free to drop thesq_bugpart and only keep the test part):-- snipsnap --
sequencer.c +1
-1@@ -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 ;
Quoted 10 lines
sequencer.c static int read_env_script(struct argv_array *env) · line 590
Quoted 8 lines
sequencer.c +1
-0+ 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='\''[email protected]'\''
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 +++ [email protected]
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 [email protected] = [email protected]
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 1Re: [RFC PATCH] sequencer: fix quoting in write_author_script
Quoted 171 lines · Johannes Schindelin
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.)
Quoted 25 lines · Phillip Wood
Quoted 15 lines · Johannes Schindelin
Quoted 11 lines · Phillip Wood
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 reasonsq_dequote()called byread_author_ident()seems to handle the broken quoting correctly.Ideally
write_author_script()would be rewritten to usesplit_ident_line()andsq_quote_buf()but this commit just fixes the immediate bug.Quoted 8 lines
Quoted 3 lines
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.Quoted 9 lines
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 scriptingrebase -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.
Quoted 60 lines
Quoted 45 lines
Quoted 17 lines
sequencer.c +1
-1@@ -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 sequencer.c static int write_author_script(const char *message) · line 656
Quoted 10 lines
sequencer.c +2
-2-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-testbranch at https://github.com/dscho/git.Quoted 13 lines
sequencer.c +2
-1strbuf_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 ;Quoted 14 lines
sequencer.c static int read_env_script(struct argv_array *env) · line 683
Quoted 10 lines
sequencer.c +8
-1for (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.
Quoted 4 lines
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...
Quoted 8 lines
Quoted 6 lines
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_bugversion can serve as a reminder to "pay down the technical debt" in the future.Quoted 21 lines
Quoted 21 lines
Again, this is pushed to the
fix-t3404-author-script-testbranch at
https://github.com/dscho/git; My fixup on top of your patch looks like this (feel free to drop thesq_bugpart and only keep the test part):-- snipsnap --
sequencer.c +1
-1@@ -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 ;Quoted 10 lines
sequencer.c static int read_env_script(struct argv_array *env) · line 590
Quoted 8 lines
sequencer.c +1
-0+ 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
Quoted 2 lines
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 "'".
Quoted 116 lines
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='\''[email protected]'\'' 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 +++ [email protected] 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 [email protected] = [email protected] 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
Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly
In reply to Phillip WoodQuoted 8 lines · Phillip Wood
Quoted 3 lines
The only consumer of a faulty author script written by the sequencer is
read_env_script()insequencer.cwhich doesn't worry about checking that quotes are paired.That's not quite true anymore, recently another consumer
read_author_ident()was added which usessq_dequote()instead of custom code. Looking more closely atwrite_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...
Quoted 7 lines · Phillip Wood
Quoted 2 lines
(I think we had code to do so in "git am"
that was rewritten in C first).The code in
builtin/am.cdoesn'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.
Hi Junio
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Phillip Wood
Quoted 2 lines
(I think we had code to do so in "git am"
that was rewritten in C first).The code in
builtin/am.cdoesn'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.
Quoted 7 lines
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
Hi Phillip,
Quoted 14 lines · Phillip Wood
Quoted 13 lines · Junio C Hamano
Quoted 7 lines · Phillip Wood
Quoted 2 lines
(I think we had code to do so in "git am"
that was rewritten in C first).The code in
builtin/am.cdoesn'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.
Quoted 8 lines
Quoted 4 lines
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
Quoted 8 lines · Johannes Schindelin
Quoted 3 lines
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-scriptfile written bygit-amwas always an implementation detail, and as there was no (intended) way to call shell scripts while runninggit-am, the only shell script to intentionally useauthor-scriptwasgit-amitself.
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.
Quoted 5 lines · Akinori MUSHA
t/t3404-rebase-interactive.sh +0
-0
t/t3404-rebase-interactive.sh test_expect_success 'rebase --keep-empty' ' · line 84
Quoted 10 lines
t/t3404-rebase-interactive.sh +7
-0@@ -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.
t/t3404-rebase-interactive.sh test_expect_success 'rebase --keep-empty' ' · line 88
Quoted 4 lines
t/t3404-rebase-interactive.sh +4
-0+ 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.
Quoted 7 lines
t/t3404-rebase-interactive.sh +2
-0+' + test_expect_success 'rebase -i with the exec command' ' git checkout master && (Signature · 1 line
2.18.0
Quoted 6 lines · Junio C Hamano
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/
Quoted 5 lines
eval "$(cat .git/rebase-merge/author-script)" &&
test ... &&
test ... &&
test ...
)Yup ;-)
Quoted 15 lines · Eric Sunshine
Quoted 6 lines · Junio C Hamano
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/
Quoted 5 lines
eval "$(cat .git/rebase-merge/author-script)" && test ... && test ... && test ... )
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(-)
t/t3404-rebase-interactive.sh +7 -5
@@ -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' '
Signature · 1 line
2.18.0-129-ge3331758f1
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!
Quoted 36 lines · Junio C Hamano
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(-)t/t3404-rebase-interactive.sh +7
-5@@ -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' 'Signature · 1 line
2.18.0-129-ge3331758f1
Hi Junio,
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed
Quoted 5 lines · Junio C Hamano
t/t3404-rebase-interactive.sh +0
-0@@ -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.
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed · line 83
Quoted 3 lines
t/t3404-rebase-interactive.sh +0
-0set_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?
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed · line 86
Quoted 8 lines
t/t3404-rebase-interactive.sh +3
-5- 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.
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed · line 87
Quoted 1 line
t/t3404-rebase-interactive.sh +1
-0+ 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.
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed · line 89
Quoted 2 lines
t/t3404-rebase-interactive.sh +2
-0+ 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.
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed · line 93
Quoted 4 lines
t/t3404-rebase-interactive.sh +1
-0+ ) ' 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 --
t/t3404-rebase-interactive.sh +5 -7
@@ -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" ) '
Quoted 12 lines · Johannes Schindelin
Hi Junio,
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed
Quoted 5 lines · Junio C Hamano
t/t3404-rebase-interactive.sh +0
-0@@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edYou missed a very long line here.
Quoted 5 lines
t/t3404-rebase-interactive.sh test_expect_success 'rebase --keep-empty' '
Quoted 3 lines
t/t3404-rebase-interactive.sh +0
-0set_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
evallater on?
You are commenting on a wrong version. Comment on the original.
Hi Junio,
Quoted 14 lines · Junio C Hamano
Quoted 12 lines · Johannes Schindelin
Hi Junio,
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed
Quoted 5 lines · Junio C Hamano
t/t3404-rebase-interactive.sh +0
-0@@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edYou missed a very long line here.
Quoted 7 lines
Quoted 5 lines
t/t3404-rebase-interactive.sh test_expect_success 'rebase --keep-empty' '
Quoted 3 lines
t/t3404-rebase-interactive.sh +0
-0set_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
evallater on?You are commenting on a wrong version. Comment on the original.
Sorry, no time. Take this review, or leave it.
Ciao,
Dscho
Quoted 12 lines · Junio C Hamano
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 ... )
Quoted 3 lines
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 <[email protected]>
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(-)
t/t3404-rebase-interactive.sh +7 -5
@@ -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' '
Signature · 1 line
2.18.0-129-ge3331758f1
Hi Akinori
Quoted 20 lines · Akinori MUSHA
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 <[email protected]>--- sequencer.c | 1 + t/t3404-rebase-interactive.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+)sequencer.c +0
-0
sequencer.c static int write_author_script(const char *message) · line 655
Quoted 5 lines
sequencer.c +1
-0@@ -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
Quoted 27 lines
sequencer.c +0
-0strbuf_release (&buf);return res;t/t3404-rebase-interactive.sh +13
-0@@ -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 && (
Hi Akinori
Quoted 37 lines · Akinori MUSHA
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 <[email protected]>--- sequencer.c | 1 + t/t3404-rebase-interactive.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+)sequencer.c +1
-0@@ -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;t/t3404-rebase-interactive.sh +2
-0@@ -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 ||:" &&
t/t3404-rebase-interactive.sh test_expect_success 'rebase --keep-empty' ' · line 89
Quoted 10 lines
t/t3404-rebase-interactive.sh +10
-0+ 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
Quoted 4 lines
t/t3404-rebase-interactive.sh +0
-0test_expect_success 'rebase -i with the exec command' ' git checkout master && (
Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly
Quoted 56 lines · Phillip Wood
Hi Akinori
Quoted 42 lines · Akinori MUSHA
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 <[email protected]>---
sequencer.c | 1 +t/t3404-rebase-interactive.sh| 13 +++++++++++++2 files changed, 14 insertions(+)sequencer.c +0
-0@@ -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;t/t3404-rebase-interactive.sh +0
-0@@ -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 &&
+ unsetGIT_AUTHOR_NAMEGIT_AUTHOR_EMAILGIT_AUTHOR_DATE&&Quoted 8 lines
+ 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
Quoted 7 lines
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_DATEin 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
Quoted 5 lines
Quoted 4 lines
test_expect_success'rebase -i with the exec command' ' git checkout master &&
(