git ate my home directory :-(

Hi!

Today I've discovered that on the build server my home directory was empty. A post-mortem analysis showed that the git-clean command I've added to my kernel build script is the evil doer. In my scripts I'm setting GIT_DIR to use git-fetch and git-reset without changing the current working directory all the time. But calling git-clean with GIT_DIR acts basically like a "rm -Rf .".

Here a small demo:

test@linux:~> git --version git version 1.8.1.4 test@linux:~> ls test@linux:~> touch a b c d e test@linux:~> mkdir x test@linux:~> cd x test@linux:~/x> git init Initialized empty Git repository in /home/test/x/.git/ test@linux:~/x> cd .. test@linux:~> ls a b c d e x test@linux:~> export GIT_DIR=/home/test/x/.git/ test@linux:~> git clean -d -f Removing a Removing b Removing c Removing d Removing e Removing x/ test@linux:~> ls test@linux:~> test@linux:~> # :-(

Is this behavior intended?

Thanks, //richard

Hi,

Richard Weinberger wrote:

Yeah, for historical reasons GIT_WORK_TREE defaults to $(pwd) when GIT_DIR is explicitly set.

In git versions including the patch 2cd83d10bb6b (setup: suppress implicit "." work-tree for bare repos, 2013-03-08, currently in "next" but not "master"), you can set GIT_IMPLICIT_WORK_TREE=0 to avoid this behavior.

Thanks for a useful example, and sorry for the trouble.

Sincerely, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

WAT?

Junio C Hamano wrote:

Is that false?

If I understand the history correctly, the ability to set the GIT_DIR envvar was meant to allow a person to keep their .git directory outside the worktree. So you can do:

	git init my-favorite-repo
	cd my-favorite-repo
	...work as usual...

	# cleaning time!
	mv .git ~/my-favorite-repo-metadata.git
	GIT_DIR=$HOME/my-favorite-repo-metadata.git; export GIT_DIR
	... work as usual...

If you want to set GIT_DIR and treat it as a bare repository, the sane way to do that is simply

	cd ~/my-favorite-bare-repository.git
	... use git as usual ...

But if something (for example relative paths used by your script) ties your cwd somewhere else, you might really want to do

	GIT_DIR=~/my-favorite-bare-repository.git; export GIT_DIR
	... work as usual ...

and as a side effect of Jeff's patch there is now a mechanism to do that:

	GIT_IMPLICIT_WORK_TREE=0; export GIT_IMPLICIT_WORK_TREE
	GIT_DIR=~/my-favorite-bare-repository.git; export GIT_DIR
	... work as usual ...

This is of course unsafe because it ties your usage to a specific version of git. And the variable is not advertised in the documentation.

Jonathan Nieder <jrnieder@gmail.com> writes:

... as usual except that you have to be at the top.

And that is why GIT_WORK_TREE was invented, so that you can anchor where the top of the tree is with that variable and then chdir around into its subdirectories.

Also later we added core.worktree so that $GIT_DIR/config can say where its associated working tree is.

We decided not to advitise it exactly because we do not intend to guarantee that will be the way that variable will work. It is an implementation detail of that "alias" stuff in the topic it addresses.

The documented way is to point at the tip with GIT_WORK_TREE.

Jonathan Nieder <jrnieder@gmail.com> writes:

And it *WILL* be that way til the end of time. Unless you are at the top level of your working tree, you are supposed to tell where the top level is with GIT_WORK_TREE when you use GIT_DIR. Always.

And that is the answer you should be giving here, not implicit stuff, which is an implementation detail to help aliases. I do not know how things will break when the end user sets and exports it to the environment, and I do not think we would want to make any promise on how it works.

Am 25.03.2013 23:06, schrieb Junio C Hamano:

Okay, I have to set GIT_DIR _and_ GIT_WORK_TREE to make my scripts safe again? I've always set only GIT_DIR because it just worked (till today...).

Thanks, //richard

Richard Weinberger wrote:

chdir-ing into the git repo without setting any GIT_* vars is probably the simplest way to go.

Richard Weinberger <richard@nod.at> writes:

That means you never run your script inside a subdirectory ;-)

If your $GIT_DIR is tied to a single working tree, a simpler way would be to add

	[core]
		worktree = /path/to/the/work/tree/

to $GIT_DIR/config.

Am 25.03.2013 23:20, schrieb Junio C Hamano:

I've used GIT_DIR in my scripts because changing the current working directory within bash scripts often causes problem with other commands. That's why I've used patters like: export GIT_DIR=/path/to/repo/.git git fetch ... do_this do_that git reset --hard FETCH_HEAD do_foo git whatever

export GIT_DIR=/path/to/another/repo/.git git fetch ... ...

But from now on I'll simply cd into the git repo...

Thanks, //richard

Junio C Hamano wrote:

That's a reasonable desire, and it means it's a good thing we noticed this before the envvar escaped to "master". People *will* use such exposed interfaces unless they are clearly marked as internal. That's just a fact of life.

Here's a rough patch to hopefully improve matters.

Longer term, it would be nice to have something like GIT_IMPLICIT_WORK_TREE exposed to let scripts cache the result of the search for .git. Maybe something like "GIT_BARE=(arbitrary value)" would be a good interface.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---

On Mon, Mar 25, 2013 at 3:13 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Maybe the environment variable for internal-use-only should be prefixed with an underscore?

-Brandon

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Monday, March 25, 2013 10:06 PM

Should this important warning be part of the git(1) documentation on the environment variables (and possibly other places) given the consequences of this case? It wasn't something I'd appreciated from a simple reading.

Philip 

On Tue, Mar 26, 2013 at 08:02:30AM -0000, Philip Oakley wrote:

Something like this, maybe?

-- 8< --
Subject: [PATCH] git.txt: document the implicit working tree setting with GIT_DIR

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git.txt | 2 ++
 1 file changed, 2 insertions(+)

On Tue, Mar 26, 2013 at 04:48:44PM +0700, Nguyen Thai Ngoc Duy wrote:

I think this is a good thing to mention, but a few nits:

  1. core.worktree is another way of setting it

  2. This can also be overridden by --bare (at least in "next").

  3. I think having core.bare set will also override this

-Peff

Jeff King <peff@peff.net> writes:

Yeah. And sorry I kept typing alias. I obviously meant "bare"; the user visible symptom is closely linked to the use of alias, but the most important aspect of the change in 2cd83d10bb6b (setup: suppress implicit "." work-tree for bare repos, 2013-03-08) is about being in a bare repository where by definition working tree does not exist.

On Tue, Mar 26, 2013 at 10:04 PM, Jeff King <peff@peff.net> wrote:

Yeah, I looked back at t1510 and gave up. I think it's still true:

-- 8< -- A few rules for repo setup:

1. GIT_DIR is relative to user's cwd. --git-dir is equivalent to
   GIT_DIR.

2. .git file is relative to parent directory. .git file is basically
   symlink in disguise. The directory where .git file points to will
   become new git_dir.

3. core.worktree is relative to git_dir.

4. GIT_WORK_TREE is relative to user's cwd. --work-tree is
   equivalent to GIT_WORK_TREE.

5. GIT_WORK_TREE/core.worktree was originally meant to work only if
   GIT_DIR is set, but earlier git didn't enforce it, and some scripts
   depend on the implementation that happened to first discover .git by
   going up from the users $cwd and then using the specified working tree
   that may or may not have any relation to where .git was found in.  This
   historical behaviour must be kept.

6. Effective GIT_WORK_TREE overrides core.worktree and core.bare

7. Effective core.worktree conflicts with core.bare

8. If GIT_DIR is set but neither worktree nor bare setting is given,
   original cwd becomes worktree.
-- 8< --

