patchtest-tool run-command: remove dead "testsuite" code

Remove the "test-tool run-command testsuite" sub-sub-command, it has not been used since 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build job, 2020-06-26), see also the earlier 6081d3898fe (ci: retire the Azure Pipelines definition, 2020-04-11) for another phasing out of the command.

This the "testsuite" functionality here was added in be5d88e1128 (test-tool run-command: learn to run (parts of) the testsuite, 2019-10-04), and then first used in 46689317ac0 (ci: also build and test with MS Visual Studio on Azure Pipelines, 2019-10-04).

I'd started out fixing a bug in the "init" pattern here. We set "next" to "-1" and then proceed to memset() the whole struct to "0", so the two "STRING_LIST_INIT_DUP" here are also redundant to our setting of the "strdup_strings" members later on.

But rather than fix that let's just remove this whole thing, as nothing is using it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

There's a trivial conflict here with ab/config-based-hooks-base. This removes code, that series adds a couple of new (but unrelated) functions in-between some of these removed functions.

 t/helper/test-run-command.c | 151 ------------------------------------
 1 file changed, 151 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I'll leave this patch hanging in the list archive until I hear from somebody from Azure camp say that they do not need it anymore and they do not plan to use it in the future. Of course, if somebody else from outside the Windows circle is using it or plans to use it in a near future, they can raise their voice heard while we are waiting for such an Ack.

Thanks.

Hi Junio,

On Tue, 7 Sep 2021, Junio C Hamano wrote:

As I mentioned, I would love for this to stay.

The reason is that there is a long-running ticket about polishing BusyBox-w32, polishing Git for Windows' support for BusyBox, and then shipping a MinGit [*1*] version _without_ Bash and _without_ Perl. Obviously, I would want to verify that it works as intended, and that's where this `testsuite` command would come in (we already bundle the test artifacts in our CI runs, so `test-tool.exe` would be available).

Ciao, Dscho

Footnote *1*: MinGit is a subset of Git for Windows, intended to be bundled by 3rd-party applications that do not require interactive Git usage, optimized for a small disk foot print. For more details, see https://github.com/git-for-windows/git/wiki/MinGit

On Thu, Sep 09 2021, Johannes Schindelin wrote:

I'm fine with it staying, I have some cooking serieses where I'll submit some tree-wide fixes that might touch this code then (e.g. general *_INIT pattern improvements).

I did find the "without bash and without Perl" part of your above confusing, most of what this program does is a re-implementation of GNU make and t/aggregate-results.sh, and indeed your be5d88e1128 (test-tool run-command: learn to run (parts of) the testsuite, 2019-10-04) says as much, and that makes sense for the reasons stated in your be5d88e1128.

In be5d88e1128 (test-tool run-command: learn to run (parts of) the testsuite, 2019-10-04) an init pattern was added that would use TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd then set the "dup" on the two string lists. Our setting of "next" to "-1" thus did nothing, we'd reset it to "0" before using it.

Let's just use the init macro for the STRING_LIST members, we can then remove the already redundant memset().

Note that while we compile this code, there's no in-tree user for the "testsuite" target being modified here anymore, see the discussion at and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].

1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This patch is the immediate reason for why I submitted https://lore.kernel.org/git/patch-1.1-d1e464da0a9-20210906T002938Z-avarab@gmail.com/, since Johannes would prefer to keep it let's fix this init pattern.

 t/helper/test-run-command.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Hi Ævar,

the commit title is misleading: it suggests that there is a bug that needs to be fixed.

The idea of the patch, however, is to avoid redundant code, and if described that way, the patch is a lot better for it.

On Thu, 9 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

The diff does too many things, some of which are your purely personal preferences and do not actually need to be changed. This is a much more to-the-point diff:

-- snip --

On Fri, Sep 10 2021, Johannes Schindelin wrote:

I picked "confusing" because this doesn't impact the end-state of the program, it's just confusing to do:

    x = -1

Followed by:

    x = 0

Which the reader might wonder about, only to find that the initial assignment wasn't needed or used for anything.

