All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Andreas Schwab <schwab@suse.de>, Jeff King <peff@peff.net>,
	Johannes Sixt <j6t@kdbg.org>,
	git@vger.kernel.org
Subject: Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early
Date: Thu, 10 Nov 2016 14:30:36 -0800	[thread overview]
Message-ID: <xmqq60nv55o3.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1611102254340.24684@virtualbox> (Johannes Schindelin's message of "Thu, 10 Nov 2016 22:55:35 +0100 (CET)")

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> OK.  sleep.pid is a reasonable easy-to-access side effect we can
>> observe to make sure that the sleep-one-second merge driver was
>> indeed invoked, which was missing from the earlier round.
>
> No, this is incorrect. The condition that we need to know applies is that
> the script is still running, and blocking if the bug reappears.

OK, I see what you are saying, and I see a few things wrong in here:

 * First, the test is titled in a misleading way.  In the context of
   a patch that was titled ad65f7e3b7 ("t6026-merge-attr: child
   processes must not inherit index.lock handles", 2016-08-18), it
   might have been clear enough to say "does not lock index", but
   the sleeping is to make sure that we would notice if the fd to
   the index.lock leaked to the child process by mistake, and the
   way to do so is that the child arranges the leaked fd to be kept
   open after it exits (by spawning "sleep").  The test was never
   about "does not lock index" (the driver does not take any lock by
   itself in the first place).

 * There are three possible outcome from this test:

   - 'git merge' fails.

     This is expected to happen only on Windows and if the code gets
     broken and starts leaking the fd.

   - 'git merge' finishes correctly, the sleep is still running when
     test_when_finished goes to cull it.

     In this case, we KNOW there wasn't any fd leak IF we are on
     Windows where a leaked FD would not allow 'git merge' to
     succeed.  But on other platforms, fd leak that may cause
     trouble for Windows friends will not be caught.

   - 'git merge' finishes correctly, the sleep is no longer
     running because the machine was heavily loaded; a workaround is
     to tolerate failure of culling it.

     In this case, we cannot tell anything from the test.  Even if
     the fd was leaked, 'git merge' may have succeeded even on
     Windows.

As everybody knows there is no appropriate timeout value that is
good for everybody.  I wonder if we can replace the sleep 1 with
something like

	( while sleep 3600; do :; done ) &

so that leaked fd will be kept even in any heavily loaded
environment instead?


  reply	other threads:[~2016-11-10 22:30 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-08 17:03 [PATCH] t6026-merge-attr: don't fail if sleep exits early Andreas Schwab
2016-11-08 20:05 ` Jeff King
2016-11-09 13:47   ` Johannes Schindelin
2016-11-09 14:36     ` Andreas Schwab
2016-11-09 15:31       ` Jeff King
2016-11-10  8:31         ` [PATCH v2] " Andreas Schwab
2016-11-10 19:20           ` Junio C Hamano
2016-11-10 21:55             ` Johannes Schindelin
2016-11-10 22:30               ` Junio C Hamano [this message]
2016-11-10 22:35                 ` Jeff King
2016-11-10 22:55                   ` Junio C Hamano
2016-11-10 23:03                     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq60nv55o3.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=peff@peff.net \
    --cc=schwab@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.