git ls-files -o under .git/ prints all repository files

git ls-files -o under .git/ prints all repository files

[Yasushi SHOJI]

Hi all,

ls-files -o prints all files under .git if you are in the .git
directory.  this is pretty dangerous since we now have git clean to
delete files marked others.

sure in UNIX env., you can easily shoot yourself in the foot. but it'd
might be nice to help newbies.

I'm not sure how we should fix this.  should we

1) prevent to run any git command under .git unless .git is also
   managed by git (ie. .git/.git or something exists)

2) prevent ls-files -o to print any files under .git/ even if we are
   in the directory.


the way to reproduce is:


$ git init
Initialized empty Git repository in .git/
$ echo hello > hello.c
$ git add .
$ git commit -m initial
Created initial commit b40c824b521f6c60434043f0cce08a88b4031ed8
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 hello.c
$ cd .git
$ git ls-files -o
HEAD
config
description
hooks/applypatch-msg
hooks/commit-msg
hooks/post-commit
hooks/post-update
hooks/pre-applypatch
hooks/pre-commit
hooks/pre-rebase
hooks/update
index
info/exclude
logs/refs/heads/master
objects/b4/0c824b521f6c60434043f0cce08a88b4031ed8
objects/bc/2d2dcb34e8313627d45ad6ef38beddf560501d
objects/ce/013625030ba8dba906f756967f9e9ca394464a
refs/heads/master
$ git clean
Removing HEAD
Not removing branches/
Removing config
Removing description
Not removing hooks/
Removing index
Not removing info/
Not removing logs/
Not removing objects/
Not removing refs/
Not removing remotes/
$ cd ..
$ git ls-files
fatal: Not a git repository
$ 
-- 
          yashi

Re: git ls-files -o under .git/ prints all repository files

[Junio C Hamano]

Yasushi SHOJI <yashi@atmark-techno.com> writes:

> ls-files -o prints all files under .git if you are in the .git
> directory.  this is pretty dangerous since we now have git clean to
> delete files marked others.
>
> sure in UNIX env., you can easily shoot yourself in the foot. but it'd
> might be nice to help newbies.

It's amusing to see that people can find obscure ways to shoot
themselves in the foot.

Amusing problems deserve an equally amusing solution.

-- >8 --
[PATCH] Make sure .git/ is not readable by anybody.

