On Sun, Jul 30, 2017 at 10:58 AM, Christian Couder
<christian.couder@xxxxxxxxx> wrote:
> On Sun, Jul 30, 2017 at 12:23 AM, Prathamesh Chavan <pc44800@xxxxxxxxx> wrote:
>
>> +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.
>
>> + 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.
Thank you for reviewing this patch.
As suggested, here we can remove the variable head, and instead use
the strbuf struct.
But in the above lines, along with strbuf_reset(&sb), later we are
also required to set
sb.buf to NULL.
>
>> + }
>> +
>> + 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);
we may even remove the variable ret, but passing strbuf_detach(&sb, NULL) here
to the function compute_summary_module_list(), and then later on free(head) at
the end of the function compute_summary_module_list().
Thanks,
Prathamesh Chavan
