Adding nested repository with slash adds files instead of gitlink

Adding nested repository with slash adds files instead of gitlink

[Heiko Voigt]

Hi,

I just discovered that when you have a slash at the end of a nested
repository, the files contained in the repository get added instead of
the gitlink.

I found this when I was adding a submodule and wanted to commit a small
change before that. You get the slash by using tab autocompletion.

Here is a recipe to reproduce:

mkdir test
cd test; git init
touch a; git add a; git commit -m a
mkdir ../test.git; (cd ../test.git; git init --bare)
git remote add origin ../test.git
git push origin master
git submodule add ../test.git submodule
git reset
git add submodule/

Now instead of just submodule gitlink there is an entry for submodule/a
in the index.

I just thought I put this out there. Will have a look if I find the time
to cook up a proper testcase and investigate.

Cheers Heiko

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Mon, Jun 18, 2018 at 1:23 PM Heiko Voigt <hvoigt@hvoigt.net> wrote:
>
> Hi,
>
> I just discovered that when you have a slash at the end of a nested
> repository, the files contained in the repository get added instead of
> the gitlink.
>
> I found this when I was adding a submodule and wanted to commit a small
> change before that. You get the slash by using tab autocompletion.
>
> Here is a recipe to reproduce:
>
> mkdir test
> cd test; git init
> touch a; git add a; git commit -m a
> mkdir ../test.git; (cd ../test.git; git init --bare)
> git remote add origin ../test.git
> git push origin master
> git submodule add ../test.git submodule
> git reset
> git add submodule/
>
> Now instead of just submodule gitlink there is an entry for submodule/a
> in the index.
>
> I just thought I put this out there. Will have a look if I find the time
> to cook up a proper testcase and investigate.

This sounds like the submodule specific code in pathspec.c, which has
been replaced with something else in bw/pathspec-sans-the-index. If
you have time, try a version without those changes (e.g. v2.13 or
before) to see if it's a possible culprit.

> Cheers Heiko



-- 
Duy

Re: Adding nested repository with slash adds files instead of gitlink

[Brandon Williams]

On 06/18, Duy Nguyen wrote:
> On Mon, Jun 18, 2018 at 1:23 PM Heiko Voigt <hvoigt@hvoigt.net> wrote:
> >
> > Hi,
> >
> > I just discovered that when you have a slash at the end of a nested
> > repository, the files contained in the repository get added instead of
> > the gitlink.
> >
> > I found this when I was adding a submodule and wanted to commit a small
> > change before that. You get the slash by using tab autocompletion.
> >
> > Here is a recipe to reproduce:
> >
> > mkdir test
> > cd test; git init
> > touch a; git add a; git commit -m a
> > mkdir ../test.git; (cd ../test.git; git init --bare)
> > git remote add origin ../test.git
> > git push origin master
> > git submodule add ../test.git submodule
> > git reset
> > git add submodule/
> >
> > Now instead of just submodule gitlink there is an entry for submodule/a
> > in the index.
> >
> > I just thought I put this out there. Will have a look if I find the time
> > to cook up a proper testcase and investigate.
> 
> This sounds like the submodule specific code in pathspec.c, which has
> been replaced with something else in bw/pathspec-sans-the-index. If
> you have time, try a version without those changes (e.g. v2.13 or
> before) to see if it's a possible culprit.

I just tested this with v2.13 and saw the same issue.  I don't actually
think this ever worked in the way you want it to Heiko.  Maybe git add
needs to be taught to be more intelligent when trying to add a submodule
which doesn't exist in the index.

-- 
Brandon Williams

Re: Adding nested repository with slash adds files instead of gitlink

[Heiko Voigt]

On Mon, Jun 18, 2018 at 11:12:15AM -0700, Brandon Williams wrote:
> On 06/18, Duy Nguyen wrote:
> > This sounds like the submodule specific code in pathspec.c, which has
> > been replaced with something else in bw/pathspec-sans-the-index. If
> > you have time, try a version without those changes (e.g. v2.13 or
> > before) to see if it's a possible culprit.
> 
> I just tested this with v2.13 and saw the same issue.  I don't actually
> think this ever worked in the way you want it to Heiko.  Maybe git add
> needs to be taught to be more intelligent when trying to add a submodule
> which doesn't exist in the index.

That was also my guess, since my feeling is that this is a quite rare
use case. Adding submodules alone is not a daily thing, let alone
selecting different changes after 'git submodule add'.

I also think git could be more intelligent here.

Cheers Heiko

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Tue, Jun 19, 2018 at 12:36 PM Heiko Voigt <hvoigt@hvoigt.net> wrote:
>
> On Mon, Jun 18, 2018 at 11:12:15AM -0700, Brandon Williams wrote:
> > On 06/18, Duy Nguyen wrote:
> > > This sounds like the submodule specific code in pathspec.c, which has
> > > been replaced with something else in bw/pathspec-sans-the-index. If
> > > you have time, try a version without those changes (e.g. v2.13 or
> > > before) to see if it's a possible culprit.
> >
> > I just tested this with v2.13 and saw the same issue.  I don't actually
> > think this ever worked in the way you want it to Heiko.  Maybe git add
> > needs to be taught to be more intelligent when trying to add a submodule
> > which doesn't exist in the index.
>
> That was also my guess, since my feeling is that this is a quite rare
> use case. Adding submodules alone is not a daily thing, let alone
> selecting different changes after 'git submodule add'.
>
> I also think git could be more intelligent here.

Ah.. the "submodule not registered in index" case. I think I remember
this (because I remember complaining about it once or two times).
Definitely agreed that git-add should do the right thing here.

Brandon already moved the submodule check outside pathspec code
(wonderful!) so adding more checks based on worktree state should not
be a big work. I think the only concern here is catching submodule
locations so we don't check submodule at the same location multiple
times.

No actually, we could do better. Let me see if I can come up with a
patch or something...
-- 
Duy

Re: Adding nested repository with slash adds files instead of gitlink

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Jun 19, 2018 at 12:36 PM Heiko Voigt <hvoigt@hvoigt.net> wrote:
>>
>> On Mon, Jun 18, 2018 at 11:12:15AM -0700, Brandon Williams wrote:
>> > On 06/18, Duy Nguyen wrote:
>> > > This sounds like the submodule specific code in pathspec.c, which has
>> > > been replaced with something else in bw/pathspec-sans-the-index. If
>> > > you have time, try a version without those changes (e.g. v2.13 or
>> > > before) to see if it's a possible culprit.
>> >
>> > I just tested this with v2.13 and saw the same issue.  I don't actually
>> > think this ever worked in the way you want it to Heiko.  Maybe git add
>> > needs to be taught to be more intelligent when trying to add a submodule
>> > which doesn't exist in the index.
>>
>> That was also my guess, since my feeling is that this is a quite rare
>> use case. Adding submodules alone is not a daily thing, let alone
>> selecting different changes after 'git submodule add'.
>>
>> I also think git could be more intelligent here.
>
> Ah.. the "submodule not registered in index" case. I think I remember
> this (because I remember complaining about it once or two times).
> Definitely agreed that git-add should do the right thing here.

I am not sure if this even needs to be implemented as "look for the
submodule in the index".  Even before submodule was added, we knew
that "git add foo/bar" should reject the request if we find foo is a
symbolic link, and we should do the same when foo/ is a directory
that is the top of a working tree under control of another
repository, no?

Hmm, what happens when we do this?

	git init
	ln -s /tmp foo
	>foo/bar
	git add foo/

I think we should say either "let's add foo symlink" or
"foo/. (directory) is beyond symlink" (the latter is preferrable,
but the former is acceptable as long as foo is pointing at a
directory; but foo could be a dangling symlink whose pointee's type
may not be discernable by "git add").

Shouldn't we be reacting pretty much the same when we see this?

	git init
	git init foo
	>foo/bar
	git add foo/

That is, either drop '/' and add 'foo' as a submodule, or say
"foo/. (directory) belongs to another repository, cannot add here"
(again, the latter is preferrable for consistency with the symlink
behaviour above).

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Tue, Jun 19, 2018 at 5:56 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Tue, Jun 19, 2018 at 12:36 PM Heiko Voigt <hvoigt@hvoigt.net> wrote:
> >>
> >> On Mon, Jun 18, 2018 at 11:12:15AM -0700, Brandon Williams wrote:
> >> > On 06/18, Duy Nguyen wrote:
> >> > > This sounds like the submodule specific code in pathspec.c, which has
> >> > > been replaced with something else in bw/pathspec-sans-the-index. If
> >> > > you have time, try a version without those changes (e.g. v2.13 or
> >> > > before) to see if it's a possible culprit.
> >> >
> >> > I just tested this with v2.13 and saw the same issue.  I don't actually
> >> > think this ever worked in the way you want it to Heiko.  Maybe git add
> >> > needs to be taught to be more intelligent when trying to add a submodule
> >> > which doesn't exist in the index.
> >>
> >> That was also my guess, since my feeling is that this is a quite rare
> >> use case. Adding submodules alone is not a daily thing, let alone
> >> selecting different changes after 'git submodule add'.
> >>
> >> I also think git could be more intelligent here.
> >
> > Ah.. the "submodule not registered in index" case. I think I remember
> > this (because I remember complaining about it once or two times).
> > Definitely agreed that git-add should do the right thing here.
>
> I am not sure if this even needs to be implemented as "look for the
> submodule in the index".  Even before submodule was added, we knew
> that "git add foo/bar" should reject the request if we find foo is a
> symbolic link, and we should do the same when foo/ is a directory
> that is the top of a working tree under control of another
> repository, no?

Exactly. I started with the intention to do something related to the
index only to slowly realize that it was not the right place. We
traverse directories and stop looking inside a symlink, we can do the
same if we realize it's a submodule.
-- 
Duy

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Tue, Jun 19, 2018 at 05:16:17PM +0200, Duy Nguyen wrote:
> No actually, we could do better. Let me see if I can come up with a
> patch or something...

OK. What we currently do is, when we search for potential untracked
paths for adding to the index, we unconditionally ignore anything
inside ".git". For example, if "foo" is a submodule, "git add ." will
visit "foo/.git" then ignore its content completely.

We could do something very similar: when we visit "foo", if "foo/.git"
exists, we ignore it as well. In other words, we extend from "ignore
anything inside a git repository" to "ignore anything inside any other
git worktree".

The following patch basically does that. If you specify "git add
foo/bar". It will still visit "foo" first, realize that it's a
submodule and drop it. At the end, it will not report foo/bar as an
untracked (i.e. add-able) entry, so you can't add it.

I didn't test it extensively to see if it breaks anything though. And
I might need to check how it affects untracked cache...

-- 8< --
diff --git a/dir.c b/dir.c
index fe9bf58e4c..8a1a5d8dd5 100644
--- a/dir.c
+++ b/dir.c
@@ -1672,6 +1672,17 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
 	if (dtype != DT_DIR && has_path_in_index)
 		return path_none;
 
+	if (dtype == DT_DIR) {
+		int path_len = path->len;
+		int is_submodule;
+
+		strbuf_addstr(path, "/.git");
+		is_submodule = is_directory(path->buf);
+		strbuf_setlen(path, path_len);
+		if (is_submodule)
+			return path_none;
+	}
+
 	/*
 	 * When we are looking at a directory P in the working tree,
 	 * there are three cases:
-- 8< --

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Tue, Jun 19, 2018 at 6:09 PM Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Jun 19, 2018 at 05:16:17PM +0200, Duy Nguyen wrote:
> > No actually, we could do better. Let me see if I can come up with a
> > patch or something...
>
> OK. What we currently do is, when we search for potential untracked
> paths for adding to the index, we unconditionally ignore anything
> inside ".git". For example, if "foo" is a submodule, "git add ." will
> visit "foo/.git" then ignore its content completely.
>
> We could do something very similar: when we visit "foo", if "foo/.git"
> exists, we ignore it as well. In other words, we extend from "ignore
> anything inside a git repository" to "ignore anything inside any other
> git worktree".
>
> The following patch basically does that. If you specify "git add
> foo/bar". It will still visit "foo" first, realize that it's a
> submodule and drop it. At the end, it will not report foo/bar as an
> untracked (i.e. add-able) entry, so you can't add it.

Another note (which I added, then thought otherwise and dropped). I
believe this approach also solves the problem that
die_path_inside_submodule() tries to work around.

When you feed a path inside a submodule, read_directory() code does
not realize it and walk through like it's part of the current worktree
(wrong!). But if read_directory() does the right thing from the
beginning, you don't need this trick. We don't even need this trick if
a submodule is not real on worktree (no ".git" directory there) but
registered in the index as a git link because the d/f check should
catch that and complain loudly anyway when you add a new entry.
-- 
Duy

Re: Adding nested repository with slash adds files instead of gitlink

[Kevin Daudt]

On Mon, Jun 18, 2018 at 01:19:19PM +0200, Heiko Voigt wrote:

Now with cc to the mailing list.

> Hi,
> 
> I just discovered that when you have a slash at the end of a nested
> repository, the files contained in the repository get added instead of
> the gitlink.
> 
> I found this when I was adding a submodule and wanted to commit a small
> change before that. You get the slash by using tab autocompletion.
> 
> Here is a recipe to reproduce:
> 
> mkdir test
> cd test; git init
> touch a; git add a; git commit -m a
> mkdir ../test.git; (cd ../test.git; git init --bare)
> git remote add origin ../test.git
> git push origin master
> git submodule add ../test.git submodule
> git reset
> git add submodule/
> 
> Now instead of just submodule gitlink there is an entry for submodule/a
> in the index.
> 
> I just thought I put this out there. Will have a look if I find the time
> to cook up a proper testcase and investigate.
> 
> Cheers Heiko

This has been the case as far as I can remember, and is basically lore
in the #git irc channel).

This can also be reproduced by just cloning a repo inside another repo
and running `git add path/`.

Re: Adding nested repository with slash adds files instead of gitlink

[Heiko Voigt]

Hi,

On Mon, Jun 18, 2018 at 05:55:44PM +0200, Kevin Daudt wrote:
> On Mon, Jun 18, 2018 at 01:19:19PM +0200, Heiko Voigt wrote:
> > I just discovered that when you have a slash at the end of a nested
> > repository, the files contained in the repository get added instead of
> > the gitlink.
[...]
> > 
> > I just thought I put this out there. Will have a look if I find the time
> > to cook up a proper testcase and investigate.
> > 
> > Cheers Heiko
> 
> This has been the case as far as I can remember, and is basically lore
> in the #git irc channel).
> 
> This can also be reproduced by just cloning a repo inside another repo
> and running `git add path/`.

Interesting and nobody complained to the mailinglist? IMO, there is no
reason 'git add path' and 'git add path/' should behave differently or
is there?

So it seems it is a very seldom operation (in my daily work it is at
least) and people just accepted it as a quirk of git.

If someone wants to look into changing this feel free, otherwise I will
have a look, but I am not sure when yet.

Cheers Heiko

Re: Adding nested repository with slash adds files instead of gitlink

[Rafael Ascensão]

On Tue, Jun 19, 2018 at 11:28 AM Heiko Voigt <hvoigt@hvoigt.net> wrote:
>
> Interesting and nobody complained to the mailinglist?
>

For reference this was sometimes called "Fake Submodules" online.

Re: Adding nested repository with slash adds files instead of gitlink

[Kevin Daudt]

On Tue, Jun 19, 2018 at 11:29:02PM +0100, Rafael Ascensão wrote:
> On Tue, Jun 19, 2018 at 11:28 AM Heiko Voigt <hvoigt@hvoigt.net> wrote:
> >
> > Interesting and nobody complained to the mailinglist?
> >
> 
> For reference this was sometimes called "Fake Submodules" online.

Do you refer to cloning a repository in another repository, and running
`git add path` (no trailing /) so that a gitlink is created without a
.gitmodules file? Because that's not what Heiko is referring to.

What this is about that when doing `git add path/` (with trailing /),
git will add the files of the cloned repository to the parent repo, and
basically ignore that it's another repository.

And I guess no one complained because it looked like a feature rather
than a bug.

Kevin

Re: Adding nested repository with slash adds files instead of gitlink

[Rafael Ascensão]

On Wed, Jun 20, 2018 at 5:39 AM Kevin Daudt <me@ikke.info> wrote:
>
> What this is about that when doing `git add path/` (with trailing /),
>

This is what I was referring to. If you search for 'Fake Submodules',
you'll see that some people were/are intentionally using this instead of
subtrees or submodules. Unfortunately the original article [1] seems to
be dead, but searching url in the mailing list archives leads to some
additional discussion on the subject [2,3].


[1]:http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb
[2]:https://public-inbox.org/git/xmqqy47o6q71.fsf@gitster.mtv.corp.google.com/
[3]:https://public-inbox.org/git/CAGZ79kZofg3jS+g0weTdco+PGo_p-_Hd-NScZ=q2UfB7tF2GPA@mail.gmail.com/

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Wed, Jun 20, 2018 at 1:55 PM Rafael Ascensão <rafa.almas@gmail.com> wrote:
>
> On Wed, Jun 20, 2018 at 5:39 AM Kevin Daudt <me@ikke.info> wrote:
> >
> > What this is about that when doing `git add path/` (with trailing /),
> >
>
> This is what I was referring to. If you search for 'Fake Submodules',
> you'll see that some people were/are intentionally using this instead of
> subtrees or submodules. Unfortunately the original article [1] seems to
> be dead, but searching url in the mailing list archives leads to some
> additional discussion on the subject [2,3].

Abusing a long standing bug does not make it a feature. I'm not
opposed to having a new option to keep that behavior, but it should
not be the default. If you use it that way, you're on your own.

> [1]:http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb
> [2]:https://public-inbox.org/git/xmqqy47o6q71.fsf@gitster.mtv.corp.google.com/
> [3]:https://public-inbox.org/git/CAGZ79kZofg3jS+g0weTdco+PGo_p-_Hd-NScZ=q2UfB7tF2GPA@mail.gmail.com/
-- 
Duy

Re: Adding nested repository with slash adds files instead of gitlink

[Rafael Ascensão]

On Tue, Jun 19, 2018 at 11:28 AM Heiko Voigt <hvoigt@hvoigt.net> wrote:
>
> Interesting and nobody complained to the mailinglist?
>

On Wed, Jun 20, 2018 at 3:57 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> Abusing a long standing bug does not make it a feature.
>

To make things clear, I wasn't defending if this should be considered a
bug or a feature. Was just clarifying that this isn't a newly discovered
thing and was reported/discussed in the past.

--
Cheers,
Rafael