[PATCH] checkout_entry: only try to create directories when no file existed there

[PATCH] checkout_entry: only try to create directories when no file existed there

[Johannes Schindelin]

It is obvious that we do not have to create directories when the file we
want to check out already existed.

Besides, it fixes the obscure use case, where you want to track a file which
is _outside_ of your working tree, by creating a symbolic link to the directory
it lives in, and adding the file with something like "git add symlink/file".
Without this patch, "git checkout symlink/file" would actually _replace_
"symlink" by a directory of the same name.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 entry.c                     |    8 +++++---
 t/t2007-checkout-symlink.sh |   11 +++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/entry.c b/entry.c
index 0625112..eacdba2 100644
--- a/entry.c
+++ b/entry.c
@@ -223,8 +223,10 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 				return error("%s is a directory", path);
 			remove_subtree(path);
 		}
-	} else if (state->not_new)
-		return 0;
-	create_directories(path, state);
+	} else {
+		if (state->not_new)
+			return 0;
+		create_directories(path, state);
+	}
 	return write_entry(ce, path, state, 0);
 }
diff --git a/t/t2007-checkout-symlink.sh b/t/t2007-checkout-symlink.sh
index 0526fce..02224eb 100755
--- a/t/t2007-checkout-symlink.sh
+++ b/t/t2007-checkout-symlink.sh
@@ -47,4 +47,15 @@ test_expect_success 'switch from dir to symlink' '
 
 '
 
+test_expect_success 'checkout does not replace symlink/file with dir/file' '
+	mkdir 123 &&
+	ln -s 123 abc &&
+	echo 1 > abc/1 &&
+	echo 2 > abc/2 &&
+	echo 3 > abc/3 &&
+	git add abc/? &&
+	echo 0 > abc/3 &&
+	git checkout abc/3 &&
+	test -h abc
+'
 test_done
-- 
1.5.3.rc4.26.g782e

Re: [PATCH] checkout_entry: only try to create directories when no file existed there

[Junio C Hamano]

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

> It is obvious that we do not have to create directories when the file we
> want to check out already existed.

It is not so obvious to me.  In fact I vaguely recall we had
complaint about opposite case where we did not honor a symlink
to be checked out as a symlink because an earlier branch had it
as a directory.

Re: [PATCH] checkout_entry: only try to create directories when no file existed there

[David Kastrup]

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

> It is obvious that we do not have to create directories when the file we
> want to check out already existed.
>
> Besides, it fixes the obscure use case, where you want to track a
> file which is _outside_ of your working tree, by creating a symbolic
> link to the directory it lives in, and adding the file with
> something like "git add symlink/file".  Without this patch, "git
> checkout symlink/file" would actually _replace_ "symlink" by a
> directory of the same name.

Uh, when git is tracking symlinks (and it does by default), this is
quite the correct thing to do.  Of course, it should rather barf with
a merge conflict.  But it has no business following links and doing
potential damage either outside or inside of the work tree, by
confusing the target of a link with the link itself.  For example, I
can easily relocate a complete git work tree with "mv" in the
directory hierarchy.  Should an update/commit/whatever then start
wreaking havoc in places that incidentally don't point to the same
location anymore?

My vote here is emphatically "no".  Just maintain the links, not
whatever they point to.

I can think of only a single reasonable exception: if the symlink
itself is _not_ registered (because is has been explicitly entered
into a .gitignore file or never added for other reasons), but its
target _has_ been added explicitly _dereferencing_ the symlink.  This
difference can only occur with directory symlinks since file symlinks
can't be distinguished in this manner from the name of the link
itself.

It would be good if somebody brought this to Johannes' attention, even
if just replying with a quotation: he chose to killfile me.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] checkout_entry: only try to create directories when no file existed there

[Johannes Schindelin]

Hi,

On Wed, 8 Aug 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > It is obvious that we do not have to create directories when the file we
> > want to check out already existed.
> 
> It is not so obvious to me.  In fact I vaguely recall we had
> complaint about opposite case where we did not honor a symlink
> to be checked out as a symlink because an earlier branch had it
> as a directory.

That case is not affected by my patch, AFAICT.  It only affects the case 
when you check out an entry, say "a/b/c/d/e/f/g".  If that file already 
exists, we have to remove it, in order to write over it.  That's the 
"unlink" which is above the hunk of the diff I sent.

Now, if we have to remove it, we did a stat() before that.  It succeeded.  
So we know that "a/b/c/d/e/f/" exists.  It might contain some symlinks, 
but it exists.

So putting the "create_directories()" into the else branch, we avoid doing 
unnecessary work.

Coincidentally, my hackish use case (don't try this at home, kids!) is 
fixed, too.

Ciao,
Dscho

Re: [PATCH] checkout_entry: only try to create directories when no file existed there

[Junio C Hamano]

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

> Now, if we have to remove it, we did a stat() before that.  It succeeded.  
> So we know that "a/b/c/d/e/f/" exists.  It might contain some symlinks, 
> but it exists.

Yes, it is exactly the thing.  It might contain a/b/c as symlink
but the index does not say a/b/c _IS_ a symlink, and the code
"fixes" that.  Otherwise, you would get inconsistent result from
"git checkout" depending on random symlink you happen to have in
your working tree.

Re: [PATCH] checkout_entry: only try to create directories when no file existed there

[Johannes Schindelin]

Hi,

On Wed, 8 Aug 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Now, if we have to remove it, we did a stat() before that.  It succeeded.  
> > So we know that "a/b/c/d/e/f/" exists.  It might contain some symlinks, 
> > but it exists.
> 
> Yes, it is exactly the thing.  It might contain a/b/c as symlink
> but the index does not say a/b/c _IS_ a symlink, and the code
> "fixes" that.  Otherwise, you would get inconsistent result from
> "git checkout" depending on random symlink you happen to have in
> your working tree.

Ah, I see what you're getting to.

If user had a symlink called, say "syml", to, say $HOME, and now switches 
to a branch which has "syml/.bash_profile", then the given code path would 
try to replace the symlink with a directory.

Hmm.

Ciao,
Dscho