Re: [PATCH 1/2] t5616: refactor packfile replacement

From: Jonathan Tan <jonathantanmy@google.com>
To: Johannes.Schindelin@gmx.de
Cc: jonathantanmy@google.com, git@vger.kernel.org
Subject: Re: [PATCH 1/2] t5616: refactor packfile replacement
Date: Wed, 15 May 2019 11:22:53 -0700	[thread overview]
Message-ID: <20190515182253.105984-1-jonathantanmy@google.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1905151032500.44@tvgsbejvaqbjf.bet>

> > +# Converts bytes into their hexadecimal representation. For example,
> > +# "printf 'ab\r\n' | hex_unpack" results in '61620d0a'.
> > +hex_unpack () {
> > +	perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)'
> > +}
> > +
> > +# Inserts $1 at the start of the string and every 2 characters thereafter.
> > +intersperse () {
> > +	sed 's/\(..\)/'$1'\1/g'
> > +}
> > +
> > +# Create a one-time-sed command to replace the existing packfile with $1.
> > +replace_packfile () {
> > +	# The protocol requires that the packfile be sent in sideband 1, hence
> > +	# the extra \x01 byte at the beginning.
> > +	printf "1,/packfile/!c %04x\\\\x01%s0000" \
> > +		"$(($(wc -c <$1) + 5))" \
> > +		"$(hex_unpack <$1 | intersperse '\\x')" \
> > +		>"$HTTPD_ROOT_PATH/one-time-sed"
> >  }
> 
> Urgh. This is not a problem *this* patch introduces, but why on Earth do
> we have to do complicated computations in shell code using an unholy mix
> of complex sed and Perl invocations, making things fragile and slow? We do
> have such a nice facility is the t/test-tool helper...

This might be a good #leftoverbits. I'm not sure which part you think
needs to be replaced - maybe the thing that goes into one-time-sed?

> The refactoring itself looks correct to me, of course.

Thanks, and thanks for taking a look at this.