Re: [PATCH v2 00/12] submodule: make "git submodule--helper" behave like "git submodule"

From: Glen Choo <chooglen@google.com>
Date: Jun 15, 2022, 18:42 UTC
Message-ID: <kl6l35g56a4w.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <cover-v2-00.12-00000000000-20220613T220150Z-avarab@gmail.com>

Super happy to see this moving forward, thanks! I'll summarize the discussion to make it easier for others to follow.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> This series:
>
>  * Removes dead code from git-submodule.sh, or dead code where it was
>    the last user.

This is 1-2/12. Not strictly 'necessary', but removing the dead code makes it easier to tell what git-submodule.sh is doing and will make the final review simpler.

These all look good to me.

Show 12 quoted lines

>  * Simplifies CLI parsing in git-submodule.sh, where it was doing
>    something overly complex for no reason.
>
>  * Brings "git submodule--helper" in line with the CLI interface of
>    "git-submodule.sh", for a follow-up series to remove the latter, as
>    we'll be able to make a new "git submodule" in C dispatch directly
>    to our new "git submodule--helper" code.
>
>  * Removes the "-v" option to "git submodule", which has been
>    supported, but was a) never documented b) never did anything
>    anyway, except as a way to negate an earlier "--quiet" option, as
>    "verbose" was always the default.

3-10/12 is the real crux of this series, which removes all of the extra parsing from "git submodule" by making "git submodule--helper" accept the same args. This is great, because it means that "submodule--helper" can just pretend to be "submodule".

One thing that this series _doesn't_ do (but the RFCs do) is to actually take advantage of this fact and remove the options parsing from git-submodule.sh, e.g.

     cmd_foo() {
        git ${wt_prefix:+-C "$wt_prefix"} submodule--helper foo \
        	${GIT_QUIET:+--quiet} "$@"
     }

or in the case dispatch in [1]. Like we discussed earlier, I think this is for the better - getting rid of the options parsing is (probably) going to be most error-prone step, so it makes some sense to do that on its own.

We agreed that 8/12 should be moved to that later series, but everything else looks good.

Show 9 quoted lines

>  * The last couple of patches are cleanup that isn't strictly
>    neccesary for the end-goal of "git submodule" in C, but cleans up
>    some more shellscript code.
>
>    The "say" function is removed from "git-sh-setup.sh", now that
>    "git-submodule.sh" doesn't use it anymore (which happened before
>    this series) we can replace the couple of remaining uses with
>    "echo", and by having "git-subtree.sh" own the code that used to
>    live in "git-sh-setup.sh".

This is 11-12/12, which removes "say" and related cruft.

The changes look ok. I'm not experienced with this area of the codebase, but I've verified the findings in the commit messages.

[1] https://lore.kernel.org/git/RFC-patch-09.20-bd0e4a4f8b8-20220610T011725Z-avarab@gmail.com