git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <christian.couder@gmail.com>
To: Prathamesh Chavan <pc44800@gmail.com>
Cc: git <git@vger.kernel.org>, Stefan Beller <sbeller@google.com>
Subject: Re: [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C
Date: Tue, 1 Aug 2017 01:27:58 +0200	[thread overview]
Message-ID: <CAP8UFD3_zX0=h9tuNA-w+30V4P8zZGyUCEnpciTkwRWpXjjmNg@mail.gmail.com> (raw)
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.

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

> +}

  parent reply	other threads:[~2017-07-31 23:28 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-31 20:56 [GSoC][PATCH 00/13] Update: Week-11 Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 01/13] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 02/13] submodule--helper: introduce for_each_submodule_list() Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 03/13] submodule: port set_name_rev() from shell to C Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' " Prathamesh Chavan
2017-07-31 21:12   ` Stefan Beller
2017-08-01 21:14     ` Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 05/13] submodule: port submodule subcommand 'sync' " Prathamesh Chavan
2017-07-31 21:19   ` Stefan Beller
2017-07-31 20:56 ` [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' " Prathamesh Chavan
2017-07-31 21:42   ` Stefan Beller
2017-08-01 21:19     ` Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 07/13] diff: change scope of the function count_lines() Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C Prathamesh Chavan
2017-07-31 22:15   ` Stefan Beller
2017-07-31 23:27   ` Christian Couder [this message]
2017-08-05 10:28     ` Prathamesh Chavan
2017-08-05 16:55       ` Christian Couder
2017-08-05 18:03         ` Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectory Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 10/13] submodule foreach: document '$sm_path' instead of '$path' Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 11/13] submodule foreach: clarify the '$toplevel' variable documentation Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 12/13] submodule foreach: document variable '$displaypath' Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 13/13] submodule: port submodule subcommand 'foreach' from shell to C Prathamesh Chavan
2017-07-31 22:20   ` Stefan Beller
  -- strict thread matches above, loose matches on Subject: below --
2017-08-07 21:18 [GSoC][PATCH 00/13] Update: Week-12 Prathamesh Chavan
2017-08-07 21:18 ` [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C Prathamesh Chavan
2017-08-07 21:43   ` Christian Couder
2017-07-24 20:34 [GSoC][PATCH 00/13] Update: Week 10 Prathamesh Chavan
2017-07-24 20:34 ` [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C Prathamesh Chavan
2017-07-25  0:09   ` Brandon Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAP8UFD3_zX0=h9tuNA-w+30V4P8zZGyUCEnpciTkwRWpXjjmNg@mail.gmail.com' \
    --to=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pc44800@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).