Re: [PATCH v5 04/40] Add Git/Packet.pm from parts of t0021/rot13-filter.pl
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 3, 2017, 19:11 UTC
- Message-ID
- <xmqqo9rwxy8k.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170803091926.1755-5-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
Show 29 quoted lines
> This will make it possible to reuse packet reading and writing
> functions in other test scripts.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> perl/Git/Packet.pm | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 71 insertions(+)
> create mode 100644 perl/Git/Packet.pm
>
> diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
> new file mode 100644
> index 0000000000..aaffecbe2a
> --- /dev/null
> +++ b/perl/Git/Packet.pm
> @@ -0,0 +1,71 @@
> +package Git::Packet;
> +use 5.008;
> +use strict;
> +use warnings;
> +BEGIN {
> + require Exporter;
> + if ($] < 5.008003) {
> + *import = \&Exporter::import;
> + } else {
> + # Exporter 5.57 which supports this invocation was
> + # released with perl 5.8.3
> + Exporter->import('import');
> + }
> +}This is merely me being curious, but do we want this boilerplate, which we do not use in perl/Git.pm but we do in perl/Git/I18N.pm?
Show 8 quoted lines
> +our @EXPORT = qw( > + packet_bin_read > + packet_txt_read > + packet_bin_write > + packet_txt_write > + packet_flush > + ); > +our @EXPORT_OK = @EXPORT;
We can see that you made sure that the only thing 05/40 needs to do is to use this package and remove the definition of these subs, without having to touch any caller by first updating the original implementation in 03/40 and then exporting these names in 04/40. Knowing that the preparation is nicely done already, it is a bit irritating to see that 05/40 is a separate patch, as we need to switch between the patches to see if there is any difference between the original implementation of the subs, and the replacement implemented in here. It would have been nicer to have changes in 04/40 and 05/40 in a single patch.