From: "Duy Nguyen" <pclouds@gmail.com>
Sent: Tuesday, March 26, 2013 9:48 AM

I didn't feel this conveyed the Dire Warning effect that would be needed to avoid the original misunderstanding.

It is easy to miss some of the potential consequences when other priorities are pressing.

As Junio wondered, perhaps rhetorically, in a later message "Why do these people set GIT_DIR without setting GIT_WORK_TREE in the first place?"

Perhaps "If the GIT_DIR environment variable is set then it specifies a path to use instead of the default `.git` for the base of the repository. Note that the current directory `.` will be used as the working GIT_WORK_TREE, if not set elsewhere. The --git-dir command-line option also sets the GIT_DIR environment variable."

Philip 

Am 26.03.2013 09:02, schrieb Philip Oakley:

BTW: Can't we change git-clean such that it will not delete any files if GIT_DIR is set and GIT_WORK_TREE is "."?

Thanks, //richard

On Tue, Mar 26, 2013 at 02:07:44PM +0100, Richard Weinberger wrote:

We could, but that would break the existing behavior for other people (and I assume you mean "when GIT_WORK_TREE is not set at all", as I would think GIT_WORK_TREE=. is explicit enough).

I am sympathetic to your data loss, but I wonder how common a problem it is in practice. Git-clean already does a dry-run by default; you have to give it `-f`. This is the first such report we've had. This seems more akin to "oops, I accidentally ran `rm -rf` in the wrong directory". Yes, it's catastrophic, but at some point you have to accept that deleting files is what rm (and git-clean) does; you can only put so many safety hoops in place.

I don't know. It's an uncommon enough case that we could deprecate "GIT_WORK_TREE is implicitly `.`" entirely, but I think it would need a deprecation period, and a way to get the same behavior (e.g., allowing "GIT_WORK_TREE=.").

-Peff

Am 26.03.2013 15:56, schrieb Jeff King:

Is there a valid use case to call git-clean with GIT_DIR set but GIT_WORK_TREE not (or to ."")? It will delete "." ;)

The data loss was not too bad. I was able to restore anything within 2 hours. But was kinda shocked that git-clean deletes files outside my git tree. I'm aware of -d. But in my case it happened within a fully automated script. I simply thought GIT_DIR=.. git-clean -f -d does the right thing...

Yeah, this sounds sane.

Thanks, //richard

P.s: I've told this story to some friends and co-workers which use git like me very day. All of them were shocked about the behavior of git-clean and GIT_DIR.

On 26 March 2013 18:06, Richard Weinberger <richard@nod.at> wrote:

Seconded. At $work lots of people started asking anxious questions about this. It was suggested it is a potential security hole, although I am not sure I agree, but the general idea being that if you could manage to set this var in someones environment then they might use git to do real damage to a system. (The counterargument being that if you can set that in someones environment you can do worse already... But im a not a security type so I cant say)

As a knee-jerk response we will be armoring various scripts we have that use git automatically to refuse to run if GIT_DIR is set. I suspect a lot of people will be doing the same.

cheers, Yves

On Tue, Mar 26, 2013 at 06:20:09PM +0100, demerphq wrote:

IMHO, that is just silly. Setting GIT_WORK_TREE=/ would be just as destructive. Or GIT_EXTERNAL_DIFF="rm -rf /" (or GIT_PAGER, etc). If there is a danger to the implicit-workdir behavior, it is due to accidental usage, not from a malicious attack.

-Peff

On 26 March 2013 18:48, Jeff King <peff@peff.net> wrote:

Yeah, that was my line of reasoning too. I'm glad to hear you agree.

cheers Yves

On Tue, Mar 26, 2013 at 06:06:17PM +0100, Richard Weinberger wrote:

