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: 19970 bytes --]

Hi Junio,

me again, just quickly, because the `t0031-reftable.sh --valgrind` run
just came back with this:

-- snip --
[...]
+ git gc
==28394== error calling PR_SET_PTRACER, vgdb might block
==28399== error calling PR_SET_PTRACER, vgdb might block
==28399== error calling PR_SET_PTRACER, vgdb might block
==28404== error calling PR_SET_PTRACER, vgdb might block
==28404== Invalid read of size 1
==28404==    at 0x4C32CF2: strlen (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==28404==    by 0x551D9AD: strdup (strdup.c:41)
==28404==    by 0x39D15A: xstrdup (wrapper.c:29)
==28404==    by 0x3DA9CA: reftable_log_record_copy_from (record.c:605)
==28404==    by 0x3DB844: record_copy_from (record.c:968)
==28404==    by 0x3D64B3: merged_iter_next (merged.c:117)
==28404==    by 0x3D656B: merged_iter_next_void (merged.c:131)
==28404==    by 0x3D597D: iterator_next (iter.c:45)
==28404==    by 0x3D5AD2: reftable_iterator_next_log (iter.c:71)
==28404==    by 0x3DE037: stack_write_compact (stack.c:718)
==28404==    by 0x3DDBEA: stack_compact_locked (stack.c:632)
==28404==    by 0x3DE5AD: stack_compact_range (stack.c:847)
==28404==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==28404==
{
   <insert_a_suppression_name_here>
   Memcheck:Addr1
   fun:strlen
   fun:strdup
   fun:xstrdup
   fun:reftable_log_record_copy_from
   fun:record_copy_from
   fun:merged_iter_next
   fun:merged_iter_next_void
   fun:iterator_next
   fun:reftable_iterator_next_log
   fun:stack_write_compact
   fun:stack_compact_locked
   fun:stack_compact_range
}
==28404==
==28404== Process terminating with default action of signal 11 (SIGSEGV)
==28404==  Access not within mapped region at address 0x0
==28404==    at 0x4C32CF2: strlen (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==28404==    by 0x551D9AD: strdup (strdup.c:41)
==28404==    by 0x39D15A: xstrdup (wrapper.c:29)
==28404==    by 0x3DA9CA: reftable_log_record_copy_from (record.c:605)
==28404==    by 0x3DB844: record_copy_from (record.c:968)
==28404==    by 0x3D64B3: merged_iter_next (merged.c:117)
==28404==    by 0x3D656B: merged_iter_next_void (merged.c:131)
==28404==    by 0x3D597D: iterator_next (iter.c:45)
==28404==    by 0x3D5AD2: reftable_iterator_next_log (iter.c:71)
==28404==    by 0x3DE037: stack_write_compact (stack.c:718)
==28404==    by 0x3DDBEA: stack_compact_locked (stack.c:632)
==28404==    by 0x3DE5AD: stack_compact_range (stack.c:847)
==28404==  If you believe this happened as a result of a stack
==28404==  overflow in your program's main thread (unlikely but
==28404==  possible), you can try to increase the size of the
==28404==  main thread stack using the --main-stacksize= flag.
==28404==  The main thread stack size used in this run was 8388608.
error: reflog died of signal 11
fatal: failed to run reflog
error: last command exited with $?=128
-- snap --

But now _really_ want to take a break from this,
Dscho

On Fri, 10 Apr 2020, Johannes Schindelin wrote:

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