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

[Christian Couder]

On Mon, Aug 7, 2017 at 11:18 PM, Prathamesh Chavan  wrote:

> +static enum {
> +   DIFF_INDEX,
> +   DIFF_FILES
> +} diff_cmd = DIFF_INDEX;

Using an enum could be a good idea, but I am not sure about using a
static variable.

> +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(_args, diff_cmd ? "diff-files" : "diff-index");

Maybe diff_cmd could be an argument of compute_summary_module_list()
instead of a static variable, as it's only used in this function and
in module_summary() below which is calling it.

[...]

> +   if (files) {
> +   if (cached)
> +   die(_("The --cached option cannot be used with the 
> --files option"));
> +   diff_cmd++;

Couldn't this be:

   diff_cmd = DIFF_FILES;

?

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

[Prathamesh Chavan]

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
generate_submodule_summary(), print_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 generate_submodule_summary() function.

The function generate_submodule_summary() takes care of generating
the summary for each submodule and then calls the function
print_summary() for printing it.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
The major changes the patch underwent through is the splitting of the
function print_submodule_summary() into generate_submodule_summary()
and print_summary()

Apart from this, there are also minor changes which include
removal of variables sha1_abbr_dst and sha1_abbr_src,
the variable remote_key wasn't freed earlier, but now it is.

 builtin/submodule--helper.c | 428 
 git-submodule.sh| 183 +--
 2 files changed, 429 insertions(+), 182 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 02787e4a4..2080f4fb9 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);
@@ -767,6 +770,430 @@ 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;
+   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, 0, 0, 0, 0, 0 }
+
+static enum {
+   DIFF_INDEX,
+   DIFF_FILES
+} diff_cmd = DIFF_INDEX;
+
+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(_rev_parse.env_array);
+   argv_array_pushl(_rev_parse.args, "rev-parse", "-q",
+"--verify", NULL);
+   argv_array_pushf(_rev_parse.args, "%s^0", sha1);
+
+   if (run_command(_rev_parse))
+   return 1;
+
+   return 0;
+}
+
+static void print_summary(struct summary_cb *info, int errmsg, int 
total_commits,
+ int missing_src, int missing_dst, const char 
*displaypath,
+ int is_sm_git_dir, struct module_cb *p)
+{
+   if (p->status == 'T') {
+   if (S_ISGITLINK(p->mod_dst))
+   printf(_("* %s %s(blob)->%s(submodule)"),
+displaypath, 
find_unique_abbrev(p->oid_src.hash, 7),
+find_unique_abbrev(p->oid_dst.hash, 7));
+   else
+   printf(_("* %s %s(submodule)->%s(blob)"),
+displaypath, 
find_unique_abbrev(p->oid_src.hash, 7),
+find_unique_abbrev(p->oid_dst.hash, 7));
+   } else {
+   printf("* %s %s...%s", displaypath, 
find_unique_abbrev(p->oid_src.hash, 7),
+find_unique_abbrev(p->oid_dst.hash, 7));
+   }
+
+   if (total_commits < 0)
+   printf(":\n");
+   else
+   printf(" (%d):\n", total_commits);
+
+   if (errmsg) {
+   /*
+* Don't give error msg for modification whose dst is not
+* submodule, i.e. 

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

[Prathamesh Chavan]

On Sat, Aug 5, 2017 at 10:25 PM, Christian Couder
 wrote:
> On Sat, Aug 5, 2017 at 12:28 PM, Prathamesh Chavan  wrote:
>> On Tue, Aug 1, 2017 at 4:57 AM, Christian Couder
>>  wrote:
>>> On Mon, Jul 31, 2017 at 10:56 PM, Prathamesh Chavan  
>>> wrote:
>>>
>> 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, );
>> strbuf_release();
>
> It looks error prone, more fagile and less efficient to me.

I think by using enum { DIFF_INDEX, DIFF_FILES },
we can avoid using strcmp() here, and make it more efficient
not only here but also in other parts of the code, since
all such steps involving strcmp could be removed.

Thanks,
Prathamesh Chavan

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

[Christian Couder]

On Sat, Aug 5, 2017 at 12:28 PM, Prathamesh Chavan  wrote:
> On Tue, Aug 1, 2017 at 4:57 AM, Christian Couder
>  wrote:
>> On Mon, Jul 31, 2017 at 10:56 PM, Prathamesh Chavan  
>> wrote:
>>
> 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, );
> strbuf_release();

It looks error prone, more fagile and less efficient to me.

> instead of:
> ret = compute_summary_module_list(sb.len ? sb.buf : NULL, );
> if (sb.len)
> strbuf_release();

I think it is ok to call strbuf_release() many times, so the "if
(sb.len)" check above is not needed.

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

[Prathamesh Chavan]

On Tue, Aug 1, 2017 at 4:57 AM, Christian Couder
 wrote:
> On Mon, Jul 31, 2017 at 10:56 PM, Prathamesh Chavan  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(_args, info->diff_cmd);
>> +   if (info->cached)
>> +   argv_array_push(_args, "--cached");
>> +   argv_array_pushl(_args, "--ignore-submodules=dirty", "--raw",
>> +NULL);
>> +   if (head)
>> +   argv_array_push(_args, head);
>> +   argv_array_push(_args, "--");
>> +   if (info->argc)
>> +   argv_array_pushv(_args, info->argv);
>> +
>> +   git_config(git_diff_basic_config, NULL);
>> +   init_revisions(, 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,
>> +, NULL);
>> +   rev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | 
>> DIFF_FORMAT_CALLBACK;
>> +   rev.diffopt.format_callback = submodule_summary_callback;
>> +   rev.diffopt.format_callback_data = 
>> +
>> +   if (!info->cached) {
>> +   if (!strcmp(info->diff_cmd, "diff-index"))
>> +   setup_work_tree();
>> +   if (read_cache_preload() < 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(, info->cached);
>> +   else
>> +   run_diff_files(, 0);
>> +   prepare_submodule_summary(info, );
>> +
>> +   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(_rev, , 0)) {
>> +   strbuf_strip_suffix(, "\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, ) < 0 || index_fd(oid.hash, 0, , 2, 
>> prefix, 3))
>> +   die("Unable to add %s to database", oid.hash);
>> +   strbuf_addstr(, oid_to_hex());
>> +   if (argc) {
>> +   argv++;
>> +   argc--;
>> +   }
>> +   } else {
>> +   strbuf_addstr(, "HEAD");
>> +   }
>> +
>> +   if (files) {
>> +   if (cached)
>> +   die(_("The --cached option cannot be used with the 
>> --files option"));
>> +   diff_cmd = "diff-files";
>> +
>> +   strbuf_reset();
>
> 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()
> 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(, NULL), );
>
> Maybe you could pass: "sb.len > 0 ? strbuf_detach(, 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()
> after calling compute_summary_module_list() and before returning from
> module_summary() instead of having 

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

[Christian Couder]

On Mon, Jul 31, 2017 at 10:56 PM, Prathamesh Chavan  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(_args, info->diff_cmd);
> +   if (info->cached)
> +   argv_array_push(_args, "--cached");
> +   argv_array_pushl(_args, "--ignore-submodules=dirty", "--raw",
> +NULL);
> +   if (head)
> +   argv_array_push(_args, head);
> +   argv_array_push(_args, "--");
> +   if (info->argc)
> +   argv_array_pushv(_args, info->argv);
> +
> +   git_config(git_diff_basic_config, NULL);
> +   init_revisions(, 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,
> +, NULL);
> +   rev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | 
> DIFF_FORMAT_CALLBACK;
> +   rev.diffopt.format_callback = submodule_summary_callback;
> +   rev.diffopt.format_callback_data = 
> +
> +   if (!info->cached) {
> +   if (!strcmp(info->diff_cmd, "diff-index"))
> +   setup_work_tree();
> +   if (read_cache_preload() < 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(, info->cached);
> +   else
> +   run_diff_files(, 0);
> +   prepare_submodule_summary(info, );
> +
> +   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(_rev, , 0)) {
> +   strbuf_strip_suffix(, "\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, ) < 0 || index_fd(oid.hash, 0, , 2, 
> prefix, 3))
> +   die("Unable to add %s to database", oid.hash);
> +   strbuf_addstr(, oid_to_hex());
> +   if (argc) {
> +   argv++;
> +   argc--;
> +   }
> +   } else {
> +   strbuf_addstr(, "HEAD");
> +   }
> +
> +   if (files) {
> +   if (cached)
> +   die(_("The --cached option cannot be used with the 
> --files option"));
> +   diff_cmd = "diff-files";
> +
> +   strbuf_reset();

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()
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(, NULL), );

Maybe you could pass: "sb.len > 0 ? strbuf_detach(, 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()
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.

> +}

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

[Stefan Beller]

On Mon, Jul 31, 2017 at 1:56 PM, Prathamesh Chavan  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 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
> 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(_rev_parse.env_array);
> +
> +   argv_array_pushl(_rev_parse.args, "rev-parse", "-q",
> +"--verify", NULL);
> +   argv_array_pushf(_rev_parse.args, "%s^0", sha1);
> +
> +   if (run_command(_rev_parse))
> +   return 1;
> +
> +   return 0;
> +}
> +
> +static void 

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

[Prathamesh Chavan]

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 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
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(_rev_parse.env_array);
+
+   argv_array_pushl(_rev_parse.args, "rev-parse", "-q",
+"--verify", NULL);
+   argv_array_pushf(_rev_parse.args, "%s^0", sha1);
+
+   if (run_command(_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 

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

[Brandon Williams]

On 07/25, Prathamesh Chavan 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 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 

This is a big one, hopefully I can try to understand everything that is
happening.

> ---
> In this new version of patch, following changes were made:
> * the way of generating sub_sha1_src and sub_sha1_dst (abbrev of sha1_src
>   and sha1_dst resp.) were changed. Since there was no direct way of
>   abbrevating a string(sha1_dst), in this patch sha1_dst was converted first
>   to an object id (converting to sha1 was avoided) and then abbrevated using
>   find_unique_abbrev().
> * A few big if() statements were reduced.
> * for reducing the two big if() statements, a new function
>   verify_submodule_object_name() was introduced.
> * this new version also corrects a few other nits.
> 
>  builtin/submodule--helper.c | 428 
> 
>  git-submodule.sh| 182 +--
>  2 files changed, 429 insertions(+), 181 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5e84fc42d..94d6254f0 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);
> @@ -762,6 +765,430 @@ 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(_rev_parse.env_array);
> +
> + argv_array_pushl(_rev_parse.args, "rev-parse", "-q",
> +  "--verify", NULL);
> + argv_array_pushf(_rev_parse.args, "%s^0", sha1);
> +
> + if (run_command(_rev_parse))
> + return 1;
> +
> + return 0;
> +}
> +
> +static void print_submodule_summary(struct summary_cb *info,
> + struct module_cb *p)
> +{

This is one large function so it may be difficult to keep track of
everything and I don't know how easy it would be to split.

> + int missing_src = 0;
> + int missing_dst = 0;
> + char *displaypath;
> + const char *sha1_abbr_src;
> + const char *sha1_abbr_dst;
> + struct object_id oid_dst;
> + int errmsg = 0;
> + int total_commits = -1;
> + const char *sha1_dst = oid_to_hex(>oid_dst);
> + const char *sha1_src = oid_to_hex(>oid_src);

You have these two variables which are defined as 'const char *' yet you
allocate memory for them at different points via 'xstrdup' and then
never free it.  Really what you should be trying to do is use 'struct
object_id' as much as you can instead of storing the 

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

[Prathamesh Chavan]

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 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
In this new version of patch, following changes were made:
* the way of generating sub_sha1_src and sub_sha1_dst (abbrev of sha1_src
  and sha1_dst resp.) were changed. Since there was no direct way of
  abbrevating a string(sha1_dst), in this patch sha1_dst was converted first
  to an object id (converting to sha1 was avoided) and then abbrevated using
  find_unique_abbrev().
* A few big if() statements were reduced.
* for reducing the two big if() statements, a new function
  verify_submodule_object_name() was introduced.
* this new version also corrects a few other nits.

 builtin/submodule--helper.c | 428 
 git-submodule.sh| 182 +--
 2 files changed, 429 insertions(+), 181 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5e84fc42d..94d6254f0 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);
@@ -762,6 +765,430 @@ 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(_rev_parse.env_array);
+
+   argv_array_pushl(_rev_parse.args, "rev-parse", "-q",
+"--verify", NULL);
+   argv_array_pushf(_rev_parse.args, "%s^0", sha1);
+
+   if (run_command(_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;
+   struct object_id oid_dst;
+   int errmsg = 0;
+   int total_commits = -1;
+   const char *sha1_dst = oid_to_hex(>oid_dst);
+   const char *sha1_src = oid_to_hex(>oid_src);
+   char *sm_git_dir = xstrfmt("%s/.git", p->sm_path);
+   int is_sm_git_dir = 0;
+
+   if (!info->cached && !strcmp(sha1_dst, sha1_to_hex(null_sha1))) {
+   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(_rev_parse.env_array);
+
+   argv_array_pushl(_rev_parse.args,
+