From: Junio C Hamano <gitster@pobox.com>
To: Christian Couder <christian.couder@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
Ben Peart <Ben.Peart@microsoft.com>,
Jonathan Tan <jonathantanmy@google.com>,
Nguyen Thai Ngoc Duy <pclouds@gmail.com>,
Mike Hommey <mh@glandium.org>,
Lars Schneider <larsxschneider@gmail.com>,
Eric Wong <e@80x24.org>,
Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions
Date: Sun, 22 Oct 2017 09:58:04 +0900 [thread overview]
Message-ID: <xmqq4lqsf17n.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20171019123030.17338-2-chriscool@tuxfamily.org> (Christian Couder's message of "Thu, 19 Oct 2017 14:30:25 +0200")
Christian Couder <christian.couder@gmail.com> writes:
> To make it possible in a following commit to move packet
> reading and writing functions into a Packet.pm module,
> let's refactor these functions, so they don't handle
> printing debug output and exiting.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> t/t0021/rot13-filter.pl | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> index ad685d92f8..e4495a52f3 100644
> --- a/t/t0021/rot13-filter.pl
> +++ b/t/t0021/rot13-filter.pl
> @@ -60,8 +60,7 @@ sub packet_bin_read {
> my $bytes_read = read STDIN, $buffer, 4;
> if ( $bytes_read == 0 ) {
> # EOF - Git stopped talking to us!
> - print $debug "STOP\n";
> - exit();
> + return ( -1, "" );
> }
> elsif ( $bytes_read != 4 ) {
> die "invalid packet: '$buffer'";
> @@ -85,7 +84,7 @@ sub packet_bin_read {
>
> sub packet_txt_read {
> my ( $res, $buf ) = packet_bin_read();
> - unless ( $buf eq '' or $buf =~ s/\n$// ) {
> + unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
> die "A non-binary line MUST be terminated by an LF.";
> }
> return ( $res, $buf );
> @@ -131,7 +130,12 @@ print $debug "init handshake complete\n";
> $debug->flush();
>
> while (1) {
> - my ( $command ) = packet_txt_read() =~ /^command=(.+)$/;
> + my ( $res, $command ) = packet_txt_read();
> + if ( $res == -1 ) {
> + print $debug "STOP\n";
> + exit();
> + }
> + $command =~ s/^command=//;
> print $debug "IN: $command";
> $debug->flush();
This was not an issue in the old code which died upon unexpected EOF
inside the lowest-level helper packet_bin_read(), but now you have
one call to packet_bin_read() and many calls to packet_txt_read()
whose return value is not checked for this new condition you are
allowing packet_bin_read() to return. This step taken alone is a
regression---let's see how the remainder of the series updates the
callers to compensate.
I initially thought that it may be more Perl-ish to return undef or
string instead of returning a 2-element list, but this code needs to
distinguish three conditions (i.e. a normal string that is 0 or more
bytes long, a flush, and an EOF), so that is not sufficient. Perl
experts on the list still may be able to suggest a better way than
the current one to do so, but that is outside the scope of this
refactoring.
Thanks for starting to work on this.
next prev parent reply other threads:[~2017-10-22 0:58 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-19 12:30 [PATCH 0/6] Create Git/Packet.pm Christian Couder
2017-10-19 12:30 ` [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions Christian Couder
2017-10-19 22:01 ` Stefan Beller
2017-10-22 0:58 ` Junio C Hamano [this message]
2017-11-05 12:50 ` Christian Couder
2017-10-19 12:30 ` [PATCH 2/6] t0021/rot13-filter: improve 'if .. elsif .. else' style Christian Couder
2017-10-19 12:30 ` [PATCH 3/6] t0021/rot13-filter: improve error message Christian Couder
2017-10-19 12:30 ` [PATCH 4/6] t0021/rot13-filter: add packet_initialize() Christian Couder
2017-10-22 1:12 ` Junio C Hamano
2017-10-27 2:57 ` Junio C Hamano
2017-10-27 5:07 ` Christian Couder
2017-10-28 14:59 ` Lars Schneider
2017-10-29 0:14 ` Junio C Hamano
2017-10-19 12:30 ` [PATCH 5/6] t0021/rot13-filter: add capability functions Christian Couder
2017-10-22 1:46 ` Junio C Hamano
2017-11-04 8:38 ` Christian Couder
2017-11-05 2:03 ` Junio C Hamano
2017-10-19 12:30 ` [PATCH 6/6] Add Git/Packet.pm from parts of t0021/rot13-filter.pl Christian Couder
2017-10-19 22:06 ` Stefan Beller
2017-10-22 2:04 ` [PATCH 0/6] Create Git/Packet.pm Junio C Hamano
2017-10-23 12:26 ` Philip Oakley
2017-10-30 18:08 ` Jeff King
2017-10-25 23:10 ` Johannes Schindelin
2017-10-26 5:38 ` Junio C Hamano
2017-10-26 9:07 ` Jacob Keller
2017-10-26 9:08 ` Bryan Turner
2017-10-26 9:12 ` Bryan Turner
2017-10-27 15:09 ` Johannes Schindelin
2017-10-27 15:05 ` Johannes Schindelin
2017-10-30 0:38 ` Junio C Hamano
2017-10-30 6:18 ` Christian Couder
2017-10-30 12:37 ` Johannes Schindelin
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq4lqsf17n.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=Ben.Peart@microsoft.com \
--cc=chriscool@tuxfamily.org \
--cc=christian.couder@gmail.com \
--cc=e@80x24.org \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
--cc=larsxschneider@gmail.com \
--cc=mh@glandium.org \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).