Re: [PATCH 00/20] submodule--helper: add tests, rm dead code, refactor & leak prep

[Glen Choo <chooglen@google.com>]

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

> This new series based on "master" splits out the non-leak fixes from
> v3 of the submodule leaks series [1]. Sorry about the churn, but with
> e.g. the thread at [2] the "leaks" series was accumulating a lot of
> non-leaks related fixes, just so that we could get to the point of
> fixing various leaks.
>
> The immediate reason I split this off was because of the new
> "remove..helper" and "test-tool" patches here. I.e. some things that
> the leaks series was changing was test-only code, or code that was
> simply unused (or we had no business using anymore).
>
> The [1] series will then be re-rolled on top of this series, which
> will hopefully make it a lot easier to digest, as it'll now *just*
> focus on leak fixes.


Thanks! I was worried about the churn at first, but most of these
patches are just the ones we already reviewed in previous versions, and
the leaks series looks a lot easier to review.

I left the bigger comments in-thread. Some themes I saw in the patches:

>   submodule tests: test usage behavior
>   submodule tests: test for "add <repository> <abs-path>"

I assume these tests are meant for catching regressions?

>   submodule--helper: remove unused "name" helper
>   submodule--helper: remove unused "list" helper

Hooray for rm-ing dead code!

>   test-tool submodule-config: remove unused "--url" handling
>   submodule--helper: move "is-active" to a test-tool
>   submodule--helper: move "check-name" to a test-tool
>   submodule--helper: move "resolve-relative-url-test" to a test-tool

Each of these commands are only used in _their own_ tests as opposed to
be used in tests for _other_ commands (or, to borrow the language of
garbage collection, the only references we have are weak references ;)),
which makes me think that we could:

a) be more ruthless in considering these dead code
b) be more explicit in calling them "unit tests"
c) (somewhere down the line) absorb them into the rest of the test suite
instead of creating a special testing escape hatch.

>   submodule--helper: add "const" to copy of "update_data"
>   submodule--helper: pass a "const struct module_clone_data" to
>     clone_submodule()

I suspect these changes to ownership make more sense in the leaks
series. For the former patch, it would explain why we don't have to
plug leaks in "update_data". For latter, the ownership rules are still a
bit confusing (for historical reasons, not anything you introduced), so
the leak fix might help explain why the new ownership is better than
what we had before.

>   submodule--helper: stop conflating "sb" in clone_submodule()
>   submodule--helper: add skeleton "goto cleanup" to update_submodule()
>   submodule--helper: don't exit() on failure, return

IMO this refactoring doesn't really stand on its own unless we also take
the leaks fixes. Might be nice to move these to that series, but I don't
feel strongly about it.

>   submodule--helper: refactor "errmsg_str" to be a "struct strbuf"
>   submodule--helper style: don't separate declared variables with \n\n
>   submodule--helper style: add \n\n after variable declarations
>   submodule--helper: rename "int res" to "int ret"
>   submodule--helper: replace memset() with { 0 }-initialization
>   submodule--helper: convert a strbuf_detach() to xstrfmt()
>   submodule--helper: fix bad config API usage

Style and API fixes that make sense on their own :)

Since we now have an extra topic for cleanup, I wonder what you think
about incorporating [1], i.e. refactor out init_submodules() to be
called from module_update(). It's a bit big, but it would make [2]
obsolete.

[1] https://lore.kernel.org/git/kl6lbktitf6e.fsf@chooglen-macbookpro.roam.corp.google.com
[2] https://lore.kernel.org/git/patch-v4-04.17-683d327752f-20220728T162442Z-avarab@gmail.com