[10/12] t/lib-rebase: prepare for testing `git rebase --rebase-merges`
diff mbox series

Message ID ae9e72b73bf2da0de3a5369748ebd358656588d9.1564049474.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • rebase -r: support merge strategies other than recursive
Related show

Commit Message

Phillip Wood via GitGitGadget July 25, 2019, 10:11 a.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

The format of the todo list is quite a bit different in the
`--rebase-merges` mode; Let's prepare the fake editor to handle those
todo lists properly, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-rebase.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

brian m. carlson July 26, 2019, 7:43 a.m. UTC | #1
On 2019-07-25 at 10:11:22, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The format of the todo list is quite a bit different in the
> `--rebase-merges` mode; Let's prepare the fake editor to handle those
> todo lists properly, too.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/lib-rebase.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> index 7ea30e5006..662a958575 100644
> --- a/t/lib-rebase.sh
> +++ b/t/lib-rebase.sh
> @@ -44,10 +44,10 @@ set_fake_editor () {
>  	rm -f "$1"
>  	echo 'rebase -i script before editing:'
>  	cat "$1".tmp
> -	action=pick
> +	action=\&

So we set action to "&" so we can use it as the result in the sed
expression below…

>  	for line in $FAKE_LINES; do
>  		case $line in
> -		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
> +		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
>  			action="$line";;
>  		exec_*|x_*|break|b)
>  			echo "$line" | sed 's/_/ /g' >> "$1";;
> @@ -61,8 +61,8 @@ set_fake_editor () {
>  			echo "$action XXXXXXX False commit" >> "$1"

but then here it doesn't look like "&" is a thing we'd want to use. Is
there something I'm missing about this particular case?
Johannes Schindelin July 26, 2019, 2:01 p.m. UTC | #2
Hi brian,

On Fri, 26 Jul 2019, brian m. carlson wrote:

> On 2019-07-25 at 10:11:22, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The format of the todo list is quite a bit different in the
> > `--rebase-merges` mode; Let's prepare the fake editor to handle those
> > todo lists properly, too.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/lib-rebase.sh | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
> > index 7ea30e5006..662a958575 100644
> > --- a/t/lib-rebase.sh
> > +++ b/t/lib-rebase.sh
> > @@ -44,10 +44,10 @@ set_fake_editor () {
> >  	rm -f "$1"
> >  	echo 'rebase -i script before editing:'
> >  	cat "$1".tmp
> > -	action=pick
> > +	action=\&
>
> So we set action to "&" so we can use it as the result in the sed
> expression below…
>
> >  	for line in $FAKE_LINES; do
> >  		case $line in
> > -		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
> > +		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
> >  			action="$line";;
> >  		exec_*|x_*|break|b)
> >  			echo "$line" | sed 's/_/ /g' >> "$1";;
> > @@ -61,8 +61,8 @@ set_fake_editor () {
> >  			echo "$action XXXXXXX False commit" >> "$1"
>
> but then here it doesn't look like "&" is a thing we'd want to use. Is
> there something I'm missing about this particular case?

Actually, the part that uses it is not shown in the patch (one of the
many, many reasons why I try to discourage patch review and encourage
code review instead). The relevant section currently looks somewhat like
this:

-- snip --
set_fake_editor () {
	write_script fake-editor.sh <<-\EOF
	case "$1" in
	*/COMMIT_EDITMSG)
		test -z "$EXPECT_HEADER_COUNT" ||
			test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" ||
			test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" ||
			exit
		test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
		test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
		exit
		;;
	esac
	test -z "$EXPECT_COUNT" ||
		test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
		exit
	test -z "$FAKE_LINES" && exit
	grep -v '^#' < "$1" > "$1".tmp
	rm -f "$1"
	echo 'rebase -i script before editing:'
	cat "$1".tmp
	action=pick
	for line in $FAKE_LINES; do
		case $line in
		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
			action="$line";;
		exec_*|x_*|break|b)
			echo "$line" | sed 's/_/ /g' >> "$1";;
		"#")
			echo '# comment' >> "$1";;
		">")
			echo >> "$1";;
		bad)
			action="badcmd";;
		fakesha)
			echo "$action XXXXXXX False commit" >> "$1"
			action=pick;;
		*)
			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
			action=pick;;
		esac
	done
	echo 'rebase -i script after editing:'
	cat "$1"
	EOF

	test_set_editor "$(pwd)/fake-editor.sh"
}
-- snap --

Most importantly, `action` is used here:

                        sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"

and I changed it to

			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"

In other words, rather than expecting the lines that are used by the
fake editor to start with `pick`, after this patch, the tests expect the
todo lists to start with a command consisting of lower-case ASCII
letters (which catches `pick`, of course, but also `label`, `reset` and
`merge`).

