threads / patch / 42232

patchpathspec: remove check_path_for_gitlink

Subject: [PATCH] pathspec: remove check_path_for_gitlink

## tl;dr

16 messages between May 5, 2016 and May 7, 2016. Diffs are folded; open one to read it.

replies: 15people: 3as markdown or json

Stefan Beller· May 5, 2016, 22:31 UTC · lore

`check_path_for_gitlink` was introduced in 9d67b61f739a (2013-01-06, add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse) but the implementation was removed in 5a76aff1a6 (2013-07-14, add: convert to use parse_pathspec).

Remove the declaration from the header as well.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 pathspec.h | 1 -
 1 file changed, 1 deletion(-)
Show changes to pathspec.h +0 −1
diff --git a/pathspec.h b/pathspec.h
index 0c11262..b596aed 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -96,7 +96,6 @@ static inline int ps_strcmp(const struct pathspec_item *item,
 
 extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec);
 extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);
-extern const char *check_path_for_gitlink(const char *path);
 extern void die_if_path_beyond_symlink(const char *path, const char *prefix);
 
 #endif /* PATHSPEC_H */

-- 
2.8.0.1.g3af9c03
Junio C Hamano· May 5, 2016, 23:09 UTC · re: Stefan Beller · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

Stefan Beller <sbeller@google.com> writes:
Show 24 quoted lines
> `check_path_for_gitlink` was introduced in 9d67b61f739a (2013-01-06,
> add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse)
> but the implementation was removed in 5a76aff1a6 (2013-07-14, add:
> convert to use parse_pathspec).
>
> Remove the declaration from the header as well.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  pathspec.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/pathspec.h b/pathspec.h
> index 0c11262..b596aed 100644
> --- a/pathspec.h
> +++ b/pathspec.h
> @@ -96,7 +96,6 @@ static inline int ps_strcmp(const struct pathspec_item *item,
>  
>  extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec);
>  extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);
> -extern const char *check_path_for_gitlink(const char *path);
>  extern void die_if_path_beyond_symlink(const char *path, const char *prefix);
>  
>  #endif /* PATHSPEC_H */
Interesting.

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?

Stefan Beller· May 5, 2016, 23:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

On Thu, May 5, 2016 at 4:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> `check_path_for_gitlink` was introduced in 9d67b61f739a (2013-01-06,
>> add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse)
>> but the implementation was removed in 5a76aff1a6 (2013-07-14, add:
>> convert to use parse_pathspec).
>>
>> Remove the declaration from the header as well.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  pathspec.h | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/pathspec.h b/pathspec.h
>> index 0c11262..b596aed 100644
>> --- a/pathspec.h
>> +++ b/pathspec.h
>> @@ -96,7 +96,6 @@ static inline int ps_strcmp(const struct pathspec_item *item,
>>
>>  extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec);
>>  extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);
>> -extern const char *check_path_for_gitlink(const char *path);
>>  extern void die_if_path_beyond_symlink(const char *path, const char *prefix);
>>
>>  #endif /* PATHSPEC_H */
>
> Interesting.
>
> 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
Junio C Hamano· May 5, 2016, 23:27 UTC · re: Stefan Beller · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
>> 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.

Stefan Beller· May 5, 2016, 23:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

On Thu, May 5, 2016 at 4:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>>> 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.

Show 8 quoted lines
>
> 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.
Junio C Hamano· May 5, 2016, 23:54 UTC · re: Stefan Beller · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
> 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'd freely admit that I have not considered its possible upsides at all. When deep/in/ is an unrelated repository, and running either

    git add deep/in/the
    git add deep/in/the/tree
would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did
    git add deep/in
I'd lose that and suddenly everything there turns into a submodule.

And that is enough for me to declare that it is not worth my time to consider possible upside of that hole. Can you tell offhand what would happen if you do "git add deep" (before adding deep/in as a submodule) without experimenting?

Stefan Beller· May 6, 2016, 00:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

On Thu, May 5, 2016 at 4:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> 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'd freely admit that I have not considered its possible upsides at
> all.  When deep/in/ is an unrelated repository, and running either
>
>     git add deep/in/the
>     git add deep/in/the/tree

I think that doesn't work (I did not test), but the crucial part is to add a trailing '/'. E.g. `git add deep/in/the/` adds the 'the/**' tree of the nested repository.

Show 6 quoted lines
>
> would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did
>
>     git add deep/in
>
> I'd lose that and suddenly everything there turns into a submodule.
Yes.
    git add deep/in # adds a submodule
however:
    git add deep/in/ # adds all files of the sub-"repo"  as indpendent files
    git commit -a -m "new files"
    git -C deep/in reset --hard HEAD^
    git diff
    # shows a difference in deep/in/the/tree/is-a-leaf.txt
Show 6 quoted lines
>
> And that is enough for me to declare that it is not worth my time to
> consider possible upside of that hole.  Can you tell offhand what
> would happen if you do "git add deep" (before adding deep/in as a
> submodule) without experimenting?
>

Not really. My expectation is to add everything *but* the deep/in/ repo as this is not exercising the bug/feature.

Junio C Hamano· May 6, 2016, 06:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> When deep/in/ is an unrelated repository, and running either
>
>     git add deep/in/the
>     git add deep/in/the/tree
>
> would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did
>
>     git add deep/in
>
> I'd lose that and suddenly everything there turns into a submodule.
Also, I recall that you floated an idea to declare that
    git add deep/in/the/tree/is-a-leaf.txt
must always behave as if this is done instead:
    git -C deep/in/the/tree/ add is-a-leaf.txt

Even though I am not a huge fan of an operation that crosses module boundaries, I think that is a sensible semantics of a "cross module boundary operation" (the actual implementation should not be iterating over pathspecs and chdir(2)ing around for each and every one of them, though), if we need "cross module boundary operation" in order to support end users working on a project with one or more submodules at the same time.

But treating the bug under discussion as a "feature" will destroy that future.

Stefan Beller· May 6, 2016, 06:58 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

On Thu, May 5, 2016 at 11:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> When deep/in/ is an unrelated repository, and running either
>>
>>     git add deep/in/the
>>     git add deep/in/the/tree
>>
>> would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did
>>
>>     git add deep/in
>>
>> I'd lose that and suddenly everything there turns into a submodule.
>
> Also, I recall that you floated an idea to declare that
>
>     git add deep/in/the/tree/is-a-leaf.txt
>
> must always behave as if this is done instead:
>
>     git -C deep/in/the/tree/ add is-a-leaf.txt
In a different thread, yeah. I am not sure about that any more.
Show 8 quoted lines
>
> Even though I am not a huge fan of an operation that crosses module
> boundaries, I think that is a sensible semantics of a "cross module
> boundary operation" (the actual implementation should not be
> iterating over pathspecs and chdir(2)ing around for each and every
> one of them, though), if we need "cross module boundary operation"
> in order to support end users working on a project with one or more
> submodules at the same time.
I agree.
>
> But treating the bug under discussion as a "feature" will destroy
> that future.

The bug under discussion was blogged about in 2010 and still persists. I'll try to find out if people actually use it.

If that bug was fixed, but I still wanted to enjoy the upsides of it, I could to that with pointing core.worktree into deep/in/tree. e.g. I have git.git and gitk-git as separate repositories, then I could still do a

    GIT_DIR=gitk-git/.git git -C git.git/gitk-git git checkout
<sha1-of-gitk-repo>
This looks more complicated than this bug/feature though.
There are 2 fundamental cases though.
 (1) The bug we're talking about (as explained in that blog), refers to 2
    independent repositories, whose work trees are nested
 (2) You seemed to bring in the notion that the nested repo is considered
    a submodule of the outer repo, i.e. they have a relationship.

I don't mind (1). It's a neat hack as these 2 repos are totally unrelated (except for the working tree in the file system being the same files). You could also achieve a similar handling by hardlinking gitk-git/gitk and git.git/gitk-git/gitk.

In (2), we have a gitlink, which by definition takes up the whole directory. So any file in that directory in the file system which represents the root of the submodule should belong to the submodule.

Junio C Hamano· May 6, 2016, 07:14 UTC · re: Stefan Beller · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

Stefan Beller <sbeller@google.com> writes:
Show 14 quoted lines
> There are 2 fundamental cases though.
>  (1) The bug we're talking about (as explained in that blog), refers to 2
>     independent repositories, whose work trees are nested
>  (2) You seemed to bring in the notion that the nested repo is considered
>     a submodule of the outer repo, i.e. they have a relationship.
>
> I don't mind (1). It's a neat hack as these 2 repos are totally unrelated
> (except for the working tree in the file system being the same files).
> You could also achieve a similar handling by hardlinking gitk-git/gitk
> and git.git/gitk-git/gitk.
>
> In (2), we have a gitlink, which by definition takes up the whole directory.
> So any file in that directory in the file system which represents the root of
> the submodule should belong to the submodule.

I certainly didn't mean to "bring in the notion" as if it is something entirely alien to the discussion. Before you "git add", it may be a "nested independent" repository, but that is merely a submodule that is untracked, yet to be added. Just like tracked files were once untracked before they got added, these are possible submodules that happened to be not tracked yet.

I do not see there is any difference between the two at all.
If deep/in is a repository yet to be added as a submodule,
    $ git add deep/in/the/tree/is-a-leaf.txt
    $ git add deep/in

in the hypothetical "git add A is equivalent to git -C $(dirname A) add $(basename A)" world should behave the same regardless of the order of these two commands (otherwise the behaviour is way too confusing).

Duy Nguyen· May 6, 2016, 10:30 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>>> 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.

Show 6 quoted lines
>>
>> 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.

Show 7 quoted lines
> 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
Stefan Beller· May 6, 2016, 17:13 UTC · re: Duy Nguyen · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

On Fri, May 6, 2016 at 3:30 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 10 quoted lines
> On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>>> 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.

Show 24 quoted lines
>
>>>
>>> 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

Here are some tests to have a clear picture of what is happening: (diff against origin/master)

Show changes to t/t7400-submodule-basic.sh +40 −1
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' '
Junio C Hamano· May 6, 2016, 19:02 UTC · re: Stefan Beller · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

Stefan Beller <sbeller@google.com> writes:
Show 14 quoted lines
> On Fri, May 6, 2016 at 3:30 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>>> 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.
That agrees with Duy's recollection below:
>> 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.
So are we all on the same page that this is a bug now?
Stefan Beller· May 6, 2016, 19:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

On Fri, May 6, 2016 at 12:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, May 6, 2016 at 3:30 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Stefan Beller <sbeller@google.com> writes:
>>>>
>>>>>> 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.
>
> That agrees with Duy's recollection below:
>
>>> 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.
>
> So are we all on the same page that this is a bug now?

It was a bug, but now people in the outside world consider it a feature. Search for "Git fake submodules" and you'll find a few users who use this technique successfully.

I do not think fixing this bug would do good. So maybe we just let it slip?
Junio C Hamano· May 6, 2016, 20:09 UTC · re: Stefan Beller · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
> It was a bug, but now people in the outside world consider it a feature.
> Search for "Git fake submodules" and you'll find a few users who use this
> technique successfully.
>
> I do not think fixing this bug would do good. So maybe we just let it slip?
I am OK with leaving it unfixed, iow, we just say this:
    If deep/in is a different repository, whether it is a submodule,
    "git add deep/in/the/tree/is-a-leaf.txt" will give an undefined
    result.

But that is totally different from accepting it as a feature. If we were to accept it as a feature (and we will not), then

    I did "git add deep/in/the/tree/is-a-leaf.txt" and have kept the
    path tracked.  Today I did "git add deep/*" and then the path
    disappeared from my project--I now only have deep/in as a
    submodule, which is not what I want.
would become a valid bug report.  I do not want to see that happen.

I.e. I am *NOT* OK with polluting the codebase with a hack to respond to such a bug report, e.g. by adding a rule that says "if a file deep/in/the/tree/is-a-leaf.txt is tracked and deep/in is a repository, 'git add deep/in' must fail".

The stance "It is a bug, but we do not fix it right now. The behaviour is undefined" also leaves the door open for a future enhancement that allows 'git add deep/in/the/tree/is-a-leaf.txt' to be an equivalent to 'git -C deep/in/the/tree add is-a-leaf.txt'.

Duy Nguyen· May 7, 2016, 07:16 UTC · re: Stefan Beller · lore

Re: [PATCH] pathspec: remove check_path_for_gitlink

On Sat, May 7, 2016 at 2:18 AM, Stefan Beller <sbeller@google.com> wrote:
Show 31 quoted lines
> On Fri, May 6, 2016 at 12:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Fri, May 6, 2016 at 3:30 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>>> On Fri, May 6, 2016 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Stefan Beller <sbeller@google.com> writes:
>>>>>
>>>>>>> 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.
>>
>> That agrees with Duy's recollection below:
>>
>>>> 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.
>>
>> So are we all on the same page that this is a bug now?
>
> It was a bug, but now people in the outside world consider it a feature.
> Search for "Git fake submodules" and you'll find a few users who use this
> technique successfully.
>
> I do not think fixing this bug would do good. So maybe we just let it slip?

I think it's because people do have some use cases for multiple worktrees (typically from different repos) to share the same filesystem portion. We never actually support that. There are a bunch of questions on how these worktrees/repos should interact. Maybe someone will finally add support for that. But this is still a bug, even though nobody has time to fix it yet. So I agree with Junio calling this "undefined behavior", not "feature".

-- 
Duy

← back to recent threads