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

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