git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Prathamesh Chavan <pc44800@gmail.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>, Christian Couder <christian.couder@gmail.com>
Subject: Re: [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C
Date: Mon, 31 Jul 2017 15:15:39 -0700
Message-ID: <CAGZ79kaWn9z47Va=VW4R2Aswws1N5n2u4Kvatn73s0YnV0pVqQ@mail.gmail.com> (raw)
In-Reply-To: <20170731205621.24305-9-pc44800@gmail.com>

On Mon, Jul 31, 2017 at 1:56 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> The submodule subcommand 'summary' is ported in the process of
> making git-submodule a builtin. The function cmd_summary() from
> git-submodule.sh is ported to functions module_summary(),
> compute_summary_module_list(), prepare_submodule_summary() and
> print_submodule_summary().
>
> The first function module_summary() parses the options of submodule
> subcommand and also acts as the front-end of this subcommand.
> After parsing them, it calls the compute_summary_module_list()
>
> The functions compute_summary_module_list() runs the diff_cmd,
> and generates the modules list, as required by the subcommand.
> The generation of this module list is done by the using the
> callback function submodule_summary_callback(), and stored in the
> structure module_cb.
>
> Once the module list is generated, prepare_submodule_summary()
> further goes through the list and filters the list, for
> eventually calling the print_submodule_summary() function.
>
> Finally, the print_submodule_summary() takes care of generating
> and printing the summary for each submodule.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> In this new version, the following changes have been made:
>
> * Firstly, about the function compute_summary_module_list().
>   This function is created to generate the list of modules, for which
>   we will generate the summary further. Since the list is actually
>   generated using the git-diff-files or git-diff-index command, but for
>   porting this, we required to create a function similar to the builtin
>   functions of the above commands. But we can't directly call cmd_diff_files()
>   and cmd_diff_index() since we don't have to display the output and instead
>   need to store it. Hence, this function is introduced.
>
> * Also, the module_cb_list *list is not freed since it is a non-heap object.
>   Hence, free() can't be using on the non-heap objects.
>
> * In the function prepare_submodule_summary(), as suggested
>   'git_config_get_string_const' was used instead of instead of '_value'
>
> * Some variables which weren't modified throughout the function-call were
>   passed as const.
>
> * The '!!' trick, which wasn't used in the last patch, is now used in this
>   new version .
>
> * the variables sha1_dst and sha1_src are removed from the function
>   print_submodule_summary(), and instead the p->oid_src and p->oid_dst are
>   used.
>
> * The variable sm_git_dir is freed at the end of the function.
>
> * variable head was no longer used in module_summary() and instead the strbuf
>   was utilized.
>
>  builtin/submodule--helper.c | 425 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 182 +------------------
>  2 files changed, 426 insertions(+), 181 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f642f9889..94438d6ce 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -13,6 +13,9 @@
>  #include "remote.h"
>  #include "refs.h"
>  #include "connect.h"
> +#include "revision.h"
> +#include "diffcore.h"
> +#include "diff.h"
>
>  typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
>                                       void *cb_data);
> @@ -766,6 +769,427 @@ static int module_name(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +struct module_cb {
> +       unsigned int mod_src;
> +       unsigned int mod_dst;
> +       struct object_id oid_src;
> +       struct object_id oid_dst;
> +       char status;
> +       const char *sm_path;
> +};
> +#define MODULE_CB_INIT { 0, 0, NULL, NULL, '\0', NULL }
> +
> +struct module_cb_list {
> +       struct module_cb **entries;
> +       int alloc, nr;
> +};
> +#define MODULE_CB_LIST_INIT { NULL, 0, 0 }
> +
> +struct summary_cb {
> +       int argc;
> +       const char **argv;
> +       const char *prefix;
> +       char *diff_cmd;
> +       unsigned int cached: 1;
> +       unsigned int for_status: 1;
> +       unsigned int quiet: 1;
> +       unsigned int files: 1;
> +       int summary_limits;
> +};
> +#define SUMMARY_CB_INIT { 0, NULL, NULL, NULL, 0, 0, 0, 0, 0 }
> +
> +static int verify_submodule_object_name(const char *sm_path, const char *sha1)
> +{
> +       struct child_process cp_rev_parse = CHILD_PROCESS_INIT;
> +
> +       cp_rev_parse.git_cmd = 1;
> +       cp_rev_parse.no_stdout = 1;
> +       cp_rev_parse.dir = sm_path;
> +       prepare_submodule_repo_env(&cp_rev_parse.env_array);
> +
> +       argv_array_pushl(&cp_rev_parse.args, "rev-parse", "-q",
> +                        "--verify", NULL);
> +       argv_array_pushf(&cp_rev_parse.args, "%s^0", sha1);
> +
> +       if (run_command(&cp_rev_parse))
> +               return 1;
> +
> +       return 0;
> +}
> +
> +static void print_submodule_summary(struct summary_cb *info,
> +                                   struct module_cb *p)
> +{
> +       int missing_src = 0;
> +       int missing_dst = 0;
> +       char *displaypath;
> +       const char *sha1_abbr_src;
> +       const char *sha1_abbr_dst;
> +       int errmsg = 0;
> +       int total_commits = -1;
> +       char *sm_git_dir = xstrfmt("%s/.git", p->sm_path);
> +       int is_sm_git_dir = 0;
> +
> +       if (!info->cached && !oidcmp(&p->oid_dst, &null_oid)) {
> +               if (S_ISGITLINK(p->mod_dst)) {
> +                       struct child_process cp_rev_parse = CHILD_PROCESS_INIT;
> +                       struct strbuf sb_rev_parse = STRBUF_INIT;
> +
> +                       cp_rev_parse.git_cmd = 1;
> +                       cp_rev_parse.no_stderr = 1;
> +                       cp_rev_parse.dir = p->sm_path;
> +                       prepare_submodule_repo_env(&cp_rev_parse.env_array);
> +
> +                       argv_array_pushl(&cp_rev_parse.args,
> +                                        "rev-parse", "HEAD", NULL);
> +                       if (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {
> +                               strbuf_strip_suffix(&sb_rev_parse, "\n");
> +
> +                               get_oid_hex(sb_rev_parse.buf, &p->oid_dst);
> +                       }
> +                       strbuf_release(&sb_rev_parse);

I think this could be replaced via
head_ref_submodule(sub->path, callback function, &where_to_store)
or is there some trickery going on, that this also works on
non-compliant submodules?
(Maybe add that as a NEEDSWORK/TODO)

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

Recently there was a discussion how to operate the
revision machinery best (search for earlier versions of
js/rebase-i-final if interested), whether we can and want
to directly set flags such as .abbrev or if we'd rather
want to push "--abbrev=0" to the diff_args before the --

bisect and archive both assign abbrev directly, so I think
we're fine here.

> +       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"))

This strcmp smells like we're encoding the state not optimally
in 'info'.  Maybe we can have an enum { DIFF_FILES, DIFF_INDEX }
instead of a string (that we assign earlier) and then have to
compare to it again.

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

This cascaded decision whether to use
setup_work_tree / read_cache_preload / read_cache
seems quite optimized, hence complicated to read. :)
I like it, though.

> +
> +       if (!summary_limits)
> +               return 0;

Good call for converting "test $summary_limit = 0 && return".
I suspected this may be an overeager optimization (as no
error checking is done at all, but that is what it is)
f2dc06a344 (git-submodule summary: limit summary size,
2008-03-11) introduced it like this.

  reply index

Thread overview: 29+ messages in thread (expand / mbox.gz / Atom feed / [top])
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
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 [this message]
2017-07-31 23:27   ` Christian Couder
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
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

Reply instructions:

You may reply publically 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 to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGZ79kaWn9z47Va=VW4R2Aswws1N5n2u4Kvatn73s0YnV0pVqQ@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pc44800@gmail.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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox