Re: [PATCH v4 00/12] ci: replace our Azure Pipeline by GitHub Actions

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 16146 bytes --]

Hi Junio,

On Thu, 9 Apr 2020, Junio C Hamano wrote:

> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
>
> > Our Azure Pipeline has served us well over the course of the past year or
> > so, steadily catching issues before the respective patches hit the next
> > branch.
> >
> > There is a GitHub-native CI system now, though, called "GitHub Actions"
> > [https://github.com/features/actions] which is essentially on par with Azure
> > Pipelines as far as our needs are concerned, and it brings a couple of
> > advantages:
> >
> >  * It is substantially easier to set up than Azure Pipelines: all you need
> >    is to add the YAML-based build definition, push to your fork on GitHub,
> >    and that's it.
> >  * The syntax is a bit easier to read than Azure Pipelines'.
> >  * We get more concurrent jobs (Azure Pipelines is limited to 10 concurrent
> >    jobs).
> >
> > With this change, users also no longer need to open a PR at
> > https://github.com/git/git or at https://github.com/gitgitgadget/git just to
> > get the benefit of a CI build.
> >
> > Sample run on top of dd/ci-musl-libc with dd/test-with-busybox merged:
> > https://github.com/sgn/git/actions/runs/73179413
> >
> > Sample run when this series applied into git-for-windows
> > https://github.com/git-for-windows/git/runs/568625109
> >
> > Change from v3:
> > - Use build matrix
> > - All dependencies are install by scripts
> > - stop repeating environment variables
> > - build failure's artifacts will be uploaded
>
> I did not see any particular thing that is bad in any of the three
> series involved; do people have further comments?

FWIW I consider this work good enough that I already merged it into Git
for Windows. It should make it easier for contributors to test their
branches "privately", in their own forks, before opening a PR (most people
do not like to have relatively trivial issues pointed out by fellow human
beings, but are much more okay with machines telling them what needs to be
improved).

Please mark this up as a vote of confidence from my side.

> I am not exactly happy that these had to be queued on top of a merge
> of two topics in flight, which makes it cumbersome to deal with a
> breakage in these two other topics, though, but that would be a pain
> only until these two topics prove to be stable enough to build on.

Yes, and the fact that `ci-musl-libc` was _not_ based on top of
`test-with-busybox` makes it a bit more awkward. I, for one, had totally
missed that the latter patch series is _required_ in order to make the
former work correctly. Hunting for the cause for almost an hour until Danh
kindly pointed out that he had fixed all the issues in `test-with-busybox`
already.

> Judging from two CI runs for 'pu' ([*1*] and [*2*]), among the
> topics that are cooking, there are only a few topics that these
> tests are unhappy about.  Perhaps those on Windows can help these
> topics to pass these tests?

I would like to point out that there is only one single topic that is
cause for sorrow here, and it is the reftable one.

Before going further, let me point out that the `pu` branch has been
broken for quite a long time now, primarily because of `bugreport` and...
of course because of `reftable`. Whenever `pu` included `reftable`, the CI
builds failed. So these `reftable` problems are not a good reason, in my
mind, to hold up the GitHub workflow patches from advancing.

Seeing the short stat `35 files changed, 6719 insertions(+)` of even a
single patch in the `reftable` patch series _really_ does not entice me to
spend time even looking at it, certainly not at a time when I am short on
time, let alone to try to find time to fix it.

However, since you asked so nicely, I did start to look into it. First,
let me present you the less controversial of two patches I want to show
you:

-- snip --
From 5f42a3f6ef9cf7d90bd274e55539b145cae40e28 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 10 Apr 2020 14:23:40 +0200
Subject: [PATCH] fixup??? Reftable support for git-core

As I had already pointed out over a month ago in
https://github.com/gitgitgadget/git/pull/539#issuecomment-589157008 this
C code violates the C standard, and MS Visual C is not as lenient as
GCC/clang on it: `struct`s cannot be initialized with `= {}`.

Compile errors aside, while this code conforms to the C syntax, it feels
more like Java when it initializes e.g. `struct object_id` only to
_immediately_ overwrite the contents.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 refs/reftable-backend.c | 52 ++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 6e845e9c649..20c94bb403b 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -31,7 +31,7 @@ static void clear_reftable_log_record(struct reftable_log_record *log)
 static void fill_reftable_log_record(struct reftable_log_record *log)
 {
 	const char *info = git_committer_info(0);
-	struct ident_split split = {};
+	struct ident_split split = { NULL };
 	int result = split_ident_line(&split, info, strlen(info));
 	int sign = 1;
 	assert(0 == result);
@@ -230,7 +230,7 @@ static int reftable_transaction_abort(struct ref_store *ref_store,
 static int reftable_check_old_oid(struct ref_store *refs, const char *refname,
 				  struct object_id *want_oid)
 {
-	struct object_id out_oid = {};
+	struct object_id out_oid;
 	int out_flags = 0;
 	const char *resolved = refs_resolve_ref_unsafe(
 		refs, refname, RESOLVE_REF_READING, &out_oid, &out_flags);
@@ -287,14 +287,14 @@ static int write_transaction_table(struct reftable_writer *writer, void *arg)
 		log->message = u->msg;

 		if (u->flags & REF_HAVE_NEW) {
-			struct object_id out_oid = {};
+			struct object_id out_oid;
 			int out_flags = 0;
 			/* Memory owned by refs_resolve_ref_unsafe, no need to
 			 * free(). */
 			const char *resolved = refs_resolve_ref_unsafe(
 				transaction->ref_store, u->refname, 0, &out_oid,
 				&out_flags);
-			struct reftable_ref_record ref = {};
+			struct reftable_ref_record ref = { NULL };
 			ref.ref_name =
 				(char *)(resolved ? resolved : u->refname);
 			log->ref_name = ref.ref_name;
@@ -376,8 +376,8 @@ static int write_delete_refs_table(struct reftable_writer *writer, void *argv)
 	}

 	for (int i = 0; i < arg->refnames->nr; i++) {
-		struct reftable_log_record log = {};
-		struct reftable_ref_record current = {};
+		struct reftable_log_record log = { NULL };
+		struct reftable_ref_record current = { NULL };
 		fill_reftable_log_record(&log);
 		log.message = xstrdup(arg->logmsg);
 		log.new_hash = NULL;
@@ -455,10 +455,10 @@ static int write_create_symref_table(struct reftable_writer *writer, void *arg)
 	}

 	{
-		struct reftable_log_record log = {};
-		struct object_id new_oid = {};
-		struct object_id old_oid = {};
-		struct reftable_ref_record current = {};
+		struct reftable_log_record log = { NULL };
+		struct object_id new_oid;
+		struct object_id old_oid;
+		struct reftable_ref_record current = { NULL };
 		reftable_stack_read_ref(create->refs->stack, create->refname, &current);

 		fill_reftable_log_record(&log);
@@ -513,7 +513,7 @@ static int write_rename_table(struct reftable_writer *writer, void *argv)
 {
 	struct write_rename_arg *arg = (struct write_rename_arg *)argv;
 	uint64_t ts = reftable_stack_next_update_index(arg->stack);
-	struct reftable_ref_record ref = {};
+	struct reftable_ref_record ref = { NULL };
 	int err = reftable_stack_read_ref(arg->stack, arg->oldname, &ref);

 	if (err) {
@@ -531,7 +531,7 @@ static int write_rename_table(struct reftable_writer *writer, void *argv)
 	ref.update_index = ts;

 	{
-		struct reftable_ref_record todo[2] = {};
+		struct reftable_ref_record todo[2] = { { NULL } };
 		todo[0].ref_name = (char *)arg->oldname;
 		todo[0].update_index = ts;
 		/* leave todo[0] empty */
@@ -545,7 +545,7 @@ static int write_rename_table(struct reftable_writer *writer, void *argv)
 	}

 	if (ref.value != NULL) {
-		struct reftable_log_record todo[2] = {};
+		struct reftable_log_record todo[2] = { { NULL } };
 		fill_reftable_log_record(&todo[0]);
 		fill_reftable_log_record(&todo[1]);

@@ -688,12 +688,12 @@ reftable_for_each_reflog_ent_newest_first(struct ref_store *ref_store,
 					  const char *refname,
 					  each_reflog_ent_fn fn, void *cb_data)
 {
-	struct reftable_iterator it = {};
+	struct reftable_iterator it = { NULL };
 	struct git_reftable_ref_store *refs =
 		(struct git_reftable_ref_store *)ref_store;
 	struct reftable_merged_table *mt = NULL;
 	int err = 0;
-	struct reftable_log_record log = {};
+	struct reftable_log_record log = { NULL };

 	if (refs->err < 0) {
 		return refs->err;
@@ -712,8 +712,8 @@ reftable_for_each_reflog_ent_newest_first(struct ref_store *ref_store,
 		}

 		{
-			struct object_id old_oid = {};
-			struct object_id new_oid = {};
+			struct object_id old_oid;
+			struct object_id new_oid;
 			const char *full_committer = "";

 			hashcpy(old_oid.hash, log.old_hash);
@@ -744,7 +744,7 @@ reftable_for_each_reflog_ent_oldest_first(struct ref_store *ref_store,
 					  const char *refname,
 					  each_reflog_ent_fn fn, void *cb_data)
 {
-	struct reftable_iterator it = {};
+	struct reftable_iterator it = { NULL };
 	struct git_reftable_ref_store *refs =
 		(struct git_reftable_ref_store *)ref_store;
 	struct reftable_merged_table *mt = NULL;
@@ -760,7 +760,7 @@ reftable_for_each_reflog_ent_oldest_first(struct ref_store *ref_store,
 	err = reftable_merged_table_seek_log(mt, &it, refname);

 	while (err == 0) {
-		struct reftable_log_record log = {};
+		struct reftable_log_record log = { NULL };
 		err = reftable_iterator_next_log(it, &log);
 		if (err != 0) {
 			break;
@@ -780,8 +780,8 @@ reftable_for_each_reflog_ent_oldest_first(struct ref_store *ref_store,

 	for (int i = len; i--;) {
 		struct reftable_log_record *log = &logs[i];
-		struct object_id old_oid = {};
-		struct object_id new_oid = {};
+		struct object_id old_oid;
+		struct object_id new_oid;
 		const char *full_committer = "";

 		hashcpy(old_oid.hash, log->old_hash);
@@ -903,8 +903,8 @@ static int reftable_reflog_expire(struct ref_store *ref_store,
 	struct reflog_expiry_arg arg = {
 		.refs = refs,
 	};
-	struct reftable_log_record log = {};
-	struct reftable_iterator it = {};
+	struct reftable_log_record log = { NULL };
+	struct reftable_iterator it = { NULL };
 	int err = 0;
 	if (refs->err < 0) {
 		return refs->err;
@@ -917,8 +917,8 @@ static int reftable_reflog_expire(struct ref_store *ref_store,
 	}

 	while (1) {
-		struct object_id ooid = {};
-		struct object_id noid = {};
+		struct object_id ooid;
+		struct object_id noid;

 		int err = reftable_iterator_next_log(it, &log);
 		if (err < 0) {
@@ -950,7 +950,7 @@ static int reftable_read_raw_ref(struct ref_store *ref_store,
 {
 	struct git_reftable_ref_store *refs =
 		(struct git_reftable_ref_store *)ref_store;
-	struct reftable_ref_record ref = {};
+	struct reftable_ref_record ref = { NULL };
 	int err = 0;
 	if (refs->err < 0) {
 		return refs->err;
-- snap --

This patch should fix the `vs-build` job in the Azure Pipeline as well as
in the GitHub workflow.

However, it does _not_ fix the test failure on Windows. When I tried to
investigate this, I wanted to compare the results between Windows and
Linux (WSL, of course, it is a major time saver for me these days because
I don't have to power up a VM, and I can access WSL files from Windows and
vice versa), and it turns out that the `000000000002-000000000002.ref`
file is different, it even has different sizes (242 bytes on Windows
instead of 268 bytes on Linux), and notably it contains the string `HEAD`
on Windows and `refs/heads/master` on Linux, but not vice versa.

So I dug a bit deeper and was stopped rudely by the fact that the
t0031-reftable.sh script produces different output every time it runs.
Because it does not even use `test_commit`.

Therefore, let me present you with this patch (whose commit message
conveys a rather alarming indication that this endeavor of fixing
`reftable` could become a major time sink):

-- snip -
From 6ba47e70a2eb8efe2116c12eb950ddb90c473d11 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 10 Apr 2020 16:10:53 +0200
Subject: [PATCH] fixup??? Reftable support for git-core

The test for the reftable functionality should use the convenience
functions we provide for test scripts. Using `test_commit` in particular
does help with reproducible output (otherwise the SHA-1s will change
together with the time the tests were run).

Currently, this here seemingly innocuous change causes quite a few
warnings throughout the test, though, e.g. this slur of warnings when
committing the last commit in the test script:

	warning: bad replace ref name: e
	warning: bad replace ref name: ber-1
	warning: bad replace ref name: ber-2
	warning: bad replace ref name: ber-3
	warning: bad replace ref name: ber-4
	warning: bad replace ref name: ber-5
	warning: bad replace ref name: ber-6
	warning: bad replace ref name: ber-7
	warning: bad replace ref name: ber-8
	warning: bad replace ref name: ber-9

This is notably _not_ a problem that was introduced by this here patch,
of course, but a very real concern about the reftable patches, most
likely the big one that introduces the reftable library in one fell
swoop.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0031-reftable.sh | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/t/t0031-reftable.sh b/t/t0031-reftable.sh
index 3ebf13c2f42..4bc7bd8167f 100755
--- a/t/t0031-reftable.sh
+++ b/t/t0031-reftable.sh
@@ -8,28 +8,26 @@ test_description='reftable basics'
 . ./test-lib.sh

 test_expect_success 'basic operation of reftable storage' '
-	git init --ref-storage=reftable repo && (
-	cd repo &&
-	echo "hello" >world.txt &&
-	git add world.txt &&
-	git commit -m "first post" &&
-	test_write_lines HEAD refs/heads/master >expect &&
+	rm -rf .git &&
+	git init --ref-storage=reftable &&
+	mv .git/hooks .git/hooks-disabled &&
+	test_commit file &&
+	test_write_lines HEAD refs/heads/master refs/tags/file >expect &&
 	git show-ref &&
 	git show-ref | cut -f2 -d" " > actual &&
 	test_cmp actual expect &&
 	for count in $(test_seq 1 10)
 	do
-		echo "hello" >>world.txt
-		git commit -m "number ${count}" world.txt ||
+		test_commit "number $count" file.t $count number-$count ||
 		return 1
 	done &&
 	git gc &&
-	nfiles=$(ls -1 .git/reftable | wc -l ) &&
-	test ${nfiles} = "2" &&
+	ls -1 .git/reftable >table-files &&
+	test_line_count = 2 table-files &&
 	git reflog refs/heads/master >output &&
 	test_line_count = 11 output &&
 	grep "commit (initial): first post" output &&
-	grep "commit: number 10" output )
+	grep "commit: number 10" output
 '

 test_done
-- snap --

While I am very happy with the post-image of this diff, I am super unhappy
about the output of it. It makes me believe that this `reftable` patch
series is in serious need of being "incrementalized" _after the fact_.
Otherwise it will be simply impossible to build enough confidence in the
correctness of it, especially given the fact that it obviously does some
incorrect things right now (see the "bad replace ref name" warning
mentioned above).

I'll take a break from this now, but I would like to encourage you to
apply both patches as `SQUASH???` on top of `hn/reftable` for the time
being.

Ciao,
Dscho

>
>
> [References]
>
> *1* https://github.com/git/git/actions/runs/74687673 is 'pu' with
>  all cooking topics.
>
> *2* https://github.com/git/git/actions/runs/74741625 is 'pu' with
>  some topics excluded.
>
>