Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> @@ -269,7 +269,7 @@ static char *get_up_path(const char *path)
> static int module_list(int argc, const char **argv, const char *prefix)
> {
> int i;
> - struct pathspec pathspec;
> + struct pathspec pathspec = { 0 };
> struct module_list list = MODULE_LIST_INIT;
>
> struct option module_list_options[] = {
> @@ -278,6 +278,7 @@ static int module_list(int argc, const char **argv, const char *prefix)
> N_("alternative anchor for relative paths")),
> OPT_END()
> };
> + int ret;
Adding such a simple variable near the top, perhaps next to "int i",
would probably make it easier to read, especially because you employ
a good pattern below to let the compiler detect unintended use of
the variable uninitialized, i.e. "ret = 1; goto cleanup;" is placed
at each error-exit codepath, and "ret = 0;" is added immediately
before the "cleanup" label. If somebody jumps to the cleanup label
without setting "ret", the compiler will catch it.
But you can do better by eliminating the need to check. Initialize
"ret" to non-zero (i.e. "assume failure"), and have each error-exit
codepath just "goto cleanup". Keep the "ret = 0;" this patch uses
immediately before the "cleanup" label.
> @@ -287,8 +288,10 @@ static int module_list(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, module_list_options,
> git_submodule_helper_usage, 0);
>
> - if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> - return 1;
> + if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
> + ret = 1;
> + goto cleanup;
> + }
Which will simplify this hunk.
The same exact pattern repeats throughout this patch, and the above
commits apply to all of the hunks, I think.
Thanks.
