git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH 6/6] add: reject nested repositories

From
Jeff King <peff@peff.net>
Date
Feb 14, 2023, 16:07 UTC
Message-ID
<Y+ux3DEd/p5emFWs@coredump.intra.peff.net>
In-Reply-To
<xmqqilg57zxq.fsf@gitster.g>
On Mon, Feb 13, 2023 at 06:17:53PM -0800, Junio C Hamano wrote:
Show 32 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> >>  "	git rm --cached %s\n"
> >>  "\n"
> >> -"See \"git help submodule\" for more information."
> >> +"See \"git help submodule\" for more information.\n"
> >> +"\n"
> >> +"If you cannot use submodules, you may bypass this check with:\n"
> >> +"\n"
> >> +"	git add --no-warn-embedded-repo %s\n"
> >>  );
> >
> > I was a little surprised by this hunk, but I guess if we are going to
> > block the user's operation from completing, we might want to tell them
> > how to get around it. But it seems odd to me that the instructions to
> > "git rm --cached" the submodule remain. If this situation is now an
> > error and not a warning, there is nothing to roll back from the index,
> > since we will have bailed before writing it.
> >
> > If we are going to start recommending --no-warn-embedded-repo here,
> > would we want to promote it from being OPT_HIDDEN_BOOL()? We do document
> > it in the manpage, but just omit it from the "-h" output, since it
> > should be rarely used. Maybe it is OK to stay that way; you don't need
> > it until you run into this situation, at which point the advice
> > hopefully has guided you in the right direction.
> 
> If we are keeping the escape hatch, it would make sense to actually
> use that escape hatch to protect existing "git add" with that,
> instead of turning them into "git submodule add" and then adjust the
> tests for the consequences (i.e. "submodule add" does more than what
> "git add [--no-warn-embedded-repo]" would), at least for these tests
> in [3,4,5/6].

Good point. I did not really look at the test modifications, but anywhere that is triggering the current warning is arguably a good spot to be using --no-warn-embedded-repo already. It is simply that the test did not bother to look at their noisy stderr. And such a modification is obviously correct, as there are no further implications for the test.

> Also I do not think it is too late for a more natural UI, e.g.
> "--allow-embedded-repo=[yes/no/warn]", to deprecate the
> "--[no-]warn-*" option.

True. We have to keep the existing form for backwards compatibility, but we can certainly add a new one.

I kind of doubt that --allow-embedded-repo=warn is useful, though. If a caller knows what it is doing is OK, then it would say "yes". And otherwise, you'd want "no". There is no situation where a caller is unsure.

-Peff
Previous: Junio C HamanoNext: Junio C Hamano
Message 16 of 40 in “add: block invalid submodules”
  1. 0/6 add: block invalid submodulesCalvin Wan, Feb 13, 2023
  2. 1/6 leak fix: cache_put_pathCalvin Wan, Feb 13, 2023
  3. Junio C HamanoFeb 13, 2023
  4. Calvin WanFeb 14, 2023
  5. Junio C HamanoFeb 14, 2023
  6. Calvin WanFeb 14, 2023
  7. Junio C HamanoFeb 14, 2023
  8. 3/6 tests: Use `git submodule add` instead of `git add`Calvin Wan, Feb 13, 2023
  9. 4/6 tests: use `git submodule add` and fix expected diffsCalvin Wan, Feb 13, 2023
  10. Junio C HamanoFeb 13, 2023
  11. Junio C HamanoFeb 13, 2023
  12. 5/6 tests: use `git submodule add` and fix expected statusCalvin Wan, Feb 13, 2023
  13. 6/6 add: reject nested repositoriesCalvin Wan, Feb 13, 2023
  14. Jeff KingFeb 13, 2023
  15. Junio C HamanoFeb 14, 2023
  16. Jeff KingFeb 14, 2023
  17. Junio C HamanoFeb 14, 2023
  18. Calvin WanFeb 14, 2023
  19. 2/6 t4041, t4060: modernize test styleCalvin Wan, Feb 13, 2023
  20. Junio C HamanoFeb 13, 2023
  21. Calvin WanFeb 14, 2023
  22. 0/6 add: block invalid submodulesCalvin Wan, Feb 28, 2023
  23. 1/6 t4041, t4060: modernize test styleCalvin Wan, Feb 28, 2023
  24. Glen ChooMar 6, 2023
  25. Calvin WanMar 6, 2023
  26. 2/6 tests: Use `git submodule add` instead of `git add`Calvin Wan, Feb 28, 2023
  27. Junio C HamanoFeb 28, 2023
  28. Calvin WanMar 3, 2023
  29. Glen ChooMar 6, 2023
  30. 3/6 tests: use `git submodule add` and fix expected diffsCalvin Wan, Feb 28, 2023
  31. Glen ChooMar 6, 2023
  32. Junio C HamanoMar 6, 2023
  33. 4/6 tests: use `git submodule add` and fix expected statusCalvin Wan, Feb 28, 2023
  34. Glen ChooMar 7, 2023
  35. 5/6 tests: remove duplicate .gitmodules pathCalvin Wan, Feb 28, 2023
  36. Junio C HamanoFeb 28, 2023
  37. Calvin WanMar 2, 2023
  38. Glen ChooMar 7, 2023
  39. 6/6 add: reject nested repositoriesCalvin Wan, Feb 28, 2023
  40. Glen ChooMar 7, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.