{"thread":{"id":"42232","subject":"[PATCH] pathspec: remove check_path_for_gitlink","startedAt":"2016-05-05T22:31:37Z","lastAt":"2016-05-07T07:16:41Z","messageCount":16,"participants":["Stefan Beller","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"285635","messageId":"1462487497-28394-1-git-send-email-sbeller@google.com","threadId":"42232","inReplyTo":null,"subject":"[PATCH] pathspec: remove check_path_for_gitlink","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-05T22:31:37Z","receivedAt":"2016-05-05T22:31:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`check_path_for_gitlink` was introduced in 9d67b61f739a (2013-01-06,\nadd.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse)\nbut the implementation was removed in 5a76aff1a6 (2013-07-14, add:\nconvert to use parse_pathspec).\n\nRemove the declaration from the header as well.\n\nSigned-off-by: Stefan Beller \n---\n pathspec.h | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/pathspec.h b/pathspec.h\nindex 0c11262..b596aed 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -96,7 +96,6 @@ static inline int ps_strcmp(const struct pathspec_item *item,\n \n extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec);\n extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);\n-extern const char *check_path_for_gitlink(const char *path);\n extern void die_if_path_beyond_symlink(const char *path, const char *prefix);\n \n #endif /* PATHSPEC_H */\n-- \n2.8.0.1.g3af9c03\n"},{"id":"285644","messageId":"xmqqbn4k85lm.fsf@gitster.mtv.corp.google.com","threadId":"42232","inReplyTo":"1462487497-28394-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T23:09:25Z","receivedAt":"2016-05-05T23:09:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> `check_path_for_gitlink` was introduced in 9d67b61f739a (2013-01-06,\n> add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse)\n> but the implementation was removed in 5a76aff1a6 (2013-07-14, add:\n> convert to use parse_pathspec).\n>\n> Remove the declaration from the header as well.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> pathspec.h | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/pathspec.h b/pathspec.h\n> index 0c11262..b596aed 100644\n> --- a/pathspec.h\n> +++ b/pathspec.h\n> @@ -96,7 +96,6 @@ static inline int ps_strcmp(const struct pathspec_item *item,\n> \n> extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec);\n> extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);\n> -extern const char *check_path_for_gitlink(const char *path);\n> extern void die_if_path_beyond_symlink(const char *path, const char *prefix);\n> \n> #endif /* PATHSPEC_H */\n\nInteresting.\n\nI wonder if the patches mentioned have something to do with the \"git\nadd deep/in/the/tree\" that fails to notice deep/in/ is an unrelated\nrepository in some way?\n"},{"id":"285646","messageId":"CAGZ79kY65Fo4+_a1B8J0h7PymGWUSoAdb1eb5YVfG55=30oPEg@mail.gmail.com","threadId":"42232","inReplyTo":"xmqqbn4k85lm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-05T23:15:10Z","receivedAt":"2016-05-05T23:15:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 5, 2016 at 4:09 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> `check_path_for_gitlink` was introduced in 9d67b61f739a (2013-01-06,\n>> add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse)\n>> but the implementation was removed in 5a76aff1a6 (2013-07-14, add:\n>> convert to use parse_pathspec).\n>>\n>> Remove the declaration from the header as well.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> pathspec.h | 1 -\n>> 1 file changed, 1 deletion(-)\n>>\n>> diff --git a/pathspec.h b/pathspec.h\n>> index 0c11262..b596aed 100644\n>> --- a/pathspec.h\n>> +++ b/pathspec.h\n>> @@ -96,7 +96,6 @@ static inline int ps_strcmp(const struct pathspec_item *item,\n>>\n>> extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec);\n>> extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);\n>> -extern const char *check_path_for_gitlink(const char *path);\n>> extern void die_if_path_beyond_symlink(const char *path, const char *prefix);\n>>\n>> #endif /* PATHSPEC_H */\n>\n> Interesting.\n>\n> I wonder if the patches mentioned have something to do with the \"git\n> add deep/in/the/tree\" that fails to notice deep/in/ is an unrelated\n> repository in some way?\n\nWhich is considered a feature now. Maybe we should add tests for that?\n\nhttp://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb\n"},{"id":"285648","messageId":"xmqqy47o6q71.fsf@gitster.mtv.corp.google.com","threadId":"42232","inReplyTo":"CAGZ79kY65Fo4+_a1B8J0h7PymGWUSoAdb1eb5YVfG55=30oPEg@mail.gmail.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T23:27:30Z","receivedAt":"2016-05-05T23:27:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> I wonder if the patches mentioned have something to do with the \"git\n>> add deep/in/the/tree\" that fails to notice deep/in/ is an unrelated\n>> repository in some way?\n>\n> Which is considered a feature now. Maybe we should add tests for that?\n>\n> http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb\n\nThat is a bug, plain and simple. Duy any ideas where we went wrong?\n\nI think we already have code to avoid adding beyond symlinks.\n\"git add deep/in/the/tree\" should refuse if deep/in is a symbolic\nlink (and happens to point at a directory that has the/tree in it).\nWe used not to catch that long time ago, but I think we fixed it.\n\nThe logic and the places to do the checks for \"no, that thing may be\na directory but is an unrelated repository\" should be the same.\n"},{"id":"285650","messageId":"CAGZ79kbmW5U+uFCnBhz4r2-ciGsWSwXHU5Va2r-MEca=iacfgQ@mail.gmail.com","threadId":"42232","inReplyTo":"xmqqy47o6q71.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-05T23:32:18Z","receivedAt":"2016-05-05T23:32:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 5, 2016 at 4:27 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> I wonder if the patches mentioned have something to do with the \"git\n>>> add deep/in/the/tree\" that fails to notice deep/in/ is an unrelated\n>>> repository in some way?\n>>\n>> Which is considered a feature now. Maybe we should add tests for that?\n>>\n>> http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb\n>\n> That is a bug, plain and simple. Duy any ideas where we went wrong?\n\nThat was my first reaction as well. However after a while of thought I actually\nlike that bug. Consider the possibilities how gitk/git-gui or other subsystems\ncan be developed. When accepting a patch for that you can either apply the\npatch in the outer or inner repository, depending on what the sender used.\n\nI am not so sure if it is a bug plain and simple, but devolved into a\n\"feature\" now.\n\n>\n> I think we already have code to avoid adding beyond symlinks.\n> \"git add deep/in/the/tree\" should refuse if deep/in is a symbolic\n> link (and happens to point at a directory that has the/tree in it).\n> We used not to catch that long time ago, but I think we fixed it.\n>\n> The logic and the places to do the checks for \"no, that thing may be\n> a directory but is an unrelated repository\" should be the same.\n"},{"id":"285651","messageId":"xmqqpot06oys.fsf@gitster.mtv.corp.google.com","threadId":"42232","inReplyTo":"CAGZ79kbmW5U+uFCnBhz4r2-ciGsWSwXHU5Va2r-MEca=iacfgQ@mail.gmail.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T23:54:03Z","receivedAt":"2016-05-05T23:54:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> That was my first reaction as well. However after a while of thought I actually\n> like that bug. Consider the possibilities how gitk/git-gui or other subsystems\n> can be developed. When accepting a patch for that you can either apply the\n> patch in the outer or inner repository, depending on what the sender used.\n>\n> I am not so sure if it is a bug plain and simple, but devolved into a\n> \"feature\" now.\n\nI'd freely admit that I have not considered its possible upsides at\nall. When deep/in/ is an unrelated repository, and running either\n\n git add deep/in/the\n git add deep/in/the/tree\n\nwould add deep/in/the/tree/is-a-leaf.txt to my index, but if I did\n\n git add deep/in\n\nI'd lose that and suddenly everything there turns into a submodule.\n\nAnd that is enough for me to declare that it is not worth my time to\nconsider possible upside of that hole. Can you tell offhand what\nwould happen if you do \"git add deep\" (before adding deep/in as a\nsubmodule) without experimenting?\n"},{"id":"285653","messageId":"CAGZ79kYB5-66tA5zoSpUTHVa-rdn551DXxVoiQ6KhOWadhAzLg@mail.gmail.com","threadId":"42232","inReplyTo":"xmqqpot06oys.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-06T00:28:35Z","receivedAt":"2016-05-06T00:28:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 5, 2016 at 4:54 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> That was my first reaction as well. However after a while of thought I actually\n>> like that bug. Consider the possibilities how gitk/git-gui or other subsystems\n>> can be developed. When accepting a patch for that you can either apply the\n>> patch in the outer or inner repository, depending on what the sender used.\n>>\n>> I am not so sure if it is a bug plain and simple, but devolved into a\n>> \"feature\" now.\n>\n> I'd freely admit that I have not considered its possible upsides at\n> all. When deep/in/ is an unrelated repository, and running either\n>\n> git add deep/in/the\n> git add deep/in/the/tree\n\nI think that doesn't work (I did not test), but the crucial part is to add\na trailing '/'. E.g. `git add deep/in/the/` adds the 'the/**' tree of\nthe nested\nrepository.\n\n>\n> would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did\n>\n> git add deep/in\n>\n> I'd lose that and suddenly everything there turns into a submodule.\n\nYes.\n\n git add deep/in # adds a submodule\n\nhowever:\n\n git add deep/in/ # adds all files of the sub-\"repo\" as indpendent files\n git commit -a -m \"new files\"\n git -C deep/in reset --hard HEAD^\n git diff\n # shows a difference in deep/in/the/tree/is-a-leaf.txt\n\n>\n> And that is enough for me to declare that it is not worth my time to\n> consider possible upside of that hole. Can you tell offhand what\n> would happen if you do \"git add deep\" (before adding deep/in as a\n> submodule) without experimenting?\n>\n\nNot really. My expectation is to add everything *but* the deep/in/ repo\nas this is not exercising the bug/feature.\n"},{"id":"285667","messageId":"xmqq60ur7ll3.fsf@gitster.mtv.corp.google.com","threadId":"42232","inReplyTo":"xmqqpot06oys.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T06:21:44Z","receivedAt":"2016-05-06T06:21:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> When deep/in/ is an unrelated repository, and running either\n>\n> git add deep/in/the\n> git add deep/in/the/tree\n>\n> would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did\n>\n> git add deep/in\n>\n> I'd lose that and suddenly everything there turns into a submodule.\n\nAlso, I recall that you floated an idea to declare that\n\n git add deep/in/the/tree/is-a-leaf.txt\n\nmust always behave as if this is done instead:\n\n git -C deep/in/the/tree/ add is-a-leaf.txt\n\nEven though I am not a huge fan of an operation that crosses module\nboundaries, I think that is a sensible semantics of a \"cross module\nboundary operation\" (the actual implementation should not be\niterating over pathspecs and chdir(2)ing around for each and every\none of them, though), if we need \"cross module boundary operation\"\nin order to support end users working on a project with one or more\nsubmodules at the same time.\n\nBut treating the bug under discussion as a \"feature\" will destroy\nthat future.\n"},{"id":"285669","messageId":"CAGZ79kaO+g7Dm5AgdYGADj=pYDLjV-ALGTxrmwrNGUy1NB=tNg@mail.gmail.com","threadId":"42232","inReplyTo":"xmqq60ur7ll3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-06T06:58:52Z","receivedAt":"2016-05-06T06:58:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 5, 2016 at 11:21 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> When deep/in/ is an unrelated repository, and running either\n>>\n>> git add deep/in/the\n>> git add deep/in/the/tree\n>>\n>> would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did\n>>\n>> git add deep/in\n>>\n>> I'd lose that and suddenly everything there turns into a submodule.\n>\n> Also, I recall that you floated an idea to declare that\n>\n> git add deep/in/the/tree/is-a-leaf.txt\n>\n> must always behave as if this is done instead:\n>\n> git -C deep/in/the/tree/ add is-a-leaf.txt\n\nIn a different thread, yeah. I am not sure about that any more.\n\n>\n> Even though I am not a huge fan of an operation that crosses module\n> boundaries, I think that is a sensible semantics of a \"cross module\n> boundary operation\" (the actual implementation should not be\n> iterating over pathspecs and chdir(2)ing around for each and every\n> one of them, though), if we need \"cross module boundary operation\"\n> in order to support end users working on a project with one or more\n> submodules at the same time.\n\nI agree.\n\n>\n> But treating the bug under discussion as a \"feature\" will destroy\n> that future.\n\nThe bug under discussion was blogged about in 2010 and still persists.\nI'll try to find out if people actually use it.\n\nIf that bug was fixed, but I still wanted to enjoy the upsides of it,\nI could to that with pointing core.worktree into deep/in/tree.\ne.g. I have git.git and gitk-git as separate repositories,\nthen I could still do a\n\n GIT_DIR=gitk-git/.git git -C git.git/gitk-git git checkout\n\n\nThis looks more complicated than this bug/feature though.\n\nThere are 2 fundamental cases though.\n (1) The bug we're talking about (as explained in that blog), refers to 2\n independent repositories, whose work trees are nested\n (2) You seemed to bring in the notion that the nested repo is considered\n a submodule of the outer repo, i.e. they have a relationship.\n\nI don't mind (1). It's a neat hack as these 2 repos are totally unrelated\n(except for the working tree in the file system being the same files).\nYou could also achieve a similar handling by hardlinking gitk-git/gitk\nand git.git/gitk-git/gitk.\n\nIn (2), we have a gitlink, which by definition takes up the whole directory.\nSo any file in that directory in the file system which represents the root of\nthe submodule should belong to the submodule.\n"},{"id":"285670","messageId":"xmqq1t5f7j4e.fsf@gitster.mtv.corp.google.com","threadId":"42232","inReplyTo":"CAGZ79kaO+g7Dm5AgdYGADj=pYDLjV-ALGTxrmwrNGUy1NB=tNg@mail.gmail.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T07:14:57Z","receivedAt":"2016-05-06T07:14:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> There are 2 fundamental cases though.\n> (1) The bug we're talking about (as explained in that blog), refers to 2\n> independent repositories, whose work trees are nested\n> (2) You seemed to bring in the notion that the nested repo is considered\n> a submodule of the outer repo, i.e. they have a relationship.\n>\n> I don't mind (1). It's a neat hack as these 2 repos are totally unrelated\n> (except for the working tree in the file system being the same files).\n> You could also achieve a similar handling by hardlinking gitk-git/gitk\n> and git.git/gitk-git/gitk.\n>\n> In (2), we have a gitlink, which by definition takes up the whole directory.\n> So any file in that directory in the file system which represents the root of\n> the submodule should belong to the submodule.\n\nI certainly didn't mean to \"bring in the notion\" as if it is\nsomething entirely alien to the discussion. Before you \"git add\",\nit may be a \"nested independent\" repository, but that is merely a\nsubmodule that is untracked, yet to be added. Just like tracked\nfiles were once untracked before they got added, these are possible\nsubmodules that happened to be not tracked yet.\n\nI do not see there is any difference between the two at all.\n\nIf deep/in is a repository yet to be added as a submodule,\n\n $ git add deep/in/the/tree/is-a-leaf.txt\n $ git add deep/in\n\nin the hypothetical \"git add A is equivalent to git -C $(dirname A)\nadd $(basename A)\" world should behave the same regardless of the\norder of these two commands (otherwise the behaviour is way too\nconfusing).\n"},{"id":"285680","messageId":"CACsJy8BbWyw37sQkAq-B_De87N3XzZA9A1fm1A8A7MzfPBtdrw@mail.gmail.com","threadId":"42232","inReplyTo":"xmqqy47o6q71.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-06T10:30:08Z","receivedAt":"2016-05-06T10:30:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> I wonder if the patches mentioned have something to do with the \"git\n>>> add deep/in/the/tree\" that fails to notice deep/in/ is an unrelated\n>>> repository in some way?\n\nThe same functionality is added in 8745024 (parse_pathspec: support\nstripping/checking submodule paths - 2013-07-14) so if it didn't fail\nto notice that before 5a76aff1a6 and did after, it's a bug.\n\n>>\n>> Which is considered a feature now. Maybe we should add tests for that?\n>>\n>> http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb\n>\n> That is a bug, plain and simple. Duy any ideas where we went wrong?\n\nI vaguely recall this symptom. It has something to do with the index,\nthe check we do requires a gitlink in the index, I think. So if the\ngitlink entry is not in the index, our protection line fails. I think\ndoing all this at pathspec level is wrong. We should wait at least\nafter read_directory() is done, by then we have a lot more info to\ndecide.\n\n> I think we already have code to avoid adding beyond symlinks.\n> \"git add deep/in/the/tree\" should refuse if deep/in is a symbolic\n> link (and happens to point at a directory that has the/tree in it).\n> We used not to catch that long time ago, but I think we fixed it.\n>\n> The logic and the places to do the checks for \"no, that thing may be\n> a directory but is an unrelated repository\" should be the same.\n-- \nDuy\n"},{"id":"285757","messageId":"CAGZ79kaBh-SogYrMcVfgE1DS464oK2Z01ZqpBiM0eSHKMPU1Fw@mail.gmail.com","threadId":"42232","inReplyTo":"CACsJy8BbWyw37sQkAq-B_De87N3XzZA9A1fm1A8A7MzfPBtdrw@mail.gmail.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-06T17:13:32Z","receivedAt":"2016-05-06T17:13:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 6, 2016 at 3:30 AM, Duy Nguyen wrote:\n> On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>>> I wonder if the patches mentioned have something to do with the \"git\n>>>> add deep/in/the/tree\" that fails to notice deep/in/ is an unrelated\n>>>> repository in some way?\n>\n> The same functionality is added in 8745024 (parse_pathspec: support\n> stripping/checking submodule paths - 2013-07-14) so if it didn't fail\n> to notice that before 5a76aff1a6 and did after, it's a bug.\n\nThe bug seems to have existed before. However in the bug we are talking\nabout the nested repo is not a submodule yet.\n\n>\n>>>\n>>> Which is considered a feature now. Maybe we should add tests for that?\n>>>\n>>> http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb\n>>\n>> That is a bug, plain and simple. Duy any ideas where we went wrong?\n>\n> I vaguely recall this symptom. It has something to do with the index,\n> the check we do requires a gitlink in the index, I think. So if the\n> gitlink entry is not in the index, our protection line fails. I think\n> doing all this at pathspec level is wrong. We should wait at least\n> after read_directory() is done, by then we have a lot more info to\n> decide.\n>\n>> I think we already have code to avoid adding beyond symlinks.\n>> \"git add deep/in/the/tree\" should refuse if deep/in is a symbolic\n>> link (and happens to point at a directory that has the/tree in it).\n>> We used not to catch that long time ago, but I think we fixed it.\n>>\n>> The logic and the places to do the checks for \"no, that thing may be\n>> a directory but is an unrelated repository\" should be the same.\n> --\n> Duy\n\nHere are some tests to have a clear picture of what is happening:\n(diff against origin/master)\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex f99f674..c9dfa11 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -37,6 +37,22 @@ test_expect_success 'setup - repository in init\nsubdirectory' '\n git add a &&\n git commit -m \"submodule commit 1\" &&\n git tag -a -m \"rev-1\" rev-1\n+ ) &&\n+ mkdir init_slash1 &&\n+ (\n+ cd init_slash1 &&\n+ git init &&\n+ echo a >a &&\n+ git add a &&\n+ git commit -m \"first commit\"\n+ ) &&\n+ mkdir init_slash2 &&\n+ (\n+ cd init_slash2 &&\n+ git init &&\n+ echo a >a &&\n+ git add a &&\n+ git commit -m \"first commit\"\n )\n '\n\n@@ -44,7 +60,30 @@ test_expect_success 'setup - commit with gitlink' '\n echo a >a &&\n echo z >z &&\n git add a init z &&\n- git commit -m \"super commit 1\"\n+ git commit -m \"super commit 1\" &&\n+ git ls-tree HEAD |grep init >actual &&\n+ grep \"160000 commit\" actual\n+'\n+\n+test_expect_success 'setup - commit with gitlink/ in between' '\n+ echo a >a &&\n+ echo z >z &&\n+ git add a init_slash1/ z &&\n+ git commit -m \"super commit 1\" &&\n+ git ls-tree HEAD |grep init_slash1 >actual &&\n+ grep \"160000 commit\" actual\n+'\n+\n+test_expect_failure 'setup - commit with gitlink/ only' '\n+ git add init_slash2/ &&\n+ git commit -m \"super commit 1\" &&\n+ git ls-tree HEAD |grep init_slash2 >actual &&\n+ grep \"160000 commit\" actual\n+'\n+\n+test_expect_success 'tear down slash tests' '\n+ rm -rf init_slash* &&\n+ git commit -a -m \"removing init_slash*\"\n '\n\n test_expect_success 'setup - hide init subdirectory' '\n"},{"id":"285771","messageId":"xmqqr3df2end.fsf@gitster.mtv.corp.google.com","threadId":"42232","inReplyTo":"CAGZ79kaBh-SogYrMcVfgE1DS464oK2Z01ZqpBiM0eSHKMPU1Fw@mail.gmail.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T19:02:46Z","receivedAt":"2016-05-06T19:02:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Fri, May 6, 2016 at 3:30 AM, Duy Nguyen wrote:\n>> On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano wrote:\n>>> Stefan Beller writes:\n>>>\n>>>>> I wonder if the patches mentioned have something to do with the \"git\n>>>>> add deep/in/the/tree\" that fails to notice deep/in/ is an unrelated\n>>>>> repository in some way?\n>>\n>> The same functionality is added in 8745024 (parse_pathspec: support\n>> stripping/checking submodule paths - 2013-07-14) so if it didn't fail\n>> to notice that before 5a76aff1a6 and did after, it's a bug.\n>\n> The bug seems to have existed before. However in the bug we are talking\n> about the nested repo is not a submodule yet.\n\nThat agrees with Duy's recollection below:\n\n>> I vaguely recall this symptom. It has something to do with the index,\n>> the check we do requires a gitlink in the index, I think. So if the\n>> gitlink entry is not in the index, our protection line fails.\n\nSo are we all on the same page that this is a bug now?\n"},{"id":"285772","messageId":"CAGZ79kaJYngaZfhx060C06J57aDqPJfdMD8xzK4dW4nvvktMLQ@mail.gmail.com","threadId":"42232","inReplyTo":"xmqqr3df2end.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-06T19:18:38Z","receivedAt":"2016-05-06T19:18:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 6, 2016 at 12:02 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Fri, May 6, 2016 at 3:30 AM, Duy Nguyen wrote:\n>>> On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano wrote:\n>>>> Stefan Beller writes:\n>>>>\n>>>>>> I wonder if the patches mentioned have something to do with the \"git\n>>>>>> add deep/in/the/tree\" that fails to notice deep/in/ is an unrelated\n>>>>>> repository in some way?\n>>>\n>>> The same functionality is added in 8745024 (parse_pathspec: support\n>>> stripping/checking submodule paths - 2013-07-14) so if it didn't fail\n>>> to notice that before 5a76aff1a6 and did after, it's a bug.\n>>\n>> The bug seems to have existed before. However in the bug we are talking\n>> about the nested repo is not a submodule yet.\n>\n> That agrees with Duy's recollection below:\n>\n>>> I vaguely recall this symptom. It has something to do with the index,\n>>> the check we do requires a gitlink in the index, I think. So if the\n>>> gitlink entry is not in the index, our protection line fails.\n>\n> So are we all on the same page that this is a bug now?\n\nIt was a bug, but now people in the outside world consider it a feature.\nSearch for \"Git fake submodules\" and you'll find a few users who use this\ntechnique successfully.\n\nI do not think fixing this bug would do good. So maybe we just let it slip?\n"},{"id":"285778","messageId":"xmqqd1oz2bjg.fsf@gitster.mtv.corp.google.com","threadId":"42232","inReplyTo":"CAGZ79kaJYngaZfhx060C06J57aDqPJfdMD8xzK4dW4nvvktMLQ@mail.gmail.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T20:09:55Z","receivedAt":"2016-05-06T20:09:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> It was a bug, but now people in the outside world consider it a feature.\n> Search for \"Git fake submodules\" and you'll find a few users who use this\n> technique successfully.\n>\n> I do not think fixing this bug would do good. So maybe we just let it slip?\n\nI am OK with leaving it unfixed, iow, we just say this:\n\n If deep/in is a different repository, whether it is a submodule,\n \"git add deep/in/the/tree/is-a-leaf.txt\" will give an undefined\n result.\n\nBut that is totally different from accepting it as a feature. If we\nwere to accept it as a feature (and we will not), then\n\n I did \"git add deep/in/the/tree/is-a-leaf.txt\" and have kept the\n path tracked. Today I did \"git add deep/*\" and then the path\n disappeared from my project--I now only have deep/in as a\n submodule, which is not what I want.\n\nwould become a valid bug report. I do not want to see that happen.\n\nI.e. I am *NOT* OK with polluting the codebase with a hack to\nrespond to such a bug report, e.g. by adding a rule that says \"if a\nfile deep/in/the/tree/is-a-leaf.txt is tracked and deep/in is a\nrepository, 'git add deep/in' must fail\".\n\nThe stance \"It is a bug, but we do not fix it right now. The\nbehaviour is undefined\" also leaves the door open for a future\nenhancement that allows 'git add deep/in/the/tree/is-a-leaf.txt' to\nbe an equivalent to 'git -C deep/in/the/tree add is-a-leaf.txt'.\n"},{"id":"285810","messageId":"CACsJy8CkJ9UF3y6jgCS9KGArLqdEZRqehu_KABFPKo+P2W4M_g@mail.gmail.com","threadId":"42232","inReplyTo":"CAGZ79kaJYngaZfhx060C06J57aDqPJfdMD8xzK4dW4nvvktMLQ@mail.gmail.com","subject":"Re: [PATCH] pathspec: remove check_path_for_gitlink","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-07T07:16:41Z","receivedAt":"2016-05-07T07:16:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, May 7, 2016 at 2:18 AM, Stefan Beller wrote:\n> On Fri, May 6, 2016 at 12:02 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> On Fri, May 6, 2016 at 3:30 AM, Duy Nguyen wrote:\n>>>> On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano wrote:\n>>>>> Stefan Beller writes:\n>>>>>\n>>>>>>> I wonder if the patches mentioned have something to do with the \"git\n>>>>>>> add deep/in/the/tree\" that fails to notice deep/in/ is an unrelated\n>>>>>>> repository in some way?\n>>>>\n>>>> The same functionality is added in 8745024 (parse_pathspec: support\n>>>> stripping/checking submodule paths - 2013-07-14) so if it didn't fail\n>>>> to notice that before 5a76aff1a6 and did after, it's a bug.\n>>>\n>>> The bug seems to have existed before. However in the bug we are talking\n>>> about the nested repo is not a submodule yet.\n>>\n>> That agrees with Duy's recollection below:\n>>\n>>>> I vaguely recall this symptom. It has something to do with the index,\n>>>> the check we do requires a gitlink in the index, I think. So if the\n>>>> gitlink entry is not in the index, our protection line fails.\n>>\n>> So are we all on the same page that this is a bug now?\n>\n> It was a bug, but now people in the outside world consider it a feature.\n> Search for \"Git fake submodules\" and you'll find a few users who use this\n> technique successfully.\n>\n> I do not think fixing this bug would do good. So maybe we just let it slip?\n\nI think it's because people do have some use cases for multiple\nworktrees (typically from different repos) to share the same\nfilesystem portion. We never actually support that. There are a bunch\nof questions on how these worktrees/repos should interact. Maybe\nsomeone will finally add support for that. But this is still a bug,\neven though nobody has time to fix it yet. So I agree with Junio\ncalling this \"undefined behavior\", not \"feature\".\n-- \nDuy\n"}]}