Stefan Beller [off-list ref] writes:
quoted
I wonder if the patches mentioned have something to do with the "git
add deep/in/the/tree" that fails to notice deep/in/ is an unrelated
repository in some way?
Which is considered a feature now. Maybe we should add tests for that?
http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb
That is a bug, plain and simple. Duy any ideas where we went wrong?
I think we already have code to avoid adding beyond symlinks.
"git add deep/in/the/tree" should refuse if deep/in is a symbolic
link (and happens to point at a directory that has the/tree in it).
We used not to catch that long time ago, but I think we fixed it.
The logic and the places to do the checks for "no, that thing may be
a directory but is an unrelated repository" should be the same.
On Thu, May 5, 2016 at 4:27 PM, Junio C Hamano [off-list ref] wrote:
Stefan Beller [off-list ref] writes:
quoted
quoted
I wonder if the patches mentioned have something to do with the "git
add deep/in/the/tree" that fails to notice deep/in/ is an unrelated
repository in some way?
Which is considered a feature now. Maybe we should add tests for that?
http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb
That is a bug, plain and simple. Duy any ideas where we went wrong?
That was my first reaction as well. However after a while of thought I actually
like that bug. Consider the possibilities how gitk/git-gui or other subsystems
can be developed. When accepting a patch for that you can either apply the
patch in the outer or inner repository, depending on what the sender used.
I am not so sure if it is a bug plain and simple, but devolved into a
"feature" now.
I think we already have code to avoid adding beyond symlinks.
"git add deep/in/the/tree" should refuse if deep/in is a symbolic
link (and happens to point at a directory that has the/tree in it).
We used not to catch that long time ago, but I think we fixed it.
The logic and the places to do the checks for "no, that thing may be
a directory but is an unrelated repository" should be the same.
On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano [off-list ref] wrote:
Stefan Beller [off-list ref] writes:
quoted
quoted
I wonder if the patches mentioned have something to do with the "git
add deep/in/the/tree" that fails to notice deep/in/ is an unrelated
repository in some way?
The same functionality is added in 8745024 (parse_pathspec: support
stripping/checking submodule paths - 2013-07-14) so if it didn't fail
to notice that before 5a76aff1a6 and did after, it's a bug.
quoted
Which is considered a feature now. Maybe we should add tests for that?
http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb
That is a bug, plain and simple. Duy any ideas where we went wrong?
I vaguely recall this symptom. It has something to do with the index,
the check we do requires a gitlink in the index, I think. So if the
gitlink entry is not in the index, our protection line fails. I think
doing all this at pathspec level is wrong. We should wait at least
after read_directory() is done, by then we have a lot more info to
decide.
I think we already have code to avoid adding beyond symlinks.
"git add deep/in/the/tree" should refuse if deep/in is a symbolic
link (and happens to point at a directory that has the/tree in it).
We used not to catch that long time ago, but I think we fixed it.
The logic and the places to do the checks for "no, that thing may be
a directory but is an unrelated repository" should be the same.
--
Duy
On Fri, May 6, 2016 at 3:30 AM, Duy Nguyen [off-list ref] wrote:
On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano [off-list ref] wrote:
quoted
Stefan Beller [off-list ref] writes:
quoted
quoted
I wonder if the patches mentioned have something to do with the "git
add deep/in/the/tree" that fails to notice deep/in/ is an unrelated
repository in some way?
The same functionality is added in 8745024 (parse_pathspec: support
stripping/checking submodule paths - 2013-07-14) so if it didn't fail
to notice that before 5a76aff1a6 and did after, it's a bug.
The bug seems to have existed before. However in the bug we are talking
about the nested repo is not a submodule yet.
quoted
quoted
Which is considered a feature now. Maybe we should add tests for that?
http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb
That is a bug, plain and simple. Duy any ideas where we went wrong?
I vaguely recall this symptom. It has something to do with the index,
the check we do requires a gitlink in the index, I think. So if the
gitlink entry is not in the index, our protection line fails. I think
doing all this at pathspec level is wrong. We should wait at least
after read_directory() is done, by then we have a lot more info to
decide.
quoted
I think we already have code to avoid adding beyond symlinks.
"git add deep/in/the/tree" should refuse if deep/in is a symbolic
link (and happens to point at a directory that has the/tree in it).
We used not to catch that long time ago, but I think we fixed it.
The logic and the places to do the checks for "no, that thing may be
a directory but is an unrelated repository" should be the same.
--
Duy
Here are some tests to have a clear picture of what is happening:
(diff against origin/master)
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index f99f674..c9dfa11 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -37,6 +37,22 @@ test_expect_success 'setup - repository in init
subdirectory' '
git add a &&
git commit -m "submodule commit 1" &&
git tag -a -m "rev-1" rev-1
+ ) &&
+ mkdir init_slash1 &&
+ (
+ cd init_slash1 &&
+ git init &&
+ echo a >a &&
+ git add a &&
+ git commit -m "first commit"
+ ) &&
+ mkdir init_slash2 &&
+ (
+ cd init_slash2 &&
+ git init &&
+ echo a >a &&
+ git add a &&
+ git commit -m "first commit"
)
'
@@ -44,7 +60,30 @@ test_expect_success 'setup - commit with gitlink' '
echo a >a &&
echo z >z &&
git add a init z &&
- git commit -m "super commit 1"
+ git commit -m "super commit 1" &&
+ git ls-tree HEAD |grep init >actual &&
+ grep "160000 commit" actual
+'
+
+test_expect_success 'setup - commit with gitlink/ in between' '
+ echo a >a &&
+ echo z >z &&
+ git add a init_slash1/ z &&
+ git commit -m "super commit 1" &&
+ git ls-tree HEAD |grep init_slash1 >actual &&
+ grep "160000 commit" actual
+'
+
+test_expect_failure 'setup - commit with gitlink/ only' '
+ git add init_slash2/ &&
+ git commit -m "super commit 1" &&
+ git ls-tree HEAD |grep init_slash2 >actual &&
+ grep "160000 commit" actual
+'
+
+test_expect_success 'tear down slash tests' '
+ rm -rf init_slash* &&
+ git commit -a -m "removing init_slash*"
'
test_expect_success 'setup - hide init subdirectory' '