[BUG (or misfeature?)] git checkout and symlinks

[BUG (or misfeature?)] git checkout and symlinks

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

  if in a branch [branch1] you track the file: dir1/file1.c
and in the branch [branch2] you track elsewhere/file1.c and dir1 be
symlink on elsewhere, then it's not possible to checkout the branch
[branch1] if your previous checkout was [branch2]. You have to manually
remove the symlink `dir1` else git complains that checkouting branch1
would overwrite dir1/file1.c.

  I'm not sure how to fix this, and it's quite painful actually :)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [BUG (or misfeature?)] git checkout and symlinks

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

>   if in a branch [branch1] you track the file: dir1/file1.c
> and in the branch [branch2] you track elsewhere/file1.c and dir1 be
> symlink on elsewhere, then it's not possible to checkout the branch
> [branch1] if your previous checkout was [branch2]. You have to manually
> remove the symlink `dir1` else git complains that checkouting branch1
> would overwrite dir1/file1.c.
>
>   I'm not sure how to fix this, and it's quite painful actually :)

Yeah, I think our handling of symlinks in both read-tree and
merge-recursive codepath are Ok for symlinks at the leaf level
but not for intermediate levels.  I think we have some patches
in the recent git (post 1.5.1) to fix (perhaps some of) the
issues, though.

Re: [BUG (or misfeature?)] git checkout and symlinks

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

On Wed, Jul 04, 2007 at 01:52:32PM -0700, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   if in a branch [branch1] you track the file: dir1/file1.c
> > and in the branch [branch2] you track elsewhere/file1.c and dir1 be
> > symlink on elsewhere, then it's not possible to checkout the branch
> > [branch1] if your previous checkout was [branch2]. You have to manually
> > remove the symlink `dir1` else git complains that checkouting branch1
> > would overwrite dir1/file1.c.
> >
> >   I'm not sure how to fix this, and it's quite painful actually :)
> 
> Yeah, I think our handling of symlinks in both read-tree and
> merge-recursive codepath are Ok for symlinks at the leaf level
> but not for intermediate levels.  I think we have some patches
> in the recent git (post 1.5.1) to fix (perhaps some of) the
> issues, though.

  that was with the git in debian unstable, 1.5.2.3 actually.  I'll try
again with HEAD to see if that's fixed.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: $ git checkout and symlinks

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

On Wed, Jul 04, 2007 at 11:05:59PM +0200, Pierre Habouzit wrote:
> On Wed, Jul 04, 2007 at 01:52:32PM -0700, Junio C Hamano wrote:
> > Pierre Habouzit <madcoder@debian.org> writes:
> > 
> > >   if in a branch $ you track the file: dir1/file1.c
> > > and in the branch $ you track elsewhere/file1.c and dir1 be
> > > symlink on elsewhere, then it's not possible to checkout the branch
> > > $. You have to manually
> > > remove the symlink `dir1` else git complains that checkouting branch1
> > > would overwrite dir1/file1.c.
> > >
> > >   I'm not sure how to fix this, and it's quite painful actually :)
> > 
> > Yeah, I think our handling of symlinks in both read-tree and
> > merge-recursive codepath are Ok for symlinks at the leaf level
> > but not for intermediate levels.  I think we have some patches
> > in the recent git (post 1.5.1) to fix (perhaps some of) the
> > issues, though.
> 
>   that was with the git in debian unstable, 1.5.2.3 actually.  I'll try
> again with HEAD to see if that's fixed.

  HEADS does not fixes it either.
  Here is how to reproduce the problem step by step:

$ git init-db 				# init a repo

$ mkdir dir
$ echo wibble > dir/a
$ git add a
$ git commit -a -m'add a' 		# have dir/a live in master

$ git checkout -b break
$ git mv dir new 			# rename dir into new
$ ln -s new dir 			# symlink dir to new
$ git add dir 				# add the symlink
$ git commit -a -m 'break things' 	# commit

$ git checkout master
fatal: Untracked working tree file 'dir/a' would be overwritten by merge.


  The same is true for current git.git HEAD, or git 1.5.2.x

Cheers,
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Teach read-tree 2-way merge to ignore intermediate symlinks

[Junio C Hamano]

