From: Junio C Hamano <gitster@pobox•com>
To: "Torsten Bögershausen" <tboegi@web•de>
Cc: "Lars Schneider" <larsxschneider@gmail•com>,
git <git@vger•kernel.org>, "Jeff King" <peff@peff•net>,
"Stefan Beller" <sbeller@google•com>,
"Jakub Narębski" <jnareb@gmail•com>,
"Martin-Louis Bright" <mlbright@gmail•com>,
ramsay@ramsayjones•plus.com
Subject: Re: [PATCH v8 00/11] Git filter protocol
Date: Thu, 29 Sep 2016 09:57:57 -0700 [thread overview]
Message-ID: <xmqqk2duhcdm.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <f7a4f828-bb1d-0ffa-e369-3b4fa476d9e5@web.de> ("Torsten Bögershausen"'s message of "Thu, 29 Sep 2016 13:57:47 +0200")
Torsten Bögershausen <tboegi@web•de> writes:
>> 1) Git exits
>> 2) The filter process receives EOF and prints "STOP" to the log
>> 3) t0021 checks the content of the log
>>
>> Sometimes 3 happened before 2 which makes the test fail.
>> (Example: https://travis-ci.org/git/git/jobs/162660563 )
>>
>> I added a this to wait until the filter process terminates:
>>
>> +wait_for_filter_termination () {
>> + while ps | grep -v grep | grep -F "/t0021/rot13-filter.pl" >/dev/null 2>&1
>> + do
>> + echo "Waiting for /t0021/rot13-filter.pl to finish..."
>> + sleep 1
>> + done
>> +}
>>
>> Does this look OK to you?
> Do we need the ps at all ?
> How about this:
>
> +wait_for_filter_termination () {
> + while ! grep "STOP" LOGFILENAME >/dev/null
> + do
> + echo "Waiting for /t0021/rot13-filter.pl to finish..."
> + sleep 1
> + done
> +}
Running "ps" and grepping for a command is not suitable for script
to reliably tell things, so it is out of question. Compared to
that, your version looks slightly better, but what if the machinery
that being tested, i.e. the part that drives the filter process, is
buggy or becomes buggy and causes the filter process that writes
"STOP" to die before it actually writes that string?
I have a feeling that the machinery being tested needs to be fixed
so that the sequence is always be:
0) Git spawns the filter process, as it needs some contents to
be filtered.
1) Git did everything it needed to do and decides that is time
to go.
2) Filter process receives EOF and prints "STOP" to the log.
3) Git waits until the filter process finishes.
4) t0021, after Git finishes, checks the log.
Repeated sleep combined with grep is probably just sweeping the real
problem under the rug. Do we have enough information to do the
above?
An inspiration may be in the way we centrally clean all tempfiles
and lockfiles before exiting. We have a central registry of these
files that need cleaning up and have a single atexit(3) handler to
clean them up. Perhaps we need a registry that filter processes
spawned by the mechanism Lars introduces in this series, and have an
atexit(3) handler that closes the pipe to them (which signals the
filters that it is time for them to go) and wait(2) on them, or
something? I do not think we want any kill(2) to be involved in
this clean-up procedure, but I do think we should wait(2) on what we
spawn, as long as these processes are meant to be shut down when the
main process of Git exits (this is different from things like
credential-cache daemon where they are expected to persist and meant
to serve multiple Git processes).
next prev parent reply other threads:[~2016-09-29 16:58 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-20 19:02 [PATCH v8 00/11] Git filter protocol larsxschneider
2016-09-20 19:02 ` [PATCH v8 01/11] pkt-line: rename packet_write() to packet_write_fmt() larsxschneider
2016-09-24 21:14 ` Jakub Narębski
2016-09-26 18:49 ` Lars Schneider
2016-09-28 23:15 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 02/11] pkt-line: extract set_packet_header() larsxschneider
2016-09-24 21:22 ` Jakub Narębski
2016-09-26 18:53 ` Lars Schneider
2016-09-20 19:02 ` [PATCH v8 03/11] run-command: move check_pipe() from write_or_die to run_command larsxschneider
2016-09-24 22:12 ` Jakub Narębski
2016-09-26 16:13 ` Lars Schneider
2016-09-26 16:21 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 04/11] pkt-line: add packet_write_fmt_gently() larsxschneider
2016-09-24 22:27 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 05/11] pkt-line: add packet_flush_gently() larsxschneider
2016-09-24 22:56 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 06/11] pkt-line: add packet_write_gently() larsxschneider
2016-09-25 11:26 ` Jakub Narębski
2016-09-26 19:21 ` Lars Schneider
2016-09-27 8:39 ` Jeff King
2016-09-27 19:33 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams larsxschneider
2016-09-25 13:46 ` Jakub Narębski
2016-09-26 20:23 ` Lars Schneider
2016-09-27 8:14 ` Lars Schneider
2016-09-27 9:00 ` Jeff King
2016-09-27 12:10 ` Lars Schneider
2016-09-27 12:13 ` Jeff King
2016-09-20 19:02 ` [PATCH v8 08/11] convert: quote filter names in error messages larsxschneider
2016-09-25 14:03 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 09/11] convert: modernize tests larsxschneider
2016-09-25 14:43 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 10/11] convert: make apply_filter() adhere to standard Git error handling larsxschneider
2016-09-25 14:47 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 11/11] convert: add filter.<driver>.process option larsxschneider
2016-09-26 22:41 ` Jakub Narębski
2016-09-30 18:56 ` Lars Schneider
2016-10-04 20:50 ` Jakub Narębski
2016-10-06 13:16 ` Lars Schneider
2016-09-27 15:37 ` Jakub Narębski
2016-09-30 19:38 ` Lars Schneider
2016-10-04 21:00 ` Jakub Narębski
2016-10-06 21:27 ` Lars Schneider
2016-09-28 23:14 ` Jakub Narębski
2016-10-01 15:34 ` Lars Schneider
2016-10-04 21:34 ` Jakub Narębski
2016-09-28 21:49 ` [PATCH v8 00/11] Git filter protocol Junio C Hamano
2016-09-29 10:28 ` Lars Schneider
2016-09-29 11:57 ` Torsten Bögershausen
2016-09-29 16:57 ` Junio C Hamano [this message]
2016-09-29 17:57 ` Lars Schneider
2016-09-29 18:18 ` Torsten Bögershausen
2016-09-29 18:38 ` Johannes Sixt
2016-09-29 21:27 ` Junio C Hamano
2016-10-01 18:59 ` Lars Schneider
2016-10-01 20:48 ` Jakub Narębski
2016-10-03 17:13 ` Lars Schneider
2016-10-04 19:04 ` Jakub Narębski
2016-10-06 13:13 ` Lars Schneider
2016-10-06 16:01 ` Jeff King
2016-10-06 17:17 ` Junio C Hamano
2016-10-03 17:02 ` Junio C Hamano
2016-10-03 17:35 ` Lars Schneider
2016-10-04 12:11 ` Jeff King
2016-10-04 16:47 ` Junio C Hamano
2016-09-29 18:02 ` Jeff King
2016-09-29 21:19 ` Junio C Hamano
2016-09-29 20:50 ` Lars Schneider
2016-09-29 21:12 ` Junio C Hamano
2016-09-29 20:59 ` Jakub Narębski
2016-09-29 21:17 ` Junio C Hamano
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=xmqqk2duhcdm.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jnareb@gmail$(echo .)com \
--cc=larsxschneider@gmail$(echo .)com \
--cc=mlbright@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=ramsay@ramsayjones$(echo .)plus.com \
--cc=sbeller@google$(echo .)com \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox