threads / discuss / 24763

git merge, .gitignore, and silently overwriting untracked files

Subject: git merge, .gitignore, and silently overwriting untracked files

## tl;dr

39 messages between Aug 17, 2010 and Oct 15, 2010.

replies: 38people: 10as markdown or json

Joshua Jensen· Aug 17, 2010, 05:21 UTC · lore

  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· Aug 17, 2010, 19:33 UTC · re: Joshua Jensen · lore

Re: git merge, .gitignore, and silently overwriting untracked files

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· Aug 18, 2010, 23:39 UTC · re: Junio C Hamano · lore

[PATCH] optionally disable overwriting of ignored files

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:

Show 8 quoted lines

>
> 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· Aug 19, 2010, 10:41 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

Clemens Buchacher <drizzd@aon.at> writes:

Show 7 quoted lines

>  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

Show 22 quoted lines

> @@ -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· Aug 20, 2010, 18:48 UTC · re: Jakub Narebski · lore

Re: [PATCH] optionally disable overwriting of ignored files

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· Aug 20, 2010, 19:01 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

  ----- Original Message -----
From: Clemens Buchacher
Date: 8/20/2010 12:48 PM

Show 5 quoted lines

> 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· Aug 20, 2010, 20:35 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

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.

Clemens Buchacher· Aug 21, 2010, 08:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] optionally disable overwriting of ignored files

On Fri, Aug 20, 2010 at 01:35:32PM -0700, Junio C Hamano wrote:

Show 6 quoted lines

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

Show 8 quoted lines

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

Show 16 quoted lines

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

Show 13 quoted lines

> 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

Junio C Hamano· Aug 22, 2010, 07:25 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

Clemens Buchacher <drizzd@aon.at> writes:

Show 6 quoted lines

> 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· Aug 22, 2010, 08:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] optionally disable overwriting of ignored files

On Sun, Aug 22, 2010 at 12:25:31AM -0700, Junio C Hamano wrote:

Show 5 quoted lines

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

Kevin Ballard· Oct 9, 2010, 22:39 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

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

Show 6 quoted lines

> 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

Clemens Buchacher· Aug 21, 2010, 13:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] optionally disable overwriting of ignored files

On Fri, Aug 20, 2010 at 01:35:32PM -0700, Junio C Hamano wrote:

Show 7 quoted lines

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

Clemens Buchacher· Oct 9, 2010, 13:52 UTC · re: Junio C Hamano · lore

[PATCH 0/5] do not overwrite untracked files in leading path

Hi,

On Fri, Aug 20, 2010 at 01:35:32PM -0700, Junio C Hamano wrote:

Show 7 quoted lines

>
>     $ 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· Oct 9, 2010, 13:52 UTC · re: Junio C Hamano · lore

[PATCH 1/5] 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. 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

Jonathan Nieder· Oct 10, 2010, 06:35 UTC · re: Clemens Buchacher · lore

Re: [PATCH 1/5] t7607: use test_commit and test_must_fail

Clemens Buchacher wrote:

Show 5 quoted lines

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

Show 11 quoted lines

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

Show 5 quoted lines

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

Another almost-noop (good).

Show 13 quoted lines

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

Show 12 quoted lines

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

[...]

Show 6 quoted lines

> -	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· Oct 10, 2010, 08:35 UTC · re: Jonathan Nieder · lore

[PATCH 1/5 v2] t7607: use test-lib functions and check MERGE_HEAD

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

Junio C Hamano· Oct 13, 2010, 21:33 UTC · re: Clemens Buchacher · lore

Re: [PATCH 1/5 v2] t7607: use test-lib functions and check MERGE_HEAD

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

Show 17 quoted lines

>
> --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· Oct 13, 2010, 21:59 UTC · re: Clemens Buchacher · lore

Re: [PATCH 1/5 v2] t7607: use test-lib functions and check MERGE_HEAD

Clemens Buchacher <drizzd@aon.at> writes:

Show 13 quoted lines