Normal git operation continues to work after doing "chmod a-r .git".
This makes a newly created git repository unreadable (but searchable)
so that people cannot do "cd .git && git clean" to shoot themselves.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 8e7540b..4310a05 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -18,7 +18,10 @@
 
 static void safe_create_dir(const char *dir, int share)
 {
-	if (mkdir(dir, 0777) < 0) {
+	mode_t mode;
+
+	mode = share ? 0777 : 0333;
+	if (mkdir(dir, mode) < 0) {
 		if (errno != EEXIST) {
 			perror(dir);
 			exit(1);

Re: git ls-files -o under .git/ prints all repository files

[Andy Parkins]

On Friday 2007, January 19 06:47, Junio C Hamano wrote:

> +	mode = share ? 0777 : 0333;

So if the repository is shared we're allowed to shoot ourselves in the foot?

Also; what does this do to .git/config .git/description?

On ocassion I've found myself doing
  mv .git/refs/remotes/origin .git/refs/remotes/up

Which this patch would break.  (Maybe I shouldn't be doing that though, so 
perhaps it should break :-))


Andy

-- 
Dr Andrew Parkins, M Eng (Hons), AMIEE
andyparkins@gmail.com

Re: git ls-files -o under .git/ prints all repository files

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> On Friday 2007, January 19 06:47, Junio C Hamano wrote:
>
>> +	mode = share ? 0777 : 0333;
>
> So if the repository is shared we're allowed to shoot ourselves in the foot?

Have you actually read the code to see what 'share' variable
means there?  It is only false when creating the toplevel .git
directory and always true for its subdirectories.

> Also; what does this do to .git/config .git/description?

Nothing unusual.  The code explicitly asks for .git/config by
name, so that does not involve readdir(".git"), which is what
the 0333 change prevents from running.

> On ocassion I've found myself doing
>   mv .git/refs/remotes/origin .git/refs/remotes/up
>
> Which this patch would break.

Does it?

And everybody commented on this thread,

	EASY.

You all should not take "amusing" too seriously.  That was a
tongue-in-cheek patch.

I am very inclined to say

	$ cd .git && git clean

or

	$ cd .git/objects && git clean        

falls into the same category as

	$ su
	# cd / && git-init-db && git clean

In other words, I am not sure if there is anything worth fixing.

Re: git ls-files -o under .git/ prints all repository files

[Andy Parkins]

On Friday 2007, January 19 08:32, Junio C Hamano wrote:

> Have you actually read the code to see what 'share' variable
> means there?  It is only false when creating the toplevel .git

Nope.  I'm an idiot :-) I assumed it was from the --shared command line 
argument.  What they say about assumptions is true isn't it?

> Nothing unusual.  The code explicitly asks for .git/config by
> name, so that does not involve readdir(".git"), which is what
> the 0333 change prevents from running.

In this case I was talking more about the user editing those files than the 
code looking for them.  I suppose if you know its there then a 
vim .git/config will be fine.

> > On ocassion I've found myself doing
> >   mv .git/refs/remotes/origin .git/refs/remotes/up
> >
> > Which this patch would break.
>
> Does it?

You're right it doesn't.  As long as you know the refs directory is there it 
doesn't stop you changing into it, and messing about in any way you want.  I 
was looking at it from the point of view of how I originally found out about 
these git inner workings - I did it by poking around in the .git directory.

> You all should not take "amusing" too seriously.  That was a
> tongue-in-cheek patch.

Fair enough.  I took it more as meaning,  "this would fix this problem /and/ 
it's funny too".  Apologies.

> In other words, I am not sure if there is anything worth fixing.

After a bit of thought; I think I agree.


Andy

-- 
Dr Andrew Parkins, M Eng (Hons), AMIEE
andyparkins@gmail.com

Re: git ls-files -o under .git/ prints all repository files

[Yasushi SHOJI]

At Thu, 18 Jan 2007 22:47:47 -0800,
Junio C Hamano wrote:
> 
> Yasushi SHOJI <yashi@atmark-techno.com> writes:
> 
> > ls-files -o prints all files under .git if you are in the .git
> > directory.  this is pretty dangerous since we now have git clean to
> > delete files marked others.
> >
> > sure in UNIX env., you can easily shoot yourself in the foot. but it'd
> > might be nice to help newbies.
> 
> It's amusing to see that people can find obscure ways to shoot
> themselves in the foot.
> 
> Amusing problems deserve an equally amusing solution.

Unfortunately, the amusing ;-) solution doesn't prevent them all.

$ cd .git/objects
$ git ls-fiels -o
0f/902e4635d4d7b8e532b485eeeb6399d0910710
bc/2d2dcb34e8313627d45ad6ef38beddf560501d
ce/013625030ba8dba906f756967f9e9ca394464a
$ git clean -d -x
Removing 0f/
Removing bc/
Removing ce/
Removing info/
Removing pack/

I presume that if the cwd is the direct decedent of the current
GIT_DIR, ls-files should print error saying "you are not in the
working dir".

or, just ignore '.git/*' all the time.

$ git ls-files -o
HEAD
config
 :
$ git ls-files -o --exclude='.git/*'
$ 

What do you think?
-- 
         yashi

Re: git ls-files -o under .git/ prints all repository files

[Simon 'corecode' Schubert]

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

Junio C Hamano wrote:
> Yasushi SHOJI <yashi@atmark-techno.com> writes:
> 
>> ls-files -o prints all files under .git if you are in the .git
>> directory.  this is pretty dangerous since we now have git clean to
>> delete files marked others.
>>
>> sure in UNIX env., you can easily shoot yourself in the foot. but it'd
>> might be nice to help newbies.
> 
> It's amusing to see that people can find obscure ways to shoot
> themselves in the foot.
> 
> Amusing problems deserve an equally amusing solution.

I guess you are not serious.  I wonder, why does git-ls-files ever list files under .git?  I'd just say:  fail if you want to list $GIT_DIR.  Maybe other tools should do so as well.

% cd .hg && hg status -A .
abort: path contains illegal component: .hg

I think this is a sensible thing to do.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: git ls-files -o under .git/ prints all repository files

[Alex Riesen]

On 1/19/07, Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:
> >
> > Amusing problems deserve an equally amusing solution.
>
> I guess you are not serious.  I wonder, why does git-ls-files ever
> list files under .git?  I'd just say:  fail if you want to list $GIT_DIR.

Not list. Clean. What's wrong with listing them?

>  Maybe other tools should do so as well.
>
> % cd .hg && hg status -A .
> abort: path contains illegal component: .hg
>
> I think this is a sensible thing to do.

No, it isn't. It is not unlikely to have repo in repo
(and some people already have them).
Mercurial is wrong here.

Re: git ls-files -o under .git/ prints all repository files

[Simon 'corecode' Schubert]

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

Alex Riesen wrote:
>> I guess you are not serious.  I wonder, why does git-ls-files ever
>> list files under .git?  I'd just say:  fail if you want to list $GIT_DIR.
> 
> Not list. Clean. What's wrong with listing them?

i would claim .git to be off limits and unrelated to the working dir (file-wise).  if you want to list files there, do a find . or so.  After all you wouldn't expect cd /usr && git-ls-files -o work there unless you have a /.git or /usr/.git, right?

>>  Maybe other tools should do so as well.
>>
>> % cd .hg && hg status -A .
>> abort: path contains illegal component: .hg
>>
>> I think this is a sensible thing to do.
> 
> No, it isn't. It is not unlikely to have repo in repo
> (and some people already have them).
> Mercurial is wrong here.

what do you mean with repo-in-repo?  something like .git/.git?  My suggestion does not break this:

% mkdir foo && cd foo && git init
% cd .git && git init
% git ls-files -o
HEAD
config
description
hooks/applypatch-msg
hooks/commit-msg
hooks/post-commit
hooks/post-update
hooks/pre-applypatch
hooks/pre-commit
hooks/pre-rebase
hooks/update
info/exclude
% git ls-files -o ..
fatal: '..' is outside repository

Here the repo root is "foo/.git" and not "foo".

So my suggestion still stands:  .git is off limits.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: git ls-files -o under .git/ prints all repository files

[Alex Riesen]

On 1/19/07, Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:
> >> I guess you are not serious.  I wonder, why does git-ls-files ever
> >> list files under .git?  I'd just say:  fail if you want to list $GIT_DIR.
> >
> > Not list. Clean. What's wrong with listing them?
>
> i would claim .git to be off limits and unrelated to the working dir
> (file-wise).  if you want to list files there, do a find . or so.
>  After all you wouldn't expect cd /usr && git-ls-files -o work there
> unless you have a /.git or /usr/.git, right?

Right, just see no practical point changing ls-file for that.

> >>  Maybe other tools should do so as well.
> >>
> >> % cd .hg && hg status -A .
> >> abort: path contains illegal component: .hg
> >>
> >> I think this is a sensible thing to do.
> >
> > No, it isn't. It is not unlikely to have repo in repo
> > (and some people already have them).
> > Mercurial is wrong here.
>
> what do you mean with repo-in-repo?  something like .git/.git?

Actually, I meant a/b/, with existing a/.git and b/.git, which is
obviously is not a case here (nor in mercurial). Stupid me

>  My suggestion does not break this:
>
> % mkdir foo && cd foo && git init
> % cd .git && git init
> % git ls-files -o
> HEAD
> config
> description
> hooks/applypatch-msg

I can imagine keeping hooks under git control.
In this case path(pwd) does contain .git component
(as in .hg example).

> Here the repo root is "foo/.git" and not "foo".
>
> So my suggestion still stands:  .git is off limits.
>

Ok. Have nothing strong against this

Re: git ls-files -o under .git/ prints all repository files

[Simon 'corecode' Schubert]

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

Alex Riesen wrote:
>> i would claim .git to be off limits and unrelated to the working dir
>> (file-wise).  if you want to list files there, do a find . or so.
>>  After all you wouldn't expect cd /usr && git-ls-files -o work there
>> unless you have a /.git or /usr/.git, right?
> Right, just see no practical point changing ls-file for that.

right.  .git should be forbidden in higher layers already.

> I can imagine keeping hooks under git control.
> In this case path(pwd) does contain .git component
> (as in .hg example).

doesn't work either:

% cd .git/hooks
% git add *
fatal: unable to add .git/hooks/applypatch-msg to index

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: git ls-files -o under .git/ prints all repository files

[Alex Riesen]

On 1/19/07, Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:
> Alex Riesen wrote:
> >> i would claim .git to be off limits and unrelated to the working dir
> >> (file-wise).  if you want to list files there, do a find . or so.
> >>  After all you wouldn't expect cd /usr && git-ls-files -o work there
> >> unless you have a /.git or /usr/.git, right?
> > Right, just see no practical point changing ls-file for that.
>
> right.  .git should be forbidden in higher layers already.

That's where I disagree. git-clean shouldn't clean it, but
git-ls-files will do no harm to the directory

> > I can imagine keeping hooks under git control.
> > In this case path(pwd) does contain .git component
> > (as in .hg example).
>
> doesn't work either:
>
> % cd .git/hooks
> % git add *
> fatal: unable to add .git/hooks/applypatch-msg to index

cd .git
git init
git add .
git commit

Works. And the path contains .git component. And git-clean
here is ok. The test should check if we are in $GIT_DIR
and probably $GIT_DIR/{objects,refs,logs}, not just below
.git (with ".git" anywhere in pwd, which the mercurial
example seem to suggest).

Re: git ls-files -o under .git/ prints all repository files

[Simon 'corecode' Schubert]

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

Alex Riesen wrote:
>> >> i would claim .git to be off limits and unrelated to the working dir
>> >> (file-wise).  if you want to list files there, do a find . or so.
>> >>  After all you wouldn't expect cd /usr && git-ls-files -o work there
>> >> unless you have a /.git or /usr/.git, right?
>> > Right, just see no practical point changing ls-file for that.
>> right.  .git should be forbidden in higher layers already.
> 
> That's where I disagree. git-clean shouldn't clean it, but
> git-ls-files will do no harm to the directory

of course git-ls-files will do no harm.  but "fixing" every consumer of git-ls-files seems wrong to me.

okay, what do I expect when doing cd .git && git-ls-files?  Either listing *all files* in the repo (like git-ls-files from the repo root) or no files at all, or failure (".git is private").

To add some facts to it:

GIT-LS-FILES(1)                                                GIT-LS-FILES(1)

NAME
       git-ls-files - Information about files in the index/working directory

That's pretty clear to me.  Working directory.  .git is *not* part of the working directory.


>> > I can imagine keeping hooks under git control.
>> > In this case path(pwd) does contain .git component
>> > (as in .hg example).
>>
>> doesn't work either:
>>
>> % cd .git/hooks
>> % git add *
>> fatal: unable to add .git/hooks/applypatch-msg to index
> 
> cd .git
> git init
> git add .
> git commit
> 
> Works. And the path contains .git component. And git-clean
> here is ok. The test should check if we are in $GIT_DIR
> and probably $GIT_DIR/{objects,refs,logs}, not just below
> .git (with ".git" anywhere in pwd, which the mercurial
> example seem to suggest).

No, the path does *not* contain a .git component.  You just committed to the root of the *inside* repo.

Of course I don't say "refuse operation if there is .git in the path".  What I mean is, "refuse operation if there is $GIT_DIR in the path".  Maybe my example was not complete enough.  With mercurial, you can as well have a .hg in .hg.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: git ls-files -o under .git/ prints all repository files

[Alex Riesen]

On 1/19/07, Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:
> Alex Riesen wrote:
> >> >> i would claim .git to be off limits and unrelated to the working dir
> >> >> (file-wise).  if you want to list files there, do a find . or so.
> >> >>  After all you wouldn't expect cd /usr && git-ls-files -o work there
> >> >> unless you have a /.git or /usr/.git, right?
> >> > Right, just see no practical point changing ls-file for that.
> >> right.  .git should be forbidden in higher layers already.
> >
> > That's where I disagree. git-clean shouldn't clean it, but
> > git-ls-files will do no harm to the directory
>
> of course git-ls-files will do no harm.  but "fixing" every consumer of
> git-ls-files seems wrong to me.

There are not that many users of ls-files, which could harm a repo.
Besides of git-clean, cannot think of any.

> okay, what do I expect when doing cd .git && git-ls-files?
>  Either listing *all files* in the repo (like git-ls-files from the
> repo root) or no files at all, or failure (".git is private").

List nothing. That's what it does. It could return non-0
(which it does not), but aside from that,... looks very sensible.

> NAME
>        git-ls-files - Information about files in the index/working directory
>
> That's pretty clear to me.  Working directory.  .git is *not* part of the working directory.
>

Alright, it is not. I can even imagine someone having a script
containing "git-ls-files -o| rm -f; git reset --hard" to get clean working dir,
and starting the script in .git one day. Make "-o" list nothing as well?

> > Works. And the path contains .git component. And git-clean
> > here is ok. The test should check if we are in $GIT_DIR
> > and probably $GIT_DIR/{objects,refs,logs}, not just below
> > .git (with ".git" anywhere in pwd, which the mercurial
> > example seem to suggest).
>
> No, the path does *not* contain a .git component.  You just
> committed to the root of the *inside* repo.

"$OLDPWD/.git/hooks". It does contain ".git" :)

Re: git ls-files -o under .git/ prints all repository files

[Simon 'corecode' Schubert]

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

Alex Riesen wrote:
>> okay, what do I expect when doing cd .git && git-ls-files?
>>  Either listing *all files* in the repo (like git-ls-files from the
>> repo root) or no files at all, or failure (".git is private").
> 
> List nothing. That's what it does. It could return non-0
> (which it does not), but aside from that,... looks very sensible.

yah, to fail or not to fail.  I'd still say listing in .git is invalid, hence fail.

> Alright, it is not. I can even imagine someone having a script
> containing "git-ls-files -o| rm -f; git reset --hard" to get clean 
> working dir,
> and starting the script in .git one day. Make "-o" list nothing as well?

yes, I actually wanted to talk about -o, but forgot to mention.  Following my reasoning above, it should fail as well.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: git ls-files -o under .git/ prints all repository files

[Andreas Ericsson]

Alex Riesen wrote:
> On 1/19/07, Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:
>> >
>>
>> % cd .hg && hg status -A .
>> abort: path contains illegal component: .hg
>>
>> I think this is a sensible thing to do.
> 
> No, it isn't. It is not unlikely to have repo in repo
> (and some people already have them).
> Mercurial is wrong here.

For managing repos inside repos (onion repos?) I think it should
be safe to abort if we're not at top-level.

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

Re: git ls-files -o under .git/ prints all repository files

[Matthias Kestenholz]

On Fri, 2007-01-19 at 14:30 +0100, Andreas Ericsson wrote:
> Alex Riesen wrote:
> > On 1/19/07, Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:
> >> >
> >>
> >> % cd .hg && hg status -A .
> >> abort: path contains illegal component: .hg
> >>
> >> I think this is a sensible thing to do.
> > 
> > No, it isn't. It is not unlikely to have repo in repo
> > (and some people already have them).
> > Mercurial is wrong here.
> 
> For managing repos inside repos (onion repos?) I think it should
> be safe to abort if we're not at top-level.
> 


Why not check for /.git/ somewhere inside the current working directory
(pwd) ? That's the way mercurial does it currently, and I think that is
a sane thing to do _if_ you want to protect the user from his own
stupidity.

Re: git ls-files -o under .git/ prints all repository files

[Johannes Schindelin]

Hi,

On Fri, 19 Jan 2007, Matthias Kestenholz wrote:

> On Fri, 2007-01-19 at 14:30 +0100, Andreas Ericsson wrote:
> > Alex Riesen wrote:
> > > On 1/19/07, Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:
> > >> >
> > >>
> > >> % cd .hg && hg status -A .
> > >> abort: path contains illegal component: .hg
> > >>
> > >> I think this is a sensible thing to do.
> > > 
> > > No, it isn't. It is not unlikely to have repo in repo
> > > (and some people already have them).
> > > Mercurial is wrong here.
> > 
> > For managing repos inside repos (onion repos?) I think it should
> > be safe to abort if we're not at top-level.
> > 
> 
> 
> Why not check for /.git/ somewhere inside the current working directory 
> (pwd) ? That's the way mercurial does it currently, and I think that is 
> a sane thing to do _if_ you want to protect the user from his own 
> stupidity.

There are valid reasons why you might want to have a (possibly 
temporary) repository _inside_ the GIT_DIR. You'd break these cases.

Ciao,
Dscho

Re: git ls-files -o under .git/ prints all repository files

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 19 Jan 2007, Matthias Kestenholz wrote:
> ...
>> Why not check for /.git/ somewhere inside the current working directory 
>> (pwd) ? That's the way mercurial does it currently, and I think that is 
>> a sane thing to do _if_ you want to protect the user from his own 
>> stupidity.
>
> There are valid reasons why you might want to have a (possibly 
> temporary) repository _inside_ the GIT_DIR. You'd break these cases.

You are right that strstr(here, "/.git/") is not a good check.

If we really care about this problem (and I am not yet starting
to think we might, but who knows, I reserve the right to change
my mind every once in a while), we could make the commands that
deal with working trees (that is, among the things under
discussion in this thread, 'git-clean' always is, and
'git-ls-files' only when it is given options like '-o', '-k',
'-m', '-i') when the cwd is GIT_DIR or a subdirectory of it.

If you did something like:

	mkdir /var/tmp/a
        cd /var/tmp/a
        git init-db
        cd .git
        GIT_DIR=.git git init-db
        git add .
	git ls-files
	echo junk >garbage
        git clean

the repository at /var/tmp/a/.git/.git ought to track HEAD,
config and friends in /var/tmp/a/.git directory.

Not that I am saying I think the above is a sensible use case.

Re: git ls-files -o under .git/ prints all repository files

[Yasushi SHOJI]

At Fri, 19 Jan 2007 11:03:17 -0800,
Junio C Hamano wrote:
> 
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Fri, 19 Jan 2007, Matthias Kestenholz wrote:
> > ...
> >> Why not check for /.git/ somewhere inside the current working directory 
> >> (pwd) ? That's the way mercurial does it currently, and I think that is 
> >> a sane thing to do _if_ you want to protect the user from his own 
> >> stupidity.
> >
> > There are valid reasons why you might want to have a (possibly 
> > temporary) repository _inside_ the GIT_DIR. You'd break these cases.
> 
> You are right that strstr(here, "/.git/") is not a good check.
> 
> If we really care about this problem (and I am not yet starting
> to think we might, but who knows, I reserve the right to change
> my mind every once in a while), we could make the commands that
> deal with working trees (that is, among the things under
> discussion in this thread, 'git-clean' always is, and
> 'git-ls-files' only when it is given options like '-o', '-k',
> '-m', '-i') when the cwd is GIT_DIR or a subdirectory of it.

I believe it should be done.  because it used to be safe during v0.9
time.  most command didn't work if you are not in the root dir of a
repo. during the development time, we have been adding feature so that
we don't have to be at a root dir to exec git.  we just forgot to
check we are under .git, the repo dir.

I assume that we can either have 1) one more bit in struct
cmd_struct's option field and fail if the command isn't allowed to run
under repository dir, or 2) some mechanism to check prefix, the third
argument of command entry point function, and behave properly.

