Re: [PATCH v4 1/5] rebase -i: add --ignore-whitespace flag

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Phillip,

On Mon, 1 Jun 2020, Phillip Wood wrote:

> On 29/05/2020 03:38, Johannes Schindelin wrote:
> >
> > On Wed, 27 May 2020, Phillip Wood wrote:
> >
> >> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> >>
> >> [...]
> >> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> >> new file mode 100755
> >> index 0000000000..fb5e747e86
> >> --- /dev/null
> >> +++ b/t/t3436-rebase-more-options.sh
> >> @@ -0,0 +1,86 @@
> >> +#!/bin/sh
> >> +#
> >> +# Copyright (c) 2019 Rohit Ashiwal
> >> +#
> >> +
> >> +test_description='tests to ensure compatibility between am and interactive backends'
> >> +
> >> +. ./test-lib.sh
> >> +
> >> +. "$TEST_DIRECTORY"/lib-rebase.sh
> >> +
> >> +# This is a special case in which both am and interactive backends
> >> +# provide the same output. It was done intentionally because
> >> +# both the backends fall short of optimal behaviour.
> >> +test_expect_success 'setup' '
> >> +	git checkout -b topic &&
> >> +	q_to_tab >file <<-\EOF &&
> >> +	line 1
> >> +	Qline 2
> >> +	line 3
> >> +	EOF
> >> +	git add file &&
> >> +	git commit -m "add file" &&
> >> +	cat >file <<-\EOF &&
> >> +	line 1
> >> +	new line 2
> >> +	line 3
> >> +	EOF
> >> +	git commit -am "update file" &&
> >> +	git tag side &&
> >> +
> >> +	git checkout --orphan master &&
> >> +	sed -e "s/^|//" >file <<-\EOF &&
> >> +	|line 1
> >> +	|        line 2
> >> +	|line 3
> >> +	EOF
> >> +	git add file &&
> >> +	git commit -m "add file" &&
> >> +	git tag main
> >> +'
> >
> > The file contents are repeated in an awfully repetitive manner. That not
> > only makes things a bit hard to read, it also makes it all too easy to
> > slip in bugs by mistake. How about something like this instead?
> >
> > 	test_commit file &&
> >
> > 	test_write_lines line1 Qline2 line3 >templ &&
> >
> > 	q_to_tab <templ >file.t &&
> > 	git commit -m tab file.t &&
> >
> > 	sed "s/Q/new /" <templ >file.t &&
> > 	git commit -m new file.t &&
> > 	git tag side &&
> >
> > 	git checkout file -- &&
> > 	sed "s/Q/        /" <templ >file.t &&
> > 	git commit -m spaces file.t
>
> I'm not totally convinced by this, I'd prefer to be able to read the
> contents rather than having to work out what sed is doing. What about
>
> test_write_lines "line 1" "	line 2" "line 3" >file &&
> add and commit
> test_write_lines "line 1" "new line 2" "line 3" >file &&
> commit and tag
> test_write_lines "line 1" "  line 2" "line 3" >file &&
> commit and tag
>
> It does not get rid of the repetition but it does dispense with having
> the work out what sed and q_to_tab are doing

Sure. Your version is still tons easier to parse for a human being.

> > 	git diff --exit-code main
> >
> >> +	test_must_fail git rebase --merge main side &&
> >> +	git rebase --abort &&
> >> +	git rebase --merge --ignore-whitespace main side &&
> >> +	test_cmp expect file
> >> +'
> >> +
> >> +test_expect_success '--ignore-whitespace is remembered when continuing' '
> >> +	cat >expect <<-\EOF &&
> >> +	line 1
> >> +	new line 2
> >> +	line 3
> >> +	EOF
> >> +	(
> >> +		set_fake_editor &&
> >> +		FAKE_LINES="break 1" git rebase -i --ignore-whitespace main side
> >> +	) &&
> >> +	git rebase --continue &&
> >> +	test_cmp expect file
> >
> > It is a bit funny to see these two invocations _specifically_ pulled out
> > from the subshell, that's not how we do things in other test scripts:
> > instead, we run all the Git commands _inside_ the subshell, and all the
> > verifications after the subshell.
>
> The idea was to only set the variable where it is used.

I understand that, but I think that it would be better to follow the
existing pattern rather than introducing an inconsistent second one.

Thanks,
Dscho