# git merge, .gitignore, and silently overwriting untracked files

39 messages from 2010-08-17 to 2010-10-15. Participants: Joshua Jensen, Junio C Hamano, Clemens Buchacher, Jakub Narebski, Matthieu Moy, Holger Hellmuth, Heiko Voigt, Johannes Sixt, Kevin Ballard, Jonathan Nieder.
Thread: https://gitlist.dev/t/24763

## Joshua Jensen, 2010-08-17 05:21

Subject: git merge, .gitignore, and silently overwriting untracked files
Message-ID: <4C6A1C5B.4030304@workspacewhiz.com>
URL: https://gitlist.dev/e/4C6A1C5B.4030304%40workspacewhiz.com

```
  Git silently overwrote an important untracked file for me today.  I 
have come up with the following transcript to reproduce the issue.

In a nutshell, if an untracked file is in .gitignore, a merge from a 
branch where the file was added will OVERWRITE the untracked file 
without warning.

This doesn't seem right.

What do others think?

Josh

-------------------------------

/s/testgitoverwrite
$ mkdir aaa

/s/testgitoverwrite
$ cd aaa

/s/testgitoverwrite/aaa
$ echo abc > abc.txt

/s/testgitoverwrite/aaa
$ ls -l
total 1
-rw-r--r--    1 Joshua   Administ        4 Aug 16 23:10 abc.txt

/s/testgitoverwrite/aaa
$ git init
Initialized empty Git repository in s:/testgitoverwrite/aaa/.git/

/s/testgitoverwrite/aaa (master)
$ git add abc.txt

/s/testgitoverwrite/aaa (master)
$ git commit -m "Added abc.txt"
[master (root-commit) 0e8bffb] Added abc.txt
  1 files changed, 1 insertions(+), 0 deletions(-)
  create mode 100644 abc.txt

/s/testgitoverwrite/aaa (master)
$ cd ..

/s/testgitoverwrite
$ git clone aaa bbb
Cloning into bbb...
done.

/s/testgitoverwrite
$ cd bbb

/s/testgitoverwrite/bbb (master)
$ ls -l
total 1
-rw-r--r--    1 Joshua   Administ        4 Aug 16 23:10 abc.txt

/s/testgitoverwrite/bbb (master)
$ echo "Important stuff" > important.txt

/s/testgitoverwrite/bbb (master)
$ git add important.txt

/s/testgitoverwrite/bbb (master)
$ git commit -m "This file is SO important"
[master 2fe9ab6] This file is SO important
  1 files changed, 1 insertions(+), 0 deletions(-)
  create mode 100644 important.txt

/s/testgitoverwrite/bbb (master)
$ cd ..

/s/testgitoverwrite
$ cd aaa

/s/testgitoverwrite/aaa (master)
$ echo "Local testing copy of important.txt" > important.txt

/s/testgitoverwrite/aaa (master)
$ git status
# On branch master
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       important.txt
nothing added to commit but untracked files present (use "git add" to track)

/s/testgitoverwrite/aaa (master)
$ git pull ../bbb
remote: Counting objects: 4, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
 From ../bbb
  * branch            HEAD       -> FETCH_HEAD
Updating 0e8bffb..2fe9ab6
error: Untracked working tree file 'important.txt' would be overwritten 
by merge.  Aborting

/s/testgitoverwrite/aaa (master)
$ git pull --rebase ../bbb
 From ../bbb
  * branch            HEAD       -> FETCH_HEAD
First, rewinding head to replay your work on top of it...
error: Untracked working tree file 'important.txt' would be overwritten 
by merge.
could not detach HEAD

/s/testgitoverwrite/aaa (master)
$ echo important.txt > .gitignore

/s/testgitoverwrite/aaa (master)
$ git status
# On branch master
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       .gitignore
nothing added to commit but untracked files present (use "git add" to track)

/s/testgitoverwrite/aaa (master)
$ cat important.txt
Local testing copy of important.txt

/s/testgitoverwrite/aaa (master)
$ git pull ../bbb
 From ../bbb
  * branch            HEAD       -> FETCH_HEAD
Updating 0e8bffb..2fe9ab6
Fast-forward
  important.txt |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
  create mode 100644 important.txt

/s/testgitoverwrite/aaa (master)
$ cat important.txt
Important stuff

```

## Junio C Hamano, 2010-08-17 19:33

Subject: Re: git merge, .gitignore, and silently overwriting untracked files
Message-ID: <7viq39avay.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7viq39avay.fsf%40alter.siamese.dyndns.org
In-Reply-To: <4C6A1C5B.4030304@workspacewhiz.com>

```
There are only two kinds of untracked paths in git.  Ones that git is
allowed to remove in order to carry out operations asked by the end user,
i.e. ones that are trashable, are registered to the gitignore mechanism,
and the ones that are precious are not in gitignore; the latter can
interfere with git and prevent it from carrying out certain operations.
E.g. an untracked and unignored file F will prevent you from switching to
a branch that has file F tracked, or that has file F/G (iow, F is a
directory in that branch) tracked.

Ancient git didn't honor gitignore and considered no files trashable,
which caused a lot of trouble to users.  It may be illuminating to check
discussions in the list archive during the period the following commits
were made.  f8a9d42 (read-tree: further loosen "working file will be lost"
check., 2006-12-04), ed93b44 (merge: loosen overcautious "working file
will be lost" check., 2006-10-08), c819353 (Fix switching to a branch with
D/F when current branch has file D., 2007-03-15).

We could enhance the mechanism to categorize untracked paths into three
instead of two: trashable, unknown and precious.

```

## Clemens Buchacher, 2010-08-18 23:39

Subject: [PATCH] optionally disable overwriting of ignored files
Message-ID: <20100818233900.GA27531@localhost>
URL: https://gitlist.dev/e/20100818233900.GA27531%40localhost
In-Reply-To: <7viq39avay.fsf@alter.siamese.dyndns.org>

```
By default, checkout and fast-forward merge will overwrite ignored
files. Make this behavior configurable.

If overwriting ignored files is disabled, it can still be enabled
on the command line by passing -i to merge or checkout.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

Hi,

On Tue, Aug 17, 2010 at 12:33:25PM -0700, Junio C Hamano wrote:
>
> Ancient git didn't honor gitignore and considered no files trashable,
> which caused a lot of trouble to users.  It may be illuminating to check
> discussions in the list archive during the period the following commits
> were made.  f8a9d42 (read-tree: further loosen "working file will be lost"
> check., 2006-12-04), ed93b44 (merge: loosen overcautious "working file
> will be lost" check., 2006-10-08), c819353 (Fix switching to a branch with
> D/F when current branch has file D., 2007-03-15).

I am not convinced this is something that most users want.

The fact that adding a file to .gitignore also marks it as
trashable to git is quite surprising to me. This is something that
the gitignore manpage should warn about loudly. Instead, I think
this behavior is completely undocumented.

As far as I can tell from the code, only fast-forward merge and
checkout overwrite ignored files by default. A regular merge will
complain about untracked files that would be overwritten (ignored
or not). I am quite familiar with that behavior and I think it's
useful to be reminded of those untracked files rather than having
them overwritten. I thought that by extension, other git commands
would behave in the same way.

Here is a patch to optionally disable overwriting of ignored files.
But I think we should even disable it by default. 

Clemens

 Documentation/config.txt |    6 ++++++
 builtin/checkout.c       |    7 +++++++
 builtin/merge.c          |    9 +++++++++
 cache.h                  |    1 +
 config.c                 |    3 +++
 environment.c            |    1 +
 6 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f81fb91..eb9bb43 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -450,6 +450,12 @@ core.excludesfile::
 	to the value of `$HOME` and "{tilde}user/" to the specified user's
 	home directory.  See linkgit:gitignore[5].
 
+core.overwriteignored::
+	Untracked files can get in the way of merge or checkout. If
+	overwriteignored is enabled, such files will be overwritten
+	to let fast-forward merges and checkouts succeed. Enabled
+	by default.
+
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5affb6f..121a6a3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -389,6 +389,11 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.gently = opts->merge && old->commit;
 		topts.verbose_update = !opts->quiet;
 		topts.fn = twoway_merge;
+		if (overwrite_ignored) {
+			topts.dir = xcalloc(1, sizeof(*topts.dir));
+			topts.dir->flags |= DIR_SHOW_IGNORED;
+			topts.dir->exclude_per_dir = ".gitignore";
+		}
 		tree = parse_tree_indirect(old->commit ?
 					   old->commit->object.sha1 :
 					   (unsigned char *)EMPTY_TREE_SHA1_BIN);
@@ -664,6 +669,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT('3', "theirs", &opts.writeout_stage, "stage",
 			    3),
 		OPT_BOOLEAN('f', "force", &opts.force, "force"),
+		OPT_BOOLEAN('i', "overwrite-ignored", &overwrite_ignored,
+		  "allow explicitly ignored files to be overwritten"),
 		OPT_BOOLEAN('m', "merge", &opts.merge, "merge"),
 		OPT_STRING(0, "conflict", &conflict_style, "style",
 			   "conflict style (merge or diff3)"),
diff --git a/builtin/merge.c b/builtin/merge.c
index 4d31e09..47b12ba 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -185,6 +185,8 @@ static struct option builtin_merge_options[] = {
 		"allow fast-forward (default)"),
 	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
 		"abort if fast-forward is not possible"),
+	OPT_BOOLEAN('i', "overwrite-ignored", &overwrite_ignored,
+	  "allow explicitly ignored files to be overwritten"),
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_CALLBACK('s', "strategy", &use_strategies, "strategy",
 		"merge strategy to use", option_parse_strategy),
@@ -682,6 +684,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	struct unpack_trees_options opts;
 	struct tree_desc t[MAX_UNPACK_TREES];
 	int i, fd, nr_trees = 0;
+	struct dir_struct dir;
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
 	refresh_cache(REFRESH_QUIET);
@@ -691,6 +694,12 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	memset(&trees, 0, sizeof(trees));
 	memset(&opts, 0, sizeof(opts));
 	memset(&t, 0, sizeof(t));
+	memset(&dir, 0, sizeof(dir));
+	if (overwrite_ignored) {
+		dir.flags |= DIR_SHOW_IGNORED;
+		dir.exclude_per_dir = ".gitignore";
+		opts.dir = &dir;
+	}
 
 	opts.head_idx = 1;
 	opts.src_index = &the_index;
diff --git a/cache.h b/cache.h
index c9fa3df..dd1b8f7 100644
--- a/cache.h
+++ b/cache.h
@@ -548,6 +548,7 @@ extern size_t packed_git_window_size;
 extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
 extern int read_replace_refs;
+extern int overwrite_ignored;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
diff --git a/config.c b/config.c
index cdcf583..bd7956e 100644
--- a/config.c
+++ b/config.c
@@ -563,6 +563,9 @@ static int git_default_core_config(const char *var, const char *value)
 	if (!strcmp(var, "core.excludesfile"))
 		return git_config_pathname(&excludes_file, var, value);
 
+	if (!strcmp(var, "core.overwriteignored"))
+		overwrite_ignored = git_config_bool(var, value);
+
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/environment.c b/environment.c
index 83d38d3..1b92f60 100644
--- a/environment.c
+++ b/environment.c
@@ -30,6 +30,7 @@ const char *apply_default_ignorewhitespace;
 int zlib_compression_level = Z_BEST_SPEED;
 int core_compression_level;
 int core_compression_seen;
+int overwrite_ignored = 1;
 int fsync_object_files;
 size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
 size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
-- 
1.7.2.1.1.g202c


```