> If you did something like:
> 
> 	mkdir /var/tmp/a
>         cd /var/tmp/a
>         git init-db
>         cd .git
>         GIT_DIR=.git git init-db
>         git add .
> 	git ls-files
> 	echo junk >garbage
>         git clean
> 
> the repository at /var/tmp/a/.git/.git ought to track HEAD,
> config and friends in /var/tmp/a/.git directory.

this should always work.
-- 
        yashi

[PATCH] Commands requiring a work tree must not run in GIT_DIR

[Johannes Schindelin]

This patch helps when you accidentally run something like git-clean
in the git directory instead of the work tree.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	On Tue, 23 Jan 2007, Yasushi SHOJI wrote:
	
	> At Fri, 19 Jan 2007 11:03:17 -0800,
	> Junio C Hamano wrote:
	> > 
	> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
	> > 
	> > > On Fri, 19 Jan 2007, Matthias Kestenholz wrote:
	> > > ...
	> > >> Why not check for /.git/ somewhere inside the current 
	> > >> working directory (pwd) ? That's the way mercurial does it 
	> > >> currently, and I think that is a sane thing to do _if_ you 
	> > >> want to protect the user from his own stupidity.
	> > >
	> > > There are valid reasons why you might want to have a 
	> > > (possibly temporary) repository _inside_ the GIT_DIR. You'd 
	> > > break these cases.
	> > 
	> > You are right that strstr(here, "/.git/") is not a good check.
	> > 
	> > If we really care about this problem (and I am not yet 
	> > starting to think we might, but who knows, I reserve the right 
	> > to change my mind every once in a while), we could make the 
	> > commands that deal with working trees (that is, among the 
	> > things under discussion in this thread, 'git-clean' always is, 
	> > and 'git-ls-files' only when it is given options like '-o', 
	> > '-k', '-m', '-i') when the cwd is GIT_DIR or a subdirectory of 
	> > it.
	> 
	> I believe it should be done.

	I only did that for the mentioned commands; your homework: find 
	all other commands (or options) which need that.

 builtin-ls-files.c  |   10 +++++++++-
 builtin-rev-parse.c |    5 +++++
 git-sh-setup.sh     |    3 ++-
 git.c               |    5 +++--
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 21c2a6e..ac89eb2 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -323,7 +323,7 @@ static const char ls_files_usage[] =
 int cmd_ls_files(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	int exc_given = 0;
+	int exc_given = 0, require_work_tree = 0;
 	struct dir_struct dir;
 
 	memset(&dir, 0, sizeof(dir));
@@ -363,14 +363,17 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		}
 		if (!strcmp(arg, "-m") || !strcmp(arg, "--modified")) {
 			show_modified = 1;
+			require_work_tree = 1;
 			continue;
 		}
 		if (!strcmp(arg, "-o") || !strcmp(arg, "--others")) {
 			show_others = 1;
+			require_work_tree = 1;
 			continue;
 		}
 		if (!strcmp(arg, "-i") || !strcmp(arg, "--ignored")) {
 			dir.show_ignored = 1;
+			require_work_tree = 1;
 			continue;
 		}
 		if (!strcmp(arg, "-s") || !strcmp(arg, "--stage")) {
@@ -379,6 +382,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		}
 		if (!strcmp(arg, "-k") || !strcmp(arg, "--killed")) {
 			show_killed = 1;
+			require_work_tree = 1;
 			continue;
 		}
 		if (!strcmp(arg, "--directory")) {
@@ -447,6 +451,10 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
+	if (require_work_tree &&
+			(is_bare_repository() || is_inside_git_dir()))
+		die("This operation must be run in a work tree");
+
 	pathspec = get_pathspec(prefix, argv + i);
 
 	/* Verify that the pathspec matches the prefix */
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 3b716fb..d53deaa 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -347,6 +347,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				printf("%s/.git\n", cwd);
 				continue;
 			}
