Re: Be more careful about updating refs

Linus Torvalds [off-list ref] writes:

On Tue, 15 Jan 2008, Linus Torvalds wrote:

quoted

This makes write_ref_sha1() more careful: it actually checks the SHA1 of 
the ref it is updating, and refuses to update a ref with an object that it 
cannot find.

Side note: this breaks some tests, because those tests do things like

	git update-ref refs/heads/master 1111111111111111111111111111111111111111 &&
		test 1111111111111111111111111111111111111111 = $(cat .git/refs/heads/master)

...
(Pet peeve on mine: people fixing assert()'s by changing the source-code, 
without ever asking themselves whether maybe the assert itself was the 
bug).

The rules for the plumbing used to be that refs can point at
anything that get_sha1() accepts.  We did not even require it to
be parse_object() happy let alone it being parse_commit() kosher.

You changed the world order.  I agree that the world order was
changed in a good way, but saying that the original test did not
check the right thing or it was a bug is not quite fair.  At
worst, we can say that it was very sloppily written by assuming
that the commands involved in the particular test would not care
about corrupted repositories whose refs point at nonexistant
bogus objects.

I'll squash the following to your patch.

---
 t/t1400-update-ref.sh |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index a90824b..71ab2dd 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -7,12 +7,19 @@ test_description='Test git update-ref and basic ref logging'
 . ./test-lib.sh
 
 Z=0000000000000000000000000000000000000000
-A=1111111111111111111111111111111111111111
-B=2222222222222222222222222222222222222222
-C=3333333333333333333333333333333333333333
-D=4444444444444444444444444444444444444444
-E=5555555555555555555555555555555555555555
-F=6666666666666666666666666666666666666666
+
+test_expect_success setup '
+
+	for name in A B C D E F
+	do
+		test_tick &&
+		T=$(git write-tree) &&
+		sha1=$(echo $name | git commit-tree $T) &&
+		eval $name=$sha1
+	done
+
+'
+
 m=refs/heads/master
 n_dir=refs/heads/gu
 n=$n_dir/fixes

On Wed, Jan 16, 2008 at 11:52:43AM -0800, Junio C Hamano wrote:

Linus Torvalds [off-list ref] writes:

quoted

On Tue, 15 Jan 2008, Linus Torvalds wrote:

quoted

This makes write_ref_sha1() more careful: it actually checks the SHA1 of 
the ref it is updating, and refuses to update a ref with an object that it 
cannot find.

Side note: this breaks some tests, because those tests do things like

	git update-ref refs/heads/master 1111111111111111111111111111111111111111 &&
		test 1111111111111111111111111111111111111111 = $(cat .git/refs/heads/master)

...
(Pet peeve on mine: people fixing assert()'s by changing the source-code, 
without ever asking themselves whether maybe the assert itself was the 
bug).

The rules for the plumbing used to be that refs can point at
anything that get_sha1() accepts.  We did not even require it to
be parse_object() happy let alone it being parse_commit() kosher.

You changed the world order.  I agree that the world order was
changed in a good way, but saying that the original test did not
check the right thing or it was a bug is not quite fair.  At
worst, we can say that it was very sloppily written by assuming
that the commands involved in the particular test would not care
about corrupted repositories whose refs point at nonexistant
bogus objects.

I'll squash the following to your patch.

I'm assuming that this original patch and the test update turned into
the following commit in master:

c3b0dec509fe136c5417422f31898b5a4e2d5e02 is first bad commit

I just thought I should warn you that this seems (git bisect tells me
so) to have caused a failure in t9301-fast-export.sh on my Mac OS X
10.4.11 machine although I haven't yet had the time to investigate
why.

Charles.

Charles Bailey schrieb:

I'm assuming that this original patch and the test update turned into
the following commit in master:

c3b0dec509fe136c5417422f31898b5a4e2d5e02 is first bad commit

I just thought I should warn you that this seems (git bisect tells me
so) to have caused a failure in t9301-fast-export.sh on my Mac OS X
10.4.11 machine although I haven't yet had the time to investigate
why.

