Re: [PATCHv2 0/5] modernize test style

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2012, 08:04 UTC
Message-ID: <7vpqcu6rfd.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1330740942-25130-1-git-send-email-tmgrennan@gmail.com>

Tom Grennan <tmgrennan@gmail.com> writes:

Show 5 quoted lines

>   t7004 (tag): modernize style
>   t5512 (ls-remote): modernize style
>   t3200 (branch): modernize style
>   t0040 (parse-options): modernize style
>   t6300 (for-each-ref): modernize style

Hmph.

We would want to keep/make the codebase uniformly conform to our style guide for longer term maintainability (this is not limited to the test scripts) but at the same time we would strongly want to avoid churning patches from interfering other patches that are still in flight that do "real work".

There are only two appropriate occasions for file-wide clean-up patches like these:

 (1) The file has not been touched for quite some time, and it is not
     expected to be touched for some time to come; or

 (2) You are going to do extensive work on the file for reasons other than
     clean-up, and nobody else is working or expected to work in the same
     area for some time to come (in other words, you _own_ the file), and
     your real work will be easier to review if it is done _after_ such a
     clean-up patch.

Let's see how active these five files are:

$ git diff --stat v1.7.9..pu --  t/t{7004,5512,3200,0040,6300}-*.sh
 t/t0040-parse-options.sh |   60 ++++++++++++++++++++++-
 t/t3200-branch.sh        |  122 ++++++++++++++++++++++++++++++++++++++++++++--
 t/t7004-tag.sh           |   96 ++++++++++++++++++++++++++++++++++++
 3 files changed, 272 insertions(+), 6 deletions(-)

Given this, I do not want to take three of your 5 patches that touch these files at this point. Please hold onto them and wait until topics that touch these test scripts graduate to 'master', and then wait a bit more until some time passes without anybody touching them, before redoing the clean-up patches.

On the other hand, we can see that 5512 has been dormant for quite a while. Note that the latter diff is against 3 cycles ago:

$ git diff --stat v1.7.6..pu --  t/t{7004,5512,3200,0040,6300}-*.sh
 t/t0040-parse-options.sh |   79 +++++++++++++++-
 t/t3200-branch.sh        |  236 +++++++++++++++++++++++++++++++++++++++++-----
 t/t6300-for-each-ref.sh  |  101 +++++++++++++++++++-
 t/t7004-tag.sh           |  128 +++++++++++++++++++------
 4 files changed, 490 insertions(+), 54 deletions(-)

so 5512 and possibly 6300 may be worth reviewing.

Thanks.