+			if (!strcmp(arg, "--is-inside-git-dir")) {
+				printf("%s\n", is_inside_git_dir() ? "true"
+						: "false");
+				continue;
+			}
 			if (!strncmp(arg, "--since=", 8)) {
 				show_datestring("--max-age=", arg+8);
 				continue;
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 6b1c142..9114c49 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -48,7 +48,8 @@ cd_to_toplevel () {
 }
 
 require_work_tree () {
-	test $(is_bare_repository) = false ||
+	test $(is_bare_repository) = false &&
+	test $(git-rev-parse --is-inside-git-dir) = false ||
 	die "fatal: $0 cannot be used without a working tree."
 }
 
diff --git a/git.c b/git.c
index 5133a07..2027d1c 100644
--- a/git.c
+++ b/git.c
@@ -302,8 +302,9 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 			prefix = setup_git_directory();
 		if (p->option & USE_PAGER)
 			setup_pager();
-		if ((p->option & NOT_BARE) && is_bare_repository())
-			die("%s cannot be used in a bare git directory", cmd);
+		if ((p->option & NOT_BARE) &&
+				(is_bare_repository() || is_inside_git_dir()))
+			die("%s must be run in a work tree", cmd);
 		trace_argv_printf(argv, argc, "trace: built-in: git");
 
 		exit(p->fn(argc, argv, prefix));

Re: [PATCH] Commands requiring a work tree must not run in GIT_DIR

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> This patch helps when you accidentally run something like git-clean
> in the git directory instead of the work tree.

I think "require_work_tree" reflects what we are trying to do
much better than NOT_BARE.  So maybe we should rename NOT_BARE
to REQUIRE_WORK_TREE.

Existing check function is_bare_repository() is sometimes used
to see if it is a bare repository regardless of where you are
(e.g. refs.c::log_ref_write()), so that function can stay as is,
but the combined check below (you seem to have a few instances
in your patch) can be made into a function require_work_tree().

> diff --git a/git.c b/git.c
> index 5133a07..2027d1c 100644
> --- a/git.c
> +++ b/git.c
> @@ -302,8 +302,9 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
>  			prefix = setup_git_directory();
>  		if (p->option & USE_PAGER)
>  			setup_pager();
> -		if ((p->option & NOT_BARE) && is_bare_repository())
> -			die("%s cannot be used in a bare git directory", cmd);
> +		if ((p->option & NOT_BARE) &&
> +				(is_bare_repository() || is_inside_git_dir()))
> +			die("%s must be run in a work tree", cmd);
>  		trace_argv_printf(argv, argc, "trace: built-in: git");
>  
>  		exit(p->fn(argc, argv, prefix));

Similar to the "conditionally require working tree" you did to
ls-files, "apply --index" and perhaps "apply --cached" (but this
is "perhaps" --- you _could_ have an index in a bare repository,
although it is debatable if there is a valid use case for it),
grep (grep_cache() but perhaps !cached for the same reason),
"read-tree -u", "rerere", "update-index" (except --index-info
and friends that feed object names directly without using
working tree), should require working tree.

On the script front, bisect should require working tree, but I
do not think anybody is stupid enough to start bisecting in a
bare repository ;-)

