git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] sequencer.c: terminate the last line of author-script properly
@ 2018-07-12 11:18 Akinori MUSHA
  2018-07-12 17:22 ` Junio C Hamano
  2018-07-12 20:13 ` Junio C Hamano
  0 siblings, 2 replies; 6+ messages in thread
From: Akinori MUSHA @ 2018-07-12 11:18 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

It looks like write_author_script() intends to write out a file in
Bourne shell syntax, but it doesn't put a closing single quote on the
last line.

This patch makes .git/rebase-merge/author-script actually parsable by
sh(1) by adding a single quote and a linefeed to terminate the line
properly.

Signed-off-by: Akinori MUSHA <knu@idaemons.org>
---
 sequencer.c                   |  1 +
 t/t3404-rebase-interactive.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 4034c0461..5f32b6df1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -651,6 +651,7 @@ static int write_author_script(const char *message)
 			strbuf_addch(&buf, *(message++));
 		else
 			strbuf_addf(&buf, "'\\\\%c'", *(message++));
+	strbuf_addstr(&buf, "'\n");
 	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
 	strbuf_release(&buf);
 	return res;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 352a52e59..345b103eb 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '
 	test_line_count = 6 actual
 '
 
+test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' '
+	test_when_finished "git rebase --abort ||:" &&
+	git checkout master &&
+	set_fake_editor &&
+	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
+	test -f .git/rebase-merge/author-script &&
+	unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
+	eval "$(cat .git/rebase-merge/author-script)" &&
+	test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&
+	test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
+	test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"
+'
+
 test_expect_success 'rebase -i with the exec command' '
 	git checkout master &&
 	(
-- 
2.18.0


-- 
Akinori MUSHA / https://akinori.org/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sequencer.c: terminate the last line of author-script properly
  2018-07-12 11:18 [PATCH] sequencer.c: terminate the last line of author-script properly Akinori MUSHA
@ 2018-07-12 17:22 ` Junio C Hamano
  2018-07-12 20:13 ` Junio C Hamano
  1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2018-07-12 17:22 UTC (permalink / raw)
  To: Akinori MUSHA; +Cc: git

"Akinori MUSHA" <knu@iDaemons.org> writes:

> It looks like write_author_script() intends to write out a file in
> Bourne shell syntax, but it doesn't put a closing single quote on the
> last line.

s/closing single quote/& and the terminating newline/?

>
> This patch makes .git/rebase-merge/author-script actually parsable by
> sh(1) by adding a single quote and a linefeed to terminate the line
> properly.

Sounds good.

I wonder why this breakage was left unnoticed for a long time,
though.  It's not like writing and reading the author-script from C
code was done first in the "rebase -i" and friends that are users of
the sequencer machinery (I think we had code to do so in "git am"
that was rewritten in C first).  Do we have a similar issue over
there as well?  If not, perhaps if we reused the existing code that
was not broken, we wouldn't have seen this breakage on the sequencer
side?

Thanks.

>
> Signed-off-by: Akinori MUSHA <knu@idaemons.org>
> ---
>  sequencer.c                   |  1 +
>  t/t3404-rebase-interactive.sh | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 4034c0461..5f32b6df1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -651,6 +651,7 @@ static int write_author_script(const char *message)
>  			strbuf_addch(&buf, *(message++));
>  		else
>  			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> +	strbuf_addstr(&buf, "'\n");
>  	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
>  	strbuf_release(&buf);
>  	return res;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 352a52e59..345b103eb 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '
>  	test_line_count = 6 actual
>  '
>  
> +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' '
> +	test_when_finished "git rebase --abort ||:" &&
> +	git checkout master &&
> +	set_fake_editor &&
> +	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
> +	test -f .git/rebase-merge/author-script &&
> +	unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> +	eval "$(cat .git/rebase-merge/author-script)" &&
> +	test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&
> +	test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
> +	test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"
> +'
> +
>  test_expect_success 'rebase -i with the exec command' '
>  	git checkout master &&
>  	(
> -- 
> 2.18.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sequencer.c: terminate the last line of author-script properly
  2018-07-12 11:18 [PATCH] sequencer.c: terminate the last line of author-script properly Akinori MUSHA
  2018-07-12 17:22 ` Junio C Hamano
@ 2018-07-12 20:13 ` Junio C Hamano
  2018-07-12 20:16   ` Eric Sunshine
  2018-07-12 20:49   ` Junio C Hamano
  1 sibling, 2 replies; 6+ messages in thread
From: Junio C Hamano @ 2018-07-12 20:13 UTC (permalink / raw)
  To: Akinori MUSHA; +Cc: git

"Akinori MUSHA" <knu@iDaemons.org> writes:

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 352a52e59..345b103eb 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '
>  	test_line_count = 6 actual
>  '
>  
> +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' '
> +	test_when_finished "git rebase --abort ||:" &&
> +	git checkout master &&
> +	set_fake_editor &&
> +	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
> +	test -f .git/rebase-merge/author-script &&
> +	unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&

Is this "unset" safe?  Some POSIX compliant shells barf if you unset
a variable that is not set, so the answer to my question is yes only
if we know these three variables are always set.

> +	eval "$(cat .git/rebase-merge/author-script)" &&
> +	test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&
> +	test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
> +	test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"

Oh, actually it is even worse than that.  What if author-script is
bogus, like in the version before your patch fixes the code?  We do
not restore the AUTHOR_NAME/EMAIL/DATE after this test_expect_success
fails.  How does that, i.e. missing some variable, affect execution
of later steps in this same test script?

I _think_ the right and safe way to fix taht is to do something like
this:

	test -f .git/rebase-merge/author-script &&
	(
		safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&
		eval "$(cat .git/rebase-merge/author-script)" &&
		test ... &&
		test ... &&
		test ...
	)

That way, we won't have to worry about GIT_AUTHOR_* variables
getting modified and affecting the tests that come later in the
script.

> +'
> +
>  test_expect_success 'rebase -i with the exec command' '
>  	git checkout master &&
>  	(
> -- 
> 2.18.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sequencer.c: terminate the last line of author-script properly
  2018-07-12 20:13 ` Junio C Hamano
@ 2018-07-12 20:16   ` Eric Sunshine
  2018-07-12 20:23     ` Junio C Hamano
  2018-07-12 20:49   ` Junio C Hamano
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Sunshine @ 2018-07-12 20:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: knu, Git List

On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano <gitster@pobox.com> wrote:
> I _think_ the right and safe way to fix taht is to do something like
> this:
>
>         test -f .git/rebase-merge/author-script &&
>         (
>                 safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&

s/safe_unset/sane_unset/

>                 eval "$(cat .git/rebase-merge/author-script)" &&
>                 test ... &&
>                 test ... &&
>                 test ...
>         )

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sequencer.c: terminate the last line of author-script properly
  2018-07-12 20:16   ` Eric Sunshine
@ 2018-07-12 20:23     ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2018-07-12 20:23 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Akinori MUSHA, Git List

Yup ;-)

On Thu, Jul 12, 2018 at 1:16 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>> I _think_ the right and safe way to fix taht is to do something like
>> this:
>>
>>         test -f .git/rebase-merge/author-script &&
>>         (
>>                 safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&
>
> s/safe_unset/sane_unset/
>
>>                 eval "$(cat .git/rebase-merge/author-script)" &&
>>                 test ... &&
>>                 test ... &&
>>                 test ...
>>         )

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sequencer.c: terminate the last line of author-script properly
  2018-07-12 20:13 ` Junio C Hamano
  2018-07-12 20:16   ` Eric Sunshine
@ 2018-07-12 20:49   ` Junio C Hamano
  1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2018-07-12 20:49 UTC (permalink / raw)
  To: Akinori MUSHA; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> I _think_ the right and safe way to fix taht is to do something like
> this:
>
> 	test -f .git/rebase-merge/author-script &&
> 	(
> 		safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&
> 		eval "$(cat .git/rebase-merge/author-script)" &&
> 		test ... &&
> 		test ... &&
> 		test ...
> 	)
>
> That way, we won't have to worry about GIT_AUTHOR_* variables
> getting modified and affecting the tests that come later in the
> script.

It turns out that the use of subshell is *essential* for this test,
as GIT_AUTHOR_* variables are exported and must remain so.  unsetting
and reading back may allows us to ensure that shell variables have
the expected value, but then they are no longer exported, which will
mean later tests will use whatever random author ident the person or
the 'bot who is running the tests, not the one expected to be used
by the test author(s).

For tonight's pushout, I'll queue this on top.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 12 Jul 2018 13:23:02 -0700
Subject: [PATCH] SQUASH???

---
 t/t3404-rebase-interactive.sh | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 2d189da2f1..b0cef509ab 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed
 	set_fake_editor &&
 	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
 	test -f .git/rebase-merge/author-script &&
-	unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
-	eval "$(cat .git/rebase-merge/author-script)" &&
-	test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&
-	test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
-	test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"
+	(
+		sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
+		eval "$(cat .git/rebase-merge/author-script)" &&
+		test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&
+		test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
+		test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"
+	)
 '
 
 test_expect_success 'rebase -i with the exec command' '
-- 
2.18.0-129-ge3331758f1


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-12 11:18 [PATCH] sequencer.c: terminate the last line of author-script properly Akinori MUSHA
2018-07-12 17:22 ` Junio C Hamano
2018-07-12 20:13 ` Junio C Hamano
2018-07-12 20:16   ` Eric Sunshine
2018-07-12 20:23     ` Junio C Hamano
2018-07-12 20:49   ` Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox