Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Hi Junio, On Thu, 26 Jul 2018, Junio C Hamano wrote: > Johannes Schindelin 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
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Johannes Schindelin 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.
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Johannes Schindelin 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.
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Hi Phillip, On Thu, 19 Jul 2018, Phillip Wood wrote: > On 18/07/18 18:17, Junio C Hamano wrote: > > Phillip Wood 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
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
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" ) '
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Hi Junio On 18/07/18 18:17, Junio C Hamano wrote: > Phillip Wood 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
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Phillip Wood 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.
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
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 --- 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(, *(message++)); else strbuf_addf(, "'%c'", *(message++)); + strbuf_addstr(, "'\n"); res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); strbuf_release(); 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 && (
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Hi Junio On 12/07/18 18:22, Junio C Hamano wrote: "Akinori MUSHA" 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 --- 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(, *(message++)); else strbuf_addf(, "'%c'", *(message++)); + strbuf_addstr(, "'\n"); res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); strbuf_release(); 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
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
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 --- 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(, *(message++)); else strbuf_addf(, "'%c'", *(message++)); + strbuf_addstr(, "'\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(); 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 && (
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
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 >
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
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
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Junio C Hamano 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 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
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Yup ;-) On Thu, Jul 12, 2018 at 1:16 PM, Eric Sunshine wrote: > On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano 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 ... >> )
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano 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 ... > )
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
"Akinori MUSHA" 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
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
"Akinori MUSHA" 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 > --- > 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(, *(message++)); > else > strbuf_addf(, "'%c'", *(message++)); > + strbuf_addstr(, "'\n"); > res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); > strbuf_release(); > 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
[PATCH] 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. Signed-off-by: Akinori MUSHA --- 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(, *(message++)); else strbuf_addf(, "'%c'", *(message++)); + strbuf_addstr(, "'\n"); res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); strbuf_release(); 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/
