Re: [GSoC][PATCH v2 08/13] submodule: port submodule subcommand 'summary' from shell to C

From: Christian Couder <christian.couder@gmail.com>
Date: Jul 30, 2017, 05:28 UTC
Message-ID: <CAP8UFD2XWm-m1CZoX121mhQ+WunCRTMJT5RjrZAXKBY3axcSJQ@mail.gmail.com>
In-Reply-To: <20170729222401.12381-9-pc44800@gmail.com>

On Sun, Jul 30, 2017 at 12:23 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:

Show 60 quoted lines

> +static int module_summary(int argc, const char **argv, const char *prefix)
> +{
> +       struct summary_cb info = SUMMARY_CB_INIT;
> +       int cached = 0;
> +       char *diff_cmd = "diff-index";
> +       int for_status = 0;
> +       int quiet = 0;
> +       int files = 0;
> +       int summary_limits = -1;
> +       struct child_process cp_rev = CHILD_PROCESS_INIT;
> +       char *head;
> +       struct strbuf sb = STRBUF_INIT;
> +       int ret;
> +
> +       struct option module_summary_options[] = {
> +               OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
> +               OPT_BOOL(0, "cached", &cached, N_("Use the commit stored in the index instead of the submodule HEAD")),
> +               OPT_BOOL(0, "files", &files, N_("To compares the commit in the index with that in the submodule HEAD")),
> +               OPT_BOOL(0, "for-status", &for_status, N_("Skip submodules with 'all' ignore_config value")),
> +               OPT_INTEGER('n', "summary-limits", &summary_limits, N_("Limit the summary size")),
> +               OPT_END()
> +       };
> +
> +       const char *const git_submodule_helper_usage[] = {
> +               N_("git submodule--helper summary [<options>] [--] [<path>]"),
> +               NULL
> +       };
> +
> +       argc = parse_options(argc, argv, prefix, module_summary_options,
> +                            git_submodule_helper_usage, 0);
> +
> +       if (!summary_limits)
> +               return 0;
> +
> +       cp_rev.git_cmd = 1;
> +       argv_array_pushl(&cp_rev.args, "rev-parse", "-q", "--verify",
> +                        argc ? argv[0] : "HEAD", NULL);
> +
> +       if (!capture_command(&cp_rev, &sb, 0)) {
> +               strbuf_strip_suffix(&sb, "\n");
> +               if (argc) {
> +                       argv++;
> +                       argc--;
> +               }
> +       } else if (!argc || !strcmp(argv[0], "HEAD")) {
> +               /* before the first commit: compare with an empty tree */
> +               struct stat st;
> +               struct object_id oid;
> +               if (fstat(0, &st) < 0 || index_fd(oid.hash, 0, &st, 2, prefix, 3))
> +                       die("Unable to add %s to database", oid.hash);
> +               strbuf_addstr(&sb, oid_to_hex(&oid));
> +               if (argc) {
> +                       argv++;
> +                       argc--;
> +               }
> +       } else {
> +               strbuf_addstr(&sb, "HEAD");
> +       }
> +
> +       head = strbuf_detach(&sb, NULL);

I am not sure this "head" variable is really needed.

Show 8 quoted lines

> +       if (files) {
> +               if (cached)
> +                       die(_("The --cached option cannot be used with the --files option"));
> +               diff_cmd = "diff-files";
> +
> +               free(head);
> +
> +               head = NULL;

If "head" isn't used, "strbuf_reset(&sb)" could be used instead. If "head" is still needed, "FREE_AND_NULL(head)" could be used.

Show 15 quoted lines

> +       }
> +
> +       info.argc = argc;
> +       info.argv = argv;
> +       info.prefix = prefix;
> +       info.cached = !!cached;
> +       info.for_status = !!for_status;
> +       info.quiet = quiet;
> +       info.files = files;
> +       info.summary_limits = summary_limits;
> +       info.diff_cmd = diff_cmd;
> +
> +       ret = compute_summary_module_list(head, &info);
> +       if (head)
> +               free(head);

"sb.buf" could be passed to compute_summary_module_list() instead of "head". In this case that function should check that head is not an empty string before using it.

If "head" is not used then strbuf_release(&sb) can be used to free any memory sb still holds. If "head" is still used, "if (head)" can be removed before "free(head)" as free() already checks if its argument is NULL.

> +       return ret;
> +

Spurious new line.

> +}