threads / patch / 7117

patchbundle: fix wrong check of read_header()'s return value & add tests

Subject: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests

## tl;dr

7 messages between Mar 6, 2007 and Mar 8, 2007. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Johannes Schindelin· Mar 6, 2007, 21:57 UTC · lore

If read_header() fails, it returns <0, not 0. Further, an open(/dev/null) was not checked for errors.

Also, this adds two tests to make sure that the bundle file looks correct, by checking if it has the header has the expected form, and that the pack contains the right amount of objects.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 builtin-bundle.c |    4 +++-
 t/t5510-fetch.sh |   14 ++++++++++++++
 2 files changed, 17 insertions(+), 1 deletions(-)
Show changes to 2 files +17 −1

builtin-bundle.c, t/t5510-fetch.sh

diff --git a/builtin-bundle.c b/builtin-bundle.c
index d0c3617..3b3bc25 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -404,6 +404,8 @@ static int unbundle(struct bundle_header *header, int bundle_fd,
 	if (verify_bundle(header, 0))
 		return -1;
 	dev_null = open("/dev/null", O_WRONLY);
+	if (dev_null < 0)
+		return error("Could not open /dev/null");
 	pid = fork_with_pipe(argv_index_pack, &bundle_fd, &dev_null);
 	if (pid < 0)
 		return error("Could not spawn index-pack");
@@ -440,7 +442,7 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 
 	memset(&header, 0, sizeof(header));
 	if (strcmp(cmd, "create") &&
-			!(bundle_fd = read_header(bundle_file, &header)))
+			(bundle_fd = read_header(bundle_file, &header)) < 0)
 		return 1;
 
 	if (!strcmp(cmd, "verify")) {
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index fa76662..ce96b4b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -90,6 +90,13 @@ test_expect_success 'create bundle 1' '
 	git bundle create bundle1 master^..master
 '
 
+test_expect_success 'header of bundle looks right' '
+	head -n 1 "$D"/bundle1 | grep "^#" &&
+	head -n 2 "$D"/bundle1 | grep "^-[0-9a-f]\{40\} " &&
+	head -n 3 "$D"/bundle1 | grep "^[0-9a-f]\{40\} " &&
+	head -n 4 "$D"/bundle1 | grep "^$"
+'
+
 test_expect_success 'create bundle 2' '
 	cd "$D" &&
 	git bundle create bundle2 master~2..master
@@ -101,6 +108,13 @@ test_expect_failure 'unbundle 1' '
 	git fetch "$D/bundle1" master:master
 '
 
+test_expect_success 'bundle 1 has only 3 files ' '
+	cd "$D" &&
+	sed "1,4d" < bundle1 > bundle.pack &&
+	git index-pack bundle.pack &&
+	test 4 = $(git verify-pack -v bundle.pack | wc -l)
+'
+
 test_expect_success 'unbundle 2' '
 	cd "$D/bundle" &&
 	git fetch ../bundle2 master:master &&

-- 
1.5.0.3.2559.g30d3b
Johannes Schindelin· Mar 7, 2007, 05:15 UTC · re: Johannes Schindelin · lore

Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests

Hi,
On Tue, 6 Mar 2007, Johannes Schindelin wrote:
> +test_expect_success 'bundle 1 has only 3 files ' '
> +	cd "$D" &&
> +	sed "1,4d" < bundle1 > bundle.pack &&

I fear this would suffer the same fate as t8001, namely that some sed would add a newline, which is plain wrong here. This is a workaround:

Show changes to t/t5510-fetch.sh +1 −1
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ce96b4b..f895072 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -110,7 +110,7 @@ test_expect_failure 'unbundle 1' '
 
 test_expect_success 'bundle 1 has only 3 files ' '
 	cd "$D" &&
-	sed "1,4d" < bundle1 > bundle.pack &&
+	dd bs=136 skip=1 if=bundle1 of=bundle.pack &&
 	git index-pack bundle.pack &&
 	test 4 = $(git verify-pack -v bundle.pack | wc -l)
 '

Ciao,
Dscho
Junio C Hamano· Mar 7, 2007, 06:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 13 quoted lines
> I fear this would suffer the same fate as t8001, namely that some sed 
> would add a newline, which is plain wrong here. This is a workaround:
>
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index ce96b4b..f895072 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -110,7 +110,7 @@ test_expect_failure 'unbundle 1' '
>  
>  test_expect_success 'bundle 1 has only 3 files ' '
>  	cd "$D" &&
> -	sed "1,4d" < bundle1 > bundle.pack &&
> +	dd bs=136 skip=1 if=bundle1 of=bundle.pack &&

We might want to reword or enhance the headers later, and 136 is a horrible workaround.

Would this work?
Show changes to t/t5510-fetch.sh +9 −2
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ce96b4b..ad589dd 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -110,9 +110,16 @@ test_expect_failure 'unbundle 1' '
 
 test_expect_success 'bundle 1 has only 3 files ' '
 	cd "$D" &&
-	sed "1,4d" < bundle1 > bundle.pack &&
+	(
+		while read x && test -n "$x"
+		do
+			:;
+		done
+		cat
+	) <bundle1 >bundle.pack &&
 	git index-pack bundle.pack &&
-	test 4 = $(git verify-pack -v bundle.pack | wc -l)
+	verify=$(git verify-pack -v bundle.pack) &&
+	test 4 = $(echo "$verify" | wc -l)
 '
 
 test_expect_success 'unbundle 2' '
Johannes Schindelin· Mar 8, 2007, 13:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests

Hi,
On Tue, 6 Mar 2007, Junio C Hamano wrote:
Show 18 quoted lines
> Would this work?
> 
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index ce96b4b..ad589dd 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -110,9 +110,16 @@ test_expect_failure 'unbundle 1' '
>  
>  test_expect_success 'bundle 1 has only 3 files ' '
>  	cd "$D" &&
> -	sed "1,4d" < bundle1 > bundle.pack &&
> +	(
> +		while read x && test -n "$x"
> +		do
> +			:;
> +		done
> +		cat
> +	) <bundle1 >bundle.pack &&

I tried to avoid that, because it was mentioned that this does not work on Cygwin for some reason I forgot.

Ciao, Dscho

Alex Riesen· Mar 8, 2007, 13:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests

On 3/8/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 12 quoted lines
> On Tue, 6 Mar 2007, Junio C Hamano wrote:
> > +     (
> > +             while read x && test -n "$x"
> > +             do
> > +                     :;
> > +             done
> > +             cat
> > +     ) <bundle1 >bundle.pack &&
>
> I tried to avoid that, because it was mentioned that this does not work on
> Cygwin for some reason I forgot.
>

Can't think of a reason why it would not. Just tried: works. It works even with \r\n line endings (which I don't understand).

