Re: [PATCH 1/2] t5351: avoid relying on `core.fsyncMethod = batch` to be supported

["brian m. carlson" <sandals@crustytoothpaste.net>]

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

On 2022-07-29 at 16:07:46, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > On FreeBSD, this mode is not supported. But since 3a251bac0d1a (trace2:
> > only include "fsync" events if we git_fsync(), 2022-07-18) t5351 will
> > fail if this mode is unsupported.
> >
> > Let's address this in the minimal fashion, by detecting that that mode
> > is unsupported and expecting a different count of hardware flushes in
> > that case.
> >
> > This fixes the CI/PR builds on FreeBSD again.
> >
> > Note: A better way would be to test only what is relevant in t5351.6
> > "unpack big object in stream (core.fsyncmethod=batch)" again instead of
> > blindly comparing the output against some exact text. But that would
> > pretty much revert the idea of above-mentioned commit, and that commit
> > was _just_ accepted into Git's main branch so one must assume that it
> > was intentional.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  bulk-checkin.c                  |  2 ++
> >  t/t5351-unpack-large-objects.sh | 10 ++++++++--
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> I am inclined to take this as-is for now.

I think this patch is a strict improvement over the status quo, despite
what I'm going to mention below.

> But it inherits from 3a251bac0d1a the general "we should be able to
> count the value" expectation, which may not be the best approach to
> run this test; asking Acks from those originally involved in this
> area and possibly ideas to test this in a more robust way.

While it may not matter in this test, I noticed that the metrics we use
need not be accurate in multi-threaded programs.  We use integers of
static intmax_t which isn't necessarily atomic across threads.  Even if
we assumed word-sized increments were atomic[0], this type might be 64
bits on a 32-bit system.

I am aware that we don't use threads in many places, but in general we
should be safely able to assume that we can perform an fsync on any
thread without thread safety problems because that's the assumption a
reasonable Unix programmer would make.  Even if it's not a problem now,
it could well be in the future, and we should either fix this (say, with
C11 atomic integers[1]) or put a note on the metrics functions that they
may be wrong and stop using them in tests.

I would, in general, prefer that if we add wrappers that wrap common
Unix functions we ensure that they provide the same guarantees as the
common Unix functions we're wrapping.  I realize I may sound fussy, but
I've been fixing giant thread-safety problems recently and it's not fun.

[0] This is not, in general, a safe assumption to make, since most RISC
architectures are load-store.
[1] This would necessitate moving to C11, which is fine with me (and
already required on Windows), but others may have objections to.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]