I observed the same (on Windows). The reason is that above-mentioned
commit introduces a call to parse_objects(). But by the time that
fast-import calls write_ref_sha1() (and, hence, this new parse_objects())
it has not yet written a pack file, and parse_objects() fails. I don't
have a clue how to fix this short of reverting the commit.

-- Hannes

On Thu, Jan 17, 2008 at 09:15:58AM +0000, Charles Bailey wrote:

c3b0dec509fe136c5417422f31898b5a4e2d5e02 is first bad commit

I just thought I should warn you that this seems (git bisect tells me
so) to have caused a failure in t9301-fast-export.sh on my Mac OS X
10.4.11 machine although I haven't yet had the time to investigate
why.

Further to this, the first problem is that the 'fast-export |
fast-import' test is failing.

As far as I can tell, fast-export is behaving no differently, the
generated export file looks identical in shape.  It seems that
fast-import is creating a bad temporary packfile for itself.

The error that I'm getting is:

fatal: unknown object type 0 in
.git/objects/pack/pack-6be2b92c2d7485fd5cefbb0d9a68827e4c23d548.pack
fast-import: dumping crash report to .git/fast_import_crash_12317

which seems to come from the unpack_entry function in sha1_file.c.

I haven't got much deeper into how the refs.c change has caused the
issue, or why on earth it seems to be a Mac only issue.

On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote:

I observed the same (on Windows). The reason is that above-mentioned
commit introduces a call to parse_objects(). But by the time that
fast-import calls write_ref_sha1() (and, hence, this new parse_objects())
it has not yet written a pack file, and parse_objects() fails. I don't
have a clue how to fix this short of reverting the commit.

OK, so it's not just Mac OS X, then.  From your description and my
initial poke at the code,  I can't immediately see a good reason why
the test should succeed on Linux, though.

Charles Bailey schrieb:

On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote:

quoted

I observed the same (on Windows). The reason is that above-mentioned
commit introduces a call to parse_objects(). But by the time that
fast-import calls write_ref_sha1() (and, hence, this new parse_objects())
it has not yet written a pack file, and parse_objects() fails. I don't
have a clue how to fix this short of reverting the commit.

OK, so it's not just Mac OS X, then.  From your description and my
initial poke at the code,  I can't immediately see a good reason why
the test should succeed on Linux, though.

My analysis is not correct. The pack file is present, but it seems to be
incomplete.

The reason is the NO_MMAP build flag. If you compile with
NO_MMAP=YesPlease on Linux, t9301 fails as well. Does this ring a bell?

-- Hannes

Hi,

On Thu, 17 Jan 2008, Johannes Sixt wrote:

Charles Bailey schrieb:

quoted

On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote:

quoted

I observed the same (on Windows). The reason is that above-mentioned
commit introduces a call to parse_objects(). But by the time that
fast-import calls write_ref_sha1() (and, hence, this new parse_objects())
it has not yet written a pack file, and parse_objects() fails. I don't
have a clue how to fix this short of reverting the commit.

OK, so it's not just Mac OS X, then.  From your description and my
initial poke at the code,  I can't immediately see a good reason why
the test should succeed on Linux, though.

My analysis is not correct. The pack file is present, but it seems to be
incomplete.

The reason is the NO_MMAP build flag. If you compile with
NO_MMAP=YesPlease on Linux, t9301 fails as well. Does this ring a bell?

Just a wild guess... Could it be that the mmap() happened before contents 
were written to that fd?  The faked mmap() is just a malloc() && pread(), 
and would thus miss out on changes written after the mmap() call.

Ciao,
Dscho

On Thu, Jan 17, 2008 at 12:58:07PM +0000, Johannes Schindelin wrote:

Just a wild guess... Could it be that the mmap() happened before contents 
were written to that fd?  The faked mmap() is just a malloc() && pread(), 
and would thus miss out on changes written after the mmap() call.

This would be my guess too.  As far as I can tell fast-import is
creating objects directly in a packfile.  I'm guessing here, but the
new checking code is probably being triggered to check (for example)
the parent ref for commits that fast-import is creating and the check
only works if mmap can successfully examine objects in the
still-being-written packfile.