threads / bug / 13216

Probably a GIT bug..

Subject: Probably a GIT bug..

## tl;dr

4 messages between Apr 22, 2008 and Apr 23, 2008.

replies: 3people: 4as markdown or json

Tomasz bla Fortuna· Apr 22, 2008, 14:43 UTC · lore

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

Junio C Hamano· Apr 23, 2008, 16:47 UTC · re: Tomasz bla Fortuna · lore

Re: Probably a GIT bug..

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

Daniel Barkalow· Apr 23, 2008, 17:10 UTC · re: Junio C Hamano · lore

Re: Probably a GIT bug..

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

Show 15 quoted lines

> 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*

Dmitry Potapov· Apr 23, 2008, 17:13 UTC · re: Junio C Hamano · lore

Re: Probably a GIT bug..

On Wed, Apr 23, 2008 at 09:47:17AM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> 
> 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