{"thread":{"id":"46125","subject":"[BUG?] gitlink without .gitmodules no longer fails recursive clone","startedAt":"2017-06-06T03:57:00Z","lastAt":"2017-06-15T06:01:38Z","messageCount":23,"participants":["Jeff King","Stefan Beller","Brandon Williams","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"321594","messageId":"20170606035650.oykbz2uc4xkr3cr2@sigill.intra.peff.net","threadId":"46125","inReplyTo":null,"subject":"[BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-06T03:56:50Z","receivedAt":"2017-06-06T03:57:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While running some regression tests with v2.13, I noticed an odd\nbehavior. If I create a repository where there's a gitlink with no\nmatching .gitmodules entry:\n\n  git init repo\n  cd repo\n  n10=1234abcdef\n  n40=$n10$n10$n10$n10\n  git update-index --add --cacheinfo 160000 $n40 foo\n  git commit -m \"gitlink without .gitmodule entry\"\n\nand then I clone it recursively with v2.12, it fails:\n\n  $ git.v2.12.3 clone --recurse-submodules . dst; echo exit=$?\n  Cloning into 'dst'...\n  done.\n  fatal: No url found for submodule path 'foo' in .gitmodules\n  exit=128\n\nBut with v2.13, it silently ignores the submodule:\n\n  $ git.v2.13.1 clone --recurse-submodules . dst; echo exit=$?\n  Cloning into 'dst'...\n  done.\n  exit=0\n\nThis bisects to your bb62e0a99 (clone: teach --recurse-submodules to\noptionally take a pathspec, 2017-03-17). That patch just sets\nsubmodule.active by default, so I think the real issue is probably in\na086f921a (submodule: decouple url and submodule interest, 2017-03-17).\n\nI also wasn't sure if this might be intentional. I.e., that we'd just\nconsider gitlink entries which aren't even configured as not-submodules\nand ignore them. I couldn't certainly see an argument for moving in that\ndirection, but it is different than what we used to do. But I couldn't\nfind anything in any of the commit messages that mentioned this either\nway, so I figured I'd punt and ask. :)\n\n-Peff\n"},{"id":"321628","messageId":"CAGZ79kY-uzardfOvrJufatYgU9bqx4XZMU_GFq5zwc-vtzM-3Q@mail.gmail.com","threadId":"46125","inReplyTo":"20170606035650.oykbz2uc4xkr3cr2@sigill.intra.peff.net","subject":"Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-06T18:01:58Z","receivedAt":"2017-06-06T18:02:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 5, 2017 at 8:56 PM, Jeff King <peff@peff.net> wrote:\n> While running some regression tests with v2.13, I noticed an odd\n> behavior. If I create a repository where there's a gitlink with no\n> matching .gitmodules entry:\n>\n>   git init repo\n>   cd repo\n>   n10=1234abcdef\n>   n40=$n10$n10$n10$n10\n>   git update-index --add --cacheinfo 160000 $n40 foo\n>   git commit -m \"gitlink without .gitmodule entry\"\n>\n> and then I clone it recursively with v2.12, it fails:\n>\n>   $ git.v2.12.3 clone --recurse-submodules . dst; echo exit=$?\n>   Cloning into 'dst'...\n>   done.\n>   fatal: No url found for submodule path 'foo' in .gitmodules\n>   exit=128\n>\n> But with v2.13, it silently ignores the submodule:\n>\n>   $ git.v2.13.1 clone --recurse-submodules . dst; echo exit=$?\n>   Cloning into 'dst'...\n>   done.\n>   exit=0\n>\n> This bisects to your bb62e0a99 (clone: teach --recurse-submodules to\n> optionally take a pathspec, 2017-03-17). That patch just sets\n> submodule.active by default, so I think the real issue is probably in\n> a086f921a (submodule: decouple url and submodule interest, 2017-03-17).\n\nIt's a feature, not a bug, IMO.\n\nWhen starting out the journey to improve submodules, one of the major\nprinciple was to not interfere with gitlinks too much, as they are used in\nways git cannot fathom (cf git-series storing patches in gitlink form).\n\nAnd building on that: You asked for recursing into *submodules*, not\ninto *gitlinks*. And submodules in the new Git have stronger requirements\nw.r.t. the gitmodules file. (You have to tell us exactly how you want your\nsubmodule to be treated, and we do not want to half-ass guess around\nthe shortcomings of a user not telling us about the submodule)\n\n> I also wasn't sure if this might be intentional. I.e., that we'd just\n> consider gitlink entries which aren't even configured as not-submodules\n> and ignore them.\n\nI think this is what we want to do, and we should do it consistently.\nThe only downside for this is that more unintentional gitlinks may be\nadded to repositories as Git will be very good at ignoring them.\n\n> I couldn't certainly see an argument for moving in that\n> direction, but it is different than what we used to do. But I couldn't\n> find anything in any of the commit messages that mentioned this either\n> way, so I figured I'd punt and ask. :)\n\nYeah, yesterday we had a big discussion if we want to publish our\nroadmap and long term vision (as a team, as a company, or as a\ncommunity?) This would help newcomers and outsiders to see where\ne.g. submodules are headed and people could speak up early if we miss\ntheir use case.\n\nThanks for asking,\nStefan\n\n>\n> -Peff\n"},{"id":"321629","messageId":"20170606181024.GA189073@google.com","threadId":"46125","inReplyTo":"CAGZ79kY-uzardfOvrJufatYgU9bqx4XZMU_GFq5zwc-vtzM-3Q@mail.gmail.com","subject":"Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-06T18:10:24Z","receivedAt":"2017-06-06T18:10:37Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/06, Stefan Beller wrote:\n> On Mon, Jun 5, 2017 at 8:56 PM, Jeff King <peff@peff.net> wrote:\n> > While running some regression tests with v2.13, I noticed an odd\n> > behavior. If I create a repository where there's a gitlink with no\n> > matching .gitmodules entry:\n> >\n> >   git init repo\n> >   cd repo\n> >   n10=1234abcdef\n> >   n40=$n10$n10$n10$n10\n> >   git update-index --add --cacheinfo 160000 $n40 foo\n> >   git commit -m \"gitlink without .gitmodule entry\"\n> >\n> > and then I clone it recursively with v2.12, it fails:\n> >\n> >   $ git.v2.12.3 clone --recurse-submodules . dst; echo exit=$?\n> >   Cloning into 'dst'...\n> >   done.\n> >   fatal: No url found for submodule path 'foo' in .gitmodules\n> >   exit=128\n> >\n> > But with v2.13, it silently ignores the submodule:\n> >\n> >   $ git.v2.13.1 clone --recurse-submodules . dst; echo exit=$?\n> >   Cloning into 'dst'...\n> >   done.\n> >   exit=0\n> >\n> > This bisects to your bb62e0a99 (clone: teach --recurse-submodules to\n> > optionally take a pathspec, 2017-03-17). That patch just sets\n> > submodule.active by default, so I think the real issue is probably in\n> > a086f921a (submodule: decouple url and submodule interest, 2017-03-17).\n> \n> It's a feature, not a bug, IMO.\n> \n> When starting out the journey to improve submodules, one of the major\n> principle was to not interfere with gitlinks too much, as they are used in\n> ways git cannot fathom (cf git-series storing patches in gitlink form).\n> \n> And building on that: You asked for recursing into *submodules*, not\n> into *gitlinks*. And submodules in the new Git have stronger requirements\n> w.r.t. the gitmodules file. (You have to tell us exactly how you want your\n> submodule to be treated, and we do not want to half-ass guess around\n> the shortcomings of a user not telling us about the submodule)\n\nJust for some background on the new behavior and how this functionality\nchanged: My series changed how 'submodule init' behaved if you have\n'submodule.active' set.  Once set (like how clone --recurse does now)\nwhen not provided any path to a submodule, a list of 'active' submodules\nmatching the 'submodule.active' pathspec will be initialized.  One of\nthe requirements to be 'active' is to have an entry in the .gitmodules\nfile so gitlinks without an entry in the .gitmodules file will simply be\nignored now.\n\n> \n> > I also wasn't sure if this might be intentional. I.e., that we'd just\n> > consider gitlink entries which aren't even configured as not-submodules\n> > and ignore them.\n> \n> I think this is what we want to do, and we should do it consistently.\n> The only downside for this is that more unintentional gitlinks may be\n> added to repositories as Git will be very good at ignoring them.\n> \n> > I couldn't certainly see an argument for moving in that\n> > direction, but it is different than what we used to do. But I couldn't\n> > find anything in any of the commit messages that mentioned this either\n> > way, so I figured I'd punt and ask. :)\n> \n> Yeah, yesterday we had a big discussion if we want to publish our\n> roadmap and long term vision (as a team, as a company, or as a\n> community?) This would help newcomers and outsiders to see where\n> e.g. submodules are headed and people could speak up early if we miss\n> their use case.\n> \n> Thanks for asking,\n> Stefan\n> \n> >\n> > -Peff\n\n-- \nBrandon Williams\n"},{"id":"321635","messageId":"20170606183914.6iowfhimo5yrvmtf@sigill.intra.peff.net","threadId":"46125","inReplyTo":"20170606181024.GA189073@google.com","subject":"Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-06T18:39:15Z","receivedAt":"2017-06-06T18:41:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 06, 2017 at 11:10:24AM -0700, Brandon Williams wrote:\n\n> > > This bisects to your bb62e0a99 (clone: teach --recurse-submodules to\n> > > optionally take a pathspec, 2017-03-17). That patch just sets\n> > > submodule.active by default, so I think the real issue is probably in\n> > > a086f921a (submodule: decouple url and submodule interest, 2017-03-17).\n> > \n> > It's a feature, not a bug, IMO.\n> > \n> > When starting out the journey to improve submodules, one of the major\n> > principle was to not interfere with gitlinks too much, as they are used in\n> > ways git cannot fathom (cf git-series storing patches in gitlink form).\n> > \n> > And building on that: You asked for recursing into *submodules*, not\n> > into *gitlinks*. And submodules in the new Git have stronger requirements\n> > w.r.t. the gitmodules file. (You have to tell us exactly how you want your\n> > submodule to be treated, and we do not want to half-ass guess around\n> > the shortcomings of a user not telling us about the submodule)\n> \n> Just for some background on the new behavior and how this functionality\n> changed: My series changed how 'submodule init' behaved if you have\n> 'submodule.active' set.  Once set (like how clone --recurse does now)\n> when not provided any path to a submodule, a list of 'active' submodules\n> matching the 'submodule.active' pathspec will be initialized.  One of\n> the requirements to be 'active' is to have an entry in the .gitmodules\n> file so gitlinks without an entry in the .gitmodules file will simply be\n> ignored now.\n\nOK, thanks for the explanation. I certainly agree that treating\n.gitmodules as the source of truth is consistent and easy to explain.\nI'll update my test to handle the new behavior (it's a regression test\nfor how GitHub Pages handles some broken setups).\n\n-Peff\n"},{"id":"321860","messageId":"20170609231935.ysolxkiuhhpa3xrd@sigill.intra.peff.net","threadId":"46125","inReplyTo":"20170606183914.6iowfhimo5yrvmtf@sigill.intra.peff.net","subject":"Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-09T23:19:35Z","receivedAt":"2017-06-09T23:19:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 06, 2017 at 02:39:14PM -0400, Jeff King wrote:\n\n> > Just for some background on the new behavior and how this functionality\n> > changed: My series changed how 'submodule init' behaved if you have\n> > 'submodule.active' set.  Once set (like how clone --recurse does now)\n> > when not provided any path to a submodule, a list of 'active' submodules\n> > matching the 'submodule.active' pathspec will be initialized.  One of\n> > the requirements to be 'active' is to have an entry in the .gitmodules\n> > file so gitlinks without an entry in the .gitmodules file will simply be\n> > ignored now.\n> \n> OK, thanks for the explanation. I certainly agree that treating\n> .gitmodules as the source of truth is consistent and easy to explain.\n> I'll update my test to handle the new behavior (it's a regression test\n> for how GitHub Pages handles some broken setups).\n\nI've been chatting with Pages folks about the kinds of errors they see,\nand I was surprised how easy it is to get into this situation.\n\nIn an ideal world the user do:\n\n  git submodule add git://host/repo.git path\n\nwhich adds the gitlink and the .gitmodules entry. But it doesn't seem\nunreasonable for somebody unfamiliar with submodules to do:\n\n  git clone git://host/repo.git path\n  git add path\n\nThis does add the entry as a gitlink, but doesn't write any sort of\n.gitmodules entry. With the old code, cloning the repository (either by\nanother user, or in our case during a Pages build), a recursive clone or\nsubmodule init would complain loudly. But now it's just quietly ignored.\nWhich seems unfortunate.\n\nI actually don't think the old behavior was that great. Somebody _else_\nwould see the error and was responsible for filtering it back to the\noriginal author. But what we really want is to warn the user when\nthey're creating the broken situation.\n\nShould \"git add\" check whether there's a matching .gitmodules entry for\nthe path and issue a warning otherwise?\n\nSort of orthogonal, but I kind of wonder if the recursive clone should\nalso issue a warning. Not for an inactive submodule (which obviously is\nnormal) but for one that is inactive because it does not even have a\n.gitmodules entry. I suppose that _could_ be considered normal in a\npost-v2.13 world (e.g., if the owner of the repo \"deactivates\" a module\nby deleting its .gitmodules entry).  But since it's indistinguishable\nfrom the broken case, perhaps that would be better done with a specific\n\"active = false\" flag in .gitmodules.\n\n-Peff\n"},{"id":"321872","messageId":"xmqqbmpw4mpo.fsf@gitster.mtv.corp.google.com","threadId":"46125","inReplyTo":"20170609231935.ysolxkiuhhpa3xrd@sigill.intra.peff.net","subject":"Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-10T02:10:11Z","receivedAt":"2017-06-10T02:10:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> In an ideal world the user do:\n>\n>   git submodule add git://host/repo.git path\n>\n> which adds the gitlink and the .gitmodules entry. But it doesn't seem\n> unreasonable for somebody unfamiliar with submodules to do:\n>\n>   git clone git://host/repo.git path\n>   git add path\n>\n> This does add the entry as a gitlink, but doesn't write any sort of\n> .gitmodules entry.\n\nI actually would think that is a perfectly valid state.  In that\noriginal repository pair (i.e. the superproject with a submodule\nwithout an entry in .gitmodules), as long as the configuration in\nthe submodule repository \"path/.git/config\" has necessary remote\ndefinitions, \"git push/fetch --recursive\" etc., should also be able\nto work without having to consult .gitmodules at the top-level\nsuperproject, I would think.\n\n> With the old code, cloning the repository (either by\n> another user, or in our case during a Pages build), a recursive clone or\n> submodule init would complain loudly. But now it's just quietly ignored.\n> Which seems unfortunate.\n\nOf course, if such an original superproject gets pushed to a\npublishing location and then the result is cloned, without an entry\nin .gitmodules, no information \"git submodule\" can use to work on\nthat \"path\" exists in that clone.  I would say it is OK to leave it\nas-is when going \"--recursive\" (what you called \"inactive because\nit does not even have a .gitmodules entry).\n\nBut even in such a clone, once the user who cloned learns where the\nsubmodule commit that is recorded in the superproject's tree can be\nobtained out-of-band and makes a clone at \"path\" manually (which\nreplicates the state the original repository pair), things that only\nneed to look at \"path/.git/config\" should be able to work (e.g. \"git\nfetch --recursive\"), I'd say.\n\n\n"},{"id":"321890","messageId":"20170610071342.gdmpgjqlho2xfvdt@sigill.intra.peff.net","threadId":"46125","inReplyTo":"xmqqbmpw4mpo.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-10T07:13:43Z","receivedAt":"2017-06-10T07:13:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 10, 2017 at 11:10:11AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > In an ideal world the user do:\n> >\n> >   git submodule add git://host/repo.git path\n> >\n> > which adds the gitlink and the .gitmodules entry. But it doesn't seem\n> > unreasonable for somebody unfamiliar with submodules to do:\n> >\n> >   git clone git://host/repo.git path\n> >   git add path\n> >\n> > This does add the entry as a gitlink, but doesn't write any sort of\n> > .gitmodules entry.\n> \n> I actually would think that is a perfectly valid state.  In that\n> original repository pair (i.e. the superproject with a submodule\n> without an entry in .gitmodules), as long as the configuration in\n> the submodule repository \"path/.git/config\" has necessary remote\n> definitions, \"git push/fetch --recursive\" etc., should also be able\n> to work without having to consult .gitmodules at the top-level\n> superproject, I would think.\n\nCertainly I think it _could_ be a valid state. But traditionally it\ncaused \"clone --recursive\" to barf. And from what Stefan and Brandon\nhave said, we are moving in the opposite direction entirely, with\n.gitmodules becoming the source of truth.\n\nI could see arguments for going in either direction (gitlinks versus\n.gitmodules as source of truth for submodules). But it bothers me that\nit's so easy to create a state that will behave in a confusing manner.\n\n> But even in such a clone, once the user who cloned learns where the\n> submodule commit that is recorded in the superproject's tree can be\n> obtained out-of-band and makes a clone at \"path\" manually (which\n> replicates the state the original repository pair), things that only\n> need to look at \"path/.git/config\" should be able to work (e.g. \"git\n> fetch --recursive\"), I'd say.\n\nI agree that's possible. But I think one problem that our submodule\nsupport has traditionally suffered from is leaving open what is possible\nand not making any policy choices, or providing any guidance. Unless you\nare an expert user who plans on circumventing the usual \"git submodule\"\nprocess (and having your collaborators do so, too), I don't think you'd\never want to do a \"git add\" as above. You want \"git submodule add\".\n\nAllowing both is not in itself wrong.  But because we provide no\nguidance for the expert route, it's easy to shoot yourself in the foot\nand not even realize it. Which is why I suggested a warning and not\nforbidding the operation. Or better still, an advise() block that tells\nyou how to recover (probably \"git submodule add\", though it might need\nto learn to handle already-present gitlinks). And which can be disabled\nif you really are an expert user.\n\n-Peff\n"},{"id":"321912","messageId":"xmqqr2ys14h2.fsf@gitster.mtv.corp.google.com","threadId":"46125","inReplyTo":"20170610071342.gdmpgjqlho2xfvdt@sigill.intra.peff.net","subject":"Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-10T11:12:25Z","receivedAt":"2017-06-10T11:12:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Certainly I think it _could_ be a valid state. But traditionally it\n> caused \"clone --recursive\" to barf. And from what Stefan and Brandon\n> have said, we are moving in the opposite direction entirely, with\n> .gitmodules becoming the source of truth.\n> \n> I could see arguments for going in either direction (gitlinks versus\n> .gitmodules as source of truth for submodules). But it bothers me that\n> it's so easy to create a state that will behave in a confusing manner.\n> ...\n> Allowing both is not in itself wrong.  But because we provide no\n> guidance for the expert route, it's easy to shoot yourself in the foot\n> and not even realize it. Which is why I suggested a warning and not\n> forbidding the operation. Or better still, an advise() block that tells\n> you how to recover (probably \"git submodule add\", though it might need\n> to learn to handle already-present gitlinks). And which can be disabled\n> if you really are an expert user.\n\nYeah, the current situation is bad in that having both .gitmodules\nfile and gitlinks in a tree allows for redundant pieces of\ninformation to go out of sync and result in contradiction.  A gitlink\nwithout an entry in the .gitmodules file is an obvious example.  An\nentry in .gitmodules file whose path does not have a gitlink in the\ntree would also be a broken case.  \n\nWhile \"git submodule\" was the only/primary interface to the\nsubmodule, it was rather easy to explain that \".gitmodules need to\nbe consistent with the index and the tree iff you do 'git submodule'\nthing\" (implying \"if you only use gitlinks to bind two or more\nprojects locally without sharing with others, you do not need 'git\nsubmodule' and you do not need .gitmodules\"), but with the recent\npush to add \"--recurse-submodules\" to everything, the extent of what\nwas historically \"iff you do 'git submodule' thing\" is getting\nlarger and it becomes more and more important to keep .gitmodules\nconsistent with gitlinks in the tree that house it.\n\nI find it a bit sad, but I do not think of a better way X-<.\n\n\n"},{"id":"321946","messageId":"CAGZ79kZuwjQ1qYm5wLXXdMurwyAieWuLETV-Cx61ipO=QcsG+Q@mail.gmail.com","threadId":"46125","inReplyTo":"xmqqbmpw4mpo.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-12T05:30:53Z","receivedAt":"2017-06-12T05:31:00Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jun 9, 2017 at 7:10 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> In an ideal world the user do:\n>>\n>>   git submodule add git://host/repo.git path\n>>\n>> which adds the gitlink and the .gitmodules entry. But it doesn't seem\n>> unreasonable for somebody unfamiliar with submodules to do:\n>>\n>>   git clone git://host/repo.git path\n>>   git add path\n>>\n>> This does add the entry as a gitlink, but doesn't write any sort of\n>> .gitmodules entry.\n>\n> I actually would think that is a perfectly valid state.\n\nme too.\n\nBut on the other hand I do not want to offend non-submodule-gitlink-users\ntoo much. So maybe:\n\n  $ git add <gitlink>\n  # Adding a raw gitlink to the index,\n  # in case you want to use a submodule,\n  # use add a .gitmodules file or use 'git submodule add'\n\n> In that\n> original repository pair (i.e. the superproject with a submodule\n> without an entry in .gitmodules), as long as the configuration in\n> the submodule repository \"path/.git/config\" has necessary remote\n> definitions, \"git push/fetch --recursive\" etc., should also be able\n> to work without having to consult .gitmodules at the top-level\n> superproject, I would think.\n\nbut these are the 2nd step, clone fails first.\n\n>\n>> With the old code, cloning the repository (either by\n>> another user, or in our case during a Pages build), a recursive clone or\n>> submodule init would complain loudly. But now it's just quietly ignored.\n>> Which seems unfortunate.\n>\n> Of course, if such an original superproject gets pushed to a\n> publishing location and then the result is cloned, without an entry\n> in .gitmodules, no information \"git submodule\" can use to work on\n> that \"path\" exists in that clone.  I would say it is OK to leave it\n> as-is when going \"--recursive\" (what you called \"inactive because\n> it does not even have a .gitmodules entry).\n>\n> But even in such a clone, once the user who cloned learns where the\n> submodule commit that is recorded in the superproject's tree can be\n> obtained out-of-band and makes a clone at \"path\" manually (which\n> replicates the state the original repository pair), things that only\n> need to look at \"path/.git/config\" should be able to work (e.g. \"git\n> fetch --recursive\"), I'd say.\n\nBut the user would never learn, because\n\n  $ git clone --recurse-submodules ...\n\ndoes not complain, but put an empty dir instead.\n\n\n>\n>\n"},{"id":"322061","messageId":"20170613091452.ew2rwx5r6axxo65p@sigill.intra.peff.net","threadId":"46125","inReplyTo":"20170609231935.ysolxkiuhhpa3xrd@sigill.intra.peff.net","subject":"Re: [BUG?] gitlink without .gitmodules no longer fails recursive clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-13T09:14:52Z","receivedAt":"2017-06-13T09:14:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 09, 2017 at 07:19:35PM -0400, Jeff King wrote:\n\n> Should \"git add\" check whether there's a matching .gitmodules entry for\n> the path and issue a warning otherwise?\n\nHere's my attempt at that.\n\n  [1/2]: add: warn when adding an embedded repository\n  [2/2]: t: move \"git add submodule\" into test blocks\n\n Documentation/config.txt                     |  3 ++\n Documentation/git-add.txt                    |  7 +++++\n advice.c                                     |  2 ++\n advice.h                                     |  1 +\n builtin/add.c                                | 45 +++++++++++++++++++++++++++-\n git-submodule.sh                             |  5 ++--\n t/t4041-diff-submodule-option.sh             |  8 +++--\n t/t4060-diff-submodule-option-diff-format.sh |  8 +++--\n t/t7401-submodule-summary.sh                 |  8 +++--\n t/t7414-submodule-mistakes.sh                | 40 +++++++++++++++++++++++++\n 10 files changed, 115 insertions(+), 12 deletions(-)\n create mode 100755 t/t7414-submodule-mistakes.sh\n\n"},{"id":"322062","messageId":"20170613092408.db22ygki6wg2t23d@sigill.intra.peff.net","threadId":"46125","inReplyTo":"20170613091452.ew2rwx5r6axxo65p@sigill.intra.peff.net","subject":"[PATCH 1/2] add: warn when adding an embedded repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-13T09:24:08Z","receivedAt":"2017-06-13T09:24:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's an easy mistake to add a repository inside another\nrepository, like:\n\n  git clone $url\n  git add .\n\nThe resulting entry is a gitlink, but there's no matching\n.gitmodules entry. Trying to use \"submodule init\" (or clone\nwith --recursive) doesn't do anything useful. Prior to\nv2.13, such an entry caused git-submodule to barf entirely.\nIn v2.13, the entry is considered \"inactive\" and quietly\nignored. Either way, no clone of your repository can do\nanything useful with the gitlink without the user manually\nadding the submodule config.\n\nIn most cases, the user probably meant to either add a real\nsubmodule, or they forgot to put the embedded repository in\ntheir .gitignore file.\n\nLet's issue a warning when we see this case. There are a few\nthings to note:\n\n  - the warning will go in the git-add porcelain; anybody\n    wanting to do low-level manipulation of the index is\n    welcome to create whatever funny states they want.\n\n  - we detect the case by looking for a newly added gitlink;\n    updates via \"git add submodule\" are perfectly reasonable,\n    and this avoids us having to investigate .gitmodules\n    entirely\n\n  - there's a command-line option to suppress the warning.\n    This is needed for git-submodule itself (which adds the\n    entry before adding any submodule config), but also\n    provides a mechanism for other scripts doing\n    submodule-like things.\n\nWe could make this a hard error instead of a warning.\nHowever, we do add lots of sub-repos in our test suite. It's\nnot _wrong_ to do so. It just creates a state where users\nmay be surprised. Pointing them in the right direction with\na gentle hint is probably the best option.\n\nThere is a config knob that can disable the (long) hint. But\nI intentionally omitted a config knob to disable the warning\nentirely. Whether the warning is sensible or not is\ngenerally about context, not about the user's preferences.\nIf there's a tool or workflow that adds gitlinks without\nmatching .gitmodules, it should probably be taught about the\nnew command-line option, rather than blanket-disabling the\nwarning.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe check for \"is this a gitlink\" is done by looking for a\ntrailing \"/\" in the added path. This feels kind of hacky,\nbut actually seems to work well in practice. We've already\nexpanded the pathspecs to real filenames via dir.c, and that\nomits trees. So anything with a trailing slash must be a\ngitlink.\n\nAnd I really didn't want to incur any extra cost in the\ncommon case here (e.g., checking for \"path/.git\"). We could\ndo it at zero-cost by pushing the check much further down\n(i.e., when we'd realize anyway that it's a gitlink), but I\ndidn't want to pollute read-cache.c with what is essentially\na porcelain warning. The actual check done there seems to be\nchecking S_ISDIR, but I didn't even want to incur an extra\nstat per-file.\n\nI also waffled on whether we should ask the submodule code\nwhether it knows about a particular path. Technically:\n\n  git config submodule.foo.path foo\n  git config submodule.foo.url git://...\n  git add foo\n\nis legal, but would still warn with this patch. I don't know\nhow much we should care (it would also be easy to do on\ntop).\n\n Documentation/config.txt      |  3 +++\n Documentation/git-add.txt     |  7 +++++++\n advice.c                      |  2 ++\n advice.h                      |  1 +\n builtin/add.c                 | 45 ++++++++++++++++++++++++++++++++++++++++++-\n git-submodule.sh              |  5 +++--\n t/t7414-submodule-mistakes.sh | 40 ++++++++++++++++++++++++++++++++++++++\n 7 files changed, 100 insertions(+), 3 deletions(-)\n create mode 100755 t/t7414-submodule-mistakes.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex dd4beec39..e909239bc 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -348,6 +348,9 @@ advice.*::\n \trmHints::\n \t\tIn case of failure in the output of linkgit:git-rm[1],\n \t\tshow directions on how to proceed from the current state.\n+\taddEmbeddedRepo::\n+\t\tAdvice on what to do when you've accidentally added one\n+\t\tgit repo inside of another.\n --\n \n core.fileMode::\ndiff --git a/Documentation/git-add.txt b/Documentation/git-add.txt\nindex 7ed63dce0..f4169fb1e 100644\n--- a/Documentation/git-add.txt\n+++ b/Documentation/git-add.txt\n@@ -165,6 +165,13 @@ for \"git add --no-all <pathspec>...\", i.e. ignored removed files.\n \tbe ignored, no matter if they are already present in the work\n \ttree or not.\n \n+--no-warn-embedded-repo::\n+\tBy default, `git add` will warn when adding an embedded\n+\trepository to the index without using `git submodule add` to\n+\tcreate an entry in `.gitmodules`. This option will suppress the\n+\twarning (e.g., if you are manually performing operations on\n+\tsubmodules).\n+\n --chmod=(+|-)x::\n \tOverride the executable bit of the added files.  The executable\n \tbit is only changed in the index, the files on disk are left\ndiff --git a/advice.c b/advice.c\nindex b84ae4960..e0611d52b 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -15,6 +15,7 @@ int advice_detached_head = 1;\n int advice_set_upstream_failure = 1;\n int advice_object_name_warning = 1;\n int advice_rm_hints = 1;\n+int advice_add_embedded_repo = 1;\n \n static struct {\n \tconst char *name;\n@@ -35,6 +36,7 @@ static struct {\n \t{ \"setupstreamfailure\", &advice_set_upstream_failure },\n \t{ \"objectnamewarning\", &advice_object_name_warning },\n \t{ \"rmhints\", &advice_rm_hints },\n+\t{ \"addembeddedrepo\", &advice_add_embedded_repo },\n \n \t/* make this an alias for backward compatibility */\n \t{ \"pushnonfastforward\", &advice_push_update_rejected }\ndiff --git a/advice.h b/advice.h\nindex b341a55ce..c84a44531 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -18,6 +18,7 @@ extern int advice_detached_head;\n extern int advice_set_upstream_failure;\n extern int advice_object_name_warning;\n extern int advice_rm_hints;\n+extern int advice_add_embedded_repo;\n \n int git_default_advice_config(const char *var, const char *value);\n __attribute__((format (printf, 1, 2)))\ndiff --git a/builtin/add.c b/builtin/add.c\nindex d9a2491e4..ea88db281 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -249,6 +249,7 @@ N_(\"The following paths are ignored by one of your .gitignore files:\\n\");\n \n static int verbose, show_only, ignored_too, refresh_only;\n static int ignore_add_errors, intent_to_add, ignore_missing;\n+static int warn_on_embedded_repo = 1;\n \n #define ADDREMOVE_DEFAULT 1\n static int addremove = ADDREMOVE_DEFAULT;\n@@ -282,6 +283,8 @@ static struct option builtin_add_options[] = {\n \tOPT_BOOL( 0 , \"ignore-errors\", &ignore_add_errors, N_(\"just skip files which cannot be added because of errors\")),\n \tOPT_BOOL( 0 , \"ignore-missing\", &ignore_missing, N_(\"check if - even missing - files are ignored in dry run\")),\n \tOPT_STRING( 0 , \"chmod\", &chmod_arg, N_(\"(+/-)x\"), N_(\"override the executable bit of the listed files\")),\n+\tOPT_HIDDEN_BOOL(0, \"warn-embedded-repo\", &warn_on_embedded_repo,\n+\t\t\tN_(\"warn when adding an embedded repository\")),\n \tOPT_END(),\n };\n \n@@ -295,6 +298,44 @@ static int add_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n+static const char embedded_advice[] = N_(\n+\"You've added another git repository inside your current repository.\\n\"\n+\"Clones of the outer repository will not also contain the contents of\\n\"\n+\"the embedded repository. If you meant to add a submodule, use:\\n\"\n+\"\\n\"\n+\"\tgit submodule add <url> %s\\n\"\n+\"\\n\"\n+\"If you added this path by mistake, you can remove it from the\\n\"\n+\"index with:\\n\"\n+\"\\n\"\n+\"\tgit rm --cached %s\\n\"\n+\"\\n\"\n+\"See \\\"git help submodule\\\" for more information.\"\n+);\n+\n+static void check_embedded_repo(const char *path)\n+{\n+\tstruct strbuf name = STRBUF_INIT;\n+\n+\tif (!warn_on_embedded_repo)\n+\t\treturn;\n+\tif (!ends_with(path, \"/\"))\n+\t\treturn;\n+\n+\t/* Drop trailing slash for aesthetics */\n+\tstrbuf_addstr(&name, path);\n+\tstrbuf_strip_suffix(&name, \"/\");\n+\n+\twarning(_(\"adding embedded git repository: %s\"), name.buf);\n+\tif (advice_add_embedded_repo) {\n+\t\tadvise(embedded_advice, name.buf, name.buf);\n+\t\t/* there may be multiple entries; advise only once */\n+\t\tadvice_add_embedded_repo = 0;\n+\t}\n+\n+\tstrbuf_release(&name);\n+}\n+\n static int add_files(struct dir_struct *dir, int flags)\n {\n \tint i, exit_status = 0;\n@@ -307,12 +348,14 @@ static int add_files(struct dir_struct *dir, int flags)\n \t\texit_status = 1;\n \t}\n \n-\tfor (i = 0; i < dir->nr; i++)\n+\tfor (i = 0; i < dir->nr; i++) {\n+\t\tcheck_embedded_repo(dir->entries[i]->name);\n \t\tif (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {\n \t\t\tif (!ignore_add_errors)\n \t\t\t\tdie(_(\"adding files failed\"));\n \t\t\texit_status = 1;\n \t\t}\n+\t}\n \treturn exit_status;\n }\n \ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c0d0e9a4c..e131760ee 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -213,7 +213,8 @@ cmd_add()\n \t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index and is not a submodule\")\"\n \tfi\n \n-\tif test -z \"$force\" && ! git add --dry-run --ignore-missing \"$sm_path\" > /dev/null 2>&1\n+\tif test -z \"$force\" &&\n+\t\t! git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" > /dev/null 2>&1\n \tthen\n \t\teval_gettextln \"The following path is ignored by one of your .gitignore files:\n \\$sm_path\n@@ -267,7 +268,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n \tfi\n \tgit config submodule.\"$sm_name\".url \"$realrepo\"\n \n-\tgit add $force \"$sm_path\" ||\n+\tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n \tdie \"$(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n \n \tgit config -f .gitmodules submodule.\"$sm_name\".path \"$sm_path\" &&\ndiff --git a/t/t7414-submodule-mistakes.sh b/t/t7414-submodule-mistakes.sh\nnew file mode 100755\nindex 000000000..8059bcb7f\n--- /dev/null\n+++ b/t/t7414-submodule-mistakes.sh\n@@ -0,0 +1,40 @@\n+#!/bin/sh\n+\n+test_description='handling of common mistakes people may make with submodules'\n+. ./test-lib.sh\n+\n+test_expect_success 'create embedded repository' '\n+\tgit init embed &&\n+\t(\n+\t\tcd embed &&\n+\t\ttest_commit one\n+\t)\n+'\n+\n+test_expect_success 'git-add on embedded repository warns' '\n+\ttest_when_finished \"git rm --cached -f embed\" &&\n+\tgit add embed 2>stderr &&\n+\ttest_i18ngrep warning stderr\n+'\n+\n+test_expect_success '--no-warn-embedded-repo suppresses warning' '\n+\ttest_when_finished \"git rm --cached -f embed\" &&\n+\tgit add --no-warn-embedded-repo embed 2>stderr &&\n+\ttest_i18ngrep ! warning stderr\n+'\n+\n+test_expect_success 'no warning when updating entry' '\n+\ttest_when_finished \"git rm --cached -f embed\" &&\n+\tgit add embed &&\n+\tgit -C embed commit --allow-empty -m two &&\n+\tgit add embed 2>stderr &&\n+\ttest_i18ngrep ! warning stderr\n+'\n+\n+test_expect_success 'submodule add does not warn' '\n+\ttest_when_finished \"git rm -rf submodule .gitmodules\" &&\n+\tgit submodule add ./embed submodule 2>stderr &&\n+\ttest_i18ngrep ! warning stderr\n+'\n+\n+test_done\n-- \n2.13.1.675.g57c06d071\n\n"},{"id":"322063","messageId":"20170613092419.hzrtbn2jvykoxsry@sigill.intra.peff.net","threadId":"46125","inReplyTo":"20170613091452.ew2rwx5r6axxo65p@sigill.intra.peff.net","subject":"[PATCH 2/2] t: move \"git add submodule\" into test blocks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-13T09:24:20Z","receivedAt":"2017-06-13T09:24:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some submodule tests do some setup outside of a test_expect\nblock. This is bad because we won't actually check the\noutcome of those commands. But it's doubly so because \"git\nadd submodule\" now produces a warning to stderr, which is\nnot suppressed by the test scripts in non-verbose mode.\n\nThis patch does the minimal to fix the annoying warnings.\nAll three of these scripts could use more cleanup of related\nsetup.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t4041-diff-submodule-option.sh             | 8 +++++---\n t/t4060-diff-submodule-option-diff-format.sh | 8 +++++---\n t/t7401-submodule-summary.sh                 | 8 +++++---\n 3 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh\nindex 2d9731b52..058ee0829 100755\n--- a/t/t4041-diff-submodule-option.sh\n+++ b/t/t4041-diff-submodule-option.sh\n@@ -430,9 +430,11 @@ test_expect_success 'deleted submodule' '\n \ttest_cmp expected actual\n '\n \n-test_create_repo sm2 &&\n-head7=$(add_file sm2 foo8 foo9) &&\n-git add sm2\n+test_expect_success 'create second submodule' '\n+\ttest_create_repo sm2 &&\n+\thead7=$(add_file sm2 foo8 foo9) &&\n+\tgit add sm2\n+'\n \n test_expect_success 'multiple submodules' '\n \tgit diff-index -p --submodule=log HEAD >actual &&\ndiff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\nindex 33ec26d75..4b168d0ed 100755\n--- a/t/t4060-diff-submodule-option-diff-format.sh\n+++ b/t/t4060-diff-submodule-option-diff-format.sh\n@@ -643,9 +643,11 @@ test_expect_success 'deleted submodule' '\n \ttest_cmp expected actual\n '\n \n-test_create_repo sm2 &&\n-head7=$(add_file sm2 foo8 foo9) &&\n-git add sm2\n+test_expect_success 'create second submodule' '\n+\ttest_create_repo sm2 &&\n+\thead7=$(add_file sm2 foo8 foo9) &&\n+\tgit add sm2\n+'\n \n test_expect_success 'multiple submodules' '\n \tgit diff-index -p --submodule=diff HEAD >actual &&\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 366746f0d..4e4c45550 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -241,9 +241,11 @@ EOF\n \ttest_cmp expected actual\n \"\n \n-test_create_repo sm2 &&\n-head7=$(add_file sm2 foo8 foo9) &&\n-git add sm2\n+test_expect_success 'create second submodule' '\n+\ttest_create_repo sm2 &&\n+\thead7=$(add_file sm2 foo8 foo9) &&\n+\tgit add sm2\n+'\n \n test_expect_success 'multiple submodules' \"\n \tgit submodule summary >actual &&\n-- \n2.13.1.675.g57c06d071\n"},{"id":"322105","messageId":"CAGZ79kbbTwQicVkRs51fV91R_7ZhDtC+FR8Z-SQzRpF2cjFfag@mail.gmail.com","threadId":"46125","inReplyTo":"20170613092408.db22ygki6wg2t23d@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] add: warn when adding an embedded repository","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-13T17:07:43Z","receivedAt":"2017-06-13T17:07:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 13, 2017 at 2:24 AM, Jeff King <peff@peff.net> wrote:\n> It's an easy mistake to add a repository inside another\n> repository, like:\n>\n>   git clone $url\n>   git add .\n>\n> The resulting entry is a gitlink, but there's no matching\n> .gitmodules entry. Trying to use \"submodule init\" (or clone\n> with --recursive) doesn't do anything useful. Prior to\n> v2.13, such an entry caused git-submodule to barf entirely.\n> In v2.13, the entry is considered \"inactive\" and quietly\n> ignored. Either way, no clone of your repository can do\n> anything useful with the gitlink without the user manually\n> adding the submodule config.\n>\n> In most cases, the user probably meant to either add a real\n> submodule, or they forgot to put the embedded repository in\n> their .gitignore file.\n>\n> Let's issue a warning when we see this case. There are a few\n> things to note:\n>\n>   - the warning will go in the git-add porcelain; anybody\n>     wanting to do low-level manipulation of the index is\n>     welcome to create whatever funny states they want.\n>\n>   - we detect the case by looking for a newly added gitlink;\n>     updates via \"git add submodule\" are perfectly reasonable,\n>     and this avoids us having to investigate .gitmodules\n>     entirely\n>\n>   - there's a command-line option to suppress the warning.\n>     This is needed for git-submodule itself (which adds the\n>     entry before adding any submodule config), but also\n>     provides a mechanism for other scripts doing\n>     submodule-like things.\n>\n> We could make this a hard error instead of a warning.\n> However, we do add lots of sub-repos in our test suite. It's\n> not _wrong_ to do so. It just creates a state where users\n> may be surprised. Pointing them in the right direction with\n> a gentle hint is probably the best option.\n\nSounds good up to here (and right).\n\n> There is a config knob that can disable the (long) hint. But\n> I intentionally omitted a config knob to disable the warning\n> entirely. Whether the warning is sensible or not is\n> generally about context, not about the user's preferences.\n> If there's a tool or workflow that adds gitlinks without\n> matching .gitmodules, it should probably be taught about the\n> new command-line option, rather than blanket-disabling the\n> warning.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The check for \"is this a gitlink\" is done by looking for a\n> trailing \"/\" in the added path. This feels kind of hacky,\n> but actually seems to work well in practice.\n\nThis whole \"slash at the end\" thing comes from extensive use\nof shell completion adding the slash at the end of a directory\nIMHO. (cf. PATHSPEC_STRIP_SUBMODULE_SLASH_* is\nthe same underlying hack.)\n\n> We've already\n> expanded the pathspecs to real filenames via dir.c, and that\n> omits trees. So anything with a trailing slash must be a\n> gitlink.\n\nOh!\n\n>\n> And I really didn't want to incur any extra cost in the\n> common case here (e.g., checking for \"path/.git\"). We could\n> do it at zero-cost by pushing the check much further down\n> (i.e., when we'd realize anyway that it's a gitlink), but I\n> didn't want to pollute read-cache.c with what is essentially\n> a porcelain warning. The actual check done there seems to be\n> checking S_ISDIR, but I didn't even want to incur an extra\n> stat per-file.\n\nmakes sense.\n\n>\n> I also waffled on whether we should ask the submodule code\n> whether it knows about a particular path. Technically:\n>\n>   git config submodule.foo.path foo\n>   git config submodule.foo.url git://...\n>   git add foo\n>\n> is legal, but would still warn with this patch. I don't know\n> how much we should care (it would also be easy to do on\n> top).\n\nAnd here I was thinking this is not legal, because you may override\nanything *except* submodule.*.path in the config. That is because\nall the other settings (such as url, active flag, branch,\nshallow recommendation) are dependent on the use case, the user,\nchanges to the environment (url) or such. The name<->path mapping\nhowever is only to be changed via changes to the tracked content.\nThat is why it would make sense to disallow overriding the path\noutside the tracked content.\n\nIn my ideal dream world of submodules we would have the following:\n\n  $ cat .gitmodules\n  [submodule \"sub42\"]\n    path = foo\n  # path only in tree!\n\n  $ cat .git/config\n  ...\n  [submodule]\n    active = .\n    active = :(exclude)Irrelevant/submodules/for/my/usecase/*\n  # note how this is user centric\n\n  $ git show refs/meta/magic/for/refs/heads/master:.gitmodules\n  [submodule \"sub42\"]\n    url = https://example.org/foo\n    branch = .\n  # Note how this is neither centering on the in-tree\n  # contents, nor the user. Instead it focuses on the\n  # project or group. It is *workflow* centric.\n  # Workflows may change over time, e.g. the url could\n  # be repointed to k.org or an in-house mirror without tree\n  # changes.\n\n\nBut back to reviewing this patch.\n\n>\n>  Documentation/config.txt      |  3 +++\n>  Documentation/git-add.txt     |  7 +++++++\n>  advice.c                      |  2 ++\n>  advice.h                      |  1 +\n>  builtin/add.c                 | 45 ++++++++++++++++++++++++++++++++++++++++++-\n>  git-submodule.sh              |  5 +++--\n>  t/t7414-submodule-mistakes.sh | 40 ++++++++++++++++++++++++++++++++++++++\n>  7 files changed, 100 insertions(+), 3 deletions(-)\n>  create mode 100755 t/t7414-submodule-mistakes.sh\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index dd4beec39..e909239bc 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -348,6 +348,9 @@ advice.*::\n>         rmHints::\n>                 In case of failure in the output of linkgit:git-rm[1],\n>                 show directions on how to proceed from the current state.\n> +       addEmbeddedRepo::\n> +               Advice on what to do when you've accidentally added one\n> +               git repo inside of another.\n>  --\n>\n>  core.fileMode::\n> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt\n> index 7ed63dce0..f4169fb1e 100644\n> --- a/Documentation/git-add.txt\n> +++ b/Documentation/git-add.txt\n> @@ -165,6 +165,13 @@ for \"git add --no-all <pathspec>...\", i.e. ignored removed files.\n>         be ignored, no matter if they are already present in the work\n>         tree or not.\n>\n> +--no-warn-embedded-repo::\n> +       By default, `git add` will warn when adding an embedded\n> +       repository to the index without using `git submodule add` to\n> +       create an entry in `.gitmodules`. This option will suppress the\n> +       warning (e.g., if you are manually performing operations on\n> +       submodules).\n> +\n>  --chmod=(+|-)x::\n>         Override the executable bit of the added files.  The executable\n>         bit is only changed in the index, the files on disk are left\n> diff --git a/advice.c b/advice.c\n> index b84ae4960..e0611d52b 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -15,6 +15,7 @@ int advice_detached_head = 1;\n>  int advice_set_upstream_failure = 1;\n>  int advice_object_name_warning = 1;\n>  int advice_rm_hints = 1;\n> +int advice_add_embedded_repo = 1;\n>\n>  static struct {\n>         const char *name;\n> @@ -35,6 +36,7 @@ static struct {\n>         { \"setupstreamfailure\", &advice_set_upstream_failure },\n>         { \"objectnamewarning\", &advice_object_name_warning },\n>         { \"rmhints\", &advice_rm_hints },\n> +       { \"addembeddedrepo\", &advice_add_embedded_repo },\n>\n>         /* make this an alias for backward compatibility */\n>         { \"pushnonfastforward\", &advice_push_update_rejected }\n> diff --git a/advice.h b/advice.h\n> index b341a55ce..c84a44531 100644\n> --- a/advice.h\n> +++ b/advice.h\n> @@ -18,6 +18,7 @@ extern int advice_detached_head;\n>  extern int advice_set_upstream_failure;\n>  extern int advice_object_name_warning;\n>  extern int advice_rm_hints;\n> +extern int advice_add_embedded_repo;\n>\n>  int git_default_advice_config(const char *var, const char *value);\n>  __attribute__((format (printf, 1, 2)))\n> diff --git a/builtin/add.c b/builtin/add.c\n> index d9a2491e4..ea88db281 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -249,6 +249,7 @@ N_(\"The following paths are ignored by one of your .gitignore files:\\n\");\n>\n>  static int verbose, show_only, ignored_too, refresh_only;\n>  static int ignore_add_errors, intent_to_add, ignore_missing;\n> +static int warn_on_embedded_repo = 1;\n>\n>  #define ADDREMOVE_DEFAULT 1\n>  static int addremove = ADDREMOVE_DEFAULT;\n> @@ -282,6 +283,8 @@ static struct option builtin_add_options[] = {\n>         OPT_BOOL( 0 , \"ignore-errors\", &ignore_add_errors, N_(\"just skip files which cannot be added because of errors\")),\n>         OPT_BOOL( 0 , \"ignore-missing\", &ignore_missing, N_(\"check if - even missing - files are ignored in dry run\")),\n>         OPT_STRING( 0 , \"chmod\", &chmod_arg, N_(\"(+/-)x\"), N_(\"override the executable bit of the listed files\")),\n> +       OPT_HIDDEN_BOOL(0, \"warn-embedded-repo\", &warn_on_embedded_repo,\n> +                       N_(\"warn when adding an embedded repository\")),\n\nWe do not have a lot of OPT_HIDDEN_BOOLs throughout the code base.\nWe should use them more often.\n\nIt makes sense though in this case.\n\n>         OPT_END(),\n>  };\n>\n> @@ -295,6 +298,44 @@ static int add_config(const char *var, const char *value, void *cb)\n>         return git_default_config(var, value, cb);\n>  }\n>\n> +static const char embedded_advice[] = N_(\n> +\"You've added another git repository inside your current repository.\\n\"\n> +\"Clones of the outer repository will not also contain the contents of\\n\"\n> +\"the embedded repository. If you meant to add a submodule, use:\\n\"\n\nThe \"will not also\" sounds a bit off to me. Maybe:\n  ...\n  Clones of the outer repository will not contain the contents\n  of the embedded repository and has no way of knowing how\n  to obtain the inner repo. If you meant to add a submodule ...\n\n\n> +\"\\n\"\n> +\"      git submodule add <url> %s\\n\"\n> +\"\\n\"\n> +\"If you added this path by mistake, you can remove it from the\\n\"\n> +\"index with:\\n\"\n> +\"\\n\"\n> +\"      git rm --cached %s\\n\"\n> +\"\\n\"\n> +\"See \\\"git help submodule\\\" for more information.\"\n\nOnce the overhaul of the submodule documentation\ncomes along[1], we rather want to point at\n\"man 7 git-submodules\", which explains the concepts and\nthen tell you about commands how to use it. For now the\ngit-submodule man page is ok.\n\n[1] https://public-inbox.org/git/20170607185354.10050-1-sbeller@google.com/\n\n\n> +);\n> +\n> +static void check_embedded_repo(const char *path)\n> +{\n> +       struct strbuf name = STRBUF_INIT;\n> +\n> +       if (!warn_on_embedded_repo)\n> +               return;\n> +       if (!ends_with(path, \"/\"))\n> +               return;\n> +\n> +       /* Drop trailing slash for aesthetics */\n> +       strbuf_addstr(&name, path);\n> +       strbuf_strip_suffix(&name, \"/\");\n> +\n> +       warning(_(\"adding embedded git repository: %s\"), name.buf);\n> +       if (advice_add_embedded_repo) {\n> +               advise(embedded_advice, name.buf, name.buf);\n> +               /* there may be multiple entries; advise only once */\n> +               advice_add_embedded_repo = 0;\n> +       }\n> +\n> +       strbuf_release(&name);\n> +}\n> +\n>  static int add_files(struct dir_struct *dir, int flags)\n>  {\n>         int i, exit_status = 0;\n> @@ -307,12 +348,14 @@ static int add_files(struct dir_struct *dir, int flags)\n>                 exit_status = 1;\n>         }\n>\n> -       for (i = 0; i < dir->nr; i++)\n> +       for (i = 0; i < dir->nr; i++) {\n> +               check_embedded_repo(dir->entries[i]->name);\n>                 if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {\n>                         if (!ignore_add_errors)\n>                                 die(_(\"adding files failed\"));\n>                         exit_status = 1;\n>                 }\n> +       }\n>         return exit_status;\n>  }\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index c0d0e9a4c..e131760ee 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -213,7 +213,8 @@ cmd_add()\n>                 die \"$(eval_gettext \"'\\$sm_path' already exists in the index and is not a submodule\")\"\n>         fi\n>\n> -       if test -z \"$force\" && ! git add --dry-run --ignore-missing \"$sm_path\" > /dev/null 2>&1\n> +       if test -z \"$force\" &&\n> +               ! git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" > /dev/null 2>&1\n>         then\n>                 eval_gettextln \"The following path is ignored by one of your .gitignore files:\n>  \\$sm_path\n> @@ -267,7 +268,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n>         fi\n>         git config submodule.\"$sm_name\".url \"$realrepo\"\n>\n> -       git add $force \"$sm_path\" ||\n> +       git add --no-warn-embedded-repo $force \"$sm_path\" ||\n>         die \"$(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n>\n>         git config -f .gitmodules submodule.\"$sm_name\".path \"$sm_path\" &&\n> diff --git a/t/t7414-submodule-mistakes.sh b/t/t7414-submodule-mistakes.sh\n> new file mode 100755\n> index 000000000..8059bcb7f\n> --- /dev/null\n> +++ b/t/t7414-submodule-mistakes.sh\n> @@ -0,0 +1,40 @@\n> +#!/bin/sh\n> +\n> +test_description='handling of common mistakes people may make with submodules'\n\nThat is one way to say it. Do we have other tests for\n\"you think it is a bug, but it is features\" ? ;)\nI like it though. :)\n\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'create embedded repository' '\n> +       git init embed &&\n> +       (\n> +               cd embed &&\n> +               test_commit one\n> +       )\n\nshorter via:\n\n  test_create_repo embed &&\n  test_commit -C embed one\n\n(and saves a shell IIRC)\n\n\n> +test_expect_success 'git-add on embedded repository warns' '\n> +       test_when_finished \"git rm --cached -f embed\" &&\n> +       git add embed 2>stderr &&\n> +       test_i18ngrep warning stderr\n> +'\n> +\n> +test_expect_success '--no-warn-embedded-repo suppresses warning' '\n> +       test_when_finished \"git rm --cached -f embed\" &&\n> +       git add --no-warn-embedded-repo embed 2>stderr &&\n> +       test_i18ngrep ! warning stderr\n> +'\n> +\n> +test_expect_success 'no warning when updating entry' '\n> +       test_when_finished \"git rm --cached -f embed\" &&\n> +       git add embed &&\n> +       git -C embed commit --allow-empty -m two &&\n> +       git add embed 2>stderr &&\n> +       test_i18ngrep ! warning stderr\n> +'\n> +\n> +test_expect_success 'submodule add does not warn' '\n> +       test_when_finished \"git rm -rf submodule .gitmodules\" &&\n> +       git submodule add ./embed submodule 2>stderr &&\n> +       test_i18ngrep ! warning stderr\n> +'\n\nThanks for these tests.\n\nThis patch looks good to me, apart from the perceived wording nits.\n\nThanks,\nStefan\n\n> +\n> +test_done\n> --\n> 2.13.1.675.g57c06d071\n>\n"},{"id":"322109","messageId":"CAGZ79kaHXwEBJN-D6u+Zj4-Df-tMKBHz3S2uuOuH9mH5b6jd_A@mail.gmail.com","threadId":"46125","inReplyTo":"20170613092419.hzrtbn2jvykoxsry@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] t: move \"git add submodule\" into test blocks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-13T17:15:36Z","receivedAt":"2017-06-13T17:15:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 13, 2017 at 2:24 AM, Jeff King <peff@peff.net> wrote:\n> Some submodule tests do some setup outside of a test_expect\n> block. This is bad because we won't actually check the\n> outcome of those commands. But it's doubly so because \"git\n> add submodule\" now produces a warning to stderr, which is\n> not suppressed by the test scripts in non-verbose mode.\n\nMakes sense.\n\n> This patch does the minimal to fix the annoying warnings.\n> All three of these scripts could use more cleanup of related\n> setup.\n\nagreed.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nReviewed-by: Stefan Beller <sbeller@google.com>\n"},{"id":"322110","messageId":"20170613171608.GN154599@google.com","threadId":"46125","inReplyTo":"CAGZ79kbbTwQicVkRs51fV91R_7ZhDtC+FR8Z-SQzRpF2cjFfag@mail.gmail.com","subject":"Re: [PATCH 1/2] add: warn when adding an embedded repository","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-13T17:16:08Z","receivedAt":"2017-06-13T17:16:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/13, Stefan Beller wrote:\n> On Tue, Jun 13, 2017 at 2:24 AM, Jeff King <peff@peff.net> wrote:\n> \n> > There is a config knob that can disable the (long) hint. But\n> > I intentionally omitted a config knob to disable the warning\n> > entirely. Whether the warning is sensible or not is\n> > generally about context, not about the user's preferences.\n> > If there's a tool or workflow that adds gitlinks without\n> > matching .gitmodules, it should probably be taught about the\n> > new command-line option, rather than blanket-disabling the\n> > warning.\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > The check for \"is this a gitlink\" is done by looking for a\n> > trailing \"/\" in the added path. This feels kind of hacky,\n> > but actually seems to work well in practice.\n> \n> This whole \"slash at the end\" thing comes from extensive use\n> of shell completion adding the slash at the end of a directory\n> IMHO. (cf. PATHSPEC_STRIP_SUBMODULE_SLASH_* is\n> the same underlying hack.)\n\nI got rid of PATHSPEC_STRIP_SUBMODULE_SLASH_* recently, just an fyi.\n\n-- \nBrandon Williams\n"},{"id":"322111","messageId":"xmqqshj37qqo.fsf@gitster.mtv.corp.google.com","threadId":"46125","inReplyTo":"20170613092408.db22ygki6wg2t23d@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] add: warn when adding an embedded repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-13T17:16:15Z","receivedAt":"2017-06-13T17:17:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I also waffled on whether we should ask the submodule code\n> whether it knows about a particular path. Technically:\n>\n>   git config submodule.foo.path foo\n>   git config submodule.foo.url git://...\n>   git add foo\n>\n> is legal, but would still warn with this patch. \n\nDid you mean \"git config -f .gitmodules\" for the first two?  If so,\nI think I agree that (1) it should be legal and (2) we should be\nable to add the check on top of this patch.\n\nOr do you really mean the case in which these are in .git/config?\n"},{"id":"322195","messageId":"20170614063614.a34ovimjpz2g24qe@sigill.intra.peff.net","threadId":"46125","inReplyTo":"CAGZ79kbbTwQicVkRs51fV91R_7ZhDtC+FR8Z-SQzRpF2cjFfag@mail.gmail.com","subject":"Re: [PATCH 1/2] add: warn when adding an embedded repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T06:36:14Z","receivedAt":"2017-06-14T06:36:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 10:07:43AM -0700, Stefan Beller wrote:\n\n> > I also waffled on whether we should ask the submodule code\n> > whether it knows about a particular path. Technically:\n> >\n> >   git config submodule.foo.path foo\n> >   git config submodule.foo.url git://...\n> >   git add foo\n> >\n> > is legal, but would still warn with this patch. I don't know\n> > how much we should care (it would also be easy to do on\n> > top).\n> \n> And here I was thinking this is not legal, because you may override\n> anything *except* submodule.*.path in the config. That is because\n> all the other settings (such as url, active flag, branch,\n> shallow recommendation) are dependent on the use case, the user,\n> changes to the environment (url) or such. The name<->path mapping\n> however is only to be changed via changes to the tracked content.\n> That is why it would make sense to disallow overriding the path\n> outside the tracked content.\n\nIt was probably a mistake to use normal config as the example. Junio\nmentioned it as a case that could work if you communicate the submodule\nURL to somebody else out-of-band. My understanding was that you could\nset whatever you like in the regular config, but I think that is just\nshowing my ignorance of submodules.\n\nPretend like I said \"-f .gitmodules\" in each line above. ;)\n\n> In my ideal dream world of submodules we would have the following:\n> \n>   $ cat .gitmodules\n>   [submodule \"sub42\"]\n>     path = foo\n>   # path only in tree!\n\nTBH, I am not sure why we need \"path\"; couldn't we just use the\nsubsection name as an implicit path?\n\n> > +       OPT_HIDDEN_BOOL(0, \"warn-embedded-repo\", &warn_on_embedded_repo,\n> > +                       N_(\"warn when adding an embedded repository\")),\n> \n> We do not have a lot of OPT_HIDDEN_BOOLs throughout the code base.\n> We should use them more often.\n> \n> It makes sense though in this case.\n\nActually, my main reason is that it's nonsense to show\n\"--warn-embedded-repo\" in the help, when it's already the default. I\nwould like to have written:\n\n  OPT_NEGBOOL(0, \"no-warn-embedded-repo\", &warn_on_embedded_repo,\n\t\tN_(\"disable warning when adding an embedded repository\"))\n\nbut we don't have such a thing (and the last discussion on it a few\nmonths ago left a lot of open questions). So given that this really\nisn't something I'd expect users to want, I figured hiding it was a good\nidea. I mentioned it in the manpage for script writers, but it's really\nnot worth cluttering \"git add -h\".\n\n> > +static const char embedded_advice[] = N_(\n> > +\"You've added another git repository inside your current repository.\\n\"\n> > +\"Clones of the outer repository will not also contain the contents of\\n\"\n> > +\"the embedded repository. If you meant to add a submodule, use:\\n\"\n> \n> The \"will not also\" sounds a bit off to me. Maybe:\n>   ...\n>   Clones of the outer repository will not contain the contents\n>   of the embedded repository and has no way of knowing how\n>   to obtain the inner repo. If you meant to add a submodule ...\n\nYeah, I think we could just strike the \"also\" (I played around with the\nwording here quite a bit and I think it was left from an earlier attempt\nwhere it made more sense).\n\nYour \"no way of knowing\" is probably a good thing to mention.\n\n> > +\"See \\\"git help submodule\\\" for more information.\"\n> \n> Once the overhaul of the submodule documentation\n> comes along[1], we rather want to point at\n> \"man 7 git-submodules\", which explains the concepts and\n> then tell you about commands how to use it. For now the\n> git-submodule man page is ok.\n> \n> [1] https://public-inbox.org/git/20170607185354.10050-1-sbeller@google.com/\n\nYeah, I poked around looking for a definitive \"here's how submodules\nwork\" intro. I'm happy one is in the works, and I agree this should\npoint there once it exists.\n\n> > +++ b/t/t7414-submodule-mistakes.sh\n> > @@ -0,0 +1,40 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='handling of common mistakes people may make with submodules'\n> \n> That is one way to say it. Do we have other tests for\n> \"you think it is a bug, but it is features\" ? ;)\n> I like it though. :)\n\nHeh. I didn't know how else to lump it together. Just \"test git add on a\nrepository\" felt like too little for its own script. I almost added it\nto t7400, but I think that script is plenty long enough as it is (it's\nalso one of the longest-running scripts, I think).\n\n> > +test_expect_success 'create embedded repository' '\n> > +       git init embed &&\n> > +       (\n> > +               cd embed &&\n> > +               test_commit one\n> > +       )\n> \n> shorter via:\n> \n>   test_create_repo embed &&\n>   test_commit -C embed one\n> \n> (and saves a shell IIRC)\n\nRight, I forgot we added -C there. Will change.\n\n> Thanks for these tests.\n> \n> This patch looks good to me, apart from the perceived wording nits.\n\nThanks. I'll re-roll with a few tweaks based on your feedback.\n\n-Peff\n"},{"id":"322196","messageId":"20170614063849.kkdzxp54r25l2wk6@sigill.intra.peff.net","threadId":"46125","inReplyTo":"xmqqshj37qqo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] add: warn when adding an embedded repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T06:38:49Z","receivedAt":"2017-06-14T06:38:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 10:16:15AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I also waffled on whether we should ask the submodule code\n> > whether it knows about a particular path. Technically:\n> >\n> >   git config submodule.foo.path foo\n> >   git config submodule.foo.url git://...\n> >   git add foo\n> >\n> > is legal, but would still warn with this patch. \n> \n> Did you mean \"git config -f .gitmodules\" for the first two?  If so,\n> I think I agree that (1) it should be legal and (2) we should be\n> able to add the check on top of this patch.\n> \n> Or do you really mean the case in which these are in .git/config?\n\nI did mean .git/config, but I think it was because I was mostly confused\nabout what was possible. In either case I think what we'd want is to ask\n\"could this be used as an active submodule\". And leave it to the\nsubmodule code to decide whatever mechanisms are legal.\n\n-Peff\n"},{"id":"322230","messageId":"20170614105437.cgdlykmtocdj5vzw@sigill.intra.peff.net","threadId":"46125","inReplyTo":"20170614063614.a34ovimjpz2g24qe@sigill.intra.peff.net","subject":"[PATCH v2 0/2] warn when adding an embedded repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T10:54:37Z","receivedAt":"2017-06-14T10:54:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 14, 2017 at 02:36:14AM -0400, Jeff King wrote:\n\n> > This patch looks good to me, apart from the perceived wording nits.\n> \n> Thanks. I'll re-roll with a few tweaks based on your feedback.\n\nHere it is. It just changes the wording and fixes the test as you\nsuggested.\n\n  [1/2]: add: warn when adding an embedded repository\n  [2/2]: t: move \"git add submodule\" into test blocks\n\nI had some thoughts on adding a third patch to cover the case where\n.gitmodules has already been updated. But I couldn't figure out how to\nmake it work. The patch I came up with is below:\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex d7cab1bd8..d20052462 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -322,6 +322,8 @@ static void check_embedded_repo(const char *path)\n \t\treturn;\n \tif (!ends_with(path, \"/\"))\n \t\treturn;\n+\tif (is_submodule_initialized(path))\n+\t\treturn;\n \n \t/* Drop trailing slash for aesthetics */\n \tstrbuf_addstr(&name, path);\ndiff --git a/t/t7414-submodule-mistakes.sh b/t/t7414-submodule-mistakes.sh\nindex f2e7df59c..aee52b218 100755\n--- a/t/t7414-submodule-mistakes.sh\n+++ b/t/t7414-submodule-mistakes.sh\n@@ -34,4 +34,14 @@ test_expect_success 'submodule add does not warn' '\n \ttest_i18ngrep ! warning stderr\n '\n \n+test_expect_success 'making your own submodule does not warn' '\n+\ttest_when_finished \"git rm --cached -f embed &&\n+\t\t\t    git rm -f .gitmodules\" &&\n+\tgit config -f .gitmodules submodule.embed.path embed &&\n+\tgit config -f .gitmodules submodule.embed.url \\\n+\t\t\t\t  git://example.com/embed.git &&\n+\tgit add embed 2>stderr &&\n+\ttest_i18ngrep ! warning stderr\n+'\n+\n test_done\n\nBut that doesn't quite work. Because the submodule data is just in\n.gitmodules, it's not actually \"initialized\". I think that might work if\ninstead it was:\n\n  git config submodule.embed.url ...\n\nbut I'm not clear on whether that should work, or if it's even something\nsomebody would want to do. So I decided to punt on the whole thing and\nlet somebody work on it who a) knows more about how submodules work or\nb) has some not-quite-submodule use case for gitlinks that they want to\nhave work (without a warning).\n\n-Peff\n"},{"id":"322232","messageId":"20170614105821.mg3465itzvqiginz@sigill.intra.peff.net","threadId":"46125","inReplyTo":"20170614105437.cgdlykmtocdj5vzw@sigill.intra.peff.net","subject":"[PATCH v2 1/2] add: warn when adding an embedded repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T10:58:22Z","receivedAt":"2017-06-14T10:58:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's an easy mistake to add a repository inside another\nrepository, like:\n\n  git clone $url\n  git add .\n\nThe resulting entry is a gitlink, but there's no matching\n.gitmodules entry. Trying to use \"submodule init\" (or clone\nwith --recursive) doesn't do anything useful. Prior to\nv2.13, such an entry caused git-submodule to barf entirely.\nIn v2.13, the entry is considered \"inactive\" and quietly\nignored. Either way, no clone of your repository can do\nanything useful with the gitlink without the user manually\nadding the submodule config.\n\nIn most cases, the user probably meant to either add a real\nsubmodule, or they forgot to put the embedded repository in\ntheir .gitignore file.\n\nLet's issue a warning when we see this case. There are a few\nthings to note:\n\n  - the warning will go in the git-add porcelain; anybody\n    wanting to do low-level manipulation of the index is\n    welcome to create whatever funny states they want.\n\n  - we detect the case by looking for a newly added gitlink;\n    updates via \"git add submodule\" are perfectly reasonable,\n    and this avoids us having to investigate .gitmodules\n    entirely\n\n  - there's a command-line option to suppress the warning.\n    This is needed for git-submodule itself (which adds the\n    entry before adding any submodule config), but also\n    provides a mechanism for other scripts doing\n    submodule-like things.\n\nWe could make this a hard error instead of a warning.\nHowever, we do add lots of sub-repos in our test suite. It's\nnot _wrong_ to do so. It just creates a state where users\nmay be surprised. Pointing them in the right direction with\na gentle hint is probably the best option.\n\nThere is a config knob that can disable the (long) hint. But\nI intentionally omitted a config knob to disable the warning\nentirely. Whether the warning is sensible or not is\ngenerally about context, not about the user's preferences.\nIf there's a tool or workflow that adds gitlinks without\nmatching .gitmodules, it should probably be taught about the\nnew command-line option, rather than blanket-disabling the\nwarning.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt      |  3 +++\n Documentation/git-add.txt     |  7 +++++++\n advice.c                      |  2 ++\n advice.h                      |  1 +\n builtin/add.c                 | 46 ++++++++++++++++++++++++++++++++++++++++++-\n git-submodule.sh              |  5 +++--\n t/t7414-submodule-mistakes.sh | 37 ++++++++++++++++++++++++++++++++++\n 7 files changed, 98 insertions(+), 3 deletions(-)\n create mode 100755 t/t7414-submodule-mistakes.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f6278a5ae..781ce3e85 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -348,6 +348,9 @@ advice.*::\n \trmHints::\n \t\tIn case of failure in the output of linkgit:git-rm[1],\n \t\tshow directions on how to proceed from the current state.\n+\taddEmbeddedRepo::\n+\t\tAdvice on what to do when you've accidentally added one\n+\t\tgit repo inside of another.\n --\n \n core.fileMode::\ndiff --git a/Documentation/git-add.txt b/Documentation/git-add.txt\nindex 7ed63dce0..f4169fb1e 100644\n--- a/Documentation/git-add.txt\n+++ b/Documentation/git-add.txt\n@@ -165,6 +165,13 @@ for \"git add --no-all <pathspec>...\", i.e. ignored removed files.\n \tbe ignored, no matter if they are already present in the work\n \ttree or not.\n \n+--no-warn-embedded-repo::\n+\tBy default, `git add` will warn when adding an embedded\n+\trepository to the index without using `git submodule add` to\n+\tcreate an entry in `.gitmodules`. This option will suppress the\n+\twarning (e.g., if you are manually performing operations on\n+\tsubmodules).\n+\n --chmod=(+|-)x::\n \tOverride the executable bit of the added files.  The executable\n \tbit is only changed in the index, the files on disk are left\ndiff --git a/advice.c b/advice.c\nindex b84ae4960..e0611d52b 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -15,6 +15,7 @@ int advice_detached_head = 1;\n int advice_set_upstream_failure = 1;\n int advice_object_name_warning = 1;\n int advice_rm_hints = 1;\n+int advice_add_embedded_repo = 1;\n \n static struct {\n \tconst char *name;\n@@ -35,6 +36,7 @@ static struct {\n \t{ \"setupstreamfailure\", &advice_set_upstream_failure },\n \t{ \"objectnamewarning\", &advice_object_name_warning },\n \t{ \"rmhints\", &advice_rm_hints },\n+\t{ \"addembeddedrepo\", &advice_add_embedded_repo },\n \n \t/* make this an alias for backward compatibility */\n \t{ \"pushnonfastforward\", &advice_push_update_rejected }\ndiff --git a/advice.h b/advice.h\nindex b341a55ce..c84a44531 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -18,6 +18,7 @@ extern int advice_detached_head;\n extern int advice_set_upstream_failure;\n extern int advice_object_name_warning;\n extern int advice_rm_hints;\n+extern int advice_add_embedded_repo;\n \n int git_default_advice_config(const char *var, const char *value);\n __attribute__((format (printf, 1, 2)))\ndiff --git a/builtin/add.c b/builtin/add.c\nindex d9a2491e4..d7cab1bd8 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -249,6 +249,7 @@ N_(\"The following paths are ignored by one of your .gitignore files:\\n\");\n \n static int verbose, show_only, ignored_too, refresh_only;\n static int ignore_add_errors, intent_to_add, ignore_missing;\n+static int warn_on_embedded_repo = 1;\n \n #define ADDREMOVE_DEFAULT 1\n static int addremove = ADDREMOVE_DEFAULT;\n@@ -282,6 +283,8 @@ static struct option builtin_add_options[] = {\n \tOPT_BOOL( 0 , \"ignore-errors\", &ignore_add_errors, N_(\"just skip files which cannot be added because of errors\")),\n \tOPT_BOOL( 0 , \"ignore-missing\", &ignore_missing, N_(\"check if - even missing - files are ignored in dry run\")),\n \tOPT_STRING( 0 , \"chmod\", &chmod_arg, N_(\"(+/-)x\"), N_(\"override the executable bit of the listed files\")),\n+\tOPT_HIDDEN_BOOL(0, \"warn-embedded-repo\", &warn_on_embedded_repo,\n+\t\t\tN_(\"warn when adding an embedded repository\")),\n \tOPT_END(),\n };\n \n@@ -295,6 +298,45 @@ static int add_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n+static const char embedded_advice[] = N_(\n+\"You've added another git repository inside your current repository.\\n\"\n+\"Clones of the outer repository will not contain the contents of\\n\"\n+\"the embedded repository and will not know how to obtain it.\\n\"\n+\"If you meant to add a submodule, use:\\n\"\n+\"\\n\"\n+\"\tgit submodule add <url> %s\\n\"\n+\"\\n\"\n+\"If you added this path by mistake, you can remove it from the\\n\"\n+\"index with:\\n\"\n+\"\\n\"\n+\"\tgit rm --cached %s\\n\"\n+\"\\n\"\n+\"See \\\"git help submodule\\\" for more information.\"\n+);\n+\n+static void check_embedded_repo(const char *path)\n+{\n+\tstruct strbuf name = STRBUF_INIT;\n+\n+\tif (!warn_on_embedded_repo)\n+\t\treturn;\n+\tif (!ends_with(path, \"/\"))\n+\t\treturn;\n+\n+\t/* Drop trailing slash for aesthetics */\n+\tstrbuf_addstr(&name, path);\n+\tstrbuf_strip_suffix(&name, \"/\");\n+\n+\twarning(_(\"adding embedded git repository: %s\"), name.buf);\n+\tif (advice_add_embedded_repo) {\n+\t\tadvise(embedded_advice, name.buf, name.buf);\n+\t\t/* there may be multiple entries; advise only once */\n+\t\tadvice_add_embedded_repo = 0;\n+\t}\n+\n+\tstrbuf_release(&name);\n+}\n+\n static int add_files(struct dir_struct *dir, int flags)\n {\n \tint i, exit_status = 0;\n@@ -307,12 +349,14 @@ static int add_files(struct dir_struct *dir, int flags)\n \t\texit_status = 1;\n \t}\n \n-\tfor (i = 0; i < dir->nr; i++)\n+\tfor (i = 0; i < dir->nr; i++) {\n+\t\tcheck_embedded_repo(dir->entries[i]->name);\n \t\tif (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {\n \t\t\tif (!ignore_add_errors)\n \t\t\t\tdie(_(\"adding files failed\"));\n \t\t\texit_status = 1;\n \t\t}\n+\t}\n \treturn exit_status;\n }\n \ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c0d0e9a4c..e131760ee 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -213,7 +213,8 @@ cmd_add()\n \t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index and is not a submodule\")\"\n \tfi\n \n-\tif test -z \"$force\" && ! git add --dry-run --ignore-missing \"$sm_path\" > /dev/null 2>&1\n+\tif test -z \"$force\" &&\n+\t\t! git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" > /dev/null 2>&1\n \tthen\n \t\teval_gettextln \"The following path is ignored by one of your .gitignore files:\n \\$sm_path\n@@ -267,7 +268,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n \tfi\n \tgit config submodule.\"$sm_name\".url \"$realrepo\"\n \n-\tgit add $force \"$sm_path\" ||\n+\tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n \tdie \"$(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n \n \tgit config -f .gitmodules submodule.\"$sm_name\".path \"$sm_path\" &&\ndiff --git a/t/t7414-submodule-mistakes.sh b/t/t7414-submodule-mistakes.sh\nnew file mode 100755\nindex 000000000..f2e7df59c\n--- /dev/null\n+++ b/t/t7414-submodule-mistakes.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description='handling of common mistakes people may make with submodules'\n+. ./test-lib.sh\n+\n+test_expect_success 'create embedded repository' '\n+\tgit init embed &&\n+\ttest_commit -C embed one\n+'\n+\n+test_expect_success 'git-add on embedded repository warns' '\n+\ttest_when_finished \"git rm --cached -f embed\" &&\n+\tgit add embed 2>stderr &&\n+\ttest_i18ngrep warning stderr\n+'\n+\n+test_expect_success '--no-warn-embedded-repo suppresses warning' '\n+\ttest_when_finished \"git rm --cached -f embed\" &&\n+\tgit add --no-warn-embedded-repo embed 2>stderr &&\n+\ttest_i18ngrep ! warning stderr\n+'\n+\n+test_expect_success 'no warning when updating entry' '\n+\ttest_when_finished \"git rm --cached -f embed\" &&\n+\tgit add embed &&\n+\tgit -C embed commit --allow-empty -m two &&\n+\tgit add embed 2>stderr &&\n+\ttest_i18ngrep ! warning stderr\n+'\n+\n+test_expect_success 'submodule add does not warn' '\n+\ttest_when_finished \"git rm -rf submodule .gitmodules\" &&\n+\tgit submodule add ./embed submodule 2>stderr &&\n+\ttest_i18ngrep ! warning stderr\n+'\n+\n+test_done\n-- \n2.13.1.766.g6bea926c5\n\n"},{"id":"322233","messageId":"20170614105825.zbxdgvcezuilpins@sigill.intra.peff.net","threadId":"46125","inReplyTo":"20170614105437.cgdlykmtocdj5vzw@sigill.intra.peff.net","subject":"[PATCH v2 2/2] t: move \"git add submodule\" into test blocks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T10:58:25Z","receivedAt":"2017-06-14T10:58:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some submodule tests do some setup outside of a test_expect\nblock. This is bad because we won't actually check the\noutcome of those commands. But it's doubly so because \"git\nadd submodule\" now produces a warning to stderr, which is\nnot suppressed by the test scripts in non-verbose mode.\n\nThis patch does the minimal to fix the annoying warnings.\nAll three of these scripts could use more cleanup of related\nsetup.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t4041-diff-submodule-option.sh             | 8 +++++---\n t/t4060-diff-submodule-option-diff-format.sh | 8 +++++---\n t/t7401-submodule-summary.sh                 | 8 +++++---\n 3 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh\nindex 2d9731b52..058ee0829 100755\n--- a/t/t4041-diff-submodule-option.sh\n+++ b/t/t4041-diff-submodule-option.sh\n@@ -430,9 +430,11 @@ test_expect_success 'deleted submodule' '\n \ttest_cmp expected actual\n '\n \n-test_create_repo sm2 &&\n-head7=$(add_file sm2 foo8 foo9) &&\n-git add sm2\n+test_expect_success 'create second submodule' '\n+\ttest_create_repo sm2 &&\n+\thead7=$(add_file sm2 foo8 foo9) &&\n+\tgit add sm2\n+'\n \n test_expect_success 'multiple submodules' '\n \tgit diff-index -p --submodule=log HEAD >actual &&\ndiff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\nindex 33ec26d75..4b168d0ed 100755\n--- a/t/t4060-diff-submodule-option-diff-format.sh\n+++ b/t/t4060-diff-submodule-option-diff-format.sh\n@@ -643,9 +643,11 @@ test_expect_success 'deleted submodule' '\n \ttest_cmp expected actual\n '\n \n-test_create_repo sm2 &&\n-head7=$(add_file sm2 foo8 foo9) &&\n-git add sm2\n+test_expect_success 'create second submodule' '\n+\ttest_create_repo sm2 &&\n+\thead7=$(add_file sm2 foo8 foo9) &&\n+\tgit add sm2\n+'\n \n test_expect_success 'multiple submodules' '\n \tgit diff-index -p --submodule=diff HEAD >actual &&\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 366746f0d..4e4c45550 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -241,9 +241,11 @@ EOF\n \ttest_cmp expected actual\n \"\n \n-test_create_repo sm2 &&\n-head7=$(add_file sm2 foo8 foo9) &&\n-git add sm2\n+test_expect_success 'create second submodule' '\n+\ttest_create_repo sm2 &&\n+\thead7=$(add_file sm2 foo8 foo9) &&\n+\tgit add sm2\n+'\n \n test_expect_success 'multiple submodules' \"\n \tgit submodule summary >actual &&\n-- \n2.13.1.766.g6bea926c5\n"},{"id":"322270","messageId":"CAGZ79kaN=XVe3OWE5DHsMfbzW_rZOdRurgSfpz52dSZDA_V6fg@mail.gmail.com","threadId":"46125","inReplyTo":"20170614063614.a34ovimjpz2g24qe@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] add: warn when adding an embedded repository","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-14T17:53:12Z","receivedAt":"2017-06-14T17:53:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 13, 2017 at 11:36 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Jun 13, 2017 at 10:07:43AM -0700, Stefan Beller wrote:\n>\n>> > I also waffled on whether we should ask the submodule code\n>> > whether it knows about a particular path. Technically:\n>> >\n>> >   git config submodule.foo.path foo\n>> >   git config submodule.foo.url git://...\n>> >   git add foo\n>> >\n>> > is legal, but would still warn with this patch. I don't know\n>> > how much we should care (it would also be easy to do on\n>> > top).\n>>\n>> And here I was thinking this is not legal, because you may override\n>> anything *except* submodule.*.path in the config. That is because\n>> all the other settings (such as url, active flag, branch,\n>> shallow recommendation) are dependent on the use case, the user,\n>> changes to the environment (url) or such. The name<->path mapping\n>> however is only to be changed via changes to the tracked content.\n>> That is why it would make sense to disallow overriding the path\n>> outside the tracked content.\n>\n> It was probably a mistake to use normal config as the example. Junio\n> mentioned it as a case that could work if you communicate the submodule\n> URL to somebody else out-of-band. My understanding was that you could\n> set whatever you like in the regular config, but I think that is just\n> showing my ignorance of submodules.\n>\n> Pretend like I said \"-f .gitmodules\" in each line above. ;)\n>\n>> In my ideal dream world of submodules we would have the following:\n>>\n>>   $ cat .gitmodules\n>>   [submodule \"sub42\"]\n>>     path = foo\n>>   # path only in tree!\n>\n> TBH, I am not sure why we need \"path\"; couldn't we just use the\n> subsection name as an implicit path?\n\nThat is what was done back in the time. But then people wanted to rename\nsubmodules (i.e. move them around in the worktree), so the path is not\nconstant, so either we'd have to move around the git dir whenever the\nsubmodule is renamed (bad idea IMO), or instead introduce a mapping\nbetween (constant name <-> variable path). So that was done.\n\nHistorically (IIUC) we had submodule.path.url which then was changed\nto submodule.name.url + name->path resolution. And as a hack(?) or\neasy way out of a problem then, the name is often the same as the path\nhence confusing people, when they see:\n\n    [submodule \"foo\"]\n        path = foo\n        url = dadada/foo\n\nWhat foo means what now? ;)\nAs a tangent: I want to make the default name different to the path.\n\nSo yeah, we want to keep the name and not mingle with implicit path.\n\nI think we may even have bugs in our code base where the\nname/path confusion shows.\n\nTalking about another tangent:\n\n  For files there is a rename detection available. For submodules\n  It is hard to imagine that there ever will be such a rename detection\n  as files have because of the explciit name<->path mapping.\n\n  We *know* when a submodule was moved. So why even try\n  to do rename detection? As we record only sha1s for a submodule\n  you could swap two submodule object names by accident.\n  Consider a superproject that contains different kernels, such as\n  a kernel for your phone/embedded device and then a kernel for\n  your workstation or other device. And these two kernels are different\n  for technical reasons but share the same history.\n\n  Now the inattentive user may make a mistake and git-add the\n  \"wrong\" kernel submodule.  The smart Git would tell that it is a\n  rename/move just as we have with files.\n\n>\n>> > +       OPT_HIDDEN_BOOL(0, \"warn-embedded-repo\", &warn_on_embedded_repo,\n>> > +                       N_(\"warn when adding an embedded repository\")),\n>>\n>> We do not have a lot of OPT_HIDDEN_BOOLs throughout the code base.\n>> We should use them more often.\n>>\n>> It makes sense though in this case.\n>\n> Actually, my main reason is that it's nonsense to show\n> \"--warn-embedded-repo\" in the help, when it's already the default. I\n> would like to have written:\n>\n>   OPT_NEGBOOL(0, \"no-warn-embedded-repo\", &warn_on_embedded_repo,\n>                 N_(\"disable warning when adding an embedded repository\"))\n>\n> but we don't have such a thing (and the last discussion on it a few\n> months ago left a lot of open questions). So given that this really\n> isn't something I'd expect users to want, I figured hiding it was a good\n> idea. I mentioned it in the manpage for script writers, but it's really\n> not worth cluttering \"git add -h\".\n\nok :) If you really wanted, you could go with a raw OPTION though. ;)\nThis is fine with me though.\n\n>\n>> > +static const char embedded_advice[] = N_(\n>> > +\"You've added another git repository inside your current repository.\\n\"\n>> > +\"Clones of the outer repository will not also contain the contents of\\n\"\n>> > +\"the embedded repository. If you meant to add a submodule, use:\\n\"\n>>\n>> The \"will not also\" sounds a bit off to me. Maybe:\n>>   ...\n>>   Clones of the outer repository will not contain the contents\n>>   of the embedded repository and has no way of knowing how\n>>   to obtain the inner repo. If you meant to add a submodule ...\n>\n> Yeah, I think we could just strike the \"also\" (I played around with the\n> wording here quite a bit and I think it was left from an earlier attempt\n> where it made more sense).\n>\n> Your \"no way of knowing\" is probably a good thing to mention.\n>\n>> > +\"See \\\"git help submodule\\\" for more information.\"\n>>\n>> Once the overhaul of the submodule documentation\n>> comes along[1], we rather want to point at\n>> \"man 7 git-submodules\", which explains the concepts and\n>> then tell you about commands how to use it. For now the\n>> git-submodule man page is ok.\n>>\n>> [1] https://public-inbox.org/git/20170607185354.10050-1-sbeller@google.com/\n>\n> Yeah, I poked around looking for a definitive \"here's how submodules\n> work\" intro. I'm happy one is in the works, and I agree this should\n> point there once it exists.\n>\n>> > +++ b/t/t7414-submodule-mistakes.sh\n>> > @@ -0,0 +1,40 @@\n>> > +#!/bin/sh\n>> > +\n>> > +test_description='handling of common mistakes people may make with submodules'\n>>\n>> That is one way to say it. Do we have other tests for\n>> \"you think it is a bug, but it is features\" ? ;)\n>> I like it though. :)\n>\n> Heh. I didn't know how else to lump it together. Just \"test git add on a\n> repository\" felt like too little for its own script. I almost added it\n> to t7400, but I think that script is plenty long enough as it is (it's\n> also one of the longest-running scripts, I think).\n\nThanks for not doing that. :)\n"},{"id":"322295","messageId":"20170615060131.cntluubjgkfsiehh@sigill.intra.peff.net","threadId":"46125","inReplyTo":"CAGZ79kaN=XVe3OWE5DHsMfbzW_rZOdRurgSfpz52dSZDA_V6fg@mail.gmail.com","subject":"Re: [PATCH 1/2] add: warn when adding an embedded repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-15T06:01:31Z","receivedAt":"2017-06-15T06:01:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 14, 2017 at 10:53:12AM -0700, Stefan Beller wrote:\n\n> >> In my ideal dream world of submodules we would have the following:\n> >>\n> >>   $ cat .gitmodules\n> >>   [submodule \"sub42\"]\n> >>     path = foo\n> >>   # path only in tree!\n> >\n> > TBH, I am not sure why we need \"path\"; couldn't we just use the\n> > subsection name as an implicit path?\n> \n> That is what was done back in the time. But then people wanted to rename\n> submodules (i.e. move them around in the worktree), so the path is not\n> constant, so either we'd have to move around the git dir whenever the\n> submodule is renamed (bad idea IMO), or instead introduce a mapping\n> between (constant name <-> variable path). So that was done.\n\nAh, right. That makes sense. I forgot that in addition to the in-tree\npath, we have to store the submodule repository itself as some name. The\nextra level of indirection there isn't strictly necessary, but it lets\nthe \"name\" act as a unique id.\n\n> Historically (IIUC) we had submodule.path.url which then was changed\n> to submodule.name.url + name->path resolution. And as a hack(?) or\n> easy way out of a problem then, the name is often the same as the path\n> hence confusing people, when they see:\n> \n>     [submodule \"foo\"]\n>         path = foo\n>         url = dadada/foo\n> \n> What foo means what now? ;)\n\nRight, I am such a person that has been confused. ;)\n\nThanks for explaining.\n\n> Talking about another tangent:\n> \n>   For files there is a rename detection available. For submodules\n>   It is hard to imagine that there ever will be such a rename detection\n>   as files have because of the explciit name<->path mapping.\n> \n>   We *know* when a submodule was moved. So why even try\n>   to do rename detection? As we record only sha1s for a submodule\n>   you could swap two submodule object names by accident.\n>   Consider a superproject that contains different kernels, such as\n>   a kernel for your phone/embedded device and then a kernel for\n>   your workstation or other device. And these two kernels are different\n>   for technical reasons but share the same history.\n\nDo you mean during the rename detection phase of a diff, check to see if\n.gitmodules registered a change in path for a particular module (by\nfinding its entry in the diff and looking at both sides), and if so then\nmark that as a rename for the submodule paths?\n\nFrom a cursory glance, that sounds like an interesting approach.\n\n-Peff\n"}]}