From: Daniel Harding <dharding@living180.net>
Date: Mon, 09 Jul 2018 18:22:22 GMT
Subject: Re: [PATCH 2/2] t3430: update to test with custom commentChar
Message-ID: <13a876a2-7fbc-de05-2e82-814c782e8a80@living180.net>
In-Reply-To: <nycvar.QRO.7.76.6.1807090944400.75@tvgsbejvaqbjf.bet>

Hi Johannes,

On Mon, 09 Jul 2018 at 10:52:13 +0300, Johannes Schindelin wrote:
> Hi Brian,
> 
> On Sun, 8 Jul 2018, brian m. carlson wrote:
> 
>> On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote:
>>> Signed-off-by: Daniel Harding <dharding@living180.net>
>>
>> I think maybe, as you suggested, a separate test for this would be
>> beneficial.  It might be as simple as modifying 'script-from-scratch' by
>> doing "sed 's/#/>/'".
> 
> It might be even simpler if you come up with a new "fake editor" to merely
> copy the todo list, then run a rebase without overridden
> commentChar, then one with overridden commentChar, then pipe the todo list
> of the first through that `sed` call:
> 
> 
>          write_script copy-todo-list.sh <<-\EOF &&
>          cp "$1" todo-list.copy
>          EOF
> 	test_config sequence.editor \""$PWD"/copy-todo-list.sh\" &&
> 	git rebase -r <base> &&
> 	sed "s/#/%/" <todo-list.copy >expect &&
> 	test_config core.commentChar % &&
> 	git rebase -r <base> &&
> 	test_cmp expect todo-list.copy

Indeed, as I thought about it more, using a "no-op" todo editor seemed 
like a good approach.  Thanks for giving me a head start - I'll play 
with that and try to get a new patch with an improved test posted in the 
next couple of days.

One question about my original patch - there I had replaced a "grep -v" 
call with a "git stripspace" call in the 'generate correct todo list' 
test.  Is relying on "git stripspace" in a test acceptable, or should 
external text manipulation tools like grep, sed etc. be preferred?

Thanks,

Daniel Harding