After this patch, the fake editor also does not try to replace whatever
command it finds by `pick`, but it keeps it as-is instead.

Ciao,
Dscho
brian m. carlson July 26, 2019, 9:08 p.m. UTC | #3
On 2019-07-26 at 14:01:03, Johannes Schindelin wrote:
> Actually, the part that uses it is not shown in the patch (one of the
> many, many reasons why I try to discourage patch review and encourage
> code review instead). The relevant section currently looks somewhat like
> this:

I feel like I may have communicated poorly earlier, so let me retry
asking this in a different way.

> -- snip --
> set_fake_editor () {
> 	write_script fake-editor.sh <<-\EOF
> 	case "$1" in
> 	*/COMMIT_EDITMSG)
> 		test -z "$EXPECT_HEADER_COUNT" ||
> 			test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" ||
> 			test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" ||
> 			exit
> 		test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
> 		test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
> 		exit
> 		;;
> 	esac
> 	test -z "$EXPECT_COUNT" ||
> 		test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
> 		exit
> 	test -z "$FAKE_LINES" && exit
> 	grep -v '^#' < "$1" > "$1".tmp
> 	rm -f "$1"
> 	echo 'rebase -i script before editing:'
> 	cat "$1".tmp
> 	action=pick

I believe you changed this line to "action=\&".

> 	for line in $FAKE_LINES; do
> 		case $line in
> 		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
> 			action="$line";;
> 		exec_*|x_*|break|b)
> 			echo "$line" | sed 's/_/ /g' >> "$1";;
> 		"#")
> 			echo '# comment' >> "$1";;
> 		">")
> 			echo >> "$1";;
> 		bad)
> 			action="badcmd";;
> 		fakesha)
> 			echo "$action XXXXXXX False commit" >> "$1"

And my question was about this line.

> 			action=pick;;
> 		*)
> 			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
> 			action=pick;;
> 		esac
> 	done
> 	echo 'rebase -i script after editing:'
> 	cat "$1"
> 	EOF
> 
> 	test_set_editor "$(pwd)/fake-editor.sh"
> }
> -- snap --
> 
> Most importantly, `action` is used here:
> 
>                         sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
> 
> and I changed it to
> 
> 			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
> 
> In other words, rather than expecting the lines that are used by the
> fake editor to start with `pick`, after this patch, the tests expect the
> todo lists to start with a command consisting of lower-case ASCII
> letters (which catches `pick`, of course, but also `label`, `reset` and
> `merge`).
> 
> After this patch, the fake editor also does not try to replace whatever
> command it finds by `pick`, but it keeps it as-is instead.

Right, that's how I read it, and that part I agree with. I think my
question is this: in what case do we execute the "fakesha" case? Are we
guaranteed that when we do, action isn't "&"? "&" seems fine for the
right-hand side of a sed s-statement, but not as the beginning of a
typical line in a sequencer file.

I ask because if we're testing a failure case, we want it to fail for
the right reason (e.g., the commit doesn't exist), and not because we're
producing invalid data. If the answer in this case is, "Well, we'll
always have something else before it which will set $action properly,"
then that's fine. This is test code, so it need not be bulletproof, but
I did want to ask.

If I'm still misunderstanding something, I apologize.
Johannes Schindelin July 31, 2019, 11:25 a.m. UTC | #4
Hi brian,

On Fri, 26 Jul 2019, brian m. carlson wrote:

> On 2019-07-26 at 14:01:03, Johannes Schindelin wrote:
> > Actually, the part that uses it is not shown in the patch (one of the
> > many, many reasons why I try to discourage patch review and encourage
> > code review instead). The relevant section currently looks somewhat like
> > this:
>
> I feel like I may have communicated poorly earlier, so let me retry
> asking this in a different way.

Actually, your communication was just fine, the misunderstanding was
entirely on my side. My apologies.

> > -- snip --
> > set_fake_editor () {
> > 	write_script fake-editor.sh <<-\EOF
> > 	case "$1" in
> > 	*/COMMIT_EDITMSG)
> > 		test -z "$EXPECT_HEADER_COUNT" ||
> > 			test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" ||
> > 			test "# # GETTEXT POISON #" = "$(sed -n '1p' < "$1")" ||
> > 			exit
> > 		test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
> > 		test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
> > 		exit
> > 		;;
> > 	esac
> > 	test -z "$EXPECT_COUNT" ||
> > 		test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
> > 		exit
> > 	test -z "$FAKE_LINES" && exit
> > 	grep -v '^#' < "$1" > "$1".tmp
> > 	rm -f "$1"
> > 	echo 'rebase -i script before editing:'
> > 	cat "$1".tmp
> > 	action=pick
>
> I believe you changed this line to "action=\&".
>
> > 	for line in $FAKE_LINES; do
> > 		case $line in
> > 		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
> > 			action="$line";;
> > 		exec_*|x_*|break|b)
> > 			echo "$line" | sed 's/_/ /g' >> "$1";;
> > 		"#")
> > 			echo '# comment' >> "$1";;
> > 		">")
> > 			echo >> "$1";;
> > 		bad)
> > 			action="badcmd";;
> > 		fakesha)
> > 			echo "$action XXXXXXX False commit" >> "$1"
>
> And my question was about this line.