>  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· Oct 9, 2010, 13:52 UTC · re: Junio C Hamano · lore

[PATCH 2/5] t7607: add leading-path tests

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

Johannes Sixt· Oct 9, 2010, 19:14 UTC · re: Clemens Buchacher · lore

Re: [PATCH 2/5] t7607: add leading-path tests

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

Clemens Buchacher· Oct 10, 2010, 08:38 UTC · re: Johannes Sixt · lore

[PATCH 2/5 v2] t7607: add leading-path tests

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:

Show 6 quoted lines

> 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

Clemens Buchacher· Oct 9, 2010, 13:52 UTC · re: Junio C Hamano · lore

[PATCH 3/5] add function check_ok_to_remove()

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

Junio C Hamano· Oct 13, 2010, 21:43 UTC · re: Clemens Buchacher · lore

Re: [PATCH 3/5] add function check_ok_to_remove()

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.

Clemens Buchacher· Oct 9, 2010, 13:52 UTC · re: Junio C Hamano · lore

[PATCH 4/5] lstat_cache: optionally return match_len

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· Oct 9, 2010, 13:53 UTC · re: Junio C Hamano · lore

[PATCH 5/5] do not overwrite files in leading path

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

Junio C Hamano· Oct 13, 2010, 21:57 UTC · re: Clemens Buchacher · lore

Re: [PATCH 5/5] do not overwrite files in leading path

Clemens Buchacher <drizzd@aon.at> writes:

Show 8 quoted lines

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

Show 10 quoted lines

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

Clemens Buchacher· Oct 13, 2010, 22:34 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/5] do not overwrite files in leading path

On Wed, Oct 13, 2010 at 02:57:59PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

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

Show 20 quoted lines

> > 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· Oct 15, 2010, 06:48 UTC · re: Clemens Buchacher · lore

Re: [PATCH 5/5] do not overwrite files in leading path

On Thu, Oct 14, 2010 at 12:34:25AM +0200, Clemens Buchacher wrote:

Show 5 quoted lines

> 
> 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· Oct 15, 2010, 18:47 UTC · re: Clemens Buchacher · lore

Re: [PATCH 5/5] do not overwrite files in leading path

Clemens Buchacher <drizzd@aon.at> writes:

Show 24 quoted lines

> 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 ;-).

Junio C Hamano· Aug 20, 2010, 20:46 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

Clemens Buchacher <drizzd@aon.at> writes:

Show 16 quoted lines

> 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· Aug 21, 2010, 06:48 UTC · re: Junio C Hamano · lore

[PATCH v2] optionally disable overwriting of ignored files

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

Matthieu Moy· Aug 23, 2010, 08:33 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

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/

Heiko Voigt· Aug 31, 2010, 18:44 UTC · re: Matthieu Moy · lore

Re: Re: [PATCH] optionally disable overwriting of ignored files

On Mon, Aug 23, 2010 at 10:33:21AM +0200, Matthieu Moy wrote:

Show 11 quoted lines

> 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

Matthieu Moy· Aug 23, 2010, 09:37 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

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· Aug 23, 2010, 13:56 UTC · re: Matthieu Moy · lore

Re: [PATCH] optionally disable overwriting of ignored files

Am 23.08.2010 11:37, schrieb Matthieu Moy:

Show 9 quoted lines

> 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· Aug 23, 2010, 15:11 UTC · re: Holger Hellmuth · lore

Re: [PATCH] optionally disable overwriting of ignored files

On Mon, Aug 23, 2010 at 03:56:39PM +0200, Holger Hellmuth wrote:

Show 7 quoted lines

> 
> 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· Aug 23, 2010, 15:57 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

Clemens Buchacher <drizzd@aon.at> writes:

Show 10 quoted lines

> 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· Aug 24, 2010, 07:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] optionally disable overwriting of ignored files

On Mon, Aug 23, 2010 at 08:57:38AM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> 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· Aug 24, 2010, 16:19 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optionally disable overwriting of ignored files

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.