Re: [PATCH 10/12] t/lib-rebase: prepare for testing `git rebase --rebase-merges`

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 10/12] t/lib-rebase: prepare for testing `git rebase --rebase-merges`
Date: Fri, 26 Jul 2019 16:01:03 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1907261555410.21907@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190726074317.GD9319@genre.crustytoothpaste.net>

[-- Attachment #1: Type: text/plain, Size: 3694 bytes --]

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