threads / patch / 9443

patchcheckout_entry: only try to create directories when no file existed there

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

## tl;dr

6 messages between Aug 8, 2007 and Aug 8, 2007. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Johannes Schindelin· Aug 8, 2007, 21:00 UTC · lore

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(-)
Show changes to 2 files +16 −3

entry.c, t/t2007-checkout-symlink.sh

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
Junio C Hamano· Aug 8, 2007, 21:17 UTC · re: Johannes Schindelin · lore

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

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.

Johannes Schindelin· Aug 8, 2007, 21:42 UTC · re: Junio C Hamano · lore

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

Hi,
On Wed, 8 Aug 2007, Junio C Hamano wrote:
Show 9 quoted lines
> 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

Junio C Hamano· Aug 8, 2007, 22:40 UTC · re: Johannes Schindelin · lore

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

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.

Johannes Schindelin· Aug 8, 2007, 22:54 UTC · re: Junio C Hamano · lore

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

Hi,
On Wed, 8 Aug 2007, Junio C Hamano wrote:
Show 11 quoted lines
> 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

David Kastrup· Aug 8, 2007, 21:27 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 9 quoted lines
> 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

← back to recent threads