Re: [PATCH v5 04/40] Add Git/Packet.pm from parts of t0021/rot13-filter.pl

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> 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?

> +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.