From: Junio C Hamano <hidden> Date: 2021-09-07 20:26:45
Ævar Arnfjörð Bjarmason [off-list ref] writes:
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.
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.
From: Johannes Schindelin <hidden> Date: 2021-09-09 11:27:02
Hi Junio,
On Tue, 7 Sep 2021, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
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.
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.
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
Hi Junio,
On Tue, 7 Sep 2021, Junio C Hamano wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
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.
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.
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
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 [off-list ref][1].
1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
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(-)
From: Johannes Schindelin <hidden> Date: 2021-09-10 11:23:49
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:
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 [off-list ref][1].
1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
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.
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 --
Hi Ævar,
the commit title is misleading: it suggests that there is a bug that needs
to be fixed.
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.
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:
quoted
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 [off-list ref][1].
1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
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.
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:
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/
:)
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 [off-list ref][1].
1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Range-diff against v1:
1: 0aa4523ab6e ! 1: 0ddf38b47ac test-tool run-command: fix confusing init pattern
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## 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 [off-list ref]
## 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(-)