On Sat, 27 Apr 2019 22:58:23 +0100 Harald van Dijk <harald@xxxxxxxxxxx> wrote:
HvD> > HvD> Please keep in mind that even without this, you would not have
HvD> > HvD> deterministic output. The commands in a pipeline could be output in any
HvD> > HvD> order, even if each command would be output on its own line.
HvD> >
HvD> > In theory, it seems like you're right and it should be possible, but in
HvD> > practice it just doesn't seem to ever happen, i.e. the output is always in
HvD> > the same left-to-right order.
[...]
HvD> It seems to mostly work that way in bash, and I have no dug into the
HvD> reason for that either. You tested ksh and zsh as well. In ksh and zsh,
HvD> I do commonly see variations in the order in which the lines are
HvD> printed, and with patched dash, I see the same variations.
HvD>
HvD> Rarely, I see out-of-order prints even in bash, so it does seem like
HvD> it's just a coincidence as a result of the way of bash is written, not
HvD> something that bash actively attempts to guarantee.
Yes, I can confirm that I also see "out of order" output using your test,
thanks. In 10 runs of the script it took 1624 iterations to see it once,
but only 9 one other time, so it's clearly not as vanishingly rare as I
thought neither. This is somewhat dispiriting, but I guess we have no
choice but to live with it. At least the logs differing by line order only
can be comfortably dealt with using something like "git -c
color.diff.{new,old}Moved=white diff --no-index --color-moved=plain" which
will only highlight the really different lines. So the patch keeping the
xtrace lines unbroken would still be useful.
HvD> > HvD> As a proof of concept that should clearly not be committed in its
HvD> > HvD> current form, please see the attached patch.
HvD> >
HvD> > Thanks, this is indeed much simpler than what I had in mind and I can
HvD> > confirm that it does work.
HvD> >
HvD> > What prevents this patch from being applied in the current form exactly?
HvD>
HvD> I removed a #ifdef FLUSHERR which suggests to me this is supposed to be
HvD> optional, not unconditional.
I'm not sure what the original intention was here (and "git blame" doesn't
help as these lines date from the "Initial import" commit), but flushing an
unbuffered stream doesn't make sense anyhow, so it didn't matter whether it
was optional or unconditional before. If the stream becomes buffered, we
just have to flush it now, however. I.e. I don't see how could the patch be
possibly improved in this area.
HvD> The macro OUTBUFSIZ should be renamed if it is no longer just the size
HvD> of the "output" buffer.
OK, sure, but this is a trivial change and the most important question
here is how the constant should be called, which is rather subjective, so
I'd rather let you choose the name you prefer for it.
HvD> I only touched the case where USE_GLIBC_STDIO is undefined. (That said,
HvD> the case where USE_GLIBC_STDIO is defined is already broken, so this may
HvD> be okay.)
Yes, defining it (which is not done anywhere currently) would result in
preverrout becoming undefined...
HvD> It wastes memory: the code should be re-worked so that output and
HvD> preverrout share the same buffer, given that there can never be pending
HvD> output for one when the other is being written to.
I'm a bit worried about this one, it doesn't seem completely obvious to me
that the 2 objects can't be used at the same time. So sharing the buffer
between them seems like a rather dangerous thing to do and it's certainly
not going to be pretty neither ("struct output" is clearly supposed to have
its own buffer and not use a shared one, as written currently). Isn't it an
overkill to do all this just to save BUFSIZ bytes _only_ when xtrace option
is used (so probably not affecting 99.99% of shell invocations)?
HvD> (This may apply to errout as well, if someone wants to enable
HvD> buffering for that.)
But errout is not supposed to be buffered, is it?
Anyhow, I'd still like to see this patch applied to dash (and hopefully
appearing in the next Debian version), but I'm not sure if I can do
anything to help with this. Please let me know if you think I can, thanks,
VZ
Attachment:
pgpTd4uwaspuc.pgp
Description: PGP signature
