Re: [PATCH v8 2/2] commit: add a commit.verbose config variable

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 20, 2016, 17:34 UTC
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:

Show 12 quoted lines

> 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".