[v2,0/3] Work around flakiness in t5500.43

Message

Linus Arver via GitGitGadget Oct. 19, 2020, 7:35 p.m. UTC

It seems that this test became flaky only recently, although I have to admit
that I have no idea why: the involved code does not seem to have changed
recently at all. It should have been fixed by 
https://lore.kernel.org/git/[e-mail]/
, but apparently wasn't completely fixed, despite what I said in that
thread.

Changes since v1:

 * Instead of papering over the underlying cause, the patch was completely
   changed to actually fix the bug and add a proper regression test for it
   (originally, I wanted to act according to the common notion that good
   programmers are lazy, oh my, see how well that worked out for me).
 * We now specifically watch out for future bugs where incomplete sideband
   messages would be dropped inadvertently rather than being reported at EOF
   or at encountering a flush/error packet.
 * Before sending 0-length data to demultiplex_sideband(), we now make sure
   that it is not marked as a data packet; Otherwise we now complain loudly
   about a bug.

Johannes Schindelin (3):
  sideband: avoid reporting incomplete sideband messages
  sideband: report unhandled incomplete sideband messages as bugs
  sideband: add defense against packets missing a band designator

 pkt-line.c               | 11 +++++++++--
 sideband.c               |  2 +-
 t/helper/test-pkt-line.c | 23 +++++++++++++++++++++++
 t/t0070-fundamental.sh   |  6 ++++++
 4 files changed, 39 insertions(+), 3 deletions(-)


base-commit: d4a392452e292ff924e79ec8458611c0f679d6d4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-753%2Fdscho%2Funflake-t5500.43-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-753/dscho/unflake-t5500.43-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/753

Range-diff vs v1:

 1:  d977644184 < -:  ---------- t5500.43: make the check a bit more robust
 -:  ---------- > 1:  e4ba96358b sideband: avoid reporting incomplete sideband messages
 -:  ---------- > 2:  9ffcc5b78e sideband: report unhandled incomplete sideband messages as bugs
 -:  ---------- > 3:  c61e560451 sideband: add defense against packets missing a band designator

Comments

Jeff King Oct. 23, 2020, 8:50 a.m. UTC | #1

On Mon, Oct 19, 2020 at 07:35:39PM +0000, Johannes Schindelin via GitGitGadget wrote:

> Changes since v1:
> 
>  * Instead of papering over the underlying cause, the patch was completely
>    changed to actually fix the bug and add a proper regression test for it
>    (originally, I wanted to act according to the common notion that good
>    programmers are lazy, oh my, see how well that worked out for me).

Thanks for fixing this. Your explanation looks thorough and the code
looks correct. Definitely patches 1 and 2 look good to me. I left a few
comments on patch 3.

-Peff

Johannes Schindelin Oct. 26, 2020, 4:04 p.m. UTC | #2

Hi,

On Fri, 23 Oct 2020, Jeff King wrote:

> On Mon, Oct 19, 2020 at 07:35:39PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > Changes since v1:
> >
> >  * Instead of papering over the underlying cause, the patch was completely
> >    changed to actually fix the bug and add a proper regression test for it
> >    (originally, I wanted to act according to the common notion that good
> >    programmers are lazy, oh my, see how well that worked out for me).
>
> Thanks for fixing this. Your explanation looks thorough and the code
> looks correct. Definitely patches 1 and 2 look good to me. I left a few
> comments on patch 3.

Thank you.

After thinking about this for several days, I will drop patch 3/3 because
it is unclear to me how to implement what you suggested without reverting
the intention of Jonathan's patch that made `pkt-line` depend on
`sideband` instead of the other way round.

Ciao,
Dscho