git-add fails after file type change

In the course of experimenting with using git for my snapshot backups, I ran into what looks like a bug in git-add: it croaks when it tries to add a file whose type has changed, specifically when a directory gets moved and a symbolic link is put in the old location pointing to the new one. Here's a simple test case:

$ git init-db
defaulting to local storage area
$ mkdir dir
$ echo foo > dir/file
$ git add .
$ git commit -m "initial commit" -a
Committing initial tree f4bc9c50d08b041f5e096fa68e243c34170f1cd8
create mode 100644 dir/file
$ mv dir dir.real
$ ln -s dir.real dir
$ git add .
fatal: unable to add dir to index

Is "git add ." the wrong thing to do here? I have been using it as a generic "pick up all the files I haven't added yet" command. Or is this a bug?

For what it's worth, "git update-index dir" and "git update-index --add dir" both fail too.

Works if you use "git mv dir dir.real".

$ git init-db
defaulting to local storage area
$ mkdir dir
$ echo foo > dir/file
$ git add .
$ git commit -m "initial commit" -a
Committing initial tree f4bc9c50d08b041f5e096fa68e243c34170f1cd8
$ git mv dir dir.real
$ ln -s dir.real dir
$ git add .
$ git commit -m "second commit" -a
$ git ls-tree HEAD
120000 blob e05f72eddb14362b836c3612c13d441b097a065a    dir
040000 tree 4a1c03029e7407c0afe9fc0320b3258e188b115e    dir.real

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

This came up during my testing of snapshot backups. The *real* sequence is more like

$ git checkout -b new-snapshot
$ rsync /live/directory .
$ git add .

In other words, I don't know in advance that there's a rename or (short of turning on verbose rsync output and parsing it) which parts of the tree have changed at all. So I can't easily use git-mv here.

It still feels like a bug that "git add" can fail with no useful diagnostic. It actually took me a fair while to figure out what was going on here -- at first I thought it was having trouble with symlinks in general, then with absolute-path symlinks (which the actual symlink in question is), then I thought maybe it was a corrupt index. It wasn't until I went back and looked at the previous snapshot that I was overlaying this one on top of that I realized there used to be a directory where that symlink lives now.

Turns out that doesn't work. It gives me the same error I get without the --replace option:

error: dir: cannot add to the index - missing --add option?
fatal: Unable to process file dir

However, "git rm" followed by "git add" does seem to work. So for now it looks like that'll be the best bet when I run into this problem (should be possible to automate it, even.) The better error message from Junio's patch will at least make the failure less mysterious.

IMO it'd still be nice if the porcelain could provide a "make the index look like the working directory" operation that worked even in the face of changes like this, adding and/or removing files as needed. But it's possible that my crazy git-as-backup-device setup is the only place where this is even an issue; typical git users can use "git mv" and never run into the problem.

-Steve

"update-index --replace --add" would be the way.

        $ git ls-files -s
        100644 fa457baf8abbf5dd3bb4cbfab0c5a4cf0523d7f8 0	1/2
        100644 00750edc07d6415dcc07ae0351e9397b0222b7ba 0	3
        $ ls -F
        ./  ../  1/  3	.git/

There is file 1/2 in directory 1.

        $ mv 1 tmp ; mv 3 1 ; mv tmp 3
        $ ls -F
        ./  ../  1  3/	.git/

I just swapped them.

        $ git update-index --replace --add 1
        $ git ls-files -s
        100644 00750edc07d6415dcc07ae0351e9397b0222b7ba 0	1
        100644 00750edc07d6415dcc07ae0351e9397b0222b7ba 0	3

You are allowing update-index to 'add' things so you would need to say --add regardless of --replace (--replace is only to allow removal of conflicting entries while adding). In the hindsight, we could have implied --add with --replace, but that is the way it is, and update-index is not a Porcelain so there is not much point fixing it now.

But I think you helped me to spot a bug ;-).

        $ git update-index --replace --add 3/2
        $ git ls-files -s
        100644 00750edc07d6415dcc07ae0351e9397b0222b7ba 0	1
	100644 00750edc07d6415dcc07ae0351e9397b0222b7ba 0	3
        100644 fa457baf8abbf5dd3bb4cbfab0c5a4cf0523d7f8 0	3/2