Re: [PATCH] Commands requiring a work tree must not run in GIT_DIR

[Johannes Schindelin]

Hi,

On Wed, 24 Jan 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > This patch helps when you accidentally run something like git-clean
> > in the git directory instead of the work tree.
> 
> I think "require_work_tree" reflects what we are trying to do
> much better than NOT_BARE.  So maybe we should rename NOT_BARE
> to REQUIRE_WORK_TREE.

Hm. Might make sense.

But there is a subtle trap here: if a repo is not bare, it does have a 
work tree. But what we want here actually is NOT_INSIDE_GIT_DIR:

It is perfectly sensible to run git-pull from inside the git dir, since it 
has to cd to the top _anyway_. So, git-pull needs a work tree, but does 
not forbid running from within GIT_DIR.

> Existing check function is_bare_repository() is sometimes used
> to see if it is a bare repository regardless of where you are
> (e.g. refs.c::log_ref_write()), so that function can stay as is,
> but the combined check below (you seem to have a few instances
> in your patch) can be made into a function require_work_tree().
> 
> > diff --git a/git.c b/git.c
> > index 5133a07..2027d1c 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -302,8 +302,9 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
> >  			prefix = setup_git_directory();
> >  		if (p->option & USE_PAGER)
> >  			setup_pager();
> > -		if ((p->option & NOT_BARE) && is_bare_repository())
> > -			die("%s cannot be used in a bare git directory", cmd);
> > +		if ((p->option & NOT_BARE) &&
> > +				(is_bare_repository() || is_inside_git_dir()))
> > +			die("%s must be run in a work tree", cmd);
> >  		trace_argv_printf(argv, argc, "trace: built-in: git");
> >  
> >  		exit(p->fn(argc, argv, prefix));
> 
> Similar to the "conditionally require working tree" you did to
> ls-files, "apply --index" and perhaps "apply --cached" (but this
> is "perhaps" --- you _could_ have an index in a bare repository,
> although it is debatable if there is a valid use case for it),
> grep (grep_cache() but perhaps !cached for the same reason),
> "read-tree -u", "rerere", "update-index" (except --index-info
> and friends that feed object names directly without using
> working tree), should require working tree.