## Jakub Narebski, 2010-08-19 10:41

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <m3aaoiq41v.fsf@localhost.localdomain>
URL: https://gitlist.dev/e/m3aaoiq41v.fsf%40localhost.localdomain
In-Reply-To: <20100818233900.GA27531@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

>  Documentation/config.txt |    6 ++++++
>  builtin/checkout.c       |    7 +++++++
>  builtin/merge.c          |    9 +++++++++
>  cache.h                  |    1 +
>  config.c                 |    3 +++
>  environment.c            |    1 +
>  6 files changed, 27 insertions(+), 0 deletions(-)

You forgot to update Documentation/git-checkout.txt and
Documentation/git-merge.txt with the new '-i' option.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5affb6f..121a6a3 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c

> @@ -664,6 +669,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  		OPT_SET_INT('3', "theirs", &opts.writeout_stage, "stage",
>  			    3),
>  		OPT_BOOLEAN('f', "force", &opts.force, "force"),
> +		OPT_BOOLEAN('i', "overwrite-ignored", &overwrite_ignored,
> +		  "allow explicitly ignored files to be overwritten"),
>  		OPT_BOOLEAN('m', "merge", &opts.merge, "merge"),
>  		OPT_STRING(0, "conflict", &conflict_style, "style",
>  			   "conflict style (merge or diff3)"),
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 4d31e09..47b12ba 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -185,6 +185,8 @@ static struct option builtin_merge_options[] = {
>  		"allow fast-forward (default)"),
>  	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
>  		"abort if fast-forward is not possible"),
> +	OPT_BOOLEAN('i', "overwrite-ignored", &overwrite_ignored,
> +	  "allow explicitly ignored files to be overwritten"),
>  	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
>  	OPT_CALLBACK('s', "strategy", &use_strategies, "strategy",
>  		"merge strategy to use", option_parse_strategy),

-- 
Jakub Narebski
Poland
ShadeHawk on #git

```

## Clemens Buchacher, 2010-08-20 18:48

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <20100820184838.GA8861@localhost>
URL: https://gitlist.dev/e/20100820184838.GA8861%40localhost
In-Reply-To: <m3aaoiq41v.fsf@localhost.localdomain>

```
On Thu, Aug 19, 2010 at 03:41:08AM -0700, Jakub Narebski wrote:
> 
> You forgot to update Documentation/git-checkout.txt and
> Documentation/git-merge.txt with the new '-i' option.

Thanks. I am just waiting for some kind of feedback. I'm not going
to write documentation for something that won't get accepted.

Clemens

```

## Joshua Jensen, 2010-08-20 19:01

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <4C6ED102.2040002@workspacewhiz.com>
URL: https://gitlist.dev/e/4C6ED102.2040002%40workspacewhiz.com
In-Reply-To: <20100820184838.GA8861@localhost>

```
  ----- Original Message -----
From: Clemens Buchacher
Date: 8/20/2010 12:48 PM
> On Thu, Aug 19, 2010 at 03:41:08AM -0700, Jakub Narebski wrote:
>> You forgot to update Documentation/git-checkout.txt and
>> Documentation/git-merge.txt with the new '-i' option.
> Thanks. I am just waiting for some kind of feedback. I'm not going
> to write documentation for something that won't get accepted.
I like the idea... :)

My vote doesn't count, though...

Josh

```

## Junio C Hamano, 2010-08-20 20:35

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <7v4oepaup7.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7v4oepaup7.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20100818233900.GA27531@localhost>

```
If (and this may be a big IF) it is reasonable to add paths to .gitignore
that you do not want to lose, then you would want to have three classes of
untracked paths: "precious but ignored", "trashable" (and by definition
ignored), and "unignored" (and by definition is not ignored and is
precious).

As I already pointed out, we don't support the "precious but ignored"
class.  So an obvious alternate solution to your particular case is not to
add such a path to the gitignore mechanism.

I have a suspicion that the approach this patch takes would not help very
much in the real life.  You just traded the lack of "precious but ignored"
with "no file is trashable, even if it is listed in .gitignore".

Granted, as long as it is not default, it won't negatively affect people
who do not enable it, other than that it may add maintenance burden on the
resulting bloated code, but I find it hard to swallow new code that does
not convincingly solve the real problem.

By the way, we seem to have a few longstanding bugs that trashes an
unignored and untracked (hence by definition precious) path during branch
switching, and does not give a correct escape hatch.

Doing this:

    $ git checkout maint
    $ echo foo >t/t2018-checkout-branch.sh
    $ git checkout master

does correctly error out because the unignored file needs to be
overwritten.  But doing this after the above still errors out, which is
probably wrong:

    $ echo t/t2018-checkout-branch.sh >>.git/info/exclude
    $ git checkout master

After doing the above:

    $ ed .git/info/exclude ;# remove the extra entry in info/excludes
    $d
    w
    q
    $ rm t/t2018-checkout-branch.sh
    $ echo foo >po
    $ git checkout pu

should error out, as "po" is a directory that has tracked contents, and we
never said the untracked regular file "po" is trashable, but the above
sequence happily checks the branch out.

```

## Junio C Hamano, 2010-08-20 20:46

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <7vtymp9fn1.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vtymp9fn1.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20100818233900.GA27531@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5affb6f..121a6a3 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -389,6 +389,11 @@ static int merge_working_tree(struct checkout_opts *opts,
>  		topts.gently = opts->merge && old->commit;
>  		topts.verbose_update = !opts->quiet;
>  		topts.fn = twoway_merge;
> +		if (overwrite_ignored) {
> +			topts.dir = xcalloc(1, sizeof(*topts.dir));
> +			topts.dir->flags |= DIR_SHOW_IGNORED;
> +			topts.dir->exclude_per_dir = ".gitignore";
> +		}
>  		tree = parse_tree_indirect(old->commit ?
>  					   old->commit->object.sha1 :
>  					   (unsigned char *)EMPTY_TREE_SHA1_BIN);

What is this patch based on?  I don't see a branch that has these two
lines adjacent to each other as the preimage...

>  		topts.fn = twoway_merge;
>  		tree = parse_tree_indirect(old->commit ?

```

## Clemens Buchacher, 2010-08-21 06:48

Subject: [PATCH v2] optionally disable overwriting of ignored files
Message-ID: <20100821064844.GA29968@localhost>
URL: https://gitlist.dev/e/20100821064844.GA29968%40localhost
In-Reply-To: <7vtymp9fn1.fsf@alter.siamese.dyndns.org>

```

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Fri, Aug 20, 2010 at 01:46:10PM -0700, Junio C Hamano wrote:
>         
> What is this patch based on?
          
Oops, I forgot to squash an intermediate commit. This is based on
v1.7.2.1.

 Documentation/config.txt |    6 ++++++
 builtin/checkout.c       |   10 +++++++---
 builtin/merge.c          |   10 +++++++---
 cache.h                  |    1 +
 config.c                 |    3 +++
 environment.c            |    1 +
 6 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f81fb91..eb9bb43 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -450,6 +450,12 @@ core.excludesfile::
 	to the value of `$HOME` and "{tilde}user/" to the specified user's
 	home directory.  See linkgit:gitignore[5].
 
+core.overwriteignored::
+	Untracked files can get in the way of merge or checkout. If
+	overwriteignored is enabled, such files will be overwritten
+	to let fast-forward merges and checkouts succeed. Enabled
+	by default.
+
 core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 1994be9..121a6a3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -389,9 +389,11 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.gently = opts->merge && old->commit;
 		topts.verbose_update = !opts->quiet;
 		topts.fn = twoway_merge;
-		topts.dir = xcalloc(1, sizeof(*topts.dir));
-		topts.dir->flags |= DIR_SHOW_IGNORED;
-		topts.dir->exclude_per_dir = ".gitignore";
+		if (overwrite_ignored) {
+			topts.dir = xcalloc(1, sizeof(*topts.dir));
+			topts.dir->flags |= DIR_SHOW_IGNORED;
+			topts.dir->exclude_per_dir = ".gitignore";
+		}
 		tree = parse_tree_indirect(old->commit ?
 					   old->commit->object.sha1 :
 					   (unsigned char *)EMPTY_TREE_SHA1_BIN);
@@ -667,6 +669,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT('3', "theirs", &opts.writeout_stage, "stage",
 			    3),
 		OPT_BOOLEAN('f', "force", &opts.force, "force"),
+		OPT_BOOLEAN('i', "overwrite-ignored", &overwrite_ignored,
+		  "allow explicitly ignored files to be overwritten"),
 		OPT_BOOLEAN('m', "merge", &opts.merge, "merge"),
 		OPT_STRING(0, "conflict", &conflict_style, "style",
 			   "conflict style (merge or diff3)"),
diff --git a/builtin/merge.c b/builtin/merge.c
index 37ce4f5..47b12ba 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -185,6 +185,8 @@ static struct option builtin_merge_options[] = {
 		"allow fast-forward (default)"),
 	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
 		"abort if fast-forward is not possible"),
+	OPT_BOOLEAN('i', "overwrite-ignored", &overwrite_ignored,
+	  "allow explicitly ignored files to be overwritten"),
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_CALLBACK('s', "strategy", &use_strategies, "strategy",
 		"merge strategy to use", option_parse_strategy),
@@ -693,9 +695,11 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	memset(&opts, 0, sizeof(opts));
 	memset(&t, 0, sizeof(t));
 	memset(&dir, 0, sizeof(dir));
-	dir.flags |= DIR_SHOW_IGNORED;
-	dir.exclude_per_dir = ".gitignore";
-	opts.dir = &dir;
+	if (overwrite_ignored) {
+		dir.flags |= DIR_SHOW_IGNORED;
+		dir.exclude_per_dir = ".gitignore";
+		opts.dir = &dir;
+	}
 
 	opts.head_idx = 1;
 	opts.src_index = &the_index;
diff --git a/cache.h b/cache.h
index c9fa3df..dd1b8f7 100644
--- a/cache.h
+++ b/cache.h
@@ -548,6 +548,7 @@ extern size_t packed_git_window_size;
 extern size_t packed_git_limit;
 extern size_t delta_base_cache_limit;
 extern int read_replace_refs;
+extern int overwrite_ignored;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
diff --git a/config.c b/config.c
index cdcf583..bd7956e 100644
--- a/config.c
+++ b/config.c
@@ -563,6 +563,9 @@ static int git_default_core_config(const char *var, const char *value)
 	if (!strcmp(var, "core.excludesfile"))
 		return git_config_pathname(&excludes_file, var, value);
 
+	if (!strcmp(var, "core.overwriteignored"))
+		overwrite_ignored = git_config_bool(var, value);
+
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/environment.c b/environment.c
index 83d38d3..1b92f60 100644
--- a/environment.c
+++ b/environment.c
@@ -30,6 +30,7 @@ const char *apply_default_ignorewhitespace;
 int zlib_compression_level = Z_BEST_SPEED;
 int core_compression_level;
 int core_compression_seen;
+int overwrite_ignored = 1;
 int fsync_object_files;
 size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
 size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
-- 
1.7.2.1.1.g202c


```