Right. It would append `& XXXXXXX False commit`, which is not a valid
todo command.

So something like

-			echo "$action XXXXXXX False commit" >> "$1"
+			test \& = "$action" && c=pick || c=$action
+			echo "$c XXXXXXX False commit" >>"$1"

would be needed.

However, what makes me really worried now is that our test suite did not
have a fit about this. The CI build passes the test suite on Windows,
macOS and Linux: https://github.com/gitgitgadget/git/runs/176651523.
>
> > 			action=pick;;
> > 		*)
> > 			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
> > 			action=pick;;
> > 		esac
> > 	done
> > 	echo 'rebase -i script after editing:'
> > 	cat "$1"
> > 	EOF
> >
> > 	test_set_editor "$(pwd)/fake-editor.sh"
> > }
> > -- snap --
> >
> > Most importantly, `action` is used here:
> >
> >                         sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
> >
> > and I changed it to
> >
> > 			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
> >
> > In other words, rather than expecting the lines that are used by the
> > fake editor to start with `pick`, after this patch, the tests expect the
> > todo lists to start with a command consisting of lower-case ASCII
> > letters (which catches `pick`, of course, but also `label`, `reset` and
> > `merge`).
> >
> > After this patch, the fake editor also does not try to replace whatever
> > command it finds by `pick`, but it keeps it as-is instead.
>
> Right, that's how I read it, and that part I agree with. I think my
> question is this: in what case do we execute the "fakesha" case? Are we
> guaranteed that when we do, action isn't "&"? "&" seems fine for the
> right-hand side of a sed s-statement, but not as the beginning of a
> typical line in a sequencer file.

Indeed, the sequencer should throw a real tantrum about this and not
even bother to start.

But then, the same would hold true for an obviously invalid commit hash.

> I ask because if we're testing a failure case, we want it to fail for
> the right reason (e.g., the commit doesn't exist), and not because we're
> producing invalid data.

Indeed. I have even come to the conclusion that our
`test_expect_failure` command encourages exactly this type of problem:
in the beginning, those test cases might actually be correct, but over
time they are prone to fail for all the wrong reasons, because we do not
actually test for a specific failure more, we just say that we expect
this test case to fail (and that this indicates a bug).

> If the answer in this case is, "Well, we'll always have something else
> before it which will set $action properly," then that's fine. This is
> test code, so it need not be bulletproof, but I did want to ask.

I think you are perfectly sane to question this, and to expect me to
double check.

So, double check I did. Turns out there is a single user of the
`fakesha` thing, and it is hidden deep in t3404, prefixed by
`test_must_fail`:

-- snip --
test_expect_success 'static check of bad SHA-1' '
	rebase_setup_and_clean bad-sha &&
	set_fake_editor &&
	test_must_fail env FAKE_LINES="1 2 edit fakesha 3 4 5 #" \
		git rebase -i --root 2>actual &&
	test_i18ngrep "edit XXXXXXX False commit" actual &&
	test_i18ngrep "You can fix this with .git rebase --edit-todo.." actual &&
	FAKE_LINES="1 2 4 5 6" git rebase --edit-todo &&
	git rebase --continue &&
	test E = $(git cat-file commit HEAD | sed -ne \$p)
'
-- snap --

As you can see, contrary to my expectations it does verify the output.
It *also* changes the action to `edit`, which is the reason why there is
no `&` ;-)

But I think you are correct, I should make sure that the fake editor is
still correct with respect to the `pick` command.

> If I'm still misunderstanding something, I apologize.

I am really impressed and inspired by your gentle language. Thank you
for this.

Ciao,
Dscho

Patch
diff mbox series

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 7ea30e5006..662a958575 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -44,10 +44,10 @@  set_fake_editor () {
 	rm -f "$1"
 	echo 'rebase -i script before editing:'
 	cat "$1".tmp
-	action=pick
+	action=\&
 	for line in $FAKE_LINES; do
 		case $line in
-		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
+		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
@@ -61,8 +61,8 @@  set_fake_editor () {
 			echo "$action XXXXXXX False commit" >> "$1"
 			action=pick;;
 		*)
-			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
-			action=pick;;
+			sed -n "${line}s/^[a-z][a-z]*/$action/p" < "$1".tmp >> "$1"
+			action=\&;;
 		esac
 	done
 	echo 'rebase -i script after editing:'