On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Sun, Mar 20, 2016 at 7:05 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote: >> On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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)". Thanks for a detailed explanation. > 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". These changes seem nice. I will update and send the patch. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html