## Clemens Buchacher, 2010-08-21 08:05

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <20100821080512.GA30141@localhost>
URL: https://gitlist.dev/e/20100821080512.GA30141%40localhost
In-Reply-To: <7v4oepaup7.fsf@alter.siamese.dyndns.org>

```
On Fri, Aug 20, 2010 at 01:35:32PM -0700, Junio C Hamano wrote:
>
> If (and this may be a big IF) it is reasonable to add paths to .gitignore
> that you do not want to lose, then you would want to have three classes of
> untracked paths: "precious but ignored", "trashable" (and by definition
> ignored), and "unignored" (and by definition is not ignored and is
> precious).

I am not sure anybody would bother categorizing their files into
several classes. On the other hand, .gitignore and
.git/info/exclude may already serve such a purpose.

Files that go into a tracked .gitignore are most likely generated
files, and therefore trashable. Files that go into
.git/info/exclude or into an untracked .gitignore (e.g. echo '*' >
precious-simulation-results/.gitignore), are not always generated
and may not be trashable. At least they would not likely get in the
way of checkout or merge.

(As you noted below, except for the untracked .gitignore part, this
is how git behaves already.)

> As I already pointed out, we don't support the "precious but ignored"
> class.  So an obvious alternate solution to your particular case is not to
> add such a path to the gitignore mechanism.

That would defeat the point of ignoring it.

> I have a suspicion that the approach this patch takes would not help very
> much in the real life.  You just traded the lack of "precious but ignored"
> with "no file is trashable, even if it is listed in .gitignore".
> 
> Granted, as long as it is not default, it won't negatively affect people
> who do not enable it, other than that it may add maintenance burden on the
> resulting bloated code, but I find it hard to swallow new code that does
> not convincingly solve the real problem.

Yes, I am not a big fan of this solution either. But if we do not
find a better solution, I think having it configurable cannot hurt.
The code required is minimal. As far as I am concerned, we can even
remove the -i part.

> By the way, we seem to have a few longstanding bugs that trashes an
> unignored and untracked (hence by definition precious) path during branch
> switching, and does not give a correct escape hatch.
> 
> Doing this:
> 
>     $ git checkout maint
>     $ echo foo >t/t2018-checkout-branch.sh
>     $ git checkout master
> 
> does correctly error out because the unignored file needs to be
> overwritten.  But doing this after the above still errors out, which is
> probably wrong:
> 
>     $ echo t/t2018-checkout-branch.sh >>.git/info/exclude
>     $ git checkout master

I understand your point. But this is actually a great example. I
have a bunch of such tests, which are not in shape for upstream,
but I want to keep them around anyways (and run them). Do you
really think that an untracked test which was added to
.git/info/exclude should be considered trashable? If it were a
generated file, it would have been added to .gitignore.

> After doing the above:
> 
>     $ ed .git/info/exclude ;# remove the extra entry in info/excludes
>     $d
>     w
>     q
>     $ rm t/t2018-checkout-branch.sh
>     $ echo foo >po
>     $ git checkout pu
> 
> should error out, as "po" is a directory that has tracked contents, and we
> never said the untracked regular file "po" is trashable, but the above
> sequence happily checks the branch out.

Ok, that's bad.

Clemens

```

## Clemens Buchacher, 2010-08-21 13:23

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <20100821132356.GA31998@localhost>
URL: https://gitlist.dev/e/20100821132356.GA31998%40localhost
In-Reply-To: <7v4oepaup7.fsf@alter.siamese.dyndns.org>

```
On Fri, Aug 20, 2010 at 01:35:32PM -0700, Junio C Hamano wrote:
>
>     $ echo foo >po
>     $ git checkout pu
> 
> should error out, as "po" is a directory that has tracked contents, and we
> never said the untracked regular file "po" is trashable, but the above
> sequence happily checks the branch out.

Looks like this case is simply overlooked in verify_absent_1(). The
following takes the existing lstat_cache() code and deals with the
FL_ERR case, which is when there is a file in the way of the
leading path.

The patch below fixes the issue and passes the test suite, but it's
lacking in various ways and I am probably breaking something in
lstat_cache(), which I do not completely understand yet.

So it's going to take some more work to fix this properly.

Clemens

 cache.h        |    1 +
 symlinks.c     |   20 ++++++++++++++++++--
 unpack-trees.c |   25 ++++++++++++++++++++++++-
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index dd1b8f7..5a8a99d 100644
--- a/cache.h
+++ b/cache.h
@@ -850,6 +850,7 @@ struct cache_def {
 extern int has_symlink_leading_path(const char *name, int len);
 extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
 extern int has_symlink_or_noent_leading_path(const char *name, int len);
+extern int find_leading_path(const char *name, int len, const char **path, int *path_len);
 extern int has_dirs_only_path(const char *name, int len, int prefix_len);
 extern void schedule_dir_for_removal(const char *name, int len);
 extern void remove_scheduled_dirs(void);
diff --git a/symlinks.c b/symlinks.c
index 8860120..3f78168 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -152,7 +152,7 @@ static int lstat_cache(struct cache_def *cache, const char *name, int len,
 	 * path types, FL_NOENT, FL_SYMLINK and FL_DIR, can be cached
 	 * for the moment!
 	 */
-	save_flags = ret_flags & track_flags & (FL_NOENT|FL_SYMLINK);
+	save_flags = ret_flags & track_flags & (FL_NOENT|FL_SYMLINK|FL_ERR);
 	if (save_flags && last_slash > 0 && last_slash <= PATH_MAX) {
 		cache->path[last_slash] = '\0';
 		cache->len = last_slash;
@@ -199,7 +199,7 @@ int has_symlink_leading_path(const char *name, int len)
 
 /*
  * Return non-zero if path 'name' has a leading symlink component or
- * if some leading path component does not exists.
+ * if some leading path component does not exist.
  */
 int has_symlink_or_noent_leading_path(const char *name, int len)
 {
@@ -210,6 +210,22 @@ int has_symlink_or_noent_leading_path(const char *name, int len)
 }
 
 /*
+ * Stat for leading path.
+ */
+int find_leading_path(const char *name, int len, const char **path, int *path_len)
+{
+	struct cache_def *cache = &default_cache;	/* FIXME */
+	int flags = lstat_cache(cache, name, len,
+			   FL_SYMLINK|FL_NOENT|FL_DIR|FL_ERR, USE_ONLY_LSTAT);
+	*path = cache->path;
+	*path_len = cache->len;
+	if (flags & FL_ERR)
+		return -1;
+	else
+		return flags & (FL_SYMLINK|FL_NOENT);
+}
+
+/*
  * Return non-zero if all path components of 'name' exists as a
  * directory.  If prefix_len > 0, we will test with the stat()
  * function instead of the lstat() function for a prefix length of
diff --git a/unpack-trees.c b/unpack-trees.c
index 8cf0da3..250ed7c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1016,12 +1016,35 @@ static int verify_absent_1(struct cache_entry *ce, const char *action,
 				 const char *error_msg)
 {
 	struct stat st;
+	const char *path;
+	int path_len;
+	int ret;
 
 	if (o->index_only || o->reset || !o->update)
 		return 0;
 
-	if (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))
+	ret = find_leading_path(ce->name, ce_namelen(ce), &path, &path_len);
+	if (ret > 0)
 		return 0;
+	else if (ret < 0) {
+		struct cache_entry *result;
+
+		/* FIXME: respect ignores etc. as below */
+
+		/*
+		 * The previous round may already have decided to
+		 * delete this path, which is in a subdirectory that
+		 * is being replaced with a blob.
+		 */
+		result = index_name_exists(&o->result, path, path_len, 0);
+		if (result) {
+			if (result->ce_flags & CE_REMOVE)
+				return 0;
+		}
+
+		return o->gently ? -1 :
+			error(ERRORMSG(o, would_lose_untracked), path, action);
+	}
 
 	if (!lstat(ce->name, &st)) {
 		int dtype = ce_to_dtype(ce);
-- 
1.7.2.1.1.g202c


```

## Junio C Hamano, 2010-08-22 07:25

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <7viq335ct0.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7viq335ct0.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20100821080512.GA30141@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

> I understand your point. But this is actually a great example. I
> have a bunch of such tests, which are not in shape for upstream,
> but I want to keep them around anyways (and run them). Do you
> really think that an untracked test which was added to
> .git/info/exclude should be considered trashable? If it were a
> generated file, it would have been added to .gitignore.

I agree that in such a workflow to keep untracked tests around, they
should not be considered trashable. But more importantly, as I have
already said, adding your untracked tests to exclude is a wrong thing to
do.

Traditionally (think "git status" output without "-s") the way to remind
oneself that some day these paths need to be added when they are ready has
been to keep them untracked but _not_ ignored, so that they will be listed
in the output.

Quite contrary to what you earlier said in another message, adding
such a path that is not trashable does defeat the point of the "ignore"
mechanism.

```

## Clemens Buchacher, 2010-08-22 08:20

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <20100822082056.GA5156@localhost>
URL: https://gitlist.dev/e/20100822082056.GA5156%40localhost
In-Reply-To: <7viq335ct0.fsf@alter.siamese.dyndns.org>

```
On Sun, Aug 22, 2010 at 12:25:31AM -0700, Junio C Hamano wrote:
> 
> Traditionally (think "git status" output without "-s") the way to remind
> oneself that some day these paths need to be added when they are ready has
> been to keep them untracked but _not_ ignored, so that they will be listed
> in the output.

But they do not have to be added. I may never want to add them. So
I do not want git status to keep reminding me. That's why I add
them to .git/info/exclude. And with the current behavior, I do not
see anything wrong with that, because git does not consider them
trashable.

