From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 20 Mar 2016 17:34:57 GMT
Subject: Re: [PATCH v8 2/2] commit: add a commit.verbose config variable
Message-ID: <CAPig+cSCrdxYb=0WOiAARrrPK07PeyTKsopPfwDOLAO4m3scnQ@mail.gmail.com>
In-Reply-To: <CAFZEwPMZk9jDpi4F9WiYhv4aTqFqn3127i1AUKtNJXwG-11tyw@mail.gmail.com>

On Sun, Mar 20, 2016 at 7:05 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> However, more intuitive would probably be to create another "editor"
>> similar to the 'check-for-diff' editor this script already uses. (The
>> 'check-for-diff' editor is an obvious example about how to go about
>> such an undertaking.) You would need to invoke 'test_set_editor' in a
>> subshell for this particular test in order to avoid clobbering the
>> global editor used by this script. Or, have a preparatory patch which
>> ditches the global setting of the editor and has each test invoke
>> 'test_set_editor' as needed (and only if needed).
>
> I guess it would complicate things as sometimes I need to check
> whether it has 1 line and sometimes 2 lines.

It's not really a big complication. If I'm reading the patch
correctly, you should be able to re-use the existing check-for-diff
"editor" for the first of the new tests for which you're currently
setting a custom GIT_EDITOR. To do so, you will need to modify
check-for-diff to also count lines and assert that only one was found,
which should work for the new test and continue working for the
existing tests.

Then, you just need to create one more "editor" for the two tests
where you set custom GIT_EDITOR and expect two lines.

By the way, I forgot to mention in the review that, rather than:

    wc -l out | grep "1"

for counting lines in a test script, you'd use:

    test_line_count = 1 out

However, if you're doing it in an "editor" (which I recommend), then you'd use:

    test $(wc -l <out) = 1

And, another "by the way": You can use the write_script() function to
simplify creation of the new "editor(s)".

In fact, it would be nice to convert creation of the check-for-diff
"editor" to use write_script, as well. So, basically, I'm suggesting
splitting the current patch into three, where the first two are
preparatory cleanups:

1. use write_script() to create the check-for-diff "editor" rather
than creating it manually

2. drop the global test_set_editor() and instead have each test invoke
it as needed (and only if needed)

3. the current patch which adds new tests along with a new "editor"

Alternatively, combine #1 and #2 into a single patch which drops the
global test_set_editor() and, as an aside, also does "while here,
let's use write_script() to create 'check-for'diff' rather than doing
so manually".