On Tue, Aug 1, 2017 at 4:57 AM, Christian Couder
<christian.couder@xxxxxxxxx> wrote:
> On Mon, Jul 31, 2017 at 10:56 PM, Prathamesh Chavan <pc44800@xxxxxxxxx> 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.
>
>> +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.
>
>> +}
>> +
>> +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;
>
> [...]
>
>> + 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.
>
>> + }
>> +
>> + 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.
>
We can avoid it to behave same for "" and NULL, by checking if diff_cmd
is "cmd_diff_files", since its value is set NULL by this case.
ret = compute_summary_module_list(strcmp(diff_cmd, "diff-files") ?
NULL: sb.buf, &info);
strbuf_release(&sb);
instead of:
ret = compute_summary_module_list(sb.len ? sb.buf : NULL, &info);
if (sb.len)
strbuf_release(&sb);
>> +}