Earlier in 16a4c61, we taught "read-tree -m -u" not to be
confused when switching from a branch that has a path frotz/filfre
to another branch that has a symlink frotz that points at xyzzy/
directory.  The fix was incomplete in that it was still confused
when coming back (i.e. switching from a branch with frotz -> xyzzy/
to another branch with frotz/filfre).

This fix is rather expensive in that for a path that is created
we would need to see if any of the leading component of that
path exists as a symbolic link in the filesystem (in which case,
we know that path itself does not exist, and the fact we already
decided to check it out tells us that in the index we already
know that symbolic link is going away as there is no D/F
conflict).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

  Re: [BUG (or misfeature?)] git checkout and symlinks

  Pierre Habouzit <madcoder@debian.org> writes:

  >   if in a branch [branch1] you track the file: dir1/file1.c
  > and in the branch [branch2] you track elsewhere/file1.c and dir1 be
  > symlink on elsewhere, then it's not possible to checkout the branch
  > [branch1] if your previous checkout was [branch2]. You have to manually
  > remove the symlink `dir1` else git complains that checkouting branch1
  > would overwrite dir1/file1.c.
  >
  >   I'm not sure how to fix this, and it's quite painful actually :)

  We probably could add a path buffer to cache the last look-up
  made by has_symlink_leading_path(), like the other caller
  does, but this is to give the fix a wider exposure and testing
  early.

 t/t2007-checkout-symlink.sh |   50 +++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.c              |    3 ++
 2 files changed, 53 insertions(+), 0 deletions(-)
 create mode 100755 t/t2007-checkout-symlink.sh

