[PATCH] fsck: warn about ".git" in trees

[PATCH] fsck: warn about ".git" in trees

[Jeff King]

On Wed, Nov 28, 2012 at 01:25:05PM -0800, Junio C Hamano wrote:

> >> * jk/fsck-dot-in-trees (2012-11-28) 1 commit
> >>  - fsck: warn about '.' and '..' in trees
> >> 
> >>  Will merge to 'next'.
> >
> > Do you have an opinion on warning about '.git', as well? It probably
> > would make more sense as a patch on top, but I thought I'd ask before
> > this got merged to next.
> 
> Yeah, it would make sense to reject what we would not record
> ourselves when the tools are used in a sane manner.

Here's the patch on top of jk/fsck-dot-in-trees.

-- >8 --
Subject: [PATCH] fsck: warn about ".git" in trees

Having a ".git" entry inside a tree can cause confusing
results on checkout. At the top-level, you could not
checkout such a tree, as it would complain about overwriting
the real ".git" directory. In a subdirectory, you might
check it out, but performing operations in the subdirectory
would confusingly consider the in-tree ".git" directory as
the repository.

The regular git tools already make it hard to accidentally
add such an entry to a tree, and do not allow such entries
to enter the index at all. Teaching fsck about it provides
an additional safety check, and let's us avoid propagating
any such bogosity when transfer.fsckObjects is on.

Signed-off-by: Jeff King <peff@peff.net>
---
 fsck.c          |  5 +++++
 t/t1450-fsck.sh | 15 +++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/fsck.c b/fsck.c
index 31c9a51..99c0497 100644
--- a/fsck.c
+++ b/fsck.c
@@ -144,6 +144,7 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 	int has_empty_name = 0;
 	int has_dot = 0;
 	int has_dotdot = 0;
+	int has_dotgit = 0;
 	int has_zero_pad = 0;
 	int has_bad_modes = 0;
 	int has_dup_entries = 0;
@@ -174,6 +175,8 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 			has_dot = 1;
 		if (!strcmp(name, ".."))
 			has_dotdot = 1;
+		if (!strcmp(name, ".git"))
+			has_dotgit = 1;
 		has_zero_pad |= *(char *)desc.buffer == '0';
 		update_tree_entry(&desc);
 
@@ -227,6 +230,8 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 		retval += error_func(&item->object, FSCK_WARN, "contains '.'");
 	if (has_dotdot)
 		retval += error_func(&item->object, FSCK_WARN, "contains '..'");
+	if (has_dotgit)
+		retval += error_func(&item->object, FSCK_WARN, "contains '.git'");
 	if (has_zero_pad)
 		retval += error_func(&item->object, FSCK_WARN, "contains zero-padded file modes");
 	if (has_bad_modes)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 0b5c30b..d730734 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -253,4 +253,19 @@ test_expect_success 'fsck notices "." and ".." in trees' '
 	)
 '
 
+test_expect_success 'fsck notices ".git" in trees' '
+	(
+		git init dotgit &&
+		cd dotgit &&
+		blob=$(echo foo | git hash-object -w --stdin) &&
+		tab=$(printf "\\t") &&
+		git mktree <<-EOF &&
+		100644 blob $blob$tab.git
+		EOF
+		git fsck 2>out &&
+		cat out &&
+		grep "warning.*\\.git" out
+	)
+'
+
 test_done
-- 
1.8.0.207.gdf2154c

Re: [PATCH] fsck: warn about ".git" in trees

[Torsten Bögershausen]

> Having a ".git" entry inside a tree can cause confusing
> results on checkout. At the top-level, you could not
> checkout such a tree, as it would complain about overwriting
> the real ".git" directory. In a subdirectory, you might
> check it out, but performing operations in the subdirectory
> would confusingly consider the in-tree ".git" directory as
> the repository.
[snip]
> +	int has_dotgit = 0;

Name like "." or ".." are handled as directories by the OS.

".git" could be a file or a directory, at least in theory,
and from the OS point of view,
but we want to have this as a reserved name.

Looking at bad directory names, which gives trouble when checking out:

Should we check for "/" or "../blabla" as well?

Re: [PATCH] fsck: warn about ".git" in trees

[Jeff King]

On Fri, Nov 30, 2012 at 08:50:41PM +0100, Torsten Bögershausen wrote:

> >Having a ".git" entry inside a tree can cause confusing
> >results on checkout. At the top-level, you could not
> >checkout such a tree, as it would complain about overwriting
> >the real ".git" directory. In a subdirectory, you might
> >check it out, but performing operations in the subdirectory
> >would confusingly consider the in-tree ".git" directory as
> >the repository.
> [snip]
> >+	int has_dotgit = 0;
> 
> Name like "." or ".." are handled as directories by the OS.

Right. In theory git could run on a system that does not treat them
specially, but in practice they are going to be problematic on most
systems.

> ".git" could be a file or a directory, at least in theory, and from
> the OS point of view, but we want to have this as a reserved name.

Exactly.

> Looking at bad directory names, which gives trouble when checking out:
> 
> Should we check for "/" or "../blabla" as well?

We do already (the error is "contains full pathnames"). We also cover
empty pathnames and some other cases.

-Peff

Re: [PATCH] fsck: warn about ".git" in trees

[Andreas Ericsson]

On 11/30/2012 08:50 PM, Torsten Bögershausen wrote:
>> Having a ".git" entry inside a tree can cause confusing
>> results on checkout. At the top-level, you could not
>> checkout such a tree, as it would complain about overwriting
>> the real ".git" directory. In a subdirectory, you might
>> check it out, but performing operations in the subdirectory
>> would confusingly consider the in-tree ".git" directory as
>> the repository.
> [snip]
>> +    int has_dotgit = 0;
> 
> Name like "." or ".." are handled as directories by the OS.
> 

The patch is for the index, where they're handled as whatever the mode
claims it is. The patch doesn't touch those parts though.

> ".git" could be a file or a directory, at least in theory,
> and from the OS point of view,
> but we want to have this as a reserved name.
> 
> Looking at bad directory names, which gives trouble when checking out:
> 
> Should we check for "/" or "../blabla" as well?
> 

Apart from the checks already in place, checking for git's internal
directory separator marker (which is '/') is enough to catch both,
and that check is done.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.