On Mon, Feb 13, 2023 at 06:17:53PM -0800, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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
