From: Junio C Hamano <hidden> Date: 2016-06-15 22:45:06
Linus Torvalds [off-list ref] writes:
On Mon, 4 Aug 2008, Junio C Hamano wrote:
quoted
I started to revisit this issue and patched "git update-index --add"
and "git add" so far. Patches follow.
Patches look good to me, but did you check the performance impact?
The rewritten 'has_symlink_leading_path()' should do ok, but it migth
still be a huge performance downside to check all the paths for things
like "git add -u".
Not yet.
I think this is a necessary "correctness" thing to do regardless of the
performance impact, and adding the logic to stop at submodule boundary
(aka gitlinks) should come before optimization.
The rewritten 'has_symlink_leading_path()' should do ok, but it migth
still be a huge performance downside to check all the paths for things
like "git add -u".
Not yet.
I think this is a necessary "correctness" thing to do regardless of the
performance impact, and adding the logic to stop at submodule boundary
(aka gitlinks) should come before optimization.
Well, "performance" is a feature too, and it's not correct to say that "X
should be fixed before optimization". If "X" slows things down, the
question should be whether it really needs fixing..
Yes, we find symlinks when we do _new_ files, but is it really so bad to
assume that existing directories that we have already added to the index
are stable? It can easily be seen as a feature too that you can force git
to ignore the symlink and see it as a real directory.
So that's why it would be interesting to hear about the performance
impact. Because this is definitely not a black-and-white "one behavior is
wrong and one behavior is right".
Linus
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:06
Hi,
On Mon, 4 Aug 2008, Linus Torvalds wrote:
On Mon, 4 Aug 2008, Junio C Hamano wrote:
quoted
quoted
The rewritten 'has_symlink_leading_path()' should do ok, but it
migth still be a huge performance downside to check all the paths
for things like "git add -u".
Not yet.
I think this is a necessary "correctness" thing to do regardless of
the performance impact, and adding the logic to stop at submodule
boundary (aka gitlinks) should come before optimization.
Well, "performance" is a feature too, and it's not correct to say that
"X should be fixed before optimization". If "X" slows things down, the
question should be whether it really needs fixing..
Yes, we find symlinks when we do _new_ files, but is it really so bad to
assume that existing directories that we have already added to the index
are stable? It can easily be seen as a feature too that you can force git
to ignore the symlink and see it as a real directory.
Actually, whatever you want, it needs fixing.
I vividly remember being quite pissed by Git replacing a symbolic link in
my working directory with a directory, and instead of updating the files
which were technically outside of the repository, Git populated that newly
created directory.
However, please note that Junio's patch affects git-add, AFAIR, not
git-update-index.
Ciao,
Dscho
I vividly remember being quite pissed by Git replacing a symbolic link in
my working directory with a directory, and instead of updating the files
which were technically outside of the repository, Git populated that newly
created directory.
Well, that can cut both ways. For example, I vividly remember a time in
the distant past when harddisks were tiny, and I didn't have insanely
high-end hardware, and I was building the X server, but had to split
things up over two partitions because each individual partition was
too full.
IOW, sometimes you may _want_ to use symlinks that way, even within one
project - with a symlink allowing you to move parts of it around
"transparently".
Of course, these days under Linux we can just use bind mounts, so the use
of symlinks to stitch together two or more different trees is fairly
old-fashioned, but is still the only option on some systems or if you
don't have root.
(These days harddisks are also generally so big that it never happens. But
on my EeePC laptop, I still end up with two filesystems, 4GB and 8GB
each. So it's not inconceivable to be in that kind of situation even
today).
Linus
From: Junio C Hamano <hidden> Date: 2016-06-15 22:45:06
Linus Torvalds [off-list ref] writes:
... Because this is definitely not a black-and-white "one behavior is
wrong and one behavior is right".
I wish I could agree with you that this is a feature, but 16a4c61
(read-tree -m -u: avoid getting confused by intermediate symlinks.,
2007-05-10) and 64cab59 (apply: do not get confused by symlinks in the
middle, 2007-05-11) came from real world breakage cases and the root cause
was that we were too lenient to allow such a "feature" that pretends the
symlink not to be there.
Right now, we are being careful only while branch switching and patch
application, but the codepaths that add directly to the index (add and
update-index) are not fixed (or "still has the feature").
I do not see a clean way to keep such a "feature" without hurting users
who suffered the bugs these two commits from May 2007 fixed.
... Because this is definitely not a black-and-white "one behavior is
wrong and one behavior is right".
I wish I could agree with you that this is a feature, but 16a4c61
(read-tree -m -u: avoid getting confused by intermediate symlinks.,
2007-05-10) and 64cab59 (apply: do not get confused by symlinks in the
middle, 2007-05-11) came from real world breakage cases and the root cause
was that we were too lenient to allow such a "feature" that pretends the
symlink not to be there.
Right now, we are being careful only while branch switching and patch
application, but the codepaths that add directly to the index (add and
update-index) are not fixed (or "still has the feature").
I do not see a clean way to keep such a "feature" without hurting users
who suffered the bugs these two commits from May 2007 fixed.
config option?
I think a command line is too much work for too little value, but if the
check could be ignored based on a config option without costing too much
it may be reasonable.
David Lang
From: Junio C Hamano <hidden> Date: 2016-06-15 22:45:06
Linus Torvalds [off-list ref] writes:
IOW, sometimes you may _want_ to use symlinks that way, even within one
project - with a symlink allowing you to move parts of it around
"transparently".
While I admit that I have managed a large directory split across
partitions grafted via symlinks in pre-git days myself, ever since you
started "tracking" symbolic links with 8ae0a8c (git and symlinks as
tracked content, 2005-05-05), you have pretty much been committed to
"track" symbolic links.
This goes even before that commit. The readdir() loop done in show-files.c
with 8695c8b (Add "show-files" command to show the list of managed (or
non-managed) files., 2005-04-11) does not dereference symbolic links
pointing at a directory elsewhere, which we still have as read_directory()
in dir.c without much change in the basic structure.
I would give some leeway to other people who made comments in this thread,
who may not be so familiar with the low-level git codebase, but I have to
say that if you claim that dereferencing symbolic links in the middle is a
feature, you are not being completely honest, you haven't thought through
the issues, and/or you simply forgot the details. I'd suspect most likely
it is the last one ;-).
The thing is, the "feature" is not very well supported, even without the
fixes from last night. If you have a symlink "sym" that points at "dir"
that has "file" in it, and if neither "sym" nor "dir/file" are tracked,
you can "git add sym/file" to add it (I called it a bug).
However:
(1) starting from the same condition, "git add ." does _not_ add it (you
get the symbolic link "sym" added to your index instead, as well as
"dir/file");
(2) after you add "sym/file" through the bug, if you say "git add .", it
will be removed from the index and you will instead have "sym" and
"dir/file" (with an ancient git before 1.5.0, you will get "unable to
add sym" error instead).
(3) after you add "sym/file", "git diff" will immediately notice that you
have removed it (this is a fairly recent fix; 1.5.4.X doesn't notice
it).
You cannot have it as a reliably usable feature without a major surgery,
and this is fundamental. You simply cannot have it both ways without
telling git which symlink is "tracked" and which are only there for
storage sizing.
If you seriously want to claim that we support such a feature, you would
at least need to:
(0) have a way for the user to say, "the project tree may have a
directory D, but I do not want to check it out as a directory because
my partition is too small. Whenever you need to create a directory
there and hang a tree underneath, instead create a symlink that
points at /export/large/D instead". Most likely this information
would go to .git/config;
(1) whereever we run "create leading directories", we notice and honor
the above configuration (mostly entry.c::create_directories() called
from entry.c::checkout_entry());
(2) whenever we need to check out a file to path D, instead of
recursively remove everything under it, we remove the symlink and
deposit the file there (mostly entry.c::remove_subtree() and
unpack-trees.c::verify_absent());
(3) whenever we traverse working tree using readdir(), notice that the
symbolic link we are looking at is the funny "pointing elsewhere but
this is really a directory" specified in (0) and recurse into the
directory pointed by it (dir.c::read_directory_recursive() but there
may be others)
(4) and we teach has_symlink_leading_path() to special case such a path
you configured in (0).
I personally do not think adding these to support such a "feature" is such
a high priority, and I do not think it is honest to claim we support such
a feature without doing any of the above. The current reality is that our
symlink support is still broken in corner cases, and being able to easily
add "sym/path" via "git add" and "git update-index --add" is one of them.
On Mon, Aug 04, 2008 at 11:11:11PM -0700, Junio C Hamano wrote:
The thing is, the "feature" is not very well supported, even without the
fixes from last night. If you have a symlink "sym" that points at "dir"
that has "file" in it, and if neither "sym" nor "dir/file" are tracked,
you can "git add sym/file" to add it (I called it a bug).
Well, I actually used this feature less than a year ago and did not have
any problem with that. Not that it is very important for me now, but I had
makefiles in a separate build directory, and I used a symlink to move
building to a tmpfs partion, which sped up building slightly.
Obviously, if a symlink points to a directory inside of the repository
and then you use "git add sym/file", it is definitely a mistake. OTOH,
let's consider the following situation:
git init
mkdir newdir
touch newdir/foo
git add newdir/foo
git commit -m 'add foo'
mv newdir /tmp/
ln -s /tmp/newdir
touch newdir/bar
git add newdir/bar
git commit -m 'add bar'
git ls-files
And you can see:
newdir/bar
newdir/foo
Git does exactly what I want here!
Anyway, I more concern with performance impact of your patch. If it
is noticeable, then it would be nice to have an option to turn this
check off.
Dmitry
While I admit that I have managed a large directory split across
partitions grafted via symlinks in pre-git days myself, ever since you
started "tracking" symbolic links with 8ae0a8c (git and symlinks as
tracked content, 2005-05-05), you have pretty much been committed to
"track" symbolic links.
Yes, but my point is:
- IF the cost is exorbitant (which was my question that triggered this:
it sure as h*ll _would_ have been too high back in the days of the
original symlink-matching code) ...
- ... then there really are valid cases that say that we could just call
it a feature.
That's really my whole argument. Saying that "ok, we will assume that
existing paths that git already knows about are stable" is not really an
odd feature. It's one we have lived with for years, and it's one that is
actually pretty dang trivial to work with. Yes, it can cause unexpected
behaviour, but if you hit it, that unexpected behavior is actually not
_that_ hard to work around.
This is all I'm arguing for. People claim that there are 'correctness'
issues. I say that 'performance issues' are important, and we can and we
_should_ take performance issues very seriously. So seriously that we'll
make performance a _feature_ if required.
Linus