On Tue, Feb 14, 2023 at 8:32 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > 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. I'll take another pass into the modified tests from previous patches and pick out ones that are not specifically submodule related tests. > >> 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. I don't see a point for "warn" as well. The default "no" case should carry over part of the deprecated warning from before.