Yes, setting GIT_DIR but not GIT_WORK_TREE has always been a valid way to work on a repository where you do not want the working tree polluted with your .git file. It's not a common setup, but people do use it. E.g., you might keep ~/mail as a git repo, but do not want the presence of ~/mail/.git to confuse your mail tools. You can keep ~/git/mail.git as just a repository, and do "cd ~/mail && GIT_DIR=~/git/mail.git git foo" (or "git --git-dir=~git/mail.git foo").

Later, we introduced GIT_WORK_TREE (and core.worktree), which provided another way of doing the same thing (instead of the "cd", you could set GIT_WORK_TREE). For the most part, I'd expect setting core.worktree to be the simplest for such setups, as once it is set, you can just do "cd ~/git/mail.git git foo", and everything should just work.

We never deprecated the original "GIT_DIR without GIT_WORK_TREE means '.' is the implicit worktree" behavior, as nobody ever complained, and it would break existing users of the feature.

We could do so now, as long as we provide an escape hatch (and I think spelling that hatch as GIT_WORK_TREE=. is probably sane, but I am open to other suggestions). And in general we try to avoid such breakage without a deprecation period to give people time to fix their scripts and workflows.

It did do the right thing; just not the one you expected. :)

The problem is not with "clean", which just happens to be a destructive command, but rather with the notion of what the git tree is when you provide GIT_DIR. Though clean is an obvious problematic command, something like "git reset --hard" could also be destructive. I don't think it makes sense to do any fix that is specific to clean. If there is a fix to be done, it should be about making the working tree lookup algorithm more obvious.

-Peff

Jeff King <peff@peff.net> writes:

A small correction is necessary as the above invites confusion.

When you are in ~/mail/subdir, because GIT_DIR alone does not give you to specify where the root-level of the working tree is, you had to "cd .." before running "GIT_DIR=~/git/mail.git git ...". By setting GIT_WORK_TREE to point at ~/mail once, you can freely chdir around inside subdirectories of ~/mail without losing sight of where the root-level is, and if your ~/git/mail.git is tied to a single working tree (and that is true in this example, it is always ~/mail), you can even set core.worktree in ~/git/mail.git/config.

These two mechanisms are *not* about allowing you to run git from any random place, e.g. "/tmp".

Yes.

If we were to do so, GIT_WORK_TREE=. would be the most sensible, but I do not think it is worth breaking. Why do these people set GIT_DIR without setting GIT_WORK_TREE in the first place?

That is the source of the confusion. Perhaps some random but popular websites are spreading bad pieces of advice?

Yes, "git add ." would happily add random cruft to your index, which is equally bad.

On Tue, Mar 26, 2013 at 11:20:58AM -0700, Junio C Hamano wrote:

Yeah, I did not talk about moving around to multiple working trees with the same GIT_DIR. I have done that, but I do not know of a workflow where it is a good practice, and not just a one-off hack.

I don't think there is a good reason. The argument, as I see it, is mainly that doing so can be confusing and destructive, and there is not a big benefit to allowing it.

I am not sure I am convinced it is worth the breakage, either. Curious as to what the code would look like, I made a straw-man series, which will follow. Note that I am not suggesting we do this, but still merely thinking about the idea.

Notably, at the end of the series a number of tests fail. A few of them are testing the GIT_DIR behavior explicitly (I fixed up t1510, but did not hunt down all of the spots), but a few of them are legitimate breakages in scripts. For example, difftool is broken because it sets GIT_DIR. That gives us an indication of what kinds of breakages we would see in real-world third-party scripts.

Eh, I would say it is bad, but not equally bad to removing your entire home directory. ;)

-Peff

If we end up in a sub-process where GIT_WORK_TREE is not set but GIT_DIR is, we assume the current directory is the root of the working tree. Since future patches will change that assumption, let's defensively start setting GIT_WORK_TREE explicitly.

Signed-off-by: Jeff King <peff@peff.net>
---
I didn't test this very well (in fact, I only noticed the issue after
having written the other two patches, as without it, those ones break
every external command which wants to run in a working tree). I would
not be surprised if there is some code path which sets GIT_DIR but not
this, or if there is some other weird fallout from having GIT_WORK_TREE
set explicitly.

 environment.c | 1 +
 1 file changed, 1 insertion(+)