As I said, maybe we have to introduce DISALLOW_INSIDE_GIT_DIR (which is 
ugly, since it is so long, but I cannot think of anything saner), and 
likewise "disallow_inside_GIT_DIR" for scripts.

> On the script front, bisect should require working tree, but I do not 
> think anybody is stupid enough to start bisecting in a bare repository 
> ;-)

OTOH it is really easy:

diff --git a/git-bisect.sh b/git-bisect.sh
index 6da31e8..f8a2b64 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -11,6 +11,7 @@ git bisect replay <logfile>	replay bisection log
 git bisect log			show bisect log.'
 
 . git-sh-setup
+require_work_tree
 
 sq() {
 	@@PERL@@ -e '


Ciao,
Dscho

Re: [PATCH] Commands requiring a work tree must not run in GIT_DIR

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I think "require_work_tree" reflects what we are trying to do
>> much better than NOT_BARE.  So maybe we should rename NOT_BARE
>> to REQUIRE_WORK_TREE.
>
> Hm. Might make sense.
>
> But there is a subtle trap here: if a repo is not bare, it does have a 
> work tree. But what we want here actually is NOT_INSIDE_GIT_DIR:

I think we are saying the same thing: "require to be IN the
working tree" (hence not in ".git/objects", for example).

> It is perfectly sensible to run git-pull from inside the git dir, since it 
> has to cd to the top _anyway_.

I would not call it "perfectly sensible".  I never understood
why anybody would want to cd to .git/ in a repository with an
working tree while actually working on the files in the working
tree (e.g. doing merges and pulls and edits and commits) [*1*].
I would say it is in the "allowing it is cheap and harmless so
why not" category.

[*1*] But it's probably just me who almost always is in Emacs;
switching to a shell terminal and saying "cd .git && vi config"
is much more expensive than "^X^F .git/config").

Re: git ls-files -o under .git/ prints all repository files

[Alex Riesen]

On 1/19/07, Junio C Hamano <junkio@cox.net> wrote:
>  static void safe_create_dir(const char *dir, int share)
>  {
> -       if (mkdir(dir, 0777) < 0) {
> +       mode_t mode;
> +
> +       mode = share ? 0777 : 0333;
> +       if (mkdir(dir, mode) < 0) {

Does not work for existing directories, does not work
on FAT and alike at all.

Re: git ls-files -o under .git/ prints all repository files

[Alex Riesen]

On 1/19/07, Yasushi SHOJI <yashi@atmark-techno.com> wrote:
> sure in UNIX env., you can easily shoot yourself in the foot. but it'd
> might be nice to help newbies.

It's is not just Unix. Humans are known for shooting themselves
in feet, heads and other parts of their bodies. It's a popular way
of taking life in own hands.

Seriously, though, git-clean can just refuse cleaning $GIT_DIR.