git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Stefan Beller <sbeller@google.com>
Date
Jul 31, 2017, 22:15 UTC
Message-ID
<CAGZ79kaWn9z47Va=VW4R2Aswws1N5n2u4Kvatn73s0YnV0pVqQ@mail.gmail.com>
In-Reply-To
<20170731205621.24305-9-pc44800@gmail.com>
On Mon, Jul 31, 2017 at 1:56 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
Show 160 quoted lines
> 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)

Show 21 quoted lines
> +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.

Show 10 quoted lines
> +       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.

Show 9 quoted lines
> +                       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.

Previous: Prathamesh ChavanNext: Christian Couder
Message 15 of 25 in “Update: Week-11”
  1. Prathamesh ChavanJul 31, 2017
  2. [GSoC][PATCH 01/13] submodule--helper: introduce get_submodule_displaypath()Prathamesh Chavan, Jul 31, 2017
  3. [GSoC][PATCH 02/13] submodule--helper: introduce for_each_submodule_list()Prathamesh Chavan, Jul 31, 2017
  4. [GSoC][PATCH 03/13] submodule: port set_name_rev() from shell to CPrathamesh Chavan, Jul 31, 2017
  5. [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' from shell to CPrathamesh Chavan, Jul 31, 2017
  6. Stefan BellerJul 31, 2017
  7. Prathamesh ChavanAug 1, 2017
  8. [GSoC][PATCH 05/13] submodule: port submodule subcommand 'sync' from shell to CPrathamesh Chavan, Jul 31, 2017
  9. Stefan BellerJul 31, 2017
  10. [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to CPrathamesh Chavan, Jul 31, 2017
  11. Stefan BellerJul 31, 2017
  12. Prathamesh ChavanAug 1, 2017
  13. [GSoC][PATCH 07/13] diff: change scope of the function count_lines()Prathamesh Chavan, Jul 31, 2017
  14. [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to CPrathamesh Chavan, Jul 31, 2017
  15. Stefan BellerJul 31, 2017
  16. Christian CouderJul 31, 2017
  17. Prathamesh ChavanAug 5, 2017
  18. Christian CouderAug 5, 2017
  19. Prathamesh ChavanAug 5, 2017
  20. [GSoC][PATCH 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectoryPrathamesh Chavan, Jul 31, 2017
  21. [GSoC][PATCH 10/13] submodule foreach: document '$sm_path' instead of '$path'Prathamesh Chavan, Jul 31, 2017
  22. [GSoC][PATCH 11/13] submodule foreach: clarify the '$toplevel' variable documentationPrathamesh Chavan, Jul 31, 2017
  23. [GSoC][PATCH 12/13] submodule foreach: document variable '$displaypath'Prathamesh Chavan, Jul 31, 2017
  24. [GSoC][PATCH 13/13] submodule: port submodule subcommand 'foreach' from shell to CPrathamesh Chavan, Jul 31, 2017
  25. Stefan BellerJul 31, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.