Re: [PATCH 0/6] fix test failure with busybox

[Danh Doan <congdanhqx@gmail.com>]

On 2020-03-19 11:51:36-0400, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2020 at 09:00:01PM +0700, Đoàn Trần Công Danh wrote:
> 
> > Despite some non-compiance from busybox's sh(1), grep(1), diff(1),
> > Alpine Linux is still a popular choice for container these days.
> > 
> > Fix false-positive failure in testsuite when run in Alpine Linux.
> > 
> > t5703.{4,5,6,7} is still failing.
> > Despite git pack-objects behaves correctly,
> > upload-pack.c works incorrectly on this platform.
> > 
> > I haven't dig deeper, yet.
> 
> I was able to reproduce the problems on Debian (with busybox installed)
> with:
> 
>   mkdir /tmp/bb
>   (cd /tmp/bb
>    bb=$(which busybox)
>    for i in $($bb --list); do
>      ln -s $bb $i
>    done)
>   PATH=/tmp/bb:$PATH make test TEST_SHELL_PATH=/tmp/bb/sh
> 
> The issue in t5703 is the sed call in get_actual_commits(). It's trying
> to cut off the early (text) part of the file, and pass through the
> binary goo of the packfile. Presumably busybox's sed isn't binary-clean.

I've checked, busybox's sed think every input is text file,
and in POSIX, every line in every text file must be terminated by
<newline>.

Thus, busybox's sed append a <newline> after `0000` marker, render the
pack file invalid.

> Our usual strategy here would be to switch to perl, which is more
> predictable about binary bytes.

Perl works fine here.

> We're also feeding this into a test-tool helper. It's possible that
> helper could be made smart enough to replace both this sed invocation
> and the one in get_actual_refs().

I looked into this direction, I guess you meant something like this:

-------------8<--------------
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 282d536384..1d62143dbc 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "test-tool.h"
 #include "pkt-line.h"
+#include "strbuf.h"
 
 static void pack_line(const char *line)
 {
@@ -53,6 +54,13 @@ static void unpack(void)
 static void unpack_sideband(void)
 {
 	struct packet_reader reader;
+	struct strbuf buf = STRBUF_INIT;
+
+	while (strbuf_getline(&buf, stdin) == 0)
+		if (strstr(buf.buf, "packfile"))
+		    break;
+	strbuf_release(&buf);
+
 	packet_reader_init(&reader, 0, NULL, 0,
 			   PACKET_READ_GENTLE_ON_EOF |
 			   PACKET_READ_CHOMP_NEWLINE);
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 7fba3063bf..a34460f7d8 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -13,10 +13,7 @@ get_actual_refs () {
 }
 
 get_actual_commits () {
-	sed -n -e '/packfile/,/0000/{
-		/packfile/d
-		p
-		}' <out | test-tool pkt-line unpack-sideband >o.pack &&
+	test-tool pkt-line unpack-sideband <out >o.pack &&
 	git index-pack o.pack &&
 	git verify-pack -v o.idx >objs &&
 	grep commit objs | cut -d" " -f1 | sort >actual_commits
----------------------->8-------------------

But, this doesn't work. as `packet_reader_read` reads directly from fd 0.

I think perl should be good for now.

-- 
Danh