Jeff King wrote:

There's no rush, but I think this is a good change. It makes the rest of the codebase more resilient to running commands from a subdir of the top level of the worktree.

It can be surprising to some users that pointing GIT_DIR to a ".git" directory does not use the working tree that surrounds the .git directory, but rather uses the current working directory as the working tree.

Git has always worked this way, and for the most part it has not been a big problem. However, given that one way of the user finding this out is by having a destructive git command impact an unexpected area of the filesystem, it would be nice to default to something less surprising and likely to cause problems (namely, having no working directory).

This breaks existing users of the feature, of course; they can adapt by setting GIT_WORK_TREE explicitly to ".", but they need to be told to do so. Therefore we'll start with a deprecation period and a warning to give them time to fix their scripts and workflows.

Signed-off-by: Jeff King <peff@peff.net>
---
 setup.c               | 21 ++++++++++++++++++++-
 t/t1510-repo-setup.sh |  8 ++++++--
 2 files changed, 26 insertions(+), 3 deletions(-)

Jeff King wrote:

Please no.  I don't want git 2.0 to be delayed forever.

If we want this warning, would something like the following do?

	warning: You have set GIT_DIR without setting GIT_WORK_TREE
	hint: In this case, GIT_WORK_TREE defaults to '.'
	hint: To suppress this message, set GIT_WORK_TREE='.'

Thanks, Jonathan

On Tue, Mar 26, 2013 at 01:21:42PM -0700, Jonathan Nieder wrote:

Please replace "2.0" with some future version, then. I just made up the number. But...

That can help by teaching people how GIT_DIR behaves in general. But the warning and hint will be small consolation to somebody who runs "GIT_DIR=foo.git git clean -f" and sees it for the first time.

If you want to argue that people would see the warning in earlier runs of git, I can kind of buy that. Although the incident that triggered this discussion probably wouldn't have (I would usually start a git-clean session with "git clean" without "-f" or "git status", either of which would have done equally well as this warning to notify the user what was going on).

Like I said earlier, though, I'm not really sure this is the direction we want to go. This series is more about seeing what the fallouts are. I probably shouldn't have included this middle patch at all, because the interesting thing is what happens when we do turn it off.

-Peff

Jeff King wrote:

Yes, I think it would have helped in this case. If I understand correctly then for a while Richard was habitually setting GIT_DIR to mean "act on this repository" and thought the worktree was automatically being set to the containing directory.

I think patch 3 is a bad direction to go because there will always be old scripts that follow what used to be the recommended way to use GIT_DIR. In the long term a warning like this that doesn't break them (or a fatal error that at least doesn't confuse them) might be a good way to go.

Thanks for your thoughtful work, as always.

Jonathan

Jeff King <peff@peff.net> writes:

Actually, I think the warning is the most important part. With the warning enabled, people should notice they are doing something potentially wrong and dangerous, so the warning essentially fixes the issue in the short term.

I also think that changing the default behavior later makes sense (but I agree that replacing "in Git 2.0" with "in a future version of Git" is better, there's no urgency for this change and people start looking forward 2.0).

Follow-through on the deprecation warning added by the last commit.

We can drop all of the IMPLICIT_WORK_TREE code now, since we default to that case.

Signed-off-by: Jeff King <peff@peff.net>
---
This would obviously come much later than patch 2, in Git 2.0 or
whatever.

But in case anyone did not read the discussion leading up to this series, this breaks many tests. It's not meant for application, but merely to look at what kinds of breakage we could see if we followed this path.

 cache.h               | 12 ------------
 environment.c         |  1 -
 git.c                 |  1 -
 setup.c               | 26 +-------------------------
 t/t1510-repo-setup.sh | 24 ++++++++++--------------
 5 files changed, 11 insertions(+), 53 deletions(-)