Re: [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C
- From
Christian Couder <christian.couder@gmail.com>
- Date
- Jul 31, 2017, 23:27 UTC
- Message-ID
- <CAP8UFD3_zX0=h9tuNA-w+30V4P8zZGyUCEnpciTkwRWpXjjmNg@mail.gmail.com>
- In-Reply-To
- <20170731205621.24305-9-pc44800@gmail.com>
On Mon, Jul 31, 2017 at 10:56 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> * variable head was no longer used in module_summary() and instead the strbuf > was utilized.
Good but there might be a few problems in the way it is used. See below.
Show 48 quoted lines
> +static int compute_summary_module_list(char *head, struct summary_cb *info)
> +{
> + struct argv_array diff_args = ARGV_ARRAY_INIT;
> + struct rev_info rev;
> + struct module_cb_list list = MODULE_CB_LIST_INIT;
> +
> + argv_array_push(&diff_args, info->diff_cmd);
> + if (info->cached)
> + argv_array_push(&diff_args, "--cached");
> + argv_array_pushl(&diff_args, "--ignore-submodules=dirty", "--raw",
> + NULL);
> + if (head)
> + argv_array_push(&diff_args, head);
> + argv_array_push(&diff_args, "--");
> + if (info->argc)
> + argv_array_pushv(&diff_args, info->argv);
> +
> + git_config(git_diff_basic_config, NULL);
> + init_revisions(&rev, info->prefix);
> + gitmodules_config();
> + rev.abbrev = 0;
> + precompose_argv(diff_args.argc, diff_args.argv);
> +
> + diff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,
> + &rev, NULL);
> + rev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;
> + rev.diffopt.format_callback = submodule_summary_callback;
> + rev.diffopt.format_callback_data = &list;
> +
> + if (!info->cached) {
> + if (!strcmp(info->diff_cmd, "diff-index"))
> + setup_work_tree();
> + if (read_cache_preload(&rev.diffopt.pathspec) < 0) {
> + perror("read_cache_preload");
> + return -1;
> + }
> + } else if (read_cache() < 0) {
> + perror("read_cache");
> + return -1;
> + }
> +
> + if (!strcmp(info->diff_cmd, "diff-index"))
> + run_diff_index(&rev, info->cached);
> + else
> + run_diff_files(&rev, 0);
> + prepare_submodule_summary(info, &list);
> +
> + free(head);It is a bit strange to have this function free() an argument it is passed.
> + return 0; > +
Spurious new line.
Show 13 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;
> + struct strbuf sb = STRBUF_INIT;[...]
Show 27 quoted lines
> + 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");
> + }
> +
> + if (files) {
> + if (cached)
> + die(_("The --cached option cannot be used with the --files option"));
> + diff_cmd = "diff-files";
> +
> + strbuf_reset(&sb);strbuf_reset() does not free the memory allocated to sb.buf...
> + sb.buf = NULL;
...then this makes sure that the memory previously allocated to sb.buf will not be free()d.
Maybe instead of the above 2 lines just: strbuf_release(&sb) Or maybe just don't set sb.buf to NULL.
Show 13 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; > + > + return compute_summary_module_list(strbuf_detach(&sb, NULL), &info);
Maybe you could pass: "sb.len > 0 ? strbuf_detach(&sb, NULL) : NULL" This way if sb has previously been released or reset, NULL will be passed.
I would suggest though to just pass sb.buf and to strbuf_release(&sb) after calling compute_summary_module_list() and before returning from module_summary() instead of having compute_summary_module_list() free its first argument. If you do that then compute_summary_module_list() should be changed so that when it is passed "" it will behave in the same way as when it is passed NULL.
> +}