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

On Mon, Jul 31, 2017 at 10:56 PM, Prathamesh Chavan [off-list ref] 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.

+}