> Quite contrary to what you earlier said in another message, adding
> such a path that is not trashable does defeat the point of the "ignore"
> mechanism.

If I understand correctly, the reason for considering ignored paths
trashable is because they are likely to have been tracked in other
versions of the project, and would therefore frequently get in the
way of commands like checkout or merge.

But the same is not true for private ignores in .git/info/exclude.
Such paths most likely never were tracked and never will be. And in
the rare case that an ignored path turns out to be a nuisance, it's
easy enough to remove it from .git/info/exclude.

So I see only advantages if such a path is not considered
trashable.

Clemens

```

## Matthieu Moy, 2010-08-23 08:33

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <vpqaaod7mpa.fsf@bauges.imag.fr>
URL: https://gitlist.dev/e/vpqaaod7mpa.fsf%40bauges.imag.fr
In-Reply-To: <20100818233900.GA27531@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

> By default, checkout and fast-forward merge will overwrite ignored
> files. Make this behavior configurable.

I'd use this option if it gets into git.git.

I didn't follow the discussions when the feature was added, and I was
basically not aware that Git could trash my ignored files this way.
I've always thought that Git took great care not to touch untracked
files, and I found this good ...

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

```

## Matthieu Moy, 2010-08-23 09:37

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <vpqd3t9656k.fsf@bauges.imag.fr>
URL: https://gitlist.dev/e/vpqd3t9656k.fsf%40bauges.imag.fr
In-Reply-To: <20100818233900.GA27531@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

> By default, checkout and fast-forward merge will overwrite ignored
> files. Make this behavior configurable.

Just mentionning an alternative, or complementary approach:

Instead of overwritting completely a .gitignore-d file, Git could
rename it, and warn the user kindly. For example:

$ git merge
...
Warning: existing ignored file 'foo' renamed to 'foo~'

(in case foo~ already exists, it's possible to use numbered backups
just like "mv --backup=existing" does for example)


That could complement your patch if core.overwriteignored is a
multiple-choice option instead of a Boolean:

- "overwrite" => current behavior
- "refuse" => your proposal
- "rename" => my proposal

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

```

## Holger Hellmuth, 2010-08-23 13:56

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <4C727E17.5070707@ira.uka.de>
URL: https://gitlist.dev/e/4C727E17.5070707%40ira.uka.de
In-Reply-To: <vpqd3t9656k.fsf@bauges.imag.fr>

```
Am 23.08.2010 11:37, schrieb Matthieu Moy:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
>> By default, checkout and fast-forward merge will overwrite ignored
>> files. Make this behavior configurable.
> 
> Just mentionning an alternative, or complementary approach:
> 
> Instead of overwritting completely a .gitignore-d file, Git could
> rename it, and warn the user kindly. For example:


You've got my vote. This is the only option that combines safety with
minimal configuration hassle.

I didn't know about this subtle difference between .gitignore and
.git/info/exclude. And while this makes sense I expect a sizable
percentage of users will never even know that .git/info/exclude exists.

And in practice I guess a lot of files will get added to .gitignore even
though they only are relevant to one user (out of carelessness or
ignorance). I know I have added patterns without making a conscious
decision about their relevance to others.

```

## Clemens Buchacher, 2010-08-23 15:11

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <20100823151146.GA15379@localhost>
URL: https://gitlist.dev/e/20100823151146.GA15379%40localhost
In-Reply-To: <4C727E17.5070707@ira.uka.de>

```
On Mon, Aug 23, 2010 at 03:56:39PM +0200, Holger Hellmuth wrote:
> 
> You've got my vote. This is the only option that combines safety with
> minimal configuration hassle.
> 
> I didn't know about this subtle difference between .gitignore and
> .git/info/exclude. And while this makes sense I expect a sizable
> percentage of users will never even know that .git/info/exclude exists.

I don't know if this subtle difference was even intentional. But it
makes sense, and we simply need to make it explicit by documenting
it and by making it optional.

And in many cases, that behavior makes sense. Imagine a generated
file is accidentally added to git, later removed from version
control and added to .gitignore instead. A common scenario, I am
sure.

Now if you start merging, rebasing or bisecting with such a
history, you _will_ run into this problem all the time.

Renaming is certainly a possibility, but it does not really solve
the problem. We will end up with a bunch of renamed, possibly
precious files in our work tree, which means we have to clean up
manually anyways.

So I think it is better to make the decision whether or not a file
is precious at the time it is added to .gitignore. In other words,
never add precious files to .gitignore, but add them to
.git/info/exclude instead.

> And in practice I guess a lot of files will get added to
> .gitignore even though they only are relevant to one user (out of
> carelessness or ignorance). I know I have added patterns without
> making a conscious decision about their relevance to others.

Then you have been using it wrong, just like I have.
Ignorant/careless users may still make that mistake, but if git
does not overwrite such files by default, at least they made a
conscious decision at some point to take that risk.

That is if we can make it the default.

Clemens

```

## Junio C Hamano, 2010-08-23 15:57

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <7veidp2ufh.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7veidp2ufh.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20100823151146.GA15379@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

> On Mon, Aug 23, 2010 at 03:56:39PM +0200, Holger Hellmuth wrote:
>> 
>> You've got my vote. This is the only option that combines safety with
>> minimal configuration hassle.
>> 
>> I didn't know about this subtle difference between .gitignore and
>> .git/info/exclude. And while this makes sense I expect a sizable
>> percentage of users will never even know that .git/info/exclude exists.
>
> I don't know if this subtle difference was even intentional.

I am sorry, what difference are you talking about?

```

## Clemens Buchacher, 2010-08-24 07:28

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <20100824072854.GA22951@localhost>
URL: https://gitlist.dev/e/20100824072854.GA22951%40localhost
In-Reply-To: <7veidp2ufh.fsf@alter.siamese.dyndns.org>

```
On Mon, Aug 23, 2010 at 08:57:38AM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > On Mon, Aug 23, 2010 at 03:56:39PM +0200, Holger Hellmuth wrote:
> >> 
> >> You've got my vote. This is the only option that combines safety with
> >> minimal configuration hassle.
> >> 
> >> I didn't know about this subtle difference between .gitignore and
> >> .git/info/exclude. And while this makes sense I expect a sizable
> >> percentage of users will never even know that .git/info/exclude exists.
> >
> > I don't know if this subtle difference was even intentional.
> 
> I am sorry, what difference are you talking about?

The fact that files in .gitignore are considered trashable, and the
ones in .git/info/exclude are not.

```

## Junio C Hamano, 2010-08-24 16:19

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <7vpqx8x9te.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vpqx8x9te.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20100824072854.GA22951@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

> The fact that files in .gitignore are considered trashable, and the
> ones in .git/info/exclude are not.

Is that a fact?

There may be a subtle bug in the ignore handling code.  The entries in
these various places are never meant to produce different behaviours.

```

## Heiko Voigt, 2010-08-31 18:44

Subject: Re: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <20100831184410.GA29001@book.hvoigt.net>
URL: https://gitlist.dev/e/20100831184410.GA29001%40book.hvoigt.net
In-Reply-To: <vpqaaod7mpa.fsf@bauges.imag.fr>

```
On Mon, Aug 23, 2010 at 10:33:21AM +0200, Matthieu Moy wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > By default, checkout and fast-forward merge will overwrite ignored
> > files. Make this behavior configurable.
> 
> I'd use this option if it gets into git.git.
> 
> I didn't follow the discussions when the feature was added, and I was
> basically not aware that Git could trash my ignored files this way.
> I've always thought that Git took great care not to touch untracked
> files, and I found this good ...

I agree here. I would like to have this as an option as well. And I
think adding a "ignored but precious" category does not make sense in
practise. My guess is that a user would only know _after_ git trashed
some precious file that it belongs into that category;) I have told my
users that git is very careful with their files and I would like to keep
this promise.

I was actually never annoyed by a failed checkout because of an
untracked file in the way. (A 'git clean -fx' always fixes this).

Thanks.

Heiko

```

## Clemens Buchacher, 2010-10-09 13:52

Subject: [PATCH 0/5] do not overwrite untracked files in leading path
Message-ID: <1286632380-7002-1-git-send-email-drizzd@aon.at>
URL: https://gitlist.dev/e/1286632380-7002-1-git-send-email-drizzd%40aon.at
In-Reply-To: <7v4oepaup7.fsf@alter.siamese.dyndns.org>

```
Hi,

On Fri, Aug 20, 2010 at 01:35:32PM -0700, Junio C Hamano wrote:
>
>     $ echo foo >po
>     $ git checkout pu
> 
> should error out, as "po" is a directory that has tracked
> contents, and we never said the untracked regular file "po" is
> trashable, but the above sequence happily checks the branch out.

The following patch series is aimed at fixing this bug.

Clemens

[PATCH 1/5] t7607: use test_commit and test_must_fail
[PATCH 2/5] t7607: add leading-path tests
[PATCH 3/5] add function check_ok_to_remove()
[PATCH 4/5] lstat_cache: optionally return match_len
[PATCH 5/5] do not overwrite files in leading path

 cache.h                    |    2 +-
 symlinks.c                 |   64 +++++++++++++++-------
 t/t7607-merge-overwrite.sh |  129 +++++++++++++++++++++++++++++---------------
 unpack-trees.c             |  121 ++++++++++++++++++++++++-----------------
 4 files changed, 200 insertions(+), 116 deletions(-)

```

## Clemens Buchacher, 2010-10-09 13:52

Subject: [PATCH 1/5] t7607: use test_commit and test_must_fail
Message-ID: <1286632380-7002-2-git-send-email-drizzd@aon.at>
URL: https://gitlist.dev/e/1286632380-7002-2-git-send-email-drizzd%40aon.at
In-Reply-To: <7v4oepaup7.fsf@alter.siamese.dyndns.org>

```
Also make sure that aborted merges do not leave
MERGE_HEAD except in the "will not overwrite
removed file" test, where we cannot do so. See
also the discussion in the following thread.

http://mid.gmane.org/7vskopwxej.fsf@gitster.siamese.dyndns.org
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/t7607-merge-overwrite.sh |   78 +++++++++++++++++++------------------------
 1 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index d82349a..6ed40b1 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -7,81 +7,73 @@ Do not overwrite changes.'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-	echo c0 > c0.c &&
-	git add c0.c &&
-	git commit -m c0 &&
-	git tag c0 &&
-	echo c1 > c1.c &&
-	git add c1.c &&
-	git commit -m c1 &&
-	git tag c1 &&
+	test_commit c0 &&
+	test_commit c1 &&
+	test_commit "c1a" "c1.t" "c1 a" &&
 	git reset --hard c0 &&
