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== { 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 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 > 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 > --- > 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, ¤t); > > 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 > 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 > --- > 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. > > > >