Re: Adding nested repository with slash adds files instead of gitlink
On Tue, Jun 19, 2018 at 11:28 AM Heiko Voigt wrote: > > Interesting and nobody complained to the mailinglist? > On Wed, Jun 20, 2018 at 3:57 PM Duy Nguyen 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
Re: Adding nested repository with slash adds files instead of gitlink
On Wed, Jun 20, 2018 at 1:55 PM Rafael Ascensão wrote: > > On Wed, Jun 20, 2018 at 5:39 AM Kevin Daudt 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@gitster.mtv.corp.google.com/ > [3]:https://public-inbox.org/git/CAGZ79kZofg3jS+g0weTdco+PGo_p-_Hd-NScZ=q2ufb7tf2...@mail.gmail.com/ -- Duy
Re: Adding nested repository with slash adds files instead of gitlink
On Wed, Jun 20, 2018 at 5:39 AM Kevin Daudt 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@gitster.mtv.corp.google.com/ [3]:https://public-inbox.org/git/CAGZ79kZofg3jS+g0weTdco+PGo_p-_Hd-NScZ=q2ufb7tf2...@mail.gmail.com/
Re: Adding nested repository with slash adds files instead of gitlink
On Tue, Jun 19, 2018 at 11:28 AM Heiko Voigt 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
On Tue, Jun 19, 2018 at 6:09 PM Duy Nguyen 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
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
On Tue, Jun 19, 2018 at 5:56 PM Junio C Hamano wrote: > > Duy Nguyen writes: > > > On Tue, Jun 19, 2018 at 12:36 PM Heiko Voigt 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 writes: > On Tue, Jun 19, 2018 at 12:36 PM Heiko Voigt 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
On Tue, Jun 19, 2018 at 12:36 PM Heiko Voigt 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
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
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
On 06/18, Duy Nguyen wrote: > On Mon, Jun 18, 2018 at 1:23 PM Heiko Voigt 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
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
On Mon, Jun 18, 2018 at 1:23 PM Heiko Voigt 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
