Jeff King <peff@xxxxxxxx> writes: >> 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. I did not mean that no "git add" that create a gitlink in existing tests should be made into "git submodule add". The ones that clearly wanted to set up tests to see what happens in a top-level with a subproject may become more realistic tests by switching to "git submodule add" and updating the expected "git diff HEAD" output to include a newly created .gitmodules file. But some of the tests are merely to see what happens with an index with a gitlink in it, and "add --no-warn" would be more appropriate for them. >> 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. Yeah, if the default becomes "no", then there isn't much point, other than just for completeness, to have "warn" as a choice. Thanks.