We've been slowly converting everything to designated initializers. It seems to make sense to just do that if the line is being touched anyway.

For instance my d385784f89b (fsck.h: use designed initializers for FSCK_OPTIONS_{DEFAULT,STRICT}, 2021-03-28) was part of a series whose initial version just changed one field at a time as you're doing below, but during early review I was asked just to use designated initializers already.

But yes, it is strictly unrelated. It's a judgement call when to do cleanups on lines you touch while you're at it, v.s. turning one patch into a potential series of really tiny changes.

Then there's the change of "struct foo x, y" to "struct foo x;\nstruct foo y;\n" above. I changed that because that tends to be the usual style, it also preempts feedback / reviewers having to look at the code out-of-bounds to see what the struct looks like in the default diff -U<n>.

So, in advance addressing another type of question, or needing to describe the context, as you brought up in https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091222260.59@tvgsbejvaqbjf.bet/ :)

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Perhaps a preliminary clean-up patch is called for in such a case?

I do not think anybody can immediately see what the difference between the old -1 and the new 0 in TESTSUITE_INIT macro means in Dscho's alternative, but if we had a preliminary clean-up whose sole change is to use designated initializers, the real "to-the-point" step would become much easier to see which member that used to be initialized to -1 is now getting zero-initialized.

And yes, changing the initializer style *and* the values the members are initialized to in a same patch is much worse than sticking to the style of the unreadable original. It buries the real change in the noise.

In be5d88e1128 (test-tool run-command: learn to run (parts of) the testsuite, 2019-10-04) an init pattern was added that would use TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd then set the "dup" on the two string lists.

Our setting of "next" to "-1" thus did nothing, we'd reset it to "0" before using it. Let's set it to "0" instead, and trust the "STRING_LIST_INIT_DUP" to set "strdup_strings" appropriately for us.

Note that while we compile this code, there's no in-tree user for the "testsuite" target being modified here anymore, see the discussion at and around <nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet>[1].

1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Range-diff against v1:
1:  0aa4523ab6e ! 1:  0ddf38b47ac test-tool run-command: fix confusing init pattern
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    test-tool run-command: fix confusing init pattern
    +    test-tool run-command: fix flip-flop init pattern
     
         In be5d88e1128 (test-tool run-command: learn to run (parts of) the
         testsuite, 2019-10-04) an init pattern was added that would use
         TESTSUITE_INIT, but then promptly memset() everything back to 0. We'd
    -    then set the "dup" on the two string lists. Our setting of "next" to
    -    "-1" thus did nothing, we'd reset it to "0" before using it.
    +    then set the "dup" on the two string lists.
     
    -    Let's just use the init macro for the STRING_LIST members, we can then
    -    remove the already redundant memset().
    +    Our setting of "next" to "-1" thus did nothing, we'd reset it to "0"
    +    before using it. Let's set it to "0" instead, and trust the
    +    "STRING_LIST_INIT_DUP" to set "strdup_strings" appropriately for us.
     
         Note that while we compile this code, there's no in-tree user for the
         "testsuite" target being modified here anymore, see the discussion at
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/helper/test-run-command.c ##
    -@@ t/helper/test-run-command.c: static int task_finished(int result,
    - }
    - 
    - struct testsuite {
    --	struct string_list tests, failed;
    -+	struct string_list tests;
    -+	struct string_list failed;
    - 	int next;
    +@@ t/helper/test-run-command.c: struct testsuite {
      	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
      };
    --#define TESTSUITE_INIT \
    + #define TESTSUITE_INIT \
     -	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
    -+#define TESTSUITE_INIT { \
    -+	.tests = STRING_LIST_INIT_DUP, \
    -+	.failed = STRING_LIST_INIT_DUP, \
    -+}
    ++	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, 0, 0, 0, 0, 0, 0, 0 }
      
      static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
      		     void **task_cb)

 t/helper/test-run-command.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Hi Ævar,

On Mon, 6 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

Sure, the code is currently unused, but I had hoped to use it when BusyBox-w32 support in Git for Windows would be more robust.

Ciao, Johannes