diff --git a/t/t2007-checkout-symlink.sh b/t/t2007-checkout-symlink.sh
new file mode 100755
index 0000000..0526fce
--- /dev/null
+++ b/t/t2007-checkout-symlink.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Junio C Hamano
+
+test_description='git checkout to switch between branches with symlink<->dir'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	mkdir frotz &&
+	echo hello >frotz/filfre &&
+	git add frotz/filfre &&
+	test_tick &&
+	git commit -m "master has file frotz/filfre" &&
+
+	git branch side &&
+
+	echo goodbye >nitfol &&
+	git add nitfol
+	test_tick &&
+	git commit -m "master adds file nitfol" &&
+
+	git checkout side &&
+
+	git rm --cached frotz/filfre &&
+	mv frotz xyzzy &&
+	ln -s xyzzy frotz &&
+	git add xyzzy/filfre frotz &&
+	test_tick &&
+	git commit -m "side moves frotz/ to xyzzy/ and adds frotz->xyzzy/"
+
+'
+
+test_expect_success 'switch from symlink to dir' '
+
+	git checkout master
+
+'
+
+rm -fr frotz xyzzy nitfol &&
+git checkout -f master || exit
+
+test_expect_success 'switch from dir to symlink' '
+
+	git checkout side
+
+'
+
+test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index cac2411..89dd279 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -495,6 +495,9 @@ static void verify_absent(const char *path, const char *action,
 	if (o->index_only || o->reset || !o->update)
 		return;
 
+	if (has_symlink_leading_path(path, NULL))
+		return;
+
 	if (!lstat(path, &st)) {
 		int cnt;
 

git-log --follow?

[Junio C Hamano]

The message I am following up is a patch to unpack-trees.c,
whose basic code structure is Daniel's work, so I wanted to CC
him and the easiest way to look his address up was to run
git-log on it.

Not so.

The "blame -C unpack-trees.c" output consists of this
distribution of origin:

    464 read-tree.c
    337 unpack-trees.c
     74 builtin-read-tree.c
     11 tree.c
      8 tree.h

and most of the work by Daniel was done when the bulk of code
was still in read-tree.c.  Naturally the log output of
unpack-trees.c does not have a single commit by him.  "git log
-- unpack-trees.c" would not follow into read-tree.c, but I
thought "git log --follow -- unpack-trees.c" is supposed to; I
tried it for the first time, but it does not seem to work as
well as I hoped.

I think this is just a testament that "following renames" is not
as useful in a real project as people seem to believe, not a
real complaint.

When 16da134 created unpack-trees.c, it initially moved only
very small part of builtin-read-tree.c to it.  Later 076b0adc
made further code movements from builtin-read-tree.c to
unpack-trees.c.

An interesting thing is that builtin-read-tree.c immediately
before 16da134 is much similar to unpack-trees.c in 076b0adc
than unpack-trees.c in 16da134, exactly because of this stepwise
code movements.  I do not think people can argue that "human
user knows he is renaming the file so recording the human
intention would have helped git a lot better" in this case, as
the human user who made 16da134 did not even intend to do a
rename.

Re: git-log --follow?

[Johannes Schindelin]

Hi,

On Thu, 12 Jul 2007, Junio C Hamano wrote:

> The message I am following up is a patch to unpack-trees.c, whose basic 
> code structure is Daniel's work, so I wanted to CC him and the easiest 
> way to look his address up was to run git-log on it.
> 
> Not so.
>
> [...]
> 
> "git log -- unpack-trees.c" would not follow into read-tree.c, but I 
> thought "git log --follow -- unpack-trees.c" is supposed to; I tried it 
> for the first time, but it does not seem to work as well as I hoped.

Your lesson as to why following renames is not as useful as some might 
want to make us believe is duly noted; will use it as back reference 
should I have to defend that view again.

However, I have to wonder why you did not solve your problem this way:

	git log --author=Daniel

Ciao,
Dscho

Re: Teach read-tree 2-way merge to ignore intermediate symlinks

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

On Thu, Jul 12, 2007 at 01:04:16AM -0700, Junio C Hamano wrote:
> Earlier in 16a4c61, we taught "read-tree -m -u" not to be
> confused when switching from a branch that has a path frotz/filfre
> to another branch that has a symlink frotz that points at xyzzy/
> directory.  The fix was incomplete in that it was still confused
> when coming back (i.e. switching from a branch with frotz -> xyzzy/
> to another branch with frotz/filfre).
> 
> This fix is rather expensive in that for a path that is created
> we would need to see if any of the leading component of that
> path exists as a symbolic link in the filesystem (in which case,
> we know that path itself does not exist, and the fact we already
> decided to check it out tells us that in the index we already
> know that symbolic link is going away as there is no D/F
> conflict).
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

  I confirm this fixes the issue I reported.
  Thanks !
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: git-log --follow?

[Linus Torvalds]

On Thu, 12 Jul 2007, Junio C Hamano wrote:
> 
> I think this is just a testament that "following renames" is not
> as useful in a real project as people seem to believe, not a
> real complaint.

Yeah. That said, what you wanted would have actually worked with my 
original strange patch to "git blame", and in particular that also would 
allow you to get a "log" for certain lines in the file.

So something like

	git blame -C -Lx,y --log

may still make sense to give people.

Here's a new version of that patch (I've long since lost the original one, 
plus it probably wouldn't apply anyway, but it was easy to re-generate).

It doesn't set up the revs thing nicely, so trying to add "-p" or "--stat" 
etc doesn't really get you what you'd want, and "--decorate" doesn't work 
since it doesn't call "cmd_log_init".

So this certainly has some room for improvement, but my point is that "git 
blame" actually knows all this, and in many ways does a better job than 
"git log" (but a very *different* job!!).

So "git log" gives you the log for a (set of) pathname(s), while with this 
patch, "git blame --log" gives you the log for the commits that can be 
*blamed* for the current state of that pathname!

Two very different things, but both are valid, and interesting things to 
do, I think. And I really think it's worth doing, if only because it's so 
simple, and fits so well with the whole "git blame" structure!

			Linus

---
diff --git a/builtin-blame.c b/builtin-blame.c
index 0519339..8de06d3 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -18,6 +18,7 @@
 #include "cache-tree.h"
 #include "path-list.h"
 #include "mailmap.h"
+#include "log-tree.h"
 
 static char blame_usage[] =
 "git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S <revs-file>] [-M] [-C] [-C] [--contents <filename>] [--incremental] [commit] [--] file\n"
@@ -34,6 +35,7 @@ static char blame_usage[] =
 "  -L n,m              Process only line range n,m, counting from 1\n"
 "  -M, -C              Find line movements within and across files\n"
 "  --incremental       Show blame entries as we find them, incrementally\n"
+"  --log               Show blame entries as we find them in 'git log' style\n"
 "  --contents file     Use <file>'s contents as the final image\n"
 "  -S revs-file        Use revisions from revs-file instead of calling git-rev-list\n";
 
@@ -45,6 +47,7 @@ static int max_score_digits;
 static int show_root;
 static int blank_boundary;
 static int incremental;
+static int blame_log;
 static int cmd_is_annotate;
 static int xdl_opts = XDF_NEED_MINIMAL;
 static struct path_list mailmap;
@@ -1431,11 +1434,22 @@ static void write_filename_info(const char *path)
  * The blame_entry is found to be guilty for the range.  Mark it
  * as such, and show it in incremental output.
  */
-static void found_guilty_entry(struct blame_entry *ent)
+static void found_guilty_entry(struct blame_entry *ent, struct rev_info *rev)
 {
 	if (ent->guilty)
 		return;
 	ent->guilty = 1;
+	if (blame_log) {
+		struct origin *suspect = ent->suspect;
+		struct commit *commit = suspect->commit;
+
+		if (commit->object.flags & METAINFO_SHOWN)
+			return;
+		commit->object.flags |= METAINFO_SHOWN;
+		log_tree_commit(rev, commit);
+		maybe_flush_or_die(stdout, "stdout");
+		return;
+	}
 	if (incremental) {
 		struct origin *suspect = ent->suspect;
 
@@ -1505,7 +1519,7 @@ static void assign_blame(struct scoreboard *sb, struct rev_info *revs, int opt)
 		/* Take responsibility for the remaining entries */
 		for (ent = sb->ent; ent; ent = ent->next)
 			if (same_suspect(ent->suspect, suspect))
-				found_guilty_entry(ent);
+				found_guilty_entry(ent, revs);
 		origin_decref(suspect);
 
 		if (DEBUG) /* sanity */
@@ -2204,6 +2218,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		}
 		else if (!strcmp("--incremental", arg))
 			incremental = 1;
+		else if (!strcmp("--log", arg))
+			save_commit_buffer = incremental = blame_log = 1;
 		else if (!strcmp("--score-debug", arg))
 			output_option |= OUTPUT_SHOW_SCORE;
 		else if (!strcmp("-f", arg) ||
@@ -2224,7 +2240,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 			argv[unk++] = arg;
 	}
 
-	if (!incremental)
+	if (blame_log || !incremental)
 		setup_pager();
 
 	if (!blame_move_score)
@@ -2324,7 +2340,14 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	argv[unk] = NULL;
 
 	init_revisions(&revs, NULL);
+
+	/* Maybe we should call "cmd_log_init()" here instead? */
+	revs.always_show_header = 1;
+	revs.commit_format = CMIT_FMT_DEFAULT;
+	revs.verbose_header = 1;
+	revs.abbrev = DEFAULT_ABBREV;
 	setup_revisions(unk, argv, &revs, NULL);
+
 	memset(&sb, 0, sizeof(sb));
 
 	/*

Re: git-log --follow?

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, 12 Jul 2007, Junio C Hamano wrote:
>> 
>> I think this is just a testament that "following renames" is not
>> as useful in a real project as people seem to believe, not a
>> real complaint.
>
> Yeah. That said, what you wanted would have actually worked with my 
> original strange patch to "git blame", and in particular that also would 
> allow you to get a "log" for certain lines in the file.

Yeah, I just tried the blame from 'pu' (I have been carrying
that original patch from you there).  The output is not very
intuitive in that it talks about each commit but it is not
apparent _why_ the command talks about that commit.  Maybe
adding "there are the lines in the final image of the blob you
are blaming that came from this commit" to the output would make
the output easier to read.

Re: git-log --follow?

[Linus Torvalds]

On Thu, 12 Jul 2007, Junio C Hamano wrote:
> 
> Yeah, I just tried the blame from 'pu' (I have been carrying
> that original patch from you there).  The output is not very
> intuitive in that it talks about each commit but it is not
> apparent _why_ the command talks about that commit.  Maybe
> adding "there are the lines in the final image of the blob you
> are blaming that came from this commit" to the output would make
> the output easier to read.

That doesn't necessarily work, at least not incrementally. The same commit 
can show up multiple times for *different* 'blame_entry' things, and I 
don't think we want to show such a commit multiple times, and I also don't 
think we know what all the blame entries are going to be until the end.

So right now I use that METAINFO_SHOWN flag to just show the commit once. 
That makes the log a *lot* more readable.

In my original patch (the one you apparently have in 'pu'), I did that 
differently (and not very well), but maybe that approach lends itself 
better to showing some kind of patch.

I think my newer version is likely better.

		Linus