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?
next prev parent 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.