On Mon, Feb 13, 2023 at 06:21:34PM +0000, Calvin Wan wrote:
> As noted in 532139940c (add: warn when adding an embedded repository,
> 2017-06-14), adding embedded repositories results in subpar experience
> compared to submodules, due to the lack of a corresponding .gitmodules
> entry, which means later clones of the top-level repository cannot
> locate the embedded repo. We expect that this situation is usually
> unintentional, which is why 532139940c added a warning message and
> advice when users attempt to add an embedded repo.
As the author of 532139940c, this escalation to an error seems like a
reasonable step to me.
The patch looks pretty reasonable to me from a cursory read, but here a
few small comments:
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index a030d33c6e..b7fb95b061 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -177,10 +177,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
> tree or not.
>
> --no-warn-embedded-repo::
> - By default, `git add` will warn when adding an embedded
> + By default, `git add` will error out when adding an embedded
The option name here is rather unfortunate, since it's no longer a
warning. But keeping it as-is for historical compatibility may be the
best option. In retrospect, I wish I'd called it --allow-embedded-repos
or something.
> diff --git a/builtin/add.c b/builtin/add.c
> index 76277df326..795d9251b9 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -421,36 +421,45 @@ static const char embedded_advice[] = N_(
> "\n"
> " 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.
> -static void check_embedded_repo(const char *path)
> +static int check_embedded_repo(const char *path)
> {
> + int ret = 0;
> struct strbuf name = STRBUF_INIT;
> static int adviced_on_embedded_repo = 0;
>
> if (!warn_on_embedded_repo)
> - return;
> + goto cleanup;
> if (!ends_with(path, "/"))
> - return;
> + goto cleanup;
> +
> + ret = 1;
I wondered about these "goto cleanup" calls here, since there is nothing
to clean up yet. But you are just piggy-backing on the "return ret",
rather than a separate "return 0" here.
And I was surprised by returning at all, since the point is to make this
a hard error. But it looks like the intent is to report an error for
every such case, and then a final die, like:
$ git add .
error: cannot add embedded git repository: foo
advice: ...
error: cannot add embedded git repository: bar
fatal: refusing to add embedded git repositories
OK. I doubt anybody cares much either way (it is more convenient if you
have a lot of cases, at the expense of making the error more verbose
when there is only one case), so this is fine.
> /* Drop trailing slash for aesthetics */
> strbuf_addstr(&name, path);
> strbuf_strip_suffix(&name, "/");
>
> - warning(_("adding embedded git repository: %s"), name.buf);
> + error(_("cannot add embedded git repository: %s"), name.buf);
> if (!adviced_on_embedded_repo &&
> advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {
> - advise(embedded_advice, name.buf, name.buf);
> + advise(embedded_advice, name.buf, name.buf, name.buf);
This triple name.buf that must match the earlier string is horrible, of
course, but nothing new in your patch. If you drop the "rm --cached"
part from the advice, it can remain as a horrible double-mention of
name.buf. :)
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index eae6a46ef3..e0bcecba6e 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -118,7 +118,7 @@ test_expect_success 'setup - repository in init subdirectory' '
> test_expect_success 'setup - commit with gitlink' '
> echo a >a &&
> echo z >z &&
> - git add a init z &&
> + git add --no-warn-embedded-repo a init z &&
> git commit -m "super commit 1"
> '
>
> @@ -771,7 +771,7 @@ test_expect_success 'set up for relative path tests' '
> git init &&
> test_commit foo
> ) &&
> - git add sub &&
> + git add --no-warn-embedded-repo sub &&
> git config -f .gitmodules submodule.sub.path sub &&
> git config -f .gitmodules submodule.sub.url ../subrepo &&
> cp .git/config pristine-.git-config &&
OK, these are cases that are warning now (because they are trying to do
something clever with the setup), but which will be blocked. Arguably
they should have --no-warn-embedded-repo already, but nobody cared
so far that their stderr was a little noisy.
I do wonder if there are any users who might do clever things like this
themselves, but they'd already have been nagged by the warning (and
hopefully discovered --no-warn-embedded-repo on their own).
-Peff