The entry '3' should have been removed when we did --replace. This index cannot be written out as a tree:

	$ git write-tree
        You have both 3 and 3/2
        fatal: git-write-tree: error building trees

Currently we need to remove '3' by hand X-<.

        $ git update-index --remove 3
        $ git ls-files -s
        100644 00750edc07d6415dcc07ae0351e9397b0222b7ba 0	1
        100644 fa457baf8abbf5dd3bb4cbfab0c5a4cf0523d7f8 0	3/2
        $ git write-tree
        77be0dd800d74913a90662e35215ee648815fc17

When replacing an existing file A with a directory B that has a file B/C in it in the index, 'update-index --replace --add B/C' did not properly remove the file to make room for the new directory.

There was a trivial logic error, most likely a cut & paste one, dating back to quite early days of git.

---

And this fixes it.

 read-cache.c     |    2 +-
 t/t0000-basic.sh |    9 +++++++++
 2 files changed, 10 insertions(+), 1 deletions(-)

index a602010..e856a2e 100644
@@ -517,7 +517,7 @@ static int has_dir_name(const struct cache_entry *ce, int pos, int ok_to_replace
 		pos = cache_name_pos(name, ntohs(create_ce_flags(len, stage)));
 		if (pos >= 0) {
 			retval = -1;
-			if (ok_to_replace)
+			if (!ok_to_replace)
 				break;
 			remove_cache_entry_at(pos);
 			continue;

index 3260d1d..0cd1c41 100755
@@ -272,4 +272,13 @@ test_expect_success \
 	 wc -l) &&
      test $numparent = 1'
 
+test_expect_success 'update-index D/F conflict' '
+	mv path0 tmp &&
+	mv path2 path0 &&
+	mv tmp path2 &&
+	git update-index --add --replace path2 path0/file2 &&
+	numpath0=$(git ls-files path0 | wc -l) &&
+	test $numpath0 = 1
+'
+
 test_done

When replacing an existing file A with a directory A that has a file A/B in it in the index, 'git add' did not succeed because it forgot to pass the allow-replace flag to add_cache_entry().

It might be safer to leave this as an error and require the user to explicitly remove the existing A first before adding A/B since it is an unusual case, but doing that automatically is much easier to use.

---
 read-cache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index e856a2e..b8d83cc 100644
@@ -358,7 +358,7 @@ int add_file_to_index(const char *path, int verbose)
 
 	if (index_path(ce->sha1, path, &st, 1))
 		die("unable to index file %s", path);
-	if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD))
+	if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
 		die("unable to add %s to index",path);
 	if (verbose)
 		printf("add '%s'\n", path);

1.4.4.2.g83c5

Good point. I've forgotten about "--replace" codepath, although that is all my code (May 7, 2005).

Maybe "git add" should internally use ADD_CACHE_OK_TO_REPLACE (or error out and have an option to enable it)?

In any case, the error message could be made a bit more helpful, like this.

---

index eae4745..a602010 100644
@@ -609,7 +609,7 @@ int add_cache_entry(struct cache_entry *ce, int option)
 	if (!skip_df_check &&
 	    check_file_directory_conflict(ce, pos, ok_to_replace)) {
 		if (!ok_to_replace)
-			return -1;
+			return error("'%s' appears as both a file and as a directory", ce->name);
 		pos = cache_name_pos(ce->name, ntohs(ce->ce_flags));
 		pos = -pos-1;
 	}

That sounds right to me. Maybe there are cases where this isn't true, but -- especially given the recent move toward making "git add" the suggested porcelain command for updating the index -- I want "git add foo" to stick the current contents of "foo" into the index, regardless of what might have been there under that name previously. I'll grant that it's not a super-common use case to rename or delete a directory and put a file in its place, but it's IMO not an unreasonable thing to want git to track without undue hassle, given that it claims to support renames. I'm fine with having to supply an extra option to get that automatic behavior.

But barring that, the patch would at least have saved me a lot of head-scratching.

-Steve

You didn't tell the index to remove dir/file. You cannot have dir and dir/file at the same time.