Probably a GIT bug..

Probably a GIT bug..

[Tomasz bla Fortuna]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

Hello,

Testcase: http://temp.thera.be/gitbug.tar
Contents:
gitbug/
gitbug/prep.sh
gitbug/fail.sh
gitbug/init.sh
gitbug/README


#v+ init.sh

#!/bin/bash

export GIT_AUTHOR_EMAIL='test@test.be'
export GIT_AUTHOR_NAME='gitbug'
export GIT_AUTHOR_DATE="Mon Apr 21 16:40:08 2008 +0200"
export GIT_COMMITTER_NAME='gitbug'
export GIT_COMMITTER_EMAIL='test@test.be'
export GIT_COMMITTER_DATE="Mon Apr 21 16:40:08 2008 +0200"

rm -rf ./.git
rm -f test test2

git init
touch test
git add test
git commit -a -m "first commit"

## prep.sh:
#!/bin/bash
mkdir .git/objects/a2
chmod a-rwx .git/objects/a2 

## fail.sh:
#!/bin/bash

touch test2
git add test2
git commit -a -m "second commit"

echo -en "\n\n\nSee the result of git-fsck:\n" 
git-fsck

#v-

Execution:
#v+
[16:39:14] bla@Vorago ~/my_tmp/gitbug $ source init.sh 
Initialized empty Git repository in .git/
Created initial commit 2231f53: first commit
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 test
[16:39:16] bla@Vorago ~/my_tmp/gitbug $ source prep.sh 
[16:39:17] bla@Vorago ~/my_tmp/gitbug $ source fail.sh 
error: sha1
file /home/bla/my_tmp/gitbug/.git/objects/a2/2c62298732a162ab9aa64c31b24c4c87cf8cd9:
Permission denied

fatal: unable to read destination tree
(a12d0088c0e538480a1586a9ac2d5de3b54b2759) Created commit 


See the result of git-fsck:
broken link from  commit a12d0088c0e538480a1586a9ac2d5de3b54b2759
              to    tree a22c62298732a162ab9aa64c31b24c4c87cf8cd9
missing tree a22c62298732a162ab9aa64c31b24c4c87cf8cd9
[16:39:19] bla@Vorago ~/my_tmp/gitbug $ 
#v-

As I understand such behaviour shouldn't have place; one can fix it by
fixing attributes and then doing git-reset+commit. Maybe with fsck also.

I guess that GIT should check if he can create a tree link before
creating a commit.

See you,
-- 
Tomasz bla Fortuna
jid: bla(at)af.gliwice.pl
pgp: 0x90746E79 @ pgp.mit.edu
www: http://bla.thera.be

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Probably a GIT bug..

[Junio C Hamano]

Tomasz bla Fortuna <bla@thera.be> writes:

> I guess that GIT should check if he can create a tree link before
> creating a commit.

Thanks.

That's a somewhat contrived example, and I do not know how you found it.

I suspect the same breakage would trigger when you ran out of disk quota
to write out the tree while you still have barely enough quota to create
the commit and update the ref, which would result in corrupt repository.
So in that sense, this breakage is more likely to bite people in reality
than the initial reaction your test may invite which is "don't do that
then".

Here is a possible fix.

-- >8 --
write-tree: properly detect failure to write tree objects

Tomasz Fortuna reported that "git commit" does not error out properly when
it cannot write tree objects out.  "git write-tree" shares the same issue,
as the failure to notice the error is deep in the logic to write tree
objects out recursively.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache-tree.c          |    7 +++-
 t/t0004-unwritable.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 39da54d..73cb340 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -341,8 +341,11 @@ static int update_one(struct cache_tree *it,
 
 	if (dryrun)
 		hash_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1);
-	else
-		write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1);
+	else if (write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1)) {
+		strbuf_release(&buffer);
+		return -1;
+	}
+
 	strbuf_release(&buffer);
 	it->entry_count = i;
 #if DEBUG
diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh
new file mode 100755
index 0000000..b355a8f
--- /dev/null
+++ b/t/t0004-unwritable.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description='detect unwritable repository and fail correctly'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	>file &&
+	git add file &&
+	git commit -m initial &&
+	echo >file &&
+	git add file
+		
+'
+
+test_expect_success 'write-tree should notice unwritable repository' '
+
+	(
+		chmod a-w .git/objects
+		test_must_fail git write-tree
+	)
+	status=$?
+	chmod 775 .git/objects
+	(exit $status)
+
+'
+
+test_expect_success 'commit should notice unwritable repository' '
+
+	(
+		chmod a-w .git/objects
+		test_must_fail git commit -m second
+	)
+	status=$?
+	chmod 775 .git/objects
+	(exit $status)
+
+'
+
+test_expect_success 'update-index should notice unwritable repository' '
+
+	(
+		echo a >file &&
+		chmod a-w .git/objects
+		test_must_fail git update-index file
+	)
+	status=$?
+	chmod 775 .git/objects
+	(exit $status)
+
+'
+
+test_expect_success 'add should notice unwritable repository' '
+
+	(
+		echo b >file &&
+		chmod a-w .git/objects
+		test_must_fail git add file
+	)
+	status=$?
+	chmod 775 .git/objects
+	(exit $status)
+
+'
+
+test_done

Re: Probably a GIT bug..

[Daniel Barkalow]

On Wed, 23 Apr 2008, Junio C Hamano wrote:

> Tomasz bla Fortuna <bla@thera.be> writes:
> 
> > I guess that GIT should check if he can create a tree link before
> > creating a commit.
> 
> Thanks.
> 
> That's a somewhat contrived example, and I do not know how you found it.
> 
> I suspect the same breakage would trigger when you ran out of disk quota
> to write out the tree while you still have barely enough quota to create
> the commit and update the ref, which would result in corrupt repository.
> So in that sense, this breakage is more likely to bite people in reality
> than the initial reaction your test may invite which is "don't do that
> then".

make
su
make install
git commit

and then log out from your desktop environment, and you've got objects 
subdirectories owned by root without realizing. Of course, you shouldn't 
do that, either, but it's an easier mistake than accidentally removing 
your own write permission to something.

	-Daniel
*This .sig left intentionally blank*

Re: Probably a GIT bug..

[Dmitry Potapov]

On Wed, Apr 23, 2008 at 09:47:17AM -0700, Junio C Hamano wrote:
> 
> That's a somewhat contrived example, and I do not know how you found it.
> 
> I suspect the same breakage would trigger when you ran out of disk quota
> to write out the tree while you still have barely enough quota to create
> the commit and update the ref, which would result in corrupt repository.

I have looked at write_sha1_file(), it appears to me that if you do not
have enough disk space (or exceed your disk quote) then this function
will die inside. For the described situation to happen, you probably
should have wrong permissions on directories (as it was simulated in the
bug report) or some other error with open() (such as exceeding the maximum
allowed number of descriptor).

So I wonder why errors in write_sha1_file() are treated differently,
i.e. the function may die on error in some error cases (such as not
enough disk space), but it indicates the error situation by return code
if open() failed.

BTW, the returned code of write_sha1_file() is also not checked in
write_tree() in mktree.c.

Dmitry