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

On Tue, Aug 1, 2017 at 4:57 AM, Christian Couder
[off-list ref] wrote:

On Mon, Jul 31, 2017 at 10:56 PM, Prathamesh Chavan [off-list ref] wrote:

quoted

* 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.

quoted

+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.

quoted

+       return 0;
+

Spurious new line.

quoted

+}
+
+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;

[...]

quoted

+       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...

quoted

+               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.

quoted

+       }
+
+       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);

quoted

+}