-	echo c2 > c2.c &&
-	git add c2.c &&
-	git commit -m c2 &&
-	git tag c2 &&
-	git reset --hard c1 &&
-	echo "c1 a" > c1.c &&
-	git add c1.c &&
-	git commit -m "c1 a" &&
-	git tag c1a &&
+	test_commit c2 &&
 	echo "VERY IMPORTANT CHANGES" > important
 '
 
 test_expect_success 'will not overwrite untracked file' '
 	git reset --hard c1 &&
-	cat important > c2.c &&
+	cp important c2.t &&
 	test_must_fail git merge c2 &&
-	test_cmp important c2.c
+	! test -f .git/MERGE_HEAD &&
+	test_cmp important c2.t
 '
 
 test_expect_success 'will not overwrite new file' '
 	git reset --hard c1 &&
-	cat important > c2.c &&
-	git add c2.c &&
+	cp important c2.t &&
+	git add c2.t &&
 	test_must_fail git merge c2 &&
-	test_cmp important c2.c
+	! test -f .git/MERGE_HEAD &&
+	test_cmp important c2.t
 '
 
 test_expect_success 'will not overwrite staged changes' '
 	git reset --hard c1 &&
-	cat important > c2.c &&
-	git add c2.c &&
-	rm c2.c &&
+	cp important c2.t &&
+	git add c2.t &&
+	rm c2.t &&
 	test_must_fail git merge c2 &&
-	git checkout c2.c &&
-	test_cmp important c2.c
+	! test -f .git/MERGE_HEAD &&
+	git checkout c2.t &&
+	test_cmp important c2.t
 '
 
 test_expect_success 'will not overwrite removed file' '
 	git reset --hard c1 &&
-	git rm c1.c &&
-	git commit -m "rm c1.c" &&
-	cat important > c1.c &&
+	git rm c1.t &&
+	git commit -m "rm c1.t" &&
+	cp important c1.t &&
 	test_must_fail git merge c1a &&
-	test_cmp important c1.c
+	test_cmp important c1.t
 '
 
 test_expect_success 'will not overwrite re-added file' '
 	git reset --hard c1 &&
-	git rm c1.c &&
-	git commit -m "rm c1.c" &&
-	cat important > c1.c &&
-	git add c1.c &&
+	git rm c1.t &&
+	git commit -m "rm c1.t" &&
+	cp important c1.t &&
+	git add c1.t &&
 	test_must_fail git merge c1a &&
-	test_cmp important c1.c
+	! test -f .git/MERGE_HEAD &&
+	test_cmp important c1.t
 '
 
 test_expect_success 'will not overwrite removed file with staged changes' '
 	git reset --hard c1 &&
-	git rm c1.c &&
-	git commit -m "rm c1.c" &&
-	cat important > c1.c &&
-	git add c1.c &&
-	rm c1.c &&
+	git rm c1.t &&
+	git commit -m "rm c1.t" &&
+	cp important c1.t &&
+	git add c1.t &&
+	rm c1.t &&
 	test_must_fail git merge c1a &&
-	git checkout c1.c &&
-	test_cmp important c1.c
+	! test -f .git/MERGE_HEAD &&
+	git checkout c1.t &&
+	test_cmp important c1.t
 '
 
 test_done
-- 
1.7.1.571.gba4d01

```

## Clemens Buchacher, 2010-10-09 13:52

Subject: [PATCH 2/5] t7607: add leading-path tests
Message-ID: <1286632380-7002-3-git-send-email-drizzd@aon.at>
URL: https://gitlist.dev/e/1286632380-7002-3-git-send-email-drizzd%40aon.at
In-Reply-To: <7v4oepaup7.fsf@alter.siamese.dyndns.org>

```

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

I added a test for symlinks as well, even though from the code it
looks like overwriting untracked symlinks is intentionally allowed.
One could argue that symlinks are valuable just like regular files.
On the other hand, the potential loss of information is relatively
small.

For consistency, I would prefer to treat symlinks just like regular
files, but I do not feel strongly about it.

Clemens

 t/t7607-merge-overwrite.sh |   51 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index 6ed40b1..01b070b 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -12,6 +12,12 @@ test_expect_success 'setup' '
 	test_commit "c1a" "c1.t" "c1 a" &&
 	git reset --hard c0 &&
 	test_commit c2 &&
+	git reset --hard c0 &&
+	mkdir sub &&
+	echo "sub/f" > sub/f &&
+	git add sub/f &&
+	git commit -m sub &&
+	git tag sub &&
 	echo "VERY IMPORTANT CHANGES" > important
 '
 
@@ -23,6 +29,14 @@ test_expect_success 'will not overwrite untracked file' '
 	test_cmp important c2.t
 '
 
+test_expect_success 'will overwrite tracked file' '
+	git reset --hard c1 &&
+	cp important c2.t &&
+	git add c2.t &&
+	git commit -m important &&
+	git checkout c2
+'
+
 test_expect_success 'will not overwrite new file' '
 	git reset --hard c1 &&
 	cp important c2.t &&
@@ -76,4 +90,41 @@ test_expect_success 'will not overwrite removed file with staged changes' '
 	test_cmp important c1.t
 '
 
+test_expect_success 'will not overwrite untracked subtree' '
+	git reset --hard c0 &&
+	rm -rf sub &&
+	mkdir -p sub/f &&
+	cp important sub/f/important &&
+	test_must_fail git merge sub &&
+	! test -f .git/MERGE_HEAD &&
+	test_cmp important sub/f/important
+'
+
+test_expect_failure 'will not overwrite untracked file in leading path' '
+	git reset --hard c0 &&
+	rm -rf sub &&
+	cp important sub &&
+	test_must_fail git merge sub &&
+	! test -f .git/MERGE_HEAD &&
+	test_cmp important sub
+'
+
+test_expect_failure 'will not overwrite untracked symlink in leading path' '
+	git reset --hard c0 &&
+	rm -rf sub &&
+	mkdir sub2 &&
+	ln -s sub2 sub &&
+	test_must_fail git merge sub &&
+	! test -f .git/MERGE_HEAD
+'
+
+test_expect_success 'will not be confused by symlink in leading path' '
+	git reset --hard c0 &&
+	rm -rf sub &&
+	ln -s sub2 sub &&
+	git add sub &&
+	git commit -m ln &&
+	git checkout sub
+'
+
 test_done
-- 
1.7.1.571.gba4d01

```

## Clemens Buchacher, 2010-10-09 13:52

Subject: [PATCH 3/5] add function check_ok_to_remove()
Message-ID: <1286632380-7002-4-git-send-email-drizzd@aon.at>
URL: https://gitlist.dev/e/1286632380-7002-4-git-send-email-drizzd%40aon.at
In-Reply-To: <7v4oepaup7.fsf@alter.siamese.dyndns.org>

```
This wraps some inline code into the function check_ok_to_remove(),
which will later be used for leading path components as well.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 unpack-trees.c |  107 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 58 insertions(+), 49 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 803445a..df1c920 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1127,14 +1127,65 @@ static int verify_clean_subdirectory(struct cache_entry *ce,
  * See if we can find a case-insensitive match in the index that also
  * matches the stat information, and assume it's that other file!
  */
-static int icase_exists(struct unpack_trees_options *o, struct cache_entry *dst, struct stat *st)
+static int icase_exists(struct unpack_trees_options *o, const char *name, int len, struct stat *st)
 {
 	struct cache_entry *src;
 
-	src = index_name_exists(o->src_index, dst->name, ce_namelen(dst), 1);
+	src = index_name_exists(o->src_index, name, len, 1);
 	return src && !ie_match_stat(o->src_index, src, st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
 }
 
+static int check_ok_to_remove(const char *name, int len, int dtype,
+			      struct cache_entry *ce, struct stat *st,
+			      enum unpack_trees_error_types error_type,
+			      struct unpack_trees_options *o)
+{
+	struct cache_entry *result;
+
+	/*
+	 * It may be that the 'lstat()' succeeded even though
+	 * target 'ce' was absent, because there is an old
+	 * entry that is different only in case..
+	 *
+	 * Ignore that lstat() if it matches.
+	 */
+	if (ignore_case && icase_exists(o, name, len, st))
+		return 0;
+
+	if (o->dir && excluded(o->dir, name, &dtype))
+		/*
+		 * ce->name is explicitly excluded, so it is Ok to
+		 * overwrite it.
+		 */
+		return 0;
+	if (S_ISDIR(st->st_mode)) {
+		/*
+		 * We are checking out path "foo" and
+		 * found "foo/." in the working tree.
+		 * This is tricky -- if we have modified
+		 * files that are in "foo/" we would lose
+		 * them.
+		 */
+		if (verify_clean_subdirectory(ce, error_type, o) < 0)
+			return -1;
+		return 0;
+	}
+
+	/*
+	 * The previous round may already have decided to
+	 * delete this path, which is in a subdirectory that
+	 * is being replaced with a blob.
+	 */
+	result = index_name_exists(&o->result, name, len, 0);
+	if (result) {
+		if (result->ce_flags & CE_REMOVE)
+			return 0;
+	}
+
+	return o->gently ? -1 :
+		add_rejected_path(o, error_type, name);
+}
+
 /*
  * We do not want to remove or overwrite a working tree file that
  * is not tracked, unless it is ignored.
@@ -1151,55 +1202,13 @@ static int verify_absent_1(struct cache_entry *ce,
 	if (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))
 		return 0;
 
-	if (!lstat(ce->name, &st)) {
-		int dtype = ce_to_dtype(ce);
-		struct cache_entry *result;
-
-		/*
-		 * It may be that the 'lstat()' succeeded even though
-		 * target 'ce' was absent, because there is an old
-		 * entry that is different only in case..
-		 *
-		 * Ignore that lstat() if it matches.
-		 */
-		if (ignore_case && icase_exists(o, ce, &st))
-			return 0;
-
-		if (o->dir && excluded(o->dir, ce->name, &dtype))
-			/*
-			 * ce->name is explicitly excluded, so it is Ok to
-			 * overwrite it.
-			 */
-			return 0;
-		if (S_ISDIR(st.st_mode)) {
-			/*
-			 * We are checking out path "foo" and
-			 * found "foo/." in the working tree.
-			 * This is tricky -- if we have modified
-			 * files that are in "foo/" we would lose
-			 * them.
-			 */
-			if (verify_clean_subdirectory(ce, error_type, o) < 0)
-				return -1;
-			return 0;
-		}
-
-		/*
-		 * The previous round may already have decided to
-		 * delete this path, which is in a subdirectory that
-		 * is being replaced with a blob.
-		 */
-		result = index_name_exists(&o->result, ce->name, ce_namelen(ce), 0);
-		if (result) {
-			if (result->ce_flags & CE_REMOVE)
-				return 0;
-		}
-
-		return o->gently ? -1 :
-			add_rejected_path(o, error_type, ce->name);
-	}
+	if (!lstat(ce->name, &st))
+		return check_ok_to_remove(ce->name, ce_namelen(ce),
+				ce_to_dtype(ce), ce, &st,
+				error_type, o);
 	return 0;
 }
