Re: [PATCHv2 0/5] modernize test style

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Tom Grennan <tmgrennan@xxxxxxxxx> writes:

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


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]