Johannes Schindelin· Mar 8, 2007, 17:12 UTC · re: Alex Riesen · lore

Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests

Hi,
On Thu, 8 Mar 2007, Alex Riesen wrote:
Show 16 quoted lines
> On 3/8/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Tue, 6 Mar 2007, Junio C Hamano wrote:
> > > +     (
> > > +             while read x && test -n "$x"
> > > +             do
> > > +                     :;
> > > +             done
> > > +             cat
> > > +     ) <bundle1 >bundle.pack &&
> > 
> > I tried to avoid that, because it was mentioned that this does not work on
> > Cygwin for some reason I forgot.
> > 
> 
> Can't think of a reason why it would not. Just tried: works.
> It works even with \r\n line endings (which I don't understand).

IIRC there was a problem when a file was detected to be text, but continued to be binary. Mark?

Ciao, Dscho

Mark Levedahl· Mar 8, 2007, 20:43 UTC · re: Johannes Schindelin · lore

Re: [PATCH] bundle: fix wrong check of read_header()'s return value & add tests

Johannes Schindelin wrote:
Show 7 quoted lines
> IIRC there was a problem when a file was detected to be text, but 
> continued to be binary. Mark?
>
> Ciao,
> Dscho
>
>   
The problem occurs with constructs like

echo "some text stuff" > file echo "some binary stuff" >> file

The second write, being an append, ends up executed in a forked process where file was opened by the parent, and unfortunately auto-detected as a text file, such that the write from the child process ends up mangling any crlf in the stream. This occurs regardless of the defined mount type and other cygwin flags. It is definitely a bug, but is attributed to looseness in POSIX with noone claiming ownership to fix.

However, the above bug is not triggered in the construct mentioned by Junio.
Mark

← back to recent threads