+
 static int verify_absent(struct cache_entry *ce,
 			 enum unpack_trees_error_types error_type,
 			 struct unpack_trees_options *o)
-- 
1.7.1.571.gba4d01

```

## Clemens Buchacher, 2010-10-09 13:52

Subject: [PATCH 4/5] lstat_cache: optionally return match_len
Message-ID: <1286632380-7002-5-git-send-email-drizzd@aon.at>
URL: https://gitlist.dev/e/1286632380-7002-5-git-send-email-drizzd%40aon.at
In-Reply-To: <7v4oepaup7.fsf@alter.siamese.dyndns.org>

```
Return match_len so that the caller can know which leading path
component matched.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 symlinks.c |   43 +++++++++++++++++++++++++++----------------
 1 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index 8860120..b7343eb 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -64,11 +64,13 @@ static inline void reset_lstat_cache(struct cache_def *cache)
  * of the prefix, where the cache should use the stat() function
  * instead of the lstat() function to test each path component.
  */
-static int lstat_cache(struct cache_def *cache, const char *name, int len,
-		       int track_flags, int prefix_len_stat_func)
+static int lstat_cache_matchlen(struct cache_def *cache,
+				const char *name, int len,
+				int *ret_flags, int track_flags,
+				int prefix_len_stat_func)
 {
 	int match_len, last_slash, last_slash_dir, previous_slash;
-	int match_flags, ret_flags, save_flags, max_len, ret;
+	int save_flags, max_len, ret;
 	struct stat st;
 
 	if (cache->track_flags != track_flags ||
@@ -90,13 +92,13 @@ static int lstat_cache(struct cache_def *cache, const char *name, int len,
 		match_len = last_slash =
 			longest_path_match(name, len, cache->path, cache->len,
 					   &previous_slash);
-		match_flags = cache->flags & track_flags & (FL_NOENT|FL_SYMLINK);
+		*ret_flags = cache->flags & track_flags & (FL_NOENT|FL_SYMLINK);
 
 		if (!(track_flags & FL_FULLPATH) && match_len == len)
 			match_len = last_slash = previous_slash;
 
-		if (match_flags && match_len == cache->len)
-			return match_flags;
+		if (*ret_flags && match_len == cache->len)
+			return match_len;
 		/*
 		 * If we now have match_len > 0, we would know that
 		 * the matched part will always be a directory.
@@ -105,16 +107,16 @@ static int lstat_cache(struct cache_def *cache, const char *name, int len,
 		 * a substring of the cache on a path component basis,
 		 * we can return immediately.
 		 */
-		match_flags = track_flags & FL_DIR;
-		if (match_flags && len == match_len)
-			return match_flags;
+		*ret_flags = track_flags & FL_DIR;
+		if (*ret_flags && len == match_len)
+			return match_len;
 	}
 
 	/*
 	 * Okay, no match from the cache so far, so now we have to
 	 * check the rest of the path components.
 	 */
-	ret_flags = FL_DIR;
+	*ret_flags = FL_DIR;
 	last_slash_dir = last_slash;
 	max_len = len < PATH_MAX ? len : PATH_MAX;
 	while (match_len < max_len) {
@@ -133,16 +135,16 @@ static int lstat_cache(struct cache_def *cache, const char *name, int len,
 			ret = lstat(cache->path, &st);
 
 		if (ret) {
-			ret_flags = FL_LSTATERR;
+			*ret_flags = FL_LSTATERR;
 			if (errno == ENOENT)
-				ret_flags |= FL_NOENT;
+				*ret_flags |= FL_NOENT;
 		} else if (S_ISDIR(st.st_mode)) {
 			last_slash_dir = last_slash;
 			continue;
 		} else if (S_ISLNK(st.st_mode)) {
-			ret_flags = FL_SYMLINK;
+			*ret_flags = FL_SYMLINK;
 		} else {
-			ret_flags = FL_ERR;
+			*ret_flags = FL_ERR;
 		}
 		break;
 	}
@@ -152,7 +154,7 @@ static int lstat_cache(struct cache_def *cache, const char *name, int len,
 	 * path types, FL_NOENT, FL_SYMLINK and FL_DIR, can be cached
 	 * for the moment!
 	 */
-	save_flags = ret_flags & track_flags & (FL_NOENT|FL_SYMLINK);
+	save_flags = *ret_flags & track_flags & (FL_NOENT|FL_SYMLINK);
 	if (save_flags && last_slash > 0 && last_slash <= PATH_MAX) {
 		cache->path[last_slash] = '\0';
 		cache->len = last_slash;
@@ -176,7 +178,16 @@ static int lstat_cache(struct cache_def *cache, const char *name, int len,
 	} else {
 		reset_lstat_cache(cache);
 	}
-	return ret_flags;
+	return match_len;
+}
+
+static int lstat_cache(struct cache_def *cache, const char *name, int len,
+		       int track_flags, int prefix_len_stat_func)
+{
+	int flags;
+	(void)lstat_cache_matchlen(cache, name, len, &flags, track_flags,
+			prefix_len_stat_func);
+	return flags;
 }
 
 #define USE_ONLY_LSTAT  0
-- 
1.7.1.571.gba4d01

```

## Clemens Buchacher, 2010-10-09 13:53

Subject: [PATCH 5/5] do not overwrite files in leading path
Message-ID: <1286632380-7002-6-git-send-email-drizzd@aon.at>
URL: https://gitlist.dev/e/1286632380-7002-6-git-send-email-drizzd%40aon.at
In-Reply-To: <7v4oepaup7.fsf@alter.siamese.dyndns.org>

```
If the work tree contains an untracked file x, and
unpack-trees wants to checkout a path x/*, the
file x is removed unconditionally.

Instead, apply the same checks that are normally
used for untracked files, and abort if the file
cannot be removed.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

It's not exactly pretty. But it's the best solution I could find
without major refactoring.

Clemens

 cache.h                    |    2 +-
 symlinks.c                 |   21 ++++++++++++++++-----
 t/t7607-merge-overwrite.sh |    2 +-
 unpack-trees.c             |   16 +++++++++++++---
 4 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 2ef2fa3..f65d6f9 100644
--- a/cache.h
+++ b/cache.h
@@ -852,7 +852,7 @@ struct cache_def {
 
 extern int has_symlink_leading_path(const char *name, int len);
 extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
-extern int has_symlink_or_noent_leading_path(const char *name, int len);
+extern int check_leading_path(const char *name, int len);
 extern int has_dirs_only_path(const char *name, int len, int prefix_len);
 extern void schedule_dir_for_removal(const char *name, int len);
 extern void remove_scheduled_dirs(void);
diff --git a/symlinks.c b/symlinks.c
index b7343eb..3cacebd 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -209,15 +209,26 @@ int has_symlink_leading_path(const char *name, int len)
 }
 
 /*
- * Return non-zero if path 'name' has a leading symlink component or
+ * Return zero if path 'name' has a leading symlink component or
  * if some leading path component does not exists.
+ *
+ * Return -1 if leading path exists and is a directory.
+ *
+ * Return path length if leading path exists and is neither a
+ * directory nor a symlink.
  */
-int has_symlink_or_noent_leading_path(const char *name, int len)
+int check_leading_path(const char *name, int len)
 {
 	struct cache_def *cache = &default_cache;	/* FIXME */
-	return lstat_cache(cache, name, len,
-			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT) &
-		(FL_SYMLINK|FL_NOENT);
+	int flags;
+	int match_len = lstat_cache_matchlen(cache, name, len, &flags,
+			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);
+	if (flags & (FL_SYMLINK|FL_NOENT))
+		return 0;
+	else if (flags & FL_DIR)
+		return -1;
+	else
+		return match_len;
 }
 
 /*
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index 01b070b..412db15 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -100,7 +100,7 @@ test_expect_success 'will not overwrite untracked subtree' '
 	test_cmp important sub/f/important
 '
 
-test_expect_failure 'will not overwrite untracked file in leading path' '
+test_expect_success 'will not overwrite untracked file in leading path' '
 	git reset --hard c0 &&
 	rm -rf sub &&
 	cp important sub &&
diff --git a/unpack-trees.c b/unpack-trees.c
index df1c920..6816113 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -182,7 +182,7 @@ static void display_error_msgs(struct unpack_trees_options *o)
  */
 static void unlink_entry(struct cache_entry *ce)
 {
-	if (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))
+	if (!check_leading_path(ce->name, ce_namelen(ce)))
 		return;
 	if (remove_or_warn(ce->ce_mode, ce->name))
 		return;
@@ -1194,18 +1194,28 @@ static int verify_absent_1(struct cache_entry *ce,
 				 enum unpack_trees_error_types error_type,
 				 struct unpack_trees_options *o)
 {
+	int len;
 	struct stat st;
 
 	if (o->index_only || o->reset || !o->update)
 		return 0;
 
-	if (has_symlink_or_noent_leading_path(ce->name, ce_namelen(ce)))
+	len = check_leading_path(ce->name, ce_namelen(ce));
+	if (!len)
 		return 0;
+	else if (len > 0) {
+		char path[PATH_MAX + 1];
+		memcpy(path, ce->name, len);
+		path[len] = 0;
+		lstat(path, &st);
 
-	if (!lstat(ce->name, &st))
+		return check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,
+				error_type, o);
+	} else if (!lstat(ce->name, &st))
 		return check_ok_to_remove(ce->name, ce_namelen(ce),
 				ce_to_dtype(ce), ce, &st,
 				error_type, o);
+
 	return 0;
 }
 
-- 
1.7.1.571.gba4d01

```

## Johannes Sixt, 2010-10-09 19:14

Subject: Re: [PATCH 2/5] t7607: add leading-path tests
Message-ID: <201010092114.51881.j6t@kdbg.org>
URL: https://gitlist.dev/e/201010092114.51881.j6t%40kdbg.org
In-Reply-To: <1286632380-7002-3-git-send-email-drizzd@aon.at>

```
On Samstag, 9. Oktober 2010, Clemens Buchacher wrote:
> +test_expect_failure 'will not overwrite untracked symlink in leading path'

> +test_expect_success 'will not be confused by symlink in leading path' '

Please guard these two with a SYMLINKS prerequisite.

-- Hannes

```

## Kevin Ballard, 2010-10-09 22:39

Subject: Re: [PATCH] optionally disable overwriting of ignored files
Message-ID: <617F82D8-DDDE-422C-B4F9-9D35B24BB420@sb.org>
URL: https://gitlist.dev/e/617F82D8-DDDE-422C-B4F9-9D35B24BB420%40sb.org
In-Reply-To: <20100821080512.GA30141@localhost>

```
On Aug 21, 2010, at 1:05 AM, Clemens Buchacher wrote:

> Files that go into a tracked .gitignore are most likely generated
> files, and therefore trashable. Files that go into
> .git/info/exclude or into an untracked .gitignore (e.g. echo '*' >
> precious-simulation-results/.gitignore), are not always generated
> and may not be trashable. At least they would not likely get in the
> way of checkout or merge.

I can think of at least one common case that doesn't match this pattern - A web app that has a config file. What I've seen frequently done is the config file is named something like foo.config.template, and that's tracked, and you're encouraged to copy this to foo.config and change the values. This file may not want to be tracked because it contains sensitive information like database passwords, so the repo may stick it in the .gitignore file to ensure that this never gets added. However, this is most definitely a "precious" file. For this case, and others like it, I am strongly in favor of Junio's suggestion to add a 3rd category "precious but untracked".

-Kevin Ballard
```

## Jonathan Nieder, 2010-10-10 06:35

Subject: Re: [PATCH 1/5] t7607: use test_commit and test_must_fail
Message-ID: <20101010063527.GC23100@burratino>
URL: https://gitlist.dev/e/20101010063527.GC23100%40burratino
In-Reply-To: <1286632380-7002-2-git-send-email-drizzd@aon.at>

```
Clemens Buchacher wrote:

> [Subject: t7607: use test_commit and test_must_fail]
>
> Also make sure that aborted merges do not leave
> MERGE_HEAD except in the "will not overwrite
> removed file" test, where we cannot do so.

Motivation?

> See
> also the discussion in the following thread.
>
> http://mid.gmane.org/7vskopwxej.fsf@gitster.siamese.dyndns.org

That subthread is about teaching merge-recursive to write its results
to the index first without touching the worktree for robustness.  It
is not immediately obvious what that has to do with not leaving
MERGE_HEAD.  Could you summarize the relevant part for the commit
message so the reader does not have to track it down?

> --- a/t/t7607-merge-overwrite.sh
> +++ b/t/t7607-merge-overwrite.sh
> @@ -7,81 +7,73 @@ Do not overwrite changes.'
>  . ./test-lib.sh
>  
>  test_expect_success 'setup' '
> -	echo c0 > c0.c &&
> -	git add c0.c &&
> -	git commit -m c0 &&
> -	git tag c0 &&
> +	test_commit c0 &&

Behavior changes:

 . filename is c0.t instead of c0.c
 . uses test_tick for timestamp

Sounds good.

> -	echo c1 > c1.c &&
> -	git add c1.c &&
> -	git commit -m c1 &&
> -	git tag c1 &&
> +	test_commit c1 &&

Another almost-noop (good).

> -	git reset --hard c0 &&
> -	echo c2 > c2.c &&
> -	git add c2.c &&
> -	git commit -m c2 &&
> -	git tag c2 &&
> -	git reset --hard c1 &&
> -	echo "c1 a" > c1.c &&
> -	git add c1.c &&
> -	git commit -m "c1 a" &&
> -	git tag c1a &&
> +	test_commit "c1a" "c1.t" "c1 a" &&
> +	git reset --hard c0 &&
> +	test_commit c2 &&

The reader might not remember the c1.t filename.  Maybe
it would be good to be explicit in the 'test_commit c1' line:

	test_commit c1 c1.t

?  Or maybe a comment could help?

	# Commit rewriting the file from c1

>  	echo "VERY IMPORTANT CHANGES" > important
>  '
>  
>  test_expect_success 'will not overwrite untracked file' '
>  	git reset --hard c1 &&
> -	cat important > c2.c &&
> +	cp important c2.t &&
>  	test_must_fail git merge c2 &&
> -	test_cmp important c2.c
> +	! test -f .git/MERGE_HEAD &&
> +	test_cmp important c2.t
>  '

Nit: if backportability is not important, maybe

	test_path_is_missing .git/MERGE_HEAD &&

for better error messages when running tests with -v and
the file is present?

[...]
> -	cat important > c2.c &&
> -	git add c2.c &&
> -	rm c2.c &&
> +	cp important c2.t &&
> +	git add c2.t &&
> +	rm c2.t &&

These changes make for a lot of noise.  Why not specify the
filenames in the setup test to keep the .c extension?

The main change (checking that MERGE_HEAD is not present
for a merge that fails due to pre-merge checks) seems good.
Thanks.

```

## Clemens Buchacher, 2010-10-10 08:35

Subject: [PATCH 1/5 v2] t7607: use test-lib functions and check MERGE_HEAD
Message-ID: <20101010083543.GA12186@localhost>
URL: https://gitlist.dev/e/20101010083543.GA12186%40localhost
In-Reply-To: <20101010063527.GC23100@burratino>

```
Use the test_commit and test_path_is_missing
functions from the test library.

Also make sure that a merge which fails due to
pre-merge checks aborts properly and does not
leave MERGE_HEAD behind.

The "will not overwrite removed file" test is an
exception to this. It notices the untracked file
at a stage where the merge is already well under
way. Therefore we cannot abort the merge without
major restructuring. See the following thread for
more details.

http://mid.gmane.org/7vskopwxej.fsf@gitster.siamese.dyndns.org

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Sun, Oct 10, 2010 at 01:35:27AM -0500, Jonathan Nieder wrote:
> 
> The main change (checking that MERGE_HEAD is not present
> for a merge that fails due to pre-merge checks) seems good.

Thanks. This new version incorporates all of your suggestions.

Clemens

 t/t7607-merge-overwrite.sh |   38 +++++++++++++++-----------------------
 1 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index d82349a..b8fab54 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -7,48 +7,38 @@ Do not overwrite changes.'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-	echo c0 > c0.c &&
-	git add c0.c &&
-	git commit -m c0 &&
-	git tag c0 &&
-	echo c1 > c1.c &&
-	git add c1.c &&
-	git commit -m c1 &&
-	git tag c1 &&
+	test_commit c0 c0.c &&
+	test_commit c1 c1.c &&
+	test_commit c1a c1.c "c1 a" &&
 	git reset --hard c0 &&
-	echo c2 > c2.c &&
-	git add c2.c &&
-	git commit -m c2 &&
-	git tag c2 &&
-	git reset --hard c1 &&
-	echo "c1 a" > c1.c &&
-	git add c1.c &&
-	git commit -m "c1 a" &&
-	git tag c1a &&
+	test_commit c2 c2.c &&
 	echo "VERY IMPORTANT CHANGES" > important
 '
 
 test_expect_success 'will not overwrite untracked file' '
 	git reset --hard c1 &&
-	cat important > c2.c &&
+	cp important c2.c &&
 	test_must_fail git merge c2 &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test_cmp important c2.c
 '
 
 test_expect_success 'will not overwrite new file' '
 	git reset --hard c1 &&
-	cat important > c2.c &&
+	cp important c2.c &&
 	git add c2.c &&
 	test_must_fail git merge c2 &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test_cmp important c2.c
 '
 
 test_expect_success 'will not overwrite staged changes' '
 	git reset --hard c1 &&
-	cat important > c2.c &&
+	cp important c2.c &&
 	git add c2.c &&
 	rm c2.c &&
 	test_must_fail git merge c2 &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	git checkout c2.c &&
 	test_cmp important c2.c
 '
@@ -57,7 +47,7 @@ test_expect_success 'will not overwrite removed file' '
 	git reset --hard c1 &&
 	git rm c1.c &&
 	git commit -m "rm c1.c" &&
-	cat important > c1.c &&
+	cp important c1.c &&
 	test_must_fail git merge c1a &&
 	test_cmp important c1.c
 '
@@ -66,9 +56,10 @@ test_expect_success 'will not overwrite re-added file' '
 	git reset --hard c1 &&
 	git rm c1.c &&
 	git commit -m "rm c1.c" &&
-	cat important > c1.c &&
+	cp important c1.c &&
 	git add c1.c &&
 	test_must_fail git merge c1a &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	test_cmp important c1.c
 '
 
@@ -76,10 +67,11 @@ test_expect_success 'will not overwrite removed file with staged changes' '
 	git reset --hard c1 &&
 	git rm c1.c &&
 	git commit -m "rm c1.c" &&
-	cat important > c1.c &&
+	cp important c1.c &&
 	git add c1.c &&
 	rm c1.c &&
 	test_must_fail git merge c1a &&
+	test_path_is_missing .git/MERGE_HEAD &&
 	git checkout c1.c &&
 	test_cmp important c1.c
 '
-- 
1.7.1.571.gba4d01


```

## Clemens Buchacher, 2010-10-10 08:38

Subject: [PATCH 2/5 v2] t7607: add leading-path tests
Message-ID: <20101010083858.GB12186@localhost>
URL: https://gitlist.dev/e/20101010083858.GB12186%40localhost
In-Reply-To: <201010092114.51881.j6t@kdbg.org>

```

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

This patch depends on PATCH 1/5 v2. The remaining patches 3 to 5
still apply cleanly.

On Sat, Oct 09, 2010 at 09:14:51PM +0200, Johannes Sixt wrote:
> On Samstag, 9. Oktober 2010, Clemens Buchacher wrote:
> > +test_expect_failure 'will not overwrite untracked symlink in leading path'
> 
> > +test_expect_success 'will not be confused by symlink in leading path' '
> 
> Please guard these two with a SYMLINKS prerequisite.

Done. Thank you.

Clemens

 t/t7607-merge-overwrite.sh |   51 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index b8fab54..77fcaa2 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -12,6 +12,12 @@ test_expect_success 'setup' '
 	test_commit c1a c1.c "c1 a" &&
 	git reset --hard c0 &&
 	test_commit c2 c2.c &&
+	git reset --hard c0 &&
+	mkdir sub &&
+	echo "sub/f" > sub/f &&
+	git add sub/f &&
+	git commit -m sub &&
+	git tag sub &&
 	echo "VERY IMPORTANT CHANGES" > important
 '
 
@@ -23,6 +29,14 @@ test_expect_success 'will not overwrite untracked file' '
 	test_cmp important c2.c
 '
 
+test_expect_success 'will overwrite tracked file' '
+	git reset --hard c1 &&
+	cp important c2.c &&
+	git add c2.c &&
+	git commit -m important &&
+	git checkout c2
+'
+
 test_expect_success 'will not overwrite new file' '
 	git reset --hard c1 &&
 	cp important c2.c &&
@@ -76,4 +90,41 @@ test_expect_success 'will not overwrite removed file with staged changes' '
 	test_cmp important c1.c
 '
 
+test_expect_success 'will not overwrite untracked subtree' '
+	git reset --hard c0 &&
+	rm -rf sub &&
+	mkdir -p sub/f &&
+	cp important sub/f/important &&
+	test_must_fail git merge sub &&
+	test_path_is_missing .git/MERGE_HEAD &&
+	test_cmp important sub/f/important
+'
+
+test_expect_failure 'will not overwrite untracked file in leading path' '
+	git reset --hard c0 &&
+	rm -rf sub &&
+	cp important sub &&
+	test_must_fail git merge sub &&
+	test_path_is_missing .git/MERGE_HEAD &&
+	test_cmp important sub
+'
+
+test_expect_failure SYMLINKS 'will not overwrite untracked symlink in leading path' '
+	git reset --hard c0 &&
+	rm -rf sub &&
+	mkdir sub2 &&
+	ln -s sub2 sub &&
+	test_must_fail git merge sub &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
+test_expect_success SYMLINKS 'will not be confused by symlink in leading path' '
+	git reset --hard c0 &&
+	rm -rf sub &&
+	ln -s sub2 sub &&
+	git add sub &&
+	git commit -m ln &&
+	git checkout sub
+'
+
 test_done
-- 
1.7.1.571.gba4d01


```

## Junio C Hamano, 2010-10-13 21:33

Subject: Re: [PATCH 1/5 v2] t7607: use test-lib functions and check MERGE_HEAD
Message-ID: <7vhbgpkcbp.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vhbgpkcbp.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20101010083543.GA12186@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

> --ew6BAiZeqk4r7MaW
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable

Please don't do this...

>
> --ew6BAiZeqk4r7MaW
> Content-Type: application/pgp-signature; name="signature.asc"
> Content-Description: Digital signature
> Content-Disposition: inline
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iQEcBAEBAgAGBQJMsXrfAAoJELKdZexG8uqM/BwH/Ai2w+DFW4L9D1SivLoxDL8/
> Z3YDRrx5oJa35ZhbBwInGJx7xwXG3Pn/mx0avGwoRwQe9cRufp6AO5hHUq3U1LNv
> ZaP4RdlScMqeuKUu8mrbjJs4kumL/sjZ59MRnBZzX1Ovdq/GbKhJqidYTvmHQc6e
> 0sngXx9Jf1WlS7m1sDztYPRJ3z2lF0js+BNHLIVOi6CgSbBJYdQzeLrvO/BVX9V0
> P2F0bGZLWTqqOSLMav5jcYFAgIv8mRqxjre+1IviFuGTuu5hX7yTx++qAiV8CK0f
> Fpe0HNQlYahbytd9qlOoqsbV5fuXkOoFqYHKmT6461u31QHaYPyBwsSWgjkF9IE=
> =3pEL
> -----END PGP SIGNATURE-----

Nor this.

```

## Junio C Hamano, 2010-10-13 21:43

Subject: Re: [PATCH 3/5] add function check_ok_to_remove()
Message-ID: <7v7hhlkbtz.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7v7hhlkbtz.fsf%40alter.siamese.dyndns.org
In-Reply-To: <1286632380-7002-4-git-send-email-drizzd@aon.at>

```
Clemens Buchacher <drizzd@aon.at> writes:

> This wraps some inline code into the function check_ok_to_remove(),
> which will later be used for leading path components as well.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>

Nice depth reduction.

```

## Junio C Hamano, 2010-10-13 21:57

Subject: Re: [PATCH 5/5] do not overwrite files in leading path
Message-ID: <7v1v7tkb6g.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7v1v7tkb6g.fsf%40alter.siamese.dyndns.org
In-Reply-To: <1286632380-7002-6-git-send-email-drizzd@aon.at>

```
Clemens Buchacher <drizzd@aon.at> writes:

> If the work tree contains an untracked file x, and
> unpack-trees wants to checkout a path x/*, the
> file x is removed unconditionally.
>
> Instead, apply the same checks that are normally
> used for untracked files, and abort if the file
> cannot be removed.
>

Too short a line is also hard to read as too long a line.

> diff --git a/cache.h b/cache.h
> index 2ef2fa3..f65d6f9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -852,7 +852,7 @@ struct cache_def {
>  
>  extern int has_symlink_leading_path(const char *name, int len);
>  extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
> -extern int has_symlink_or_noent_leading_path(const char *name, int len);
> +extern int check_leading_path(const char *name, int len);

This is an API regression.  "check" in function name "check-blah" does not
tell what you are checking (is it checking if the path is Ok?  is it
checking if the path has symlinked components?  is it checking if some
part of the path overlaps with tracked content?  is it checking if you
have already run lstat(2) on some of the leading path components?), nor
for what purpose the check can be used (it is Ok to overwrite it?, is it
Ok to create a directory there?  is it Ok to create a file there?).

At least the old name told us what it checked, didn't it?

The approach you took looks sound, though.  What did you feel was "not
exactly pretty" about it?

Thanks.

```

## Junio C Hamano, 2010-10-13 21:59

Subject: Re: [PATCH 1/5 v2] t7607: use test-lib functions and check MERGE_HEAD
Message-ID: <7v62x5iwj7.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7v62x5iwj7.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20101010083543.GA12186@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

>  test_expect_success 'will not overwrite untracked file' '
>  	git reset --hard c1 &&
> -	cat important > c2.c &&
> +	cp important c2.c &&
>  	test_must_fail git merge c2 &&
> +	test_path_is_missing .git/MERGE_HEAD &&
>  	test_cmp important c2.c
>  '
>  
>  test_expect_success 'will not overwrite new file' '
>  	git reset --hard c1 &&
> -	cat important > c2.c &&
> +	cp important c2.c &&

Why?

```

## Clemens Buchacher, 2010-10-13 22:34

Subject: Re: [PATCH 5/5] do not overwrite files in leading path
Message-ID: <20101013223425.GB2558@localhost>
URL: https://gitlist.dev/e/20101013223425.GB2558%40localhost
In-Reply-To: <7v1v7tkb6g.fsf@alter.siamese.dyndns.org>

```
On Wed, Oct 13, 2010 at 02:57:59PM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > If the work tree contains an untracked file x, and
> > unpack-trees wants to checkout a path x/*, the
> > file x is removed unconditionally.
> >
> > Instead, apply the same checks that are normally
> > used for untracked files, and abort if the file
> > cannot be removed.
> >
> 
> Too short a line is also hard to read as too long a line.

Ok.

> > diff --git a/cache.h b/cache.h
> > index 2ef2fa3..f65d6f9 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -852,7 +852,7 @@ struct cache_def {
> >  
> >  extern int has_symlink_leading_path(const char *name, int len);
> >  extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
> > -extern int has_symlink_or_noent_leading_path(const char *name, int len);
> > +extern int check_leading_path(const char *name, int len);
> 
> This is an API regression.  "check" in function name "check-blah" does not
> tell what you are checking (is it checking if the path is Ok?  is it
> checking if the path has symlinked components?  is it checking if some
> part of the path overlaps with tracked content?  is it checking if you
> have already run lstat(2) on some of the leading path components?), nor
> for what purpose the check can be used (it is Ok to overwrite it?, is it
> Ok to create a directory there?  is it Ok to create a file there?).
>
> At least the old name told us what it checked, didn't it?

Yes, but that's not what it does any more. I worded it vague on
purpose, because there is really no way to explain all it does in
one function name. This is the full description.

/*
 * Return zero if path 'name' has a leading symlink component or
 * if some leading path component does not exists.
 *
 * Return -1 if leading path exists and is a directory.
 *
 * Return path length if leading path exists and is neither a
 * directory nor a symlink.
 */

But if you prefer, we can call it "verify_clean_leading_path"?

> The approach you took looks sound, though.  What did you feel was "not
> exactly pretty" about it?

First, check_leading_path bothers me. It does too many things and
encodes all of that in one integer return value.

Second, I also have to copy the offending path manually to a local
path variable, even though lstat_cache already has that path stored
in a buffer.

Third, check_ok_to_remove expects a cache_entry. But in case of a
match in check_leading_path, we do not have one and pass NULL to
check_ok_to_rmove. It will not be used anyways, because
check_leading_path should return > 0 only for regular files, and
the cache_entry is only used for directories in
verify_clean_subdirectory.

If lstat returns with an error other than NOENT, or if
check_ok_to_remove is called with anything other than a directory
and cache_entry is NULL, we get a segmentation fault. Before, an
error was simply ignored. I don't know which is worse.

On the other hand, I think the patch fixes a nasty bug, so maybe we
can accept those issues for now.

Clemens

```

## Clemens Buchacher, 2010-10-15 06:48

Subject: Re: [PATCH 5/5] do not overwrite files in leading path
Message-ID: <20101015064835.GA12119@localhost>
URL: https://gitlist.dev/e/20101015064835.GA12119%40localhost
In-Reply-To: <20101013223425.GB2558@localhost>

```
On Thu, Oct 14, 2010 at 12:34:25AM +0200, Clemens Buchacher wrote:
> 
> If lstat returns with an error other than NOENT, or if
> check_ok_to_remove is called with anything other than a directory
> and cache_entry is NULL, we get a segmentation fault. Before, an
> error was simply ignored. I don't know which is worse.

I suppose we only need the following additional changes.

- die if lstat returns an error other than ENOENT.

- Rewrite verify_clean_subdirectory to not require a cache_entry.

- Expose lstat result and path cache to the caller of
  lstat_cache_matchlen() in verify_absent_1().

- rewrite check_leading_path (or 'verify_clean_path') to
  check the full path and return zero if
  + the leading path contains a symlink or
  + the leading path exists, but the full path does not
  and returns the path length of the offending entry otherwise.
  
I think that's manageable and much cleaner. I will start to work on
it as soon as possible.

Thank you for your review.

Clemens

```

## Junio C Hamano, 2010-10-15 18:47

Subject: Re: [PATCH 5/5] do not overwrite files in leading path
Message-ID: <7vfww7e1j7.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vfww7e1j7.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20101015064835.GA12119@localhost>

```
Clemens Buchacher <drizzd@aon.at> writes:

> On Thu, Oct 14, 2010 at 12:34:25AM +0200, Clemens Buchacher wrote:
>> 
>> If lstat returns with an error other than NOENT, or if
>> check_ok_to_remove is called with anything other than a directory
>> and cache_entry is NULL, we get a segmentation fault. Before, an
>> error was simply ignored. I don't know which is worse.
>
> I suppose we only need the following additional changes.
>
> - die if lstat returns an error other than ENOENT.
>
> - Rewrite verify_clean_subdirectory to not require a cache_entry.
>
> - Expose lstat result and path cache to the caller of
>   lstat_cache_matchlen() in verify_absent_1().
>
> - rewrite check_leading_path (or 'verify_clean_path') to
>   check the full path and return zero if
>   + the leading path contains a symlink or
>   + the leading path exists, but the full path does not
>   and returns the path length of the offending entry otherwise.
>   
> I think that's manageable and much cleaner. I will start to work on
> it as soon as possible.

Thanks.  I like seeing people thinking things through ;-).

```