{"thread":{"id":"53791","subject":"[GSoC][PATCH 0/4] submodule: port 'summary' from Shell to C","startedAt":"2020-07-02T19:24:38Z","lastAt":"2020-08-10T13:24:13Z","messageCount":21,"participants":["Shourya Shukla","Johannes Schindelin","Philip Oakley","Đoàn Trần Công Danh","Christian Couder","Kaartic Sivaraam"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"400976","messageId":"20200702192409.21865-1-shouryashukla.oo@gmail.com","threadId":"53791","inReplyTo":null,"subject":"[GSoC][PATCH 0/4] submodule: port 'summary' from Shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-02T19:24:05Z","receivedAt":"2020-07-02T19:24:38Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hello all!\n\nThis is third subcommand in line to be converted as a part of my GSoC\nproject. The other two were 'set-url' and 'set-branch' both of which\nhave been merged into Git. This port has been taken off from what\nPrathamesh Chavan left as a part of his GSoC project in 2017 under\nStefan Beller and Christian Couder. As written in my proposal and\nadvised by my mentors Kaartic and Christian, it was decided to re-use\nPC's work for multiple reasons.\n\nHere is the link to the v1 of his patch:\nhttps://lore.kernel.org/git/20170731205621.24305-9-pc44800@gmail.com/\n\nHere is the link to the v2 of his patch:\nhttps://lore.kernel.org/git/20170807211900.15001-9-pc44800@gmail.com/\n\nEven though this patch series draws on Prathamesh' work, a lot extra\nfeatures had to be added along with some remodelling. A summary of the 4\ncommits in this patch are as follows: the patch consists of 3\npreparatory patches preceding the main commit porting 'git submodule summary'.\n\nBeing a bit more verbose about the prepatory patches:\n\n    1. a8dcc59a6a amends an extra linefeed between callback struct(s)\n       macro(s) of subcommands 'init', 'status' and 'sync' so as to make\n       the whole format of subcommands a bit more uniform and\n       consistent.\n\n    2. 5f17ca37e0 renames the helper functions\n       'show_submodule_summary()', 'prepare_submodule_summary()' and\n       'print_submodule_summary()' used by the 'builtin_diff()' function\n       of diff.c This was done so as to avoid any ambiguity later on\n       when the helper functions used by 'summary' will be defined in\n       the upcoming port. The functions are renamed to\n       '*_diff_submodule_summary()' so as to classify them as the one\n       being used by the diff machinery.\n\n    3. 5945ab4113 changes the scope of the 'count_lines()' function\n       defined in diff.c so as to make it usable by other entities of\n       the code. This was originally a patch authored by PC with a small\n       change that the function instead of being defined as an 'extern\n       int', it is now defined as 'int' since f758a7f8ac4 by Nguyễn Thái\n       Ngọc Duy removed extern from the function declarations in diff.h\n\nNow moving on to the main patch 4/4 that will port summary.\n\n    17738e9df4 ports the subcommand summary from Shell to C. I will not\n    be repetitive by stating how all the functions communicate since\n    this is covered in the commit message. What I will focus on is the\n    bits I had to tweak along with any extra functions added. Originally\n    the functions were: module_summary(),\n    compute_summary_submodule_list(), submodule_summary_callback(),\n    prepare_submodule_summary(), generate_submodule_summary(),\n    print_submodule_summary() and verify_submodule_object_name. I had to\n    add another function called get_diff_cmd() which fetches a const\n    char * (a diff-index or a diff-files) in response to the enum provided\n    to it and bugs out in case of an unexpected enum value. I also had\n    to tweak the enum declared by PC by NOT keeping it static and\n    tweaking its definition just a little. These were suggested by\n    Christian on PC's patch at the time.\n\n    There were also some changes to function parameters such as those\n    of index_fd() by passing the the_index parameter to it or the case\n    of submodule_from_path() where it was needed to add another parameter\n    of the_repository. This all must have been due to change of function\n    defintions in the past 3 years.\n\n    Even after all this, there were two problems: t7418 had been failing\n    (something which I feel might have been overlooked/not tested by PC\n    during his time) and the CI build failed. First let's cover the test\n    failure.\n\n    The main test for testing summary is t7401. t7418 is another test\n    which involves sparse checkouts and submodules. The test creates a\n    sparse checkout clone excluding the .gitmodules file to test the\n    functionality of the '--for-status' option. After quite a thorough\n    investigation me, Kaartic and Christian figured out that this port\n    fails to handle cases where a gitfile exists instead of a .git dir\n    since all the tests in t7401 did not any submodules which were\n    cloned in any way but rather just exist in a nearby directory and\n    are used in our tests (this is something I feel should be fixed and\n    hence I am saving this for later). But anyway, we decided that it\n    will be excellent to check for a non-bare repo instead of checking\n    for a gitdir therefore replacing the function is_git_directory()\n    with is_nonbare_repository() in the first non-nested if-check in\n    generate_submodule_summary().\n\n    Regarding the CI failure. The build was failing due to the\n    function oidcmp() in the first if-check of\n    generate_submodule_summary(). The CI build suggested that we use\n    oideq() instead so that the build passes and on doing the change, it\n    did!\n\n    There were some other cosmetic changes such as wrapping of\n    columns upto 80 chars (to follow the CodingGuidelines) and\n    improving some error messages and option descriptions,\n\nFinally, since it was originally PC's work, I have kept him as the author\nfor the final 2 commits and kept the date as-is (from 2017). This patch\nis not perfect and hence needs the feedback from all of you :)\n\nHere is a 'git log --oneline' of the commits:\n\n    17738e9df4 submodule: port submodule subcommand 'summary' from shell to C\n    5945ab4113 diff: change scope of the function count_lines()\n    5f17ca37e0 submodule: rename helper functions to avoid ambiguity\n    a8dcc59a6a submodule: amend extra line feed between callback struct and macro\n\nThanks,\nShourya Shukla\n\n\nPrathamesh Chavan (2):\n  diff: change scope of the function count_lines()\n  submodule: port submodule subcommand 'summary' from shell to C\n\nShourya Shukla (2):\n  submodule: amend extra line feed between callback struct and macro\n  submodule: rename helper functions to avoid ambiguity\n\n builtin/submodule--helper.c | 454 +++++++++++++++++++++++++++++++++++-\n diff.c                      |   4 +-\n diff.h                      |   1 +\n git-submodule.sh            | 186 +--------------\n submodule.c                 |  10 +-\n submodule.h                 |   2 +-\n 6 files changed, 461 insertions(+), 196 deletions(-)\n\n-- \n2.27.0\n\n"},{"id":"400977","messageId":"20200702192409.21865-2-shouryashukla.oo@gmail.com","threadId":"53791","inReplyTo":"20200702192409.21865-1-shouryashukla.oo@gmail.com","subject":"[PATCH 1/4] submodule: amend extra line feed between callback struct and macro","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-02T19:24:06Z","receivedAt":"2020-07-02T19:24:44Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"All subcommands of 'git submodule' using a callback mechanism had\nabsence of an extra linefeed between their callback structs and\nmacros. Subcommands 'init', 'status' and 'sync' did not follow suit.\nAmend the extra line feed.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\n builtin/submodule--helper.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 59c1e1217c..eea3932c40 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -612,7 +612,6 @@ struct init_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n };\n-\n #define INIT_CB_INIT { NULL, 0 }\n \n static void init_submodule(const char *path, const char *prefix,\n@@ -742,7 +741,6 @@ struct status_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n };\n-\n #define STATUS_CB_INIT { NULL, 0 }\n \n static void print_status(unsigned int flags, char state, const char *path,\n@@ -933,7 +931,6 @@ struct sync_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n };\n-\n #define SYNC_CB_INIT { NULL, 0 }\n \n static void sync_submodule(const char *path, const char *prefix,\n-- \n2.27.0\n\n"},{"id":"400978","messageId":"20200702192409.21865-3-shouryashukla.oo@gmail.com","threadId":"53791","inReplyTo":"20200702192409.21865-1-shouryashukla.oo@gmail.com","subject":"[PATCH 2/4] submodule: rename helper functions to avoid ambiguity","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-02T19:24:07Z","receivedAt":"2020-07-02T19:24:54Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"The helper functions: show_submodule_summary(),\nprepare_submodule_summary() and print_submodule_summary() are used by\nthe builtin_diff() function in diff.c to generate a summary of\nsubmodules in the context of a diff. Functions with similar names are to\nbe introduced in the upcoming port of submodule's summary subcommand.\n\nSo, rename the helper functions to '*_diff_submodule_summary()' to avoid\nambiguity.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\n diff.c      |  2 +-\n submodule.c | 10 +++++-----\n submodule.h |  2 +-\n 3 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d24aaa3047..4a2c631c37 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3429,7 +3429,7 @@ static void builtin_diff(const char *name_a,\n \tif (o->submodule_format == DIFF_SUBMODULE_LOG &&\n \t    (!one->mode || S_ISGITLINK(one->mode)) &&\n \t    (!two->mode || S_ISGITLINK(two->mode))) {\n-\t\tshow_submodule_summary(o, one->path ? one->path : two->path,\n+\t\tshow_submodule_diff_summary(o, one->path ? one->path : two->path,\n \t\t\t\t&one->oid, &two->oid,\n \t\t\t\ttwo->dirty_submodule);\n \t\treturn;\ndiff --git a/submodule.c b/submodule.c\nindex e2ef5698c8..097902ee67 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -438,7 +438,7 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,\n \t */\n }\n \n-static int prepare_submodule_summary(struct rev_info *rev, const char *path,\n+static int prepare_submodule_diff_summary(struct rev_info *rev, const char *path,\n \t\tstruct commit *left, struct commit *right,\n \t\tstruct commit_list *merge_bases)\n {\n@@ -459,7 +459,7 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,\n \treturn prepare_revision_walk(rev);\n }\n \n-static void print_submodule_summary(struct repository *r, struct rev_info *rev, struct diff_options *o)\n+static void print_submodule_diff_summary(struct repository *r, struct rev_info *rev, struct diff_options *o)\n {\n \tstatic const char format[] = \"  %m %s\";\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -610,7 +610,7 @@ static void show_submodule_header(struct diff_options *o,\n \tstrbuf_release(&sb);\n }\n \n-void show_submodule_summary(struct diff_options *o, const char *path,\n+void show_submodule_diff_summary(struct diff_options *o, const char *path,\n \t\tstruct object_id *one, struct object_id *two,\n \t\tunsigned dirty_submodule)\n {\n@@ -632,12 +632,12 @@ void show_submodule_summary(struct diff_options *o, const char *path,\n \t\tgoto out;\n \n \t/* Treat revision walker failure the same as missing commits */\n-\tif (prepare_submodule_summary(&rev, path, left, right, merge_bases)) {\n+\tif (prepare_submodule_diff_summary(&rev, path, left, right, merge_bases)) {\n \t\tdiff_emit_submodule_error(o, \"(revision walker failed)\\n\");\n \t\tgoto out;\n \t}\n \n-\tprint_submodule_summary(sub, &rev, o);\n+\tprint_submodule_diff_summary(sub, &rev, o);\n \n out:\n \tif (merge_bases)\ndiff --git a/submodule.h b/submodule.h\nindex 4dad649f94..22db9e1832 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -69,7 +69,7 @@ int parse_submodule_update_strategy(const char *value,\n \t\t\t\t    struct submodule_update_strategy *dst);\n const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);\n void handle_ignore_submodules_arg(struct diff_options *, const char *);\n-void show_submodule_summary(struct diff_options *o, const char *path,\n+void show_submodule_diff_summary(struct diff_options *o, const char *path,\n \t\t\t    struct object_id *one, struct object_id *two,\n \t\t\t    unsigned dirty_submodule);\n void show_submodule_inline_diff(struct diff_options *o, const char *path,\n-- \n2.27.0\n\n"},{"id":"400979","messageId":"20200702192409.21865-4-shouryashukla.oo@gmail.com","threadId":"53791","inReplyTo":"20200702192409.21865-1-shouryashukla.oo@gmail.com","subject":"[PATCH 3/4] diff: change scope of the function count_lines()","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-02T19:24:08Z","receivedAt":"2020-07-02T19:25:04Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"From: Prathamesh Chavan <pc44800@gmail.com>\n\nChange the scope of function count_lines for allowing the function\nto be reused in other parts of the code as well.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Stefan Beller <sbeller@google.com>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Prathamesh Chavan <pc44800@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\n diff.c | 2 +-\n diff.h | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4a2c631c37..39ddbcf451 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -547,7 +547,7 @@ struct emit_callback {\n \tstruct strbuf *header;\n };\n \n-static int count_lines(const char *data, int size)\n+int count_lines(const char *data, int size)\n {\n \tint count, ch, completely_empty = 1, nl_just_seen = 0;\n \tcount = 0;\ndiff --git a/diff.h b/diff.h\nindex 9443dc1b00..bd96c8c434 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -495,6 +495,7 @@ void free_diffstat_info(struct diffstat_t *diffstat);\n int parse_long_opt(const char *opt, const char **argv,\n \t\t   const char **optarg);\n \n+int count_lines(const char *data, int size);\n int git_diff_basic_config(const char *var, const char *value, void *cb);\n int git_diff_heuristic_config(const char *var, const char *value, void *cb);\n void init_diff_ui_defaults(void);\n-- \n2.27.0\n\n"},{"id":"400980","messageId":"20200702192409.21865-5-shouryashukla.oo@gmail.com","threadId":"53791","inReplyTo":"20200702192409.21865-1-shouryashukla.oo@gmail.com","subject":"[PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-02T19:24:09Z","receivedAt":"2020-07-02T19:25:12Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"From: Prathamesh Chavan <pc44800@gmail.com>\n\nThe submodule subcommand 'summary' is ported in the process of\nmaking git-submodule a builtin. The function cmd_summary() from\ngit-submodule.sh is ported to functions module_summary(),\ncompute_summary_module_list(), prepare_submodule_summary() and\ngenerate_submodule_summary(), print_submodule_summary().\n\nThe first function module_summary() parses the options of submodule\nsubcommand and also acts as the front-end of this subcommand.\nAfter parsing them, it calls the compute_summary_module_list()\n\nThe functions compute_summary_module_list() runs the diff_cmd,\nand generates the modules list, as required by the subcommand.\nThe generation of this module list is done by the using the\ncallback function submodule_summary_callback(), and stored in the\nstructure module_cb.\n\nOnce the module list is generated, prepare_submodule_summary()\nfurther goes through the list and filters the list, for\neventually calling the generate_submodule_summary() function.\n\nThe function generate_submodule_summary() takes care of generating\nthe summary for each submodule and then calls the function\nprint_submodule_summary() for printing it.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Stefan Beller <sbeller@google.com>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Prathamesh Chavan <pc44800@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\n builtin/submodule--helper.c | 451 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 186 +--------------\n 2 files changed, 452 insertions(+), 185 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex eea3932c40..1dbdb934f1 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -927,6 +927,456 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct module_cb {\n+\tunsigned int mod_src;\n+\tunsigned int mod_dst;\n+\tstruct object_id oid_src;\n+\tstruct object_id oid_dst;\n+\tchar status;\n+\tconst char *sm_path;\n+};\n+#define MODULE_CB_INIT { 0, 0, NULL, NULL, '\\0', NULL }\n+\n+struct module_cb_list {\n+\tstruct module_cb **entries;\n+\tint alloc, nr;\n+};\n+#define MODULE_CB_LIST_INIT { NULL, 0, 0 }\n+\n+struct summary_cb {\n+\tint argc;\n+\tconst char **argv;\n+\tconst char *prefix;\n+\tunsigned int cached: 1;\n+\tunsigned int for_status: 1;\n+\tunsigned int quiet: 1;\n+\tunsigned int files: 1;\n+\tint summary_limit;\n+};\n+#define SUMMARY_CB_INIT { 0, NULL, NULL, 0, 0, 0, 0, 0 }\n+\n+enum diff_cmd {\n+\tDIFF_INDEX,\n+\tDIFF_FILES\n+};\n+\n+static int verify_submodule_object_name(const char *sm_path,\n+\t\t\t\t\t  const char *sha1)\n+{\n+\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n+\n+\tcp_rev_parse.git_cmd = 1;\n+\tcp_rev_parse.no_stdout = 1;\n+\tcp_rev_parse.dir = sm_path;\n+\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n+\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\", \"-q\",\n+\t\t\t \"--verify\", NULL);\n+\targv_array_pushf(&cp_rev_parse.args, \"%s^0\", sha1);\n+\n+\tif (run_command(&cp_rev_parse))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n+static void print_submodule_summary(struct summary_cb *info, int errmsg,\n+\t\t\t\t      int total_commits, int missing_src,\n+\t\t\t\t      int missing_dst, const char *displaypath,\n+\t\t\t\t      int is_sm_git_dir, struct module_cb *p)\n+{\n+\tif (p->status == 'T') {\n+\t\tif (S_ISGITLINK(p->mod_dst))\n+\t\t\tprintf(_(\"* %s %s(blob)->%s(submodule)\"),\n+\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n+\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n+\t\telse\n+\t\t\tprintf(_(\"* %s %s(submodule)->%s(blob)\"),\n+\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n+\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n+\t} else {\n+\t\tprintf(\"* %s %s...%s\",\n+\t\t\tdisplaypath, find_unique_abbrev(&p->oid_src, 7),\n+\t\t\tfind_unique_abbrev(&p->oid_dst, 7));\n+\t}\n+\n+\tif (total_commits < 0)\n+\t\tprintf(\":\\n\");\n+\telse\n+\t\tprintf(\" (%d):\\n\", total_commits);\n+\n+\tif (errmsg) {\n+\t\t/*\n+\t\t * Don't give error msg for modification whose dst is not\n+\t\t * submodule, i.e. deleted or changed to blob\n+\t\t */\n+\t\tif (S_ISGITLINK(p->mod_src)) {\n+\t\t\tif (missing_src && missing_dst) {\n+\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commits %s and %s\\n\"),\n+\t\t\t\t       displaypath, oid_to_hex(&p->oid_src),\n+\t\t\t\t       oid_to_hex(&p->oid_dst));\n+\t\t\t} else if (missing_src) {\n+\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n+\t\t\t\t       displaypath, oid_to_hex(&p->oid_src));\n+\t\t\t} else {\n+\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n+\t\t\t\t       displaypath, oid_to_hex(&p->oid_dst));\n+\t\t\t}\n+\t\t}\n+\t} else if (is_sm_git_dir) {\n+\t\tstruct child_process cp_log = CHILD_PROCESS_INIT;\n+\n+\t\tcp_log.git_cmd = 1;\n+\t\tcp_log.dir = p->sm_path;\n+\t\tprepare_submodule_repo_env(&cp_log.env_array);\n+\t\targv_array_pushl(&cp_log.args, \"log\", NULL);\n+\n+\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst)) {\n+\t\t\tif (info->summary_limit > 0)\n+\t\t\t\targv_array_pushf(&cp_log.args, \"-%d\",\n+\t\t\t\t\t\t info->summary_limit);\n+\n+\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  %m %s\",\n+\t\t\t\t\t \"--first-parent\", NULL);\n+\t\t\targv_array_pushf(&cp_log.args, \"%s...%s\",\n+\t\t\t\t\t oid_to_hex(&p->oid_src),\n+\t\t\t\t\t oid_to_hex(&p->oid_dst));\n+\t\t} else if (S_ISGITLINK(p->mod_dst)) {\n+\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  > %s\",\n+\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_dst), NULL);\n+\t\t} else {\n+\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  < %s\",\n+\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_src), NULL);\n+\t\t}\n+\t\trun_command(&cp_log);\n+\t}\n+\tprintf(\"\\n\");\n+}\n+\n+static void generate_submodule_summary(struct summary_cb *info,\n+\t\t\t\t       struct module_cb *p)\n+{\n+\tint missing_src = 0;\n+\tint missing_dst = 0;\n+\tchar *displaypath;\n+\tint errmsg = 0;\n+\tint total_commits = -1;\n+\tint is_sm_git_dir = 0;\n+\tstruct strbuf sm_git_dir_sb = STRBUF_INIT;\n+\n+\tif (!info->cached && oideq(&p->oid_dst, &null_oid)) {\n+\t\tif (S_ISGITLINK(p->mod_dst)) {\n+\t\t\t/*\n+\t\t\t * NEEDSWORK: avoid using separate process with\n+\t\t\t * the help of the function head_ref_submodule()\n+\t\t\t */\n+\t\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf sb_rev_parse = STRBUF_INIT;\n+\n+\t\t\tcp_rev_parse.git_cmd = 1;\n+\t\t\tcp_rev_parse.no_stderr = 1;\n+\t\t\tcp_rev_parse.dir = p->sm_path;\n+\t\t\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n+\n+\t\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n+\t\t\t\t\t \"HEAD\", NULL);\n+\t\t\tif (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n+\t\t\t\tstrbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n+\t\t\t\tget_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n+\t\t\t}\n+\t\t\tstrbuf_release(&sb_rev_parse);\n+\t\t} else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n+\t\t\tstruct child_process cp_hash_object = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf sb_hash_object = STRBUF_INIT;\n+\n+\t\t\tcp_hash_object.git_cmd = 1;\n+\t\t\targv_array_pushl(&cp_hash_object.args, \"hash-object\",\n+\t\t\t\t\t p->sm_path, NULL);\n+\t\t\tif (!capture_command(&cp_hash_object,\n+\t\t\t\t\t     &sb_hash_object, 0)) {\n+\t\t\t\tstrbuf_strip_suffix(&sb_hash_object, \"\\n\");\n+\t\t\t\tget_oid_hex(sb_hash_object.buf, &p->oid_dst);\n+\t\t\t}\n+\t\t\tstrbuf_release(&sb_hash_object);\n+\t\t} else {\n+\t\t\tif (p->mod_dst)\n+\t\t\t\tdie(_(\"unexpected mode %d\\n\"), p->mod_dst);\n+\t\t}\n+\t}\n+\n+\tstrbuf_addstr(&sm_git_dir_sb, p->sm_path);\n+\tif (is_nonbare_repository_dir(&sm_git_dir_sb))\n+\t\tis_sm_git_dir = 1;\n+\n+\tif (is_sm_git_dir && S_ISGITLINK(p->mod_src))\n+\t\tmissing_src = verify_submodule_object_name(p->sm_path,\n+\t\t\t\t\t\t\t   oid_to_hex(&p->oid_src));\n+\n+\tif (is_sm_git_dir && S_ISGITLINK(p->mod_dst))\n+\t\tmissing_dst = verify_submodule_object_name(p->sm_path,\n+\t\t\t\t\t\t\t   oid_to_hex(&p->oid_dst));\n+\n+\tdisplaypath = get_submodule_displaypath(p->sm_path, info->prefix);\n+\n+\tif (!missing_dst && !missing_src) {\n+\t\tif (is_sm_git_dir) {\n+\t\t\tstruct child_process cp_rev_list = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf sb_rev_list = STRBUF_INIT;\n+\t\t\tchar *range;\n+\n+\t\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n+\t\t\t\trange = xstrfmt(\"%s...%s\", oid_to_hex(&p->oid_src),\n+\t\t\t\t\t\toid_to_hex(&p->oid_dst));\n+\t\t\telse if (S_ISGITLINK(p->mod_src))\n+\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_src));\n+\t\t\telse\n+\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_dst));\n+\n+\t\t\tcp_rev_list.git_cmd = 1;\n+\t\t\tcp_rev_list.dir = p->sm_path;\n+\t\t\tprepare_submodule_repo_env(&cp_rev_list.env_array);\n+\n+\t\t\targv_array_pushl(&cp_rev_list.args, \"rev-list\",\n+\t\t\t\t\t \"--first-parent\", range, \"--\", NULL);\n+\t\t\tif (!capture_command(&cp_rev_list, &sb_rev_list, 0)) {\n+\t\t\t\tif (sb_rev_list.len)\n+\t\t\t\t\ttotal_commits = count_lines(sb_rev_list.buf,\n+\t\t\t\t\t\t\t\t    sb_rev_list.len);\n+\t\t\t\telse\n+\t\t\t\t\ttotal_commits = 0;\n+\t\t\t}\n+\n+\t\t\tfree(range);\n+\t\t\tstrbuf_release(&sb_rev_list);\n+\t\t}\n+\t} else {\n+\t\terrmsg = 1;\n+\t}\n+\n+\tprint_submodule_summary(info, errmsg, total_commits,\n+\t\t\t\tmissing_src, missing_dst,\n+\t\t      \t\tdisplaypath, is_sm_git_dir, p);\n+\n+\tfree(displaypath);\n+\tstrbuf_release(&sm_git_dir_sb);\n+}\n+\n+static void prepare_submodule_summary(struct summary_cb *info,\n+\t\t\t\t      struct module_cb_list *list)\n+{\n+\tint i;\n+\tfor (i = 0; i < list->nr; i++) {\n+\t\tstruct module_cb *p = list->entries[i];\n+\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n+\n+\t\tif (p->status == 'D' || p->status == 'T') {\n+\t\t\tgenerate_submodule_summary(info, p);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (info->for_status) {\n+\t\t\tchar *config_key;\n+\t\t\tconst char *ignore_config = \"none\";\n+\t\t\tconst char *value;\n+\t\t\tconst struct submodule *sub = submodule_from_path(the_repository,\n+\t\t\t\t\t\t\t\t\t  &null_oid,\n+\t\t\t\t\t\t\t\t\t  p->sm_path);\n+\n+\t\t\tif (sub && p->status != 'A') {\n+\t\t\t\tconfig_key = xstrfmt(\"submodule.%s.ignore\",\n+\t\t\t\t\t\t     sub->name);\n+\t\t\t\tif (!git_config_get_string_const(config_key, &value))\n+\t\t\t\t\tignore_config = value;\n+\t\t\t\telse if (sub->ignore)\n+\t\t\t\t\tignore_config = sub->ignore;\n+\n+\t\t\t\tfree(config_key);\n+\t\t\t\tif (!strcmp(ignore_config, \"all\"))\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n+\t\t/* Also show added or modified modules which are checked out */\n+\t\tcp_rev_parse.dir = p->sm_path;\n+\t\tcp_rev_parse.git_cmd = 1;\n+\t\tcp_rev_parse.no_stderr = 1;\n+\t\tcp_rev_parse.no_stdout = 1;\n+\n+\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n+\t\t\t\t \"--git-dir\", NULL);\n+\n+\t\tif (!run_command(&cp_rev_parse))\n+\t\t\tgenerate_submodule_summary(info, p);\n+\t}\n+}\n+\n+static void submodule_summary_callback(struct diff_queue_struct *q,\n+\t\t\t\t       struct diff_options *options,\n+\t\t\t\t       void *data)\n+{\n+\tint i;\n+\tstruct module_cb_list *list = data;\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tstruct module_cb *temp;\n+\n+\t\tif (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode))\n+\t\t\tcontinue;\n+\t\ttemp = (struct module_cb*)malloc(sizeof(struct module_cb));\n+\t\ttemp->mod_src = p->one->mode;\n+\t\ttemp->mod_dst = p->two->mode;\n+\t\ttemp->oid_src = p->one->oid;\n+\t\ttemp->oid_dst = p->two->oid;\n+\t\ttemp->status = p->status;\n+\t\ttemp->sm_path = xstrdup(p->one->path);\n+\n+\t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n+\t\tlist->entries[list->nr++] = temp;\n+\t}\n+}\n+\n+static const char *get_diff_cmd(enum diff_cmd diff_cmd)\n+{\n+\tswitch (diff_cmd) {\n+\tcase DIFF_INDEX: return \"diff-index\";\n+\tcase DIFF_FILES: return \"diff-files\";\n+\tdefault: BUG(\"bad diff_cmd value %d\", diff_cmd);\n+\t}\n+}\n+\n+static int compute_summary_module_list(char *head,\n+\t\t\t\t         struct summary_cb *info,\n+\t\t\t\t         enum diff_cmd diff_cmd)\n+{\n+\tstruct argv_array diff_args = ARGV_ARRAY_INIT;\n+\tstruct rev_info rev;\n+\tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n+\n+\targv_array_push(&diff_args, get_diff_cmd(diff_cmd));\n+\tif (info->cached)\n+\t\targv_array_push(&diff_args, \"--cached\");\n+\targv_array_pushl(&diff_args, \"--ignore-submodules=dirty\", \"--raw\",\n+\t\t\t NULL);\n+\tif (head)\n+\t\targv_array_push(&diff_args, head);\n+\targv_array_push(&diff_args, \"--\");\n+\tif (info->argc)\n+\t\targv_array_pushv(&diff_args, info->argv);\n+\n+\tgit_config(git_diff_basic_config, NULL);\n+\tinit_revisions(&rev, info->prefix);\n+\trev.abbrev = 0;\n+\tprecompose_argv(diff_args.argc, diff_args.argv);\n+\n+\tdiff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,\n+\t\t\t\t\t &rev, NULL);\n+\trev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = submodule_summary_callback;\n+\trev.diffopt.format_callback_data = &list;\n+\n+\tif (!info->cached) {\n+\t\tif (diff_cmd ==  DIFF_INDEX)\n+\t\t\tsetup_work_tree();\n+\t\tif (read_cache_preload(&rev.diffopt.pathspec) < 0) {\n+\t\t\tperror(\"read_cache_preload\");\n+\t\t\treturn -1;\n+\t\t}\n+\t} else if (read_cache() < 0) {\n+\t\tperror(\"read_cache\");\n+\t\treturn -1;\n+\t}\n+\n+\tif (diff_cmd == DIFF_INDEX)\n+\t\trun_diff_index(&rev, info->cached);\n+\telse\n+\t\trun_diff_files(&rev, 0);\n+\tprepare_submodule_summary(info, &list);\n+\treturn 0;\n+}\n+\n+static int module_summary(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct summary_cb info = SUMMARY_CB_INIT;\n+\tint cached = 0;\n+\tint for_status = 0;\n+\tint quiet = 0;\n+\tint files = 0;\n+\tint summary_limit = -1;\n+\tstruct child_process cp_rev = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tenum diff_cmd diff_cmd = DIFF_INDEX;\n+\tint ret;\n+\n+\tstruct option module_summary_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output of summarising submodules\")),\n+\t\tOPT_BOOL(0, \"cached\", &cached,\n+\t\t\t N_(\"Use the commit stored in the index instead of the submodule HEAD\")),\n+\t\tOPT_BOOL(0, \"files\", &files,\n+\t\t\t N_(\"To compare the commit in the index with that in the submodule HEAD\")),\n+\t\tOPT_BOOL(0, \"for-status\", &for_status,\n+\t\t\t N_(\"Skip submodules with 'ignore_config' value set to 'all'\")),\n+\t\tOPT_INTEGER('n', \"summary-limit\", &summary_limit,\n+\t\t\t     N_(\"Limit the summary size\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper summary [<options>] [commit] [--] [<path>]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_summary_options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tif (!summary_limit)\n+\t\treturn 0;\n+\n+\tcp_rev.git_cmd = 1;\n+\targv_array_pushl(&cp_rev.args, \"rev-parse\", \"-q\", \"--verify\",\n+\t\t\t argc ? argv[0] : \"HEAD\", NULL);\n+\n+\tif (!capture_command(&cp_rev, &sb, 0)) {\n+\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\tif (argc) {\n+\t\t\targv++;\n+\t\t\targc--;\n+\t\t}\n+\t} else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n+\t\t/* before the first commit: compare with an empty tree */\n+\t\tstruct stat st;\n+\t\tstruct object_id oid;\n+\t\tif (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,\n+\t\t\t\t\t\t  prefix, 3))\n+\t\t\tdie(\"Unable to add %s to database\", oid.hash);\n+\t\tstrbuf_addstr(&sb, oid_to_hex(&oid));\n+\t\tif (argc) {\n+\t\t\targv++;\n+\t\t\targc--;\n+\t\t}\n+\t} else {\n+\t\tstrbuf_addstr(&sb, \"HEAD\");\n+\t}\n+\n+\tif (files) {\n+\t\tif (cached)\n+\t\t\tdie(_(\"--cached and --files are mutually exclusive\"));\n+\t\tdiff_cmd = DIFF_FILES;\n+\t}\n+\n+\tinfo.argc = argc;\n+\tinfo.argv = argv;\n+\tinfo.prefix = prefix;\n+\tinfo.cached = !!cached;\n+\tinfo.for_status = !!for_status;\n+\tinfo.quiet = quiet;\n+\tinfo.files = files;\n+\tinfo.summary_limit = summary_limit;\n+\n+\tret = compute_summary_module_list((diff_cmd == DIFF_FILES) ? NULL : sb.buf,\n+\t\t\t\t\t   &info, diff_cmd);\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n struct sync_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n@@ -2341,6 +2791,7 @@ static struct cmd_struct commands[] = {\n \t{\"print-default-remote\", print_default_remote, 0},\n \t{\"sync\", module_sync, SUPPORT_SUPER_PREFIX},\n \t{\"deinit\", module_deinit, 0},\n+\t{\"summary\", module_summary, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43eb6051d2..899b8a409a 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -59,31 +59,6 @@ die_if_unmatched ()\n \tfi\n }\n \n-#\n-# Print a submodule configuration setting\n-#\n-# $1 = submodule name\n-# $2 = option name\n-# $3 = default value\n-#\n-# Checks in the usual git-config places first (for overrides),\n-# otherwise it falls back on .gitmodules.  This allows you to\n-# distribute project-wide defaults in .gitmodules, while still\n-# customizing individual repositories if necessary.  If the option is\n-# not in .gitmodules either, print a default value.\n-#\n-get_submodule_config () {\n-\tname=\"$1\"\n-\toption=\"$2\"\n-\tdefault=\"$3\"\n-\tvalue=$(git config submodule.\"$name\".\"$option\")\n-\tif test -z \"$value\"\n-\tthen\n-\t\tvalue=$(git submodule--helper config submodule.\"$name\".\"$option\")\n-\tfi\n-\tprintf '%s' \"${value:-$default}\"\n-}\n-\n isnumber()\n {\n \tn=$(($1 + 0)) 2>/dev/null && test \"$n\" = \"$1\"\n@@ -831,166 +806,7 @@ cmd_summary() {\n \t\tshift\n \tdone\n \n-\ttest $summary_limit = 0 && return\n-\n-\tif rev=$(git rev-parse -q --verify --default HEAD ${1+\"$1\"})\n-\tthen\n-\t\thead=$rev\n-\t\ttest $# = 0 || shift\n-\telif test -z \"$1\" || test \"$1\" = \"HEAD\"\n-\tthen\n-\t\t# before the first commit: compare with an empty tree\n-\t\thead=$(git hash-object -w -t tree --stdin </dev/null)\n-\t\ttest -z \"$1\" || shift\n-\telse\n-\t\thead=\"HEAD\"\n-\tfi\n-\n-\tif [ -n \"$files\" ]\n-\tthen\n-\t\ttest -n \"$cached\" &&\n-\t\tdie \"$(gettext \"The --cached option cannot be used with the --files option\")\"\n-\t\tdiff_cmd=diff-files\n-\t\thead=\n-\tfi\n-\n-\tcd_to_toplevel\n-\teval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n-\t# Get modified modules cared by user\n-\tmodules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- \"$@\" |\n-\t\tsane_egrep '^:([0-7]* )?160000' |\n-\t\twhile read -r mod_src mod_dst sha1_src sha1_dst status sm_path\n-\t\tdo\n-\t\t\t# Always show modules deleted or type-changed (blob<->module)\n-\t\t\tif test \"$status\" = D || test \"$status\" = T\n-\t\t\tthen\n-\t\t\t\tprintf '%s\\n' \"$sm_path\"\n-\t\t\t\tcontinue\n-\t\t\tfi\n-\t\t\t# Respect the ignore setting for --for-status.\n-\t\t\tif test -n \"$for_status\"\n-\t\t\tthen\n-\t\t\t\tname=$(git submodule--helper name \"$sm_path\")\n-\t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n-\t\t\t\ttest $status != A && test $ignore_config = all && continue\n-\t\t\tfi\n-\t\t\t# Also show added or modified modules which are checked out\n-\t\t\tGIT_DIR=\"$sm_path/.git\" git rev-parse --git-dir >/dev/null 2>&1 &&\n-\t\t\tprintf '%s\\n' \"$sm_path\"\n-\t\tdone\n-\t)\n-\n-\ttest -z \"$modules\" && return\n-\n-\tgit $diff_cmd $cached --ignore-submodules=dirty --raw $head -- $modules |\n-\tsane_egrep '^:([0-7]* )?160000' |\n-\tcut -c2- |\n-\twhile read -r mod_src mod_dst sha1_src sha1_dst status name\n-\tdo\n-\t\tif test -z \"$cached\" &&\n-\t\t\tis_zero_oid $sha1_dst\n-\t\tthen\n-\t\t\tcase \"$mod_dst\" in\n-\t\t\t160000)\n-\t\t\t\tsha1_dst=$(GIT_DIR=\"$name/.git\" git rev-parse HEAD)\n-\t\t\t\t;;\n-\t\t\t100644 | 100755 | 120000)\n-\t\t\t\tsha1_dst=$(git hash-object $name)\n-\t\t\t\t;;\n-\t\t\t000000)\n-\t\t\t\t;; # removed\n-\t\t\t*)\n-\t\t\t\t# unexpected type\n-\t\t\t\teval_gettextln \"unexpected mode \\$mod_dst\" >&2\n-\t\t\t\tcontinue ;;\n-\t\t\tesac\n-\t\tfi\n-\t\tmissing_src=\n-\t\tmissing_dst=\n-\n-\t\ttest $mod_src = 160000 &&\n-\t\t! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_src^0 >/dev/null &&\n-\t\tmissing_src=t\n-\n-\t\ttest $mod_dst = 160000 &&\n-\t\t! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n-\t\tmissing_dst=t\n-\n-\t\tdisplay_name=$(git submodule--helper relative-path \"$name\" \"$wt_prefix\")\n-\n-\t\ttotal_commits=\n-\t\tcase \"$missing_src,$missing_dst\" in\n-\t\tt,)\n-\t\t\terrmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_src\")\"\n-\t\t\t;;\n-\t\t,t)\n-\t\t\terrmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_dst\")\"\n-\t\t\t;;\n-\t\tt,t)\n-\t\t\terrmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commits \\$sha1_src and \\$sha1_dst\")\"\n-\t\t\t;;\n-\t\t*)\n-\t\t\terrmsg=\n-\t\t\ttotal_commits=$(\n-\t\t\tif test $mod_src = 160000 && test $mod_dst = 160000\n-\t\t\tthen\n-\t\t\t\trange=\"$sha1_src...$sha1_dst\"\n-\t\t\telif test $mod_src = 160000\n-\t\t\tthen\n-\t\t\t\trange=$sha1_src\n-\t\t\telse\n-\t\t\t\trange=$sha1_dst\n-\t\t\tfi\n-\t\t\tGIT_DIR=\"$name/.git\" \\\n-\t\t\tgit rev-list --first-parent $range -- | wc -l\n-\t\t\t)\n-\t\t\ttotal_commits=\" ($(($total_commits + 0)))\"\n-\t\t\t;;\n-\t\tesac\n-\n-\t\tsha1_abbr_src=$(GIT_DIR=\"$name/.git\" git rev-parse --short $sha1_src 2>/dev/null ||\n-\t\t\techo $sha1_src | cut -c1-7)\n-\t\tsha1_abbr_dst=$(GIT_DIR=\"$name/.git\" git rev-parse --short $sha1_dst 2>/dev/null ||\n-\t\t\techo $sha1_dst | cut -c1-7)\n-\n-\t\tif test $status = T\n-\t\tthen\n-\t\t\tblob=\"$(gettext \"blob\")\"\n-\t\t\tsubmodule=\"$(gettext \"submodule\")\"\n-\t\t\tif test $mod_dst = 160000\n-\t\t\tthen\n-\t\t\t\techo \"* $display_name $sha1_abbr_src($blob)->$sha1_abbr_dst($submodule)$total_commits:\"\n-\t\t\telse\n-\t\t\t\techo \"* $display_name $sha1_abbr_src($submodule)->$sha1_abbr_dst($blob)$total_commits:\"\n-\t\t\tfi\n-\t\telse\n-\t\t\techo \"* $display_name $sha1_abbr_src...$sha1_abbr_dst$total_commits:\"\n-\t\tfi\n-\t\tif test -n \"$errmsg\"\n-\t\tthen\n-\t\t\t# Don't give error msg for modification whose dst is not submodule\n-\t\t\t# i.e. deleted or changed to blob\n-\t\t\ttest $mod_dst = 160000 && echo \"$errmsg\"\n-\t\telse\n-\t\t\tif test $mod_src = 160000 && test $mod_dst = 160000\n-\t\t\tthen\n-\t\t\t\tlimit=\n-\t\t\t\ttest $summary_limit -gt 0 && limit=\"-$summary_limit\"\n-\t\t\t\tGIT_DIR=\"$name/.git\" \\\n-\t\t\t\tgit log $limit --pretty='format:  %m %s' \\\n-\t\t\t\t--first-parent $sha1_src...$sha1_dst\n-\t\t\telif test $mod_dst = 160000\n-\t\t\tthen\n-\t\t\t\tGIT_DIR=\"$name/.git\" \\\n-\t\t\t\tgit log --pretty='format:  > %s' -1 $sha1_dst\n-\t\t\telse\n-\t\t\t\tGIT_DIR=\"$name/.git\" \\\n-\t\t\t\tgit log --pretty='format:  < %s' -1 $sha1_src\n-\t\t\tfi\n-\t\t\techo\n-\t\tfi\n-\t\techo\n-\tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper summary ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} ${for_status:+--for-status} ${files:+--files} ${cached:+--cached} ${summary_limit:+-n $summary_limit} \"$@\"\n }\n #\n # List all submodules, prefixed with:\n-- \n2.27.0\n\n"},{"id":"401012","messageId":"nycvar.QRO.7.76.6.2007031653570.50@tvgsbejvaqbjf.bet","threadId":"53791","inReplyTo":"20200702192409.21865-2-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 1/4] submodule: amend extra line feed between callback struct and macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-03T14:57:55Z","receivedAt":"2020-07-03T14:58:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\n\"amend\" sounds a little awkward to describe what the patch does. Maybe\n\"submodule: remove extra empty lines\"?\n\nOn Fri, 3 Jul 2020, Shourya Shukla wrote:\n\n> All subcommands of 'git submodule' using a callback mechanism had\n> absence of an extra linefeed between their callback structs and\n> macros. Subcommands 'init', 'status' and 'sync' did not follow suit.\n> Amend the extra line feed.\n\nMaybe a native reader can suggest something that flows a bit easier? I am\nnot a native English speaker, but I'd prefer something along those lines:\n\n\tMany `submodule--helper` subcommands follow the convention a\n\tstruct defines their callback data, and the declaration of said\n\tstruct is followed immediately by a macro to use in static\n\tinitializers, without any separating empty line.\n\n\tLet's align the `init`, `status` and `sync` subcommands with that\n\tconvention.\n\nThe patch obviously does what the commit message promises.\n\nCiao,\nDscho\n\n> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n>  builtin/submodule--helper.c | 3 ---\n>  1 file changed, 3 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 59c1e1217c..eea3932c40 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -612,7 +612,6 @@ struct init_cb {\n>  \tconst char *prefix;\n>  \tunsigned int flags;\n>  };\n> -\n>  #define INIT_CB_INIT { NULL, 0 }\n>\n>  static void init_submodule(const char *path, const char *prefix,\n> @@ -742,7 +741,6 @@ struct status_cb {\n>  \tconst char *prefix;\n>  \tunsigned int flags;\n>  };\n> -\n>  #define STATUS_CB_INIT { NULL, 0 }\n>\n>  static void print_status(unsigned int flags, char state, const char *path,\n> @@ -933,7 +931,6 @@ struct sync_cb {\n>  \tconst char *prefix;\n>  \tunsigned int flags;\n>  };\n> -\n>  #define SYNC_CB_INIT { NULL, 0 }\n>\n>  static void sync_submodule(const char *path, const char *prefix,\n> --\n> 2.27.0\n>\n>\n"},{"id":"401013","messageId":"nycvar.QRO.7.76.6.2007031659580.50@tvgsbejvaqbjf.bet","threadId":"53791","inReplyTo":"20200702192409.21865-4-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 3/4] diff: change scope of the function count_lines()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-03T15:07:13Z","receivedAt":"2020-07-03T15:07:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\nOn Fri, 3 Jul 2020, Shourya Shukla wrote:\n\n> From: Prathamesh Chavan <pc44800@gmail.com>\n>\n> Change the scope of function count_lines for allowing the function\n> to be reused in other parts of the code as well.\n\nIt may be just me, but I'd rather see the word \"visibility\" instead of\n\"scope\" here. I mistook the subject line to indicate that the function is\nchanged to serve an (at least slightly) different purpose than before,\nwhich is not actually the case.\n\nAnother alternative to \"visibility\" might be to imitate existing commit\nmessages, such as e4cb659ebdd (diff: export diffstat interface,\n2019-11-13), 22184497a36 (factor out refresh_and_write_cache function,\n2019-09-11) or ef283b3699f (apply: make parse_git_diff_header public,\n2019-07-11).\n\nIn addition, as with all such changes, we need to be careful to consider\nwhether unrelated function names coming in from system headers might\nclash. In this case, I think `count_lines()` is a bit too generic, but\nwill probably not clash. Personally, I would probably have opted for\n`count_lines_in_string()`.\n\nCiao,\nJohannes\n\n>\n> Mentored-by: Christian Couder <christian.couder@gmail.com>\n> Mentored-by: Stefan Beller <sbeller@google.com>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n>  diff.c | 2 +-\n>  diff.h | 1 +\n>  2 files changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 4a2c631c37..39ddbcf451 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -547,7 +547,7 @@ struct emit_callback {\n>  \tstruct strbuf *header;\n>  };\n>\n> -static int count_lines(const char *data, int size)\n> +int count_lines(const char *data, int size)\n>  {\n>  \tint count, ch, completely_empty = 1, nl_just_seen = 0;\n>  \tcount = 0;\n> diff --git a/diff.h b/diff.h\n> index 9443dc1b00..bd96c8c434 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -495,6 +495,7 @@ void free_diffstat_info(struct diffstat_t *diffstat);\n>  int parse_long_opt(const char *opt, const char **argv,\n>  \t\t   const char **optarg);\n>\n> +int count_lines(const char *data, int size);\n>  int git_diff_basic_config(const char *var, const char *value, void *cb);\n>  int git_diff_heuristic_config(const char *var, const char *value, void *cb);\n>  void init_diff_ui_defaults(void);\n> --\n> 2.27.0\n>\n>\n"},{"id":"401014","messageId":"8a36a3cd-02a3-f4fa-7b86-0d4884c88449@iee.email","threadId":"53791","inReplyTo":"nycvar.QRO.7.76.6.2007031653570.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/4] submodule: amend extra line feed between callback struct and macro","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-07-03T15:37:06Z","receivedAt":"2020-07-03T15:37:11Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"\nSuggestion...\n\nOn 03/07/2020 15:57, Johannes Schindelin wrote:\n> Maybe a native reader can suggest something that flows a bit easier? I am\n> not a native English speaker, but I'd prefer something along those lines:\n>\n> \tMany `submodule--helper` subcommands follow the convention a\ns/convention a/convention that a/    feels nicer for me\n> \tstruct defines their callback data, and the declaration of said\ns/said/that/   maybe.  'said' is a bit too much like 'patent speak', but\notherwise either word is OK.\n> \tstruct is followed immediately by a macro to use in static\n> \tinitializers, without any separating empty line.\n>\n> \tLet's align the `init`, `status` and `sync` subcommands with that\n> \tconvention.\nNice summary.\n\nPhilip\nNative Yorkshire speaker, but not that 'literate' ;-)\n"},{"id":"401016","messageId":"nycvar.QRO.7.76.6.2007031712160.50@tvgsbejvaqbjf.bet","threadId":"53791","inReplyTo":"20200702192409.21865-5-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-03T20:46:37Z","receivedAt":"2020-07-03T20:47:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\n[exchanging Stefan Beller's dysfunct @google address for their private\none; I encourage you to do the same in the next iteration, probably\nby editing the `Mentored-by:` line.]\n\nOn Fri, 3 Jul 2020, Shourya Shukla wrote:\n\n> From: Prathamesh Chavan <pc44800@gmail.com>\n>\n> The submodule subcommand 'summary' is ported in the process of\n> making git-submodule a builtin. The function cmd_summary() from\n> git-submodule.sh is ported to functions module_summary(),\n> compute_summary_module_list(), prepare_submodule_summary() and\n> generate_submodule_summary(), print_submodule_summary().\n>\n> The first function module_summary() parses the options of submodule\n> subcommand and also acts as the front-end of this subcommand.\n> After parsing them, it calls the compute_summary_module_list()\n\nMissing full-stop, and probably the sentence also wanted to say \"function\"\nat the end.\n\n> The functions compute_summary_module_list() runs the diff_cmd,\n> and generates the modules list, as required by the subcommand.\n> The generation of this module list is done by the using the\n\ns/the using/using/\n\n> callback function submodule_summary_callback(), and stored in the\n> structure module_cb.\n\nThis explains nicely what the patch does. But the commit message should\nnot really repeat what can be readily deduced from the patch; It should\nfocus on the motivation and on interesting background information that is\n_not_ readily deduced from the patch.\n\nFor example, I see that `$diff_cmd` is called twice in the shell script\nversion, once to \"get modified modules cared by user\" and then _again_,\nwith that list of modified modules.\n\nI would have liked to see a reasoning in the commit message that explains\nwhy this has to be so in the C version. I get why it is complicated in a\nshell script (which lacks proper objects, after all), but I would have\nexpected the C version to be able to accumulate the information with a\nsingle pass.\n\n(Before writing the following paragraph, I actually reviewed the patch\nfrom bottom to top, in the caller->callee direction.)\n\nAh. I see that this indeed is the case: there is only one pass in the C\nversion. That's a useful piece of metadata for the commit message, I\nthink, much more useful than describing the call tree of the functions.\n\nAnother thing worth mentioning in the commit message is that we use the\ncombination of setting a child_process' working directory to the submodule\npath and then calling `prepare_submodule_repo_env()` which also sets\n`GIT_DIR` to `.git`, so that we can be certain that those spawned\nprocesses will not access the superproject's ODB by mistake.\n\nWhen reading my suggestions, please keep in mind that I reviewed the\nfunctions in caller->callee order, i.e. I started at the end of the patch\nand then worked my way up.\n\nAll in all, I like the function structure, but I think there is still a\nbit room for improvement in a v2.\n\n> Once the module list is generated, prepare_submodule_summary()\n> further goes through the list and filters the list, for\n> eventually calling the generate_submodule_summary() function.\n>\n> The function generate_submodule_summary() takes care of generating\n> the summary for each submodule and then calls the function\n> print_submodule_summary() for printing it.\n>\n> Mentored-by: Christian Couder <christian.couder@gmail.com>\n> Mentored-by: Stefan Beller <sbeller@google.com>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n>  builtin/submodule--helper.c | 451 ++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            | 186 +--------------\n>  2 files changed, 452 insertions(+), 185 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index eea3932c40..1dbdb934f1 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -927,6 +927,456 @@ static int module_name(int argc, const char **argv, const char *prefix)\n>  \treturn 0;\n>  }\n>\n> +struct module_cb {\n> +\tunsigned int mod_src;\n> +\tunsigned int mod_dst;\n> +\tstruct object_id oid_src;\n> +\tstruct object_id oid_dst;\n> +\tchar status;\n> +\tconst char *sm_path;\n> +};\n> +#define MODULE_CB_INIT { 0, 0, NULL, NULL, '\\0', NULL }\n> +\n> +struct module_cb_list {\n> +\tstruct module_cb **entries;\n> +\tint alloc, nr;\n> +};\n> +#define MODULE_CB_LIST_INIT { NULL, 0, 0 }\n> +\n> +struct summary_cb {\n> +\tint argc;\n> +\tconst char **argv;\n> +\tconst char *prefix;\n> +\tunsigned int cached: 1;\n> +\tunsigned int for_status: 1;\n> +\tunsigned int quiet: 1;\n> +\tunsigned int files: 1;\n> +\tint summary_limit;\n> +};\n> +#define SUMMARY_CB_INIT { 0, NULL, NULL, 0, 0, 0, 0, 0 }\n> +\n> +enum diff_cmd {\n> +\tDIFF_INDEX,\n> +\tDIFF_FILES\n> +};\n> +\n> +static int verify_submodule_object_name(const char *sm_path,\n> +\t\t\t\t\t  const char *sha1)\n> +{\n> +\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> +\n> +\tcp_rev_parse.git_cmd = 1;\n> +\tcp_rev_parse.no_stdout = 1;\n> +\tcp_rev_parse.dir = sm_path;\n\nSo here we specify `sm_path` as current working directory.\n\n> +\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n\nAnd this implicitly sets `GIT_DIR=.git`. Good.\n\n> +\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\", \"-q\",\n> +\t\t\t \"--verify\", NULL);\n> +\targv_array_pushf(&cp_rev_parse.args, \"%s^0\", sha1);\n\nAfter this, we should also append `--` to make sure that we're not parsing\nthis as a file name.\n\nTwo comments about naming: `sha1` is pretty misleading here, as we do not\nrequire it to be a SHA-1 (especially in the future in which we switch to\nSHA-256). Besides, what we're really asking for (via that `^0` suffix) is\na committish. Therefore, I would propose to use `committish` both in the\nparameter name as well as the function name.\n\n> +\n> +\tif (run_command(&cp_rev_parse))\n> +\t\treturn 1;\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void print_submodule_summary(struct summary_cb *info, int errmsg,\n> +\t\t\t\t      int total_commits, int missing_src,\n> +\t\t\t\t      int missing_dst, const char *displaypath,\n> +\t\t\t\t      int is_sm_git_dir, struct module_cb *p)\n> +{\n> +\tif (p->status == 'T') {\n> +\t\tif (S_ISGITLINK(p->mod_dst))\n> +\t\t\tprintf(_(\"* %s %s(blob)->%s(submodule)\"),\n> +\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n\nThe shell script version does this:\n\n                sha1_abbr_src=$(GIT_DIR=\"$name/.git\" git rev-parse --short $sha1_src 2>/dev/null ||\n                        echo $sha1_src | cut -c1-7)\n\nThat is not quite the same, as it looks for the abbreviation _in the\nsubmodule_, not in the current project. So I think `find_unique_abbrev()`\nis not correct here.\n\nThe funny thing is that we _already_ will have called `git rev-parse\n--verify` for both `p->oid_src` and `p->oid_dst` in the submodule, in the\ncaller of this function! And while we throw away the result, and while we\ndo not pass `--short`, there is no reason why we shouldn't be able to do\nprecisely that.\n\n> +\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n> +\t\telse\n> +\t\t\tprintf(_(\"* %s %s(submodule)->%s(blob)\"),\n> +\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n> +\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n> +\t} else {\n> +\t\tprintf(\"* %s %s...%s\",\n> +\t\t\tdisplaypath, find_unique_abbrev(&p->oid_src, 7),\n> +\t\t\tfind_unique_abbrev(&p->oid_dst, 7));\n> +\t}\n> +\n> +\tif (total_commits < 0)\n> +\t\tprintf(\":\\n\");\n> +\telse\n> +\t\tprintf(\" (%d):\\n\", total_commits);\n> +\n> +\tif (errmsg) {\n> +\t\t/*\n> +\t\t * Don't give error msg for modification whose dst is not\n> +\t\t * submodule, i.e. deleted or changed to blob\n> +\t\t */\n> +\t\tif (S_ISGITLINK(p->mod_src)) {\n> +\t\t\tif (missing_src && missing_dst) {\n> +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commits %s and %s\\n\"),\n> +\t\t\t\t       displaypath, oid_to_hex(&p->oid_src),\n> +\t\t\t\t       oid_to_hex(&p->oid_dst));\n> +\t\t\t} else if (missing_src) {\n> +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n> +\t\t\t\t       displaypath, oid_to_hex(&p->oid_src));\n> +\t\t\t} else {\n> +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n> +\t\t\t\t       displaypath, oid_to_hex(&p->oid_dst));\n> +\t\t\t}\n> +\t\t}\n> +\t} else if (is_sm_git_dir) {\n> +\t\tstruct child_process cp_log = CHILD_PROCESS_INIT;\n> +\n> +\t\tcp_log.git_cmd = 1;\n> +\t\tcp_log.dir = p->sm_path;\n> +\t\tprepare_submodule_repo_env(&cp_log.env_array);\n\nSince the working directory is set to the top-level directory of the\nsubmodule, and since `prepare_submodule_repo_env()` sets `GIT_DIR` to\n`.git`, I think that the `is_sm_git_dir` condition is unnecessary. In\nfact, the entire `is_sm_git_dir` parameter (and local variable in the\ncaller, see more on that below) can go away.\n\n> +\t\targv_array_pushl(&cp_log.args, \"log\", NULL);\n> +\n> +\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst)) {\n> +\t\t\tif (info->summary_limit > 0)\n> +\t\t\t\targv_array_pushf(&cp_log.args, \"-%d\",\n> +\t\t\t\t\t\t info->summary_limit);\n> +\n> +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  %m %s\",\n> +\t\t\t\t\t \"--first-parent\", NULL);\n> +\t\t\targv_array_pushf(&cp_log.args, \"%s...%s\",\n> +\t\t\t\t\t oid_to_hex(&p->oid_src),\n> +\t\t\t\t\t oid_to_hex(&p->oid_dst));\n> +\t\t} else if (S_ISGITLINK(p->mod_dst)) {\n> +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  > %s\",\n> +\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_dst), NULL);\n> +\t\t} else {\n> +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  < %s\",\n> +\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_src), NULL);\n> +\t\t}\n> +\t\trun_command(&cp_log);\n> +\t}\n> +\tprintf(\"\\n\");\n> +}\n\nIt looks as if there is a whole lot of `oid_to_hex(&p->oid_src)` in that\nfunction. Together with the realization that we need the abbreviated\nversion of that at least in one place, and the other realization that we\nalready call `rev-parse --verify` for both `oid_src` and `oid_dst` in the\ncaller of this function, it seems to suggest itself that we would actually\nwant to pass the `--short` option, too, and to capture the output, and\npass it down to `print_submodule_summary()` _instead of_ `missing_src` and\n`missing_dst` (e.g. as `src_abbrev` and `dst_abbrev`).\n> +\n> +static void generate_submodule_summary(struct summary_cb *info,\n> +\t\t\t\t       struct module_cb *p)\n> +{\n> +\tint missing_src = 0;\n> +\tint missing_dst = 0;\n> +\tchar *displaypath;\n> +\tint errmsg = 0;\n> +\tint total_commits = -1;\n> +\tint is_sm_git_dir = 0;\n> +\tstruct strbuf sm_git_dir_sb = STRBUF_INIT;\n> +\n> +\tif (!info->cached && oideq(&p->oid_dst, &null_oid)) {\n> +\t\tif (S_ISGITLINK(p->mod_dst)) {\n> +\t\t\t/*\n> +\t\t\t * NEEDSWORK: avoid using separate process with\n> +\t\t\t * the help of the function head_ref_submodule()\n\nI don't quite understand this comment. There is no `head_ref_submodule()`\nfunction.\n\n> +\t\t\t */\n> +\t\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> +\t\t\tstruct strbuf sb_rev_parse = STRBUF_INIT;\n> +\n> +\t\t\tcp_rev_parse.git_cmd = 1;\n> +\t\t\tcp_rev_parse.no_stderr = 1;\n> +\t\t\tcp_rev_parse.dir = p->sm_path;\n> +\t\t\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n> +\n> +\t\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n> +\t\t\t\t\t \"HEAD\", NULL);\n> +\t\t\tif (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n> +\t\t\t\tstrbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n> +\t\t\t\tget_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n> +\t\t\t}\n> +\t\t\tstrbuf_release(&sb_rev_parse);\n> +\t\t} else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n> +\t\t\tstruct child_process cp_hash_object = CHILD_PROCESS_INIT;\n> +\t\t\tstruct strbuf sb_hash_object = STRBUF_INIT;\n> +\n> +\t\t\tcp_hash_object.git_cmd = 1;\n> +\t\t\targv_array_pushl(&cp_hash_object.args, \"hash-object\",\n> +\t\t\t\t\t p->sm_path, NULL);\n> +\t\t\tif (!capture_command(&cp_hash_object,\n> +\t\t\t\t\t     &sb_hash_object, 0)) {\n> +\t\t\t\tstrbuf_strip_suffix(&sb_hash_object, \"\\n\");\n> +\t\t\t\tget_oid_hex(sb_hash_object.buf, &p->oid_dst);\n> +\t\t\t}\n> +\t\t\tstrbuf_release(&sb_hash_object);\n\nIt would probably be shorter, less error-prone, and quicker to use\n`index_fd()` directly.\n\nBTW I am not quite sure that this code does the correct thing in case of a\nsymlink: it hashes the contents of the symlink target (if it is a file,\notherwise it errors out). But that is hardly an issue introduced by the\nconversion, that's just copied from `git-submodule.sh`.\n\n> +\t\t} else {\n> +\t\t\tif (p->mod_dst)\n> +\t\t\t\tdie(_(\"unexpected mode %d\\n\"), p->mod_dst);\n\nHmm. This does not match what the shell script version does:\n\n                        *)\n                                # unexpected type\n                                eval_gettextln \"unexpected mode \\$mod_dst\" >&2\n                                continue ;;\n\nI think we should also just write the message to `stderr` and continue,\nnot `die()`.\n\nIn addition to that, I am missing the C code for this case:\n\n                        000000)\n                                ;; # removed\n\nIt is quite possible that our test suite does not cover this case (or did\nthe test suite fail for you?). If that is indeed the case, it would be\nreally good to add a test case as part of this patch series, to gain\nconfidence in the correctness of the conversion.\n\n> +\t\t}\n> +\t}\n> +\n> +\tstrbuf_addstr(&sm_git_dir_sb, p->sm_path);\n\nI have to admit that I am not loving the name `sm_git_dir_sb`. Why not\n`submodule_git_dir`? I guess you copied it from elsewhere in\n`submodule--helper.c`...\n\n> +\tif (is_nonbare_repository_dir(&sm_git_dir_sb))\n> +\t\tis_sm_git_dir = 1;\n\nSo here, we verify whether there is a repository at `p->sm_path`. I don't\nsee that in the shell script version:\n\n                missing_src=\n                missing_dst=\n\n                test $mod_src = 160000 &&\n                ! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_src^0 >/dev/null &&\n                missing_src=t\n\n                test $mod_dst = 160000 &&\n                ! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n                missing_dst=t\n\nLet's read a bit further.\n\n> +\n> +\tif (is_sm_git_dir && S_ISGITLINK(p->mod_src))\n> +\t\tmissing_src = verify_submodule_object_name(p->sm_path,\n> +\t\t\t\t\t\t\t   oid_to_hex(&p->oid_src));\n\nAh, and `verify_submodule_object_name()` uses `p->sm_path` as working\ndirectory. But that's not what the shell script version did: it specified\nthe `GIT_DIR` explicitly.\n\nAnd by using the `prepare_submodule_repo_env()` function in\n`verify_submodule_object_name()`, we specify `GIT_DIR` implicitly, as I\npointed out in my comment on that function.\n\nSo I think that `is_sm_git_dir` might be\n\n> +\n> +\tif (is_sm_git_dir && S_ISGITLINK(p->mod_dst))\n> +\t\tmissing_dst = verify_submodule_object_name(p->sm_path,\n> +\t\t\t\t\t\t\t   oid_to_hex(&p->oid_dst));\n> +\n> +\tdisplaypath = get_submodule_displaypath(p->sm_path, info->prefix);\n> +\n> +\tif (!missing_dst && !missing_src) {\n> +\t\tif (is_sm_git_dir) {\n> +\t\t\tstruct child_process cp_rev_list = CHILD_PROCESS_INIT;\n> +\t\t\tstruct strbuf sb_rev_list = STRBUF_INIT;\n> +\t\t\tchar *range;\n> +\n> +\t\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n> +\t\t\t\trange = xstrfmt(\"%s...%s\", oid_to_hex(&p->oid_src),\n> +\t\t\t\t\t\toid_to_hex(&p->oid_dst));\n> +\t\t\telse if (S_ISGITLINK(p->mod_src))\n> +\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_src));\n> +\t\t\telse\n> +\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_dst));\n> +\n> +\t\t\tcp_rev_list.git_cmd = 1;\n> +\t\t\tcp_rev_list.dir = p->sm_path;\n> +\t\t\tprepare_submodule_repo_env(&cp_rev_list.env_array);\n\nAgain, due to setting the working directory to `p->sm_path` and\n(implicitly, via `prepare_submodule_repo_env()`) `GIT_DIR` to `.git`, I do\nnot think that we have to guard this block beind `is_sm_git_dir`.\n\n> +\n> +\t\t\targv_array_pushl(&cp_rev_list.args, \"rev-list\",\n> +\t\t\t\t\t \"--first-parent\", range, \"--\", NULL);\n\nSince `argv_array_push()` duplicates the strings, anyway, we can totally\navoid the need for the `range` variable:\n\n\t\t\tif (IS_GITLINK(p->mod_src) && IS_GITLINK(p->mod_dst))\n\t\t\t\targv_array_pushf(&cp_rev_list.args, \"%s...%s\",\n\t\t\t\t\t\t oid_to_hex(&p->oid_src),\n\t\t\t\t\t\t oid_to_hex(&p->oid_dst));\n\t\t\telse\n\t\t\t\targv_array_push(&cp_rev_list.args, IS_GITLINK(p->mod_src) ?\n\t\t\t\t\t\toid_to_hex(&p->oid_src) :\n\t\t\t\t\t\toid_to_hex(&p->oid_dst));\n\n> +\t\t\tif (!capture_command(&cp_rev_list, &sb_rev_list, 0)) {\n> +\t\t\t\tif (sb_rev_list.len)\n> +\t\t\t\t\ttotal_commits = count_lines(sb_rev_list.buf,\n> +\t\t\t\t\t\t\t\t    sb_rev_list.len);\n\nThat's actually not necessary. `git rev-list --count` will give you a nice\nnumber, no need to capture a potentially large amount of memory only to\ncount the lines.\n\nThis may also make the patch obsolete that makes `count_lines()` public.\n\n> +\t\t\t\telse\n> +\t\t\t\t\ttotal_commits = 0;\n> +\t\t\t}\n\n> +\n> +\t\t\tfree(range);\n> +\t\t\tstrbuf_release(&sb_rev_list);\n> +\t\t}\n> +\t} else {\n> +\t\terrmsg = 1;\n> +\t}\n\nI am missing the equivalent for these lines here:\n\n                t,)\n                        errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_src\")\"\n                        ;;\n                ,t)\n                        errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_dst\")\"\n                        ;;\n                t,t)\n                        errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commits \\$sha1_src and \\$ ha1_dst\")\"\n                        ;;\n\nI am not quite sure whether it is a good idea to leave it to the\n`print_submodule_summary()` function to generate the `errmsg`. I think I'd\nrather have it a `char *` than an `int`.\n\n> +\n> +\tprint_submodule_summary(info, errmsg, total_commits,\n> +\t\t\t\tmissing_src, missing_dst,\n> +\t\t      \t\tdisplaypath, is_sm_git_dir, p);\n> +\n> +\tfree(displaypath);\n> +\tstrbuf_release(&sm_git_dir_sb);\n> +}\n> +\n> +static void prepare_submodule_summary(struct summary_cb *info,\n> +\t\t\t\t      struct module_cb_list *list)\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < list->nr; i++) {\n> +\t\tstruct module_cb *p = list->entries[i];\n> +\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> +\n> +\t\tif (p->status == 'D' || p->status == 'T') {\n> +\t\t\tgenerate_submodule_summary(info, p);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tif (info->for_status) {\n> +\t\t\tchar *config_key;\n\nSince the `config_key` is only used within the `if()` block it would be\nbetter to declare it within that block.\n\n> +\t\t\tconst char *ignore_config = \"none\";\n\nSince the only value we ever care about is \"all\", how about turning this\ninto an `int`, setting it to `0` here, and later assigning it to\n`!strcmp(value, \"all\")` and `!strcmp(sub->ignore, \"all\")`, respectively?\n\nI mean, I get it. Unix shell scripts are all about passing around text.\nAnd it is much easier to just translate that to C faithfully. But that\ndoes not make it good C code. C has data types, and proper C code makes\nuse of that.\n\n> +\t\t\tconst char *value;\n\nIf you want to save on lines, you can cuddle this together with other\ndeclarations of the same type. Even so, it could be scoped more narrowly.\n\n> +\t\t\tconst struct submodule *sub = submodule_from_path(the_repository,\n> +\t\t\t\t\t\t\t\t\t  &null_oid,\n> +\t\t\t\t\t\t\t\t\t  p->sm_path);\n> +\n> +\t\t\tif (sub && p->status != 'A') {\n\nGood. The shell script version _always_ retrieved the `.ignore` config\nvalue, even if the `status` is `A`. Your version is much better.\n\nBut why bother calling `submodule_from_path()` if the status is `A`?\n\nI could actually see the `const struct submodule *sub;` declaration be\npulled out of this scope, and combining the `if (info->for_status &&\np->status != 'A'), and the moving the assignment of `sub` into the `else\nif ((sub = submodule_from_path(r, &null_oid, p->sm_path)) &&\nsub->ignore)`.\n\nThat would save us one entire indentation level.\n\n> +\t\t\t\tconfig_key = xstrfmt(\"submodule.%s.ignore\",\n> +\t\t\t\t\t\t     sub->name);\n> +\t\t\t\tif (!git_config_get_string_const(config_key, &value))\n> +\t\t\t\t\tignore_config = value;\n> +\t\t\t\telse if (sub->ignore)\n> +\t\t\t\t\tignore_config = sub->ignore;\n> +\n> +\t\t\t\tfree(config_key);\n> +\t\t\t\tif (!strcmp(ignore_config, \"all\"))\n> +\t\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\t/* Also show added or modified modules which are checked out */\n> +\t\tcp_rev_parse.dir = p->sm_path;\n> +\t\tcp_rev_parse.git_cmd = 1;\n> +\t\tcp_rev_parse.no_stderr = 1;\n> +\t\tcp_rev_parse.no_stdout = 1;\n> +\n> +\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n> +\t\t\t\t \"--git-dir\", NULL);\n> +\n> +\t\tif (!run_command(&cp_rev_parse))\n\nI wonder whether we really need to waste an entire spawned process on\nfiguring out whether `p->sm_path` refers to an active repository. Wouldn't\n`is_submodule_active(r, p->sm_path)` fulfill the same purpose?\n\n> +\t\t\tgenerate_submodule_summary(info, p);\n> +\t}\n> +}\n> +\n> +static void submodule_summary_callback(struct diff_queue_struct *q,\n> +\t\t\t\t       struct diff_options *options,\n> +\t\t\t\t       void *data)\n> +{\n> +\tint i;\n> +\tstruct module_cb_list *list = data;\n> +\tfor (i = 0; i < q->nr; i++) {\n> +\t\tstruct diff_filepair *p = q->queue[i];\n> +\t\tstruct module_cb *temp;\n> +\n> +\t\tif (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode))\n> +\t\t\tcontinue;\n> +\t\ttemp = (struct module_cb*)malloc(sizeof(struct module_cb));\n> +\t\ttemp->mod_src = p->one->mode;\n> +\t\ttemp->mod_dst = p->two->mode;\n> +\t\ttemp->oid_src = p->one->oid;\n> +\t\ttemp->oid_dst = p->two->oid;\n> +\t\ttemp->status = p->status;\n> +\t\ttemp->sm_path = xstrdup(p->one->path);\n> +\n> +\t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n> +\t\tlist->entries[list->nr++] = temp;\n> +\t}\n> +}\n> +\n> +static const char *get_diff_cmd(enum diff_cmd diff_cmd)\n> +{\n> +\tswitch (diff_cmd) {\n> +\tcase DIFF_INDEX: return \"diff-index\";\n> +\tcase DIFF_FILES: return \"diff-files\";\n> +\tdefault: BUG(\"bad diff_cmd value %d\", diff_cmd);\n> +\t}\n> +}\n> +\n> +static int compute_summary_module_list(char *head,\n> +\t\t\t\t         struct summary_cb *info,\n> +\t\t\t\t         enum diff_cmd diff_cmd)\n> +{\n> +\tstruct argv_array diff_args = ARGV_ARRAY_INIT;\n> +\tstruct rev_info rev;\n> +\tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n> +\n> +\targv_array_push(&diff_args, get_diff_cmd(diff_cmd));\n> +\tif (info->cached)\n> +\t\targv_array_push(&diff_args, \"--cached\");\n> +\targv_array_pushl(&diff_args, \"--ignore-submodules=dirty\", \"--raw\",\n> +\t\t\t NULL);\n> +\tif (head)\n> +\t\targv_array_push(&diff_args, head);\n> +\targv_array_push(&diff_args, \"--\");\n> +\tif (info->argc)\n> +\t\targv_array_pushv(&diff_args, info->argv);\n> +\n> +\tgit_config(git_diff_basic_config, NULL);\n> +\tinit_revisions(&rev, info->prefix);\n> +\trev.abbrev = 0;\n> +\tprecompose_argv(diff_args.argc, diff_args.argv);\n> +\n> +\tdiff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,\n> +\t\t\t\t\t &rev, NULL);\n> +\trev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n> +\trev.diffopt.format_callback = submodule_summary_callback;\n> +\trev.diffopt.format_callback_data = &list;\n> +\n> +\tif (!info->cached) {\n> +\t\tif (diff_cmd ==  DIFF_INDEX)\n\nPlease substitute the double-space by a single one.\n\n> +\t\t\tsetup_work_tree();\n> +\t\tif (read_cache_preload(&rev.diffopt.pathspec) < 0) {\n> +\t\t\tperror(\"read_cache_preload\");\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t} else if (read_cache() < 0) {\n> +\t\tperror(\"read_cache\");\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tif (diff_cmd == DIFF_INDEX)\n> +\t\trun_diff_index(&rev, info->cached);\n> +\telse\n> +\t\trun_diff_files(&rev, 0);\n> +\tprepare_submodule_summary(info, &list);\n> +\treturn 0;\n> +}\n> +\n> +static int module_summary(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct summary_cb info = SUMMARY_CB_INIT;\n> +\tint cached = 0;\n> +\tint for_status = 0;\n> +\tint quiet = 0;\n> +\tint files = 0;\n> +\tint summary_limit = -1;\n> +\tstruct child_process cp_rev = CHILD_PROCESS_INIT;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tenum diff_cmd diff_cmd = DIFF_INDEX;\n> +\tint ret;\n> +\n> +\tstruct option module_summary_options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output of summarising submodules\")),\n> +\t\tOPT_BOOL(0, \"cached\", &cached,\n> +\t\t\t N_(\"Use the commit stored in the index instead of the submodule HEAD\")),\n> +\t\tOPT_BOOL(0, \"files\", &files,\n> +\t\t\t N_(\"To compare the commit in the index with that in the submodule HEAD\")),\n> +\t\tOPT_BOOL(0, \"for-status\", &for_status,\n> +\t\t\t N_(\"Skip submodules with 'ignore_config' value set to 'all'\")),\n> +\t\tOPT_INTEGER('n', \"summary-limit\", &summary_limit,\n> +\t\t\t     N_(\"Limit the summary size\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tconst char *const git_submodule_helper_usage[] = {\n> +\t\tN_(\"git submodule--helper summary [<options>] [commit] [--] [<path>]\"),\n> +\t\tNULL\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, module_summary_options,\n> +\t\t\t     git_submodule_helper_usage, 0);\n> +\n> +\tif (!summary_limit)\n> +\t\treturn 0;\n> +\n> +\tcp_rev.git_cmd = 1;\n> +\targv_array_pushl(&cp_rev.args, \"rev-parse\", \"-q\", \"--verify\",\n> +\t\t\t argc ? argv[0] : \"HEAD\", NULL);\n\nOy. Why not simply call `get_oid()`? No need to spawn a new process.\n\n> +\n> +\tif (!capture_command(&cp_rev, &sb, 0)) {\n> +\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n> +\t\tif (argc) {\n> +\t\t\targv++;\n> +\t\t\targc--;\n> +\t\t}\n> +\t} else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n> +\t\t/* before the first commit: compare with an empty tree */\n> +\t\tstruct stat st;\n> +\t\tstruct object_id oid;\n> +\t\tif (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,\n> +\t\t\t\t\t\t  prefix, 3))\n> +\t\t\tdie(\"Unable to add %s to database\", oid.hash);\n\nUmm. The original reads:\n\n                # before the first commit: compare with an empty tree\n                head=$(git hash-object -w -t tree --stdin </dev/null)\n\nIt does not actually read from `stdin`. It reads from `/dev/null`,\nredirected to the input. And what it _actually_ does is to generate the\nOID of the empty tree.\n\nBut we already _have_ the OID of the empty tree! It's\n`the_hash_algo->empty_tree`.\n\nI hope that this is covered by the test suite. Please check that. The test\nwould succeed with your version, but only because tests are run with\n`stdin` redirected from `/dev/null` by default.\n\n> +\t\tstrbuf_addstr(&sb, oid_to_hex(&oid));\n> +\t\tif (argc) {\n> +\t\t\targv++;\n> +\t\t\targc--;\n> +\t\t}\n> +\t} else {\n> +\t\tstrbuf_addstr(&sb, \"HEAD\");\n> +\t}\n\nThe conversion to C would make for a fine excuse to simplify the logic.\n\n> +\tif (files) {\n> +\t\tif (cached)\n> +\t\t\tdie(_(\"--cached and --files are mutually exclusive\"));\n> +\t\tdiff_cmd = DIFF_FILES;\n> +\t}\n> +\n> +\tinfo.argc = argc;\n> +\tinfo.argv = argv;\n> +\tinfo.prefix = prefix;\n> +\tinfo.cached = !!cached;\n> +\tinfo.for_status = !!for_status;\n> +\tinfo.quiet = quiet;\n> +\tinfo.files = files;\n> +\tinfo.summary_limit = summary_limit;\n> +\n> +\tret = compute_summary_module_list((diff_cmd == DIFF_FILES) ? NULL : sb.buf,\n> +\t\t\t\t\t   &info, diff_cmd);\n\nIt would be better to pass the OID as `struct object_id *`, not as string.\n\nOther than that, this patch nicely follows previous conversions from Unix\nshell scripts to C.\n\nWell done,\nJohannes\n\n> +\tstrbuf_release(&sb);\n> +\treturn ret;\n> +}\n> +\n>  struct sync_cb {\n>  \tconst char *prefix;\n>  \tunsigned int flags;\n> @@ -2341,6 +2791,7 @@ static struct cmd_struct commands[] = {\n>  \t{\"print-default-remote\", print_default_remote, 0},\n>  \t{\"sync\", module_sync, SUPPORT_SUPER_PREFIX},\n>  \t{\"deinit\", module_deinit, 0},\n> +\t{\"summary\", module_summary, SUPPORT_SUPER_PREFIX},\n>  \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n>  \t{\"push-check\", push_check, 0},\n>  \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 43eb6051d2..899b8a409a 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -59,31 +59,6 @@ die_if_unmatched ()\n>  \tfi\n>  }\n>\n> -#\n> -# Print a submodule configuration setting\n> -#\n> -# $1 = submodule name\n> -# $2 = option name\n> -# $3 = default value\n> -#\n> -# Checks in the usual git-config places first (for overrides),\n> -# otherwise it falls back on .gitmodules.  This allows you to\n> -# distribute project-wide defaults in .gitmodules, while still\n> -# customizing individual repositories if necessary.  If the option is\n> -# not in .gitmodules either, print a default value.\n> -#\n> -get_submodule_config () {\n> -\tname=\"$1\"\n> -\toption=\"$2\"\n> -\tdefault=\"$3\"\n> -\tvalue=$(git config submodule.\"$name\".\"$option\")\n> -\tif test -z \"$value\"\n> -\tthen\n> -\t\tvalue=$(git submodule--helper config submodule.\"$name\".\"$option\")\n> -\tfi\n> -\tprintf '%s' \"${value:-$default}\"\n> -}\n> -\n>  isnumber()\n>  {\n>  \tn=$(($1 + 0)) 2>/dev/null && test \"$n\" = \"$1\"\n> @@ -831,166 +806,7 @@ cmd_summary() {\n>  \t\tshift\n>  \tdone\n>\n> -\ttest $summary_limit = 0 && return\n> -\n> -\tif rev=$(git rev-parse -q --verify --default HEAD ${1+\"$1\"})\n> -\tthen\n> -\t\thead=$rev\n> -\t\ttest $# = 0 || shift\n> -\telif test -z \"$1\" || test \"$1\" = \"HEAD\"\n> -\tthen\n> -\t\t# before the first commit: compare with an empty tree\n> -\t\thead=$(git hash-object -w -t tree --stdin </dev/null)\n> -\t\ttest -z \"$1\" || shift\n> -\telse\n> -\t\thead=\"HEAD\"\n> -\tfi\n> -\n> -\tif [ -n \"$files\" ]\n> -\tthen\n> -\t\ttest -n \"$cached\" &&\n> -\t\tdie \"$(gettext \"The --cached option cannot be used with the --files option\")\"\n> -\t\tdiff_cmd=diff-files\n> -\t\thead=\n> -\tfi\n> -\n> -\tcd_to_toplevel\n> -\teval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n> -\t# Get modified modules cared by user\n> -\tmodules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- \"$@\" |\n> -\t\tsane_egrep '^:([0-7]* )?160000' |\n> -\t\twhile read -r mod_src mod_dst sha1_src sha1_dst status sm_path\n> -\t\tdo\n> -\t\t\t# Always show modules deleted or type-changed (blob<->module)\n> -\t\t\tif test \"$status\" = D || test \"$status\" = T\n> -\t\t\tthen\n> -\t\t\t\tprintf '%s\\n' \"$sm_path\"\n> -\t\t\t\tcontinue\n> -\t\t\tfi\n> -\t\t\t# Respect the ignore setting for --for-status.\n> -\t\t\tif test -n \"$for_status\"\n> -\t\t\tthen\n> -\t\t\t\tname=$(git submodule--helper name \"$sm_path\")\n> -\t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n> -\t\t\t\ttest $status != A && test $ignore_config = all && continue\n> -\t\t\tfi\n> -\t\t\t# Also show added or modified modules which are checked out\n> -\t\t\tGIT_DIR=\"$sm_path/.git\" git rev-parse --git-dir >/dev/null 2>&1 &&\n> -\t\t\tprintf '%s\\n' \"$sm_path\"\n> -\t\tdone\n> -\t)\n> -\n> -\ttest -z \"$modules\" && return\n> -\n> -\tgit $diff_cmd $cached --ignore-submodules=dirty --raw $head -- $modules |\n> -\tsane_egrep '^:([0-7]* )?160000' |\n> -\tcut -c2- |\n> -\twhile read -r mod_src mod_dst sha1_src sha1_dst status name\n> -\tdo\n> -\t\tif test -z \"$cached\" &&\n> -\t\t\tis_zero_oid $sha1_dst\n> -\t\tthen\n> -\t\t\tcase \"$mod_dst\" in\n> -\t\t\t160000)\n> -\t\t\t\tsha1_dst=$(GIT_DIR=\"$name/.git\" git rev-parse HEAD)\n> -\t\t\t\t;;\n> -\t\t\t100644 | 100755 | 120000)\n> -\t\t\t\tsha1_dst=$(git hash-object $name)\n> -\t\t\t\t;;\n> -\t\t\t000000)\n> -\t\t\t\t;; # removed\n> -\t\t\t*)\n> -\t\t\t\t# unexpected type\n> -\t\t\t\teval_gettextln \"unexpected mode \\$mod_dst\" >&2\n> -\t\t\t\tcontinue ;;\n> -\t\t\tesac\n> -\t\tfi\n> -\t\tmissing_src=\n> -\t\tmissing_dst=\n> -\n> -\t\ttest $mod_src = 160000 &&\n> -\t\t! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_src^0 >/dev/null &&\n> -\t\tmissing_src=t\n> -\n> -\t\ttest $mod_dst = 160000 &&\n> -\t\t! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n> -\t\tmissing_dst=t\n> -\n> -\t\tdisplay_name=$(git submodule--helper relative-path \"$name\" \"$wt_prefix\")\n> -\n> -\t\ttotal_commits=\n> -\t\tcase \"$missing_src,$missing_dst\" in\n> -\t\tt,)\n> -\t\t\terrmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_src\")\"\n> -\t\t\t;;\n> -\t\t,t)\n> -\t\t\terrmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_dst\")\"\n> -\t\t\t;;\n> -\t\tt,t)\n> -\t\t\terrmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commits \\$sha1_src and \\$sha1_dst\")\"\n> -\t\t\t;;\n> -\t\t*)\n> -\t\t\terrmsg=\n> -\t\t\ttotal_commits=$(\n> -\t\t\tif test $mod_src = 160000 && test $mod_dst = 160000\n> -\t\t\tthen\n> -\t\t\t\trange=\"$sha1_src...$sha1_dst\"\n> -\t\t\telif test $mod_src = 160000\n> -\t\t\tthen\n> -\t\t\t\trange=$sha1_src\n> -\t\t\telse\n> -\t\t\t\trange=$sha1_dst\n> -\t\t\tfi\n> -\t\t\tGIT_DIR=\"$name/.git\" \\\n> -\t\t\tgit rev-list --first-parent $range -- | wc -l\n> -\t\t\t)\n> -\t\t\ttotal_commits=\" ($(($total_commits + 0)))\"\n> -\t\t\t;;\n> -\t\tesac\n> -\n> -\t\tsha1_abbr_src=$(GIT_DIR=\"$name/.git\" git rev-parse --short $sha1_src 2>/dev/null ||\n> -\t\t\techo $sha1_src | cut -c1-7)\n> -\t\tsha1_abbr_dst=$(GIT_DIR=\"$name/.git\" git rev-parse --short $sha1_dst 2>/dev/null ||\n> -\t\t\techo $sha1_dst | cut -c1-7)\n> -\n> -\t\tif test $status = T\n> -\t\tthen\n> -\t\t\tblob=\"$(gettext \"blob\")\"\n> -\t\t\tsubmodule=\"$(gettext \"submodule\")\"\n> -\t\t\tif test $mod_dst = 160000\n> -\t\t\tthen\n> -\t\t\t\techo \"* $display_name $sha1_abbr_src($blob)->$sha1_abbr_dst($submodule)$total_commits:\"\n> -\t\t\telse\n> -\t\t\t\techo \"* $display_name $sha1_abbr_src($submodule)->$sha1_abbr_dst($blob)$total_commits:\"\n> -\t\t\tfi\n> -\t\telse\n> -\t\t\techo \"* $display_name $sha1_abbr_src...$sha1_abbr_dst$total_commits:\"\n> -\t\tfi\n> -\t\tif test -n \"$errmsg\"\n> -\t\tthen\n> -\t\t\t# Don't give error msg for modification whose dst is not submodule\n> -\t\t\t# i.e. deleted or changed to blob\n> -\t\t\ttest $mod_dst = 160000 && echo \"$errmsg\"\n> -\t\telse\n> -\t\t\tif test $mod_src = 160000 && test $mod_dst = 160000\n> -\t\t\tthen\n> -\t\t\t\tlimit=\n> -\t\t\t\ttest $summary_limit -gt 0 && limit=\"-$summary_limit\"\n> -\t\t\t\tGIT_DIR=\"$name/.git\" \\\n> -\t\t\t\tgit log $limit --pretty='format:  %m %s' \\\n> -\t\t\t\t--first-parent $sha1_src...$sha1_dst\n> -\t\t\telif test $mod_dst = 160000\n> -\t\t\tthen\n> -\t\t\t\tGIT_DIR=\"$name/.git\" \\\n> -\t\t\t\tgit log --pretty='format:  > %s' -1 $sha1_dst\n> -\t\t\telse\n> -\t\t\t\tGIT_DIR=\"$name/.git\" \\\n> -\t\t\t\tgit log --pretty='format:  < %s' -1 $sha1_src\n> -\t\t\tfi\n> -\t\t\techo\n> -\t\tfi\n> -\t\techo\n> -\tdone\n> +\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper summary ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} ${for_status:+--for-status} ${files:+--files} ${cached:+--cached} ${summary_limit:+-n $summary_limit} \"$@\"\n>  }\n>  #\n>  # List all submodules, prefixed with:\n> --\n> 2.27.0\n>\n>\n"},{"id":"401021","messageId":"20200704152104.GA45598@konoha","threadId":"53791","inReplyTo":"8a36a3cd-02a3-f4fa-7b86-0d4884c88449@iee.email","subject":"Re: [PATCH 1/4] submodule: amend extra line feed between callback struct and macro","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-04T15:21:04Z","receivedAt":"2020-07-04T15:21:14Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 03/07 04:37, Philip Oakley wrote:\n> \n> Suggestion...\n> \n> On 03/07/2020 15:57, Johannes Schindelin wrote:\n> > Maybe a native reader can suggest something that flows a bit easier? I am\n> > not a native English speaker, but I'd prefer something along those lines:\n> >\n> > \tMany `submodule--helper` subcommands follow the convention a\n> s/convention a/convention that a/    feels nicer for me\n\nI did not get this one. Are you asking to replace \"convention\" with \"a\"\nonly?\n\n> > \tstruct defines their callback data, and the declaration of said\n> s/said/that/   maybe.  'said' is a bit too much like 'patent speak', but\n> otherwise either word is OK.\n\nOkay will do!\n\n> > \tstruct is followed immediately by a macro to use in static\n> > \tinitializers, without any separating empty line.\n> >\n> > \tLet's align the `init`, `status` and `sync` subcommands with that\n> > \tconvention.\n> Nice summary.\n> \n> Philip\n> Native Yorkshire speaker, but not that 'literate' ;-)\n\nThis made me chuckle. Nice!\n"},{"id":"401022","messageId":"20200704152802.GA46571@konoha","threadId":"53791","inReplyTo":"nycvar.QRO.7.76.6.2007031659580.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/4] diff: change scope of the function count_lines()","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-04T15:28:02Z","receivedAt":"2020-07-04T15:28:12Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 03/07 05:07, Johannes Schindelin wrote:\n> Hi Shourya,\n> \n> On Fri, 3 Jul 2020, Shourya Shukla wrote:\n> \n> > From: Prathamesh Chavan <pc44800@gmail.com>\n> >\n> > Change the scope of function count_lines for allowing the function\n> > to be reused in other parts of the code as well.\n> \n> It may be just me, but I'd rather see the word \"visibility\" instead of\n> \"scope\" here. I mistook the subject line to indicate that the function is\n> changed to serve an (at least slightly) different purpose than before,\n> which is not actually the case.\n> \n> Another alternative to \"visibility\" might be to imitate existing commit\n> messages, such as e4cb659ebdd (diff: export diffstat interface,\n> 2019-11-13), 22184497a36 (factor out refresh_and_write_cache function,\n> 2019-09-11) or ef283b3699f (apply: make parse_git_diff_header public,\n> 2019-07-11).\n\nThese are some excellent commit messages! I will take inspiration from\nthese. Thanks Dscho! :)\n\n> In addition, as with all such changes, we need to be careful to consider\n> whether unrelated function names coming in from system headers might\n> clash. In this case, I think `count_lines()` is a bit too generic, but\n> will probably not clash. Personally, I would probably have opted for\n> `count_lines_in_string()`.\n\nI do not see it clashing with any other functions. Do we need to change\nthe name still (maybe to make the functions purpose even more clear)?\n"},{"id":"401023","messageId":"20200704153917.GD4087@danh.dev","threadId":"53791","inReplyTo":"20200704152104.GA45598@konoha","subject":"Re: [PATCH 1/4] submodule: amend extra line feed between callback struct and macro","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-07-04T15:39:37Z","receivedAt":"2020-07-04T15:39:42Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-07-04 20:51:04+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:\n> On 03/07 04:37, Philip Oakley wrote:\n> > \n> > Suggestion...\n> > \n> > On 03/07/2020 15:57, Johannes Schindelin wrote:\n> > > Maybe a native reader can suggest something that flows a bit easier? I am\n> > > not a native English speaker, but I'd prefer something along those lines:\n> > >\n> > > \tMany `submodule--helper` subcommands follow the convention a\n> > s/convention a/convention that a/    feels nicer for me\n> \n> I did not get this one. Are you asking to replace \"convention\" with \"a\"\n> only?\n\nHe probably meant put \"that\" between \"convention\" and \"a\"\nPull ed/vi/sed out and that try this command on that line :)\n\n\ts/convention a/convention that a/\n\n\n-- \nDanh\n"},{"id":"401025","messageId":"CAP8UFD1PZ=S401xUAiwcC4LJ+m-X0L2Lv6jQ84OfTZD4hP0K_w@mail.gmail.com","threadId":"53791","inReplyTo":"20200704152802.GA46571@konoha","subject":"Re: [PATCH 3/4] diff: change scope of the function count_lines()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-07-04T21:46:54Z","receivedAt":"2020-07-04T21:47:08Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Jul 4, 2020 at 5:28 PM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n>\n> On 03/07 05:07, Johannes Schindelin wrote:\n\n> > In addition, as with all such changes, we need to be careful to consider\n> > whether unrelated function names coming in from system headers might\n> > clash. In this case, I think `count_lines()` is a bit too generic, but\n> > will probably not clash. Personally, I would probably have opted for\n> > `count_lines_in_string()`.\n>\n> I do not see it clashing with any other functions. Do we need to change\n> the name still (maybe to make the functions purpose even more clear)?\n\nThis patch might not be needed at all according to Dscho's review of\nanother patch in this series.\n"},{"id":"401029","messageId":"0028d1be-9479-b761-8617-cc73c4bb3858@iee.email","threadId":"53791","inReplyTo":"20200704153917.GD4087@danh.dev","subject":"Re: [PATCH 1/4] submodule: amend extra line feed between callback struct and macro","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-07-05T10:52:32Z","receivedAt":"2020-07-05T10:52:37Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 04/07/2020 16:39, Đoàn Trần Công Danh wrote:\n> On 2020-07-04 20:51:04+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:\n>> On 03/07 04:37, Philip Oakley wrote:\n>>> Suggestion...\n>>>\n>>> On 03/07/2020 15:57, Johannes Schindelin wrote:\n>>>> Maybe a native reader can suggest something that flows a bit easier? I am\n>>>> not a native English speaker, but I'd prefer something along those lines:\n>>>>\n>>>> \tMany `submodule--helper` subcommands follow the convention a\n>>> s/convention a/convention that a/    feels nicer for me\n>> I did not get this one. Are you asking to replace \"convention\" with \"a\"\n>> only?\n> He probably meant put \"that\" between \"convention\" and \"a\"\n> Pull ed/vi/sed out and that try this command on that line :)\n>\n> \ts/convention a/convention that a/\n>\n>\nCorrect.\n\nPhilip\n"},{"id":"401031","messageId":"20200705173458.GA5204@konoha","threadId":"53791","inReplyTo":"nycvar.QRO.7.76.6.2007031712160.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-05T17:34:58Z","receivedAt":"2020-07-05T17:35:10Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hello Dscho!\n\nBefore replying, one thing I want to clarify is that I ask various\nthings for double-checking since if I get even the slightest confusion I\nstart overthinking and destroying it all for myself. Please bear with me\nand confirm/clarify stuff wherever possible. Would be of great help! :)\n\n> Hi Shourya,\n> \n> [exchanging Stefan Beller's dysfunct @google address for their private\n> one; I encourage you to do the same in the next iteration, probably\n> by editing the `Mentored-by:` line.]\n\nI think you missed to mention it.\n\n> On Fri, 3 Jul 2020, Shourya Shukla wrote:\n> \n> > From: Prathamesh Chavan <pc44800@gmail.com>\n> >\n> > The submodule subcommand 'summary' is ported in the process of\n> > making git-submodule a builtin. The function cmd_summary() from\n> > git-submodule.sh is ported to functions module_summary(),\n> > compute_summary_module_list(), prepare_submodule_summary() and\n> > generate_submodule_summary(), print_submodule_summary().\n> >\n> > The first function module_summary() parses the options of submodule\n> > subcommand and also acts as the front-end of this subcommand.\n> > After parsing them, it calls the compute_summary_module_list()\n> \n> Missing full-stop, and probably the sentence also wanted to say \"function\"\n> at the end.\n\nI will correct. Thanks for pointing out!\n\n> > The functions compute_summary_module_list() runs the diff_cmd,\n> > and generates the modules list, as required by the subcommand.\n> > The generation of this module list is done by the using the\n> \n> s/the using/using/\n\nWill amend!\n\n> > callback function submodule_summary_callback(), and stored in the\n> > structure module_cb.\n> \n> This explains nicely what the patch does. But the commit message should\n> not really repeat what can be readily deduced from the patch; It should\n> focus on the motivation and on interesting background information that is\n> _not_ readily deduced from the patch.\n\nI understand. I will follow your suggestions regarding my patch.\n\n> For example, I see that `$diff_cmd` is called twice in the shell script\n> version, once to \"get modified modules cared by user\" and then _again_,\n> with that list of modified modules.\n> \n> I would have liked to see a reasoning in the commit message that explains\n> why this has to be so in the C version. I get why it is complicated in a\n> shell script (which lacks proper objects, after all), but I would have\n> expected the C version to be able to accumulate the information with a\n> single pass.\n> \n> (Before writing the following paragraph, I actually reviewed the patch\n> from bottom to top, in the caller->callee direction.)\n> \n> Ah. I see that this indeed is the case: there is only one pass in the C\n> version. That's a useful piece of metadata for the commit message, I\n> think, much more useful than describing the call tree of the functions.\n\nYup that it worth mentioning.\n\n> Another thing worth mentioning in the commit message is that we use the\n> combination of setting a child_process' working directory to the submodule\n> path and then calling `prepare_submodule_repo_env()` which also sets\n> `GIT_DIR` to `.git`, so that we can be certain that those spawned\n> processes will not access the superproject's ODB by mistake.\n> \n> When reading my suggestions, please keep in mind that I reviewed the\n> functions in caller->callee order, i.e. I started at the end of the patch\n> and then worked my way up.\n> \n> All in all, I like the function structure, but I think there is still a\n> bit room for improvement in a v2.\n\n> > +static int verify_submodule_object_name(const char *sm_path,\n> > +\t\t\t\t\t  const char *sha1)\n> > +{\n> > +\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> > + > > +\tcp_rev_parse.git_cmd = 1;\n> > +\tcp_rev_parse.no_stdout = 1;\n> > +\tcp_rev_parse.dir = sm_path;\n> \n> So here we specify `sm_path` as current working directory.\n> \n> > +\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n> \n> And this implicitly sets `GIT_DIR=.git`. Good.\n> \n> > +\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\", \"-q\",\n> > +\t\t\t \"--verify\", NULL);\n> > +\targv_array_pushf(&cp_rev_parse.args, \"%s^0\", sha1);\n> \n> After this, we should also append `--` to make sure that we're not parsing\n> this as a file name.\n\nWill do!\n\n> Two comments about naming: `sha1` is pretty misleading here, as we do not\n> require it to be a SHA-1 (especially in the future in which we switch to\n> SHA-256). Besides, what we're really asking for (via that `^0` suffix) is\n> a committish. Therefore, I would propose to use `committish` both in the\n> parameter name as well as the function name.\n\nI am not aware of this change. I will take this suggestion into account.\n\n> > +\n> > +\tif (run_command(&cp_rev_parse))\n> > +\t\treturn 1;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static void print_submodule_summary(struct summary_cb *info, int errmsg,\n> > +\t\t\t\t      int total_commits, int missing_src,\n> > +\t\t\t\t      int missing_dst, const char *displaypath,\n> > +\t\t\t\t      int is_sm_git_dir, struct module_cb *p)\n> > +{\n> > +\tif (p->status == 'T') {\n> > +\t\tif (S_ISGITLINK(p->mod_dst))\n> > +\t\t\tprintf(_(\"* %s %s(blob)->%s(submodule)\"),\n> > +\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n> \n> The shell script version does this:\n> \n>                 sha1_abbr_src=$(GIT_DIR=\"$name/.git\" git rev-parse --short $sha1_src 2>/dev/null ||\n>                         echo $sha1_src | cut -c1-7)\n> \n> That is not quite the same, as it looks for the abbreviation _in the\n> submodule_, not in the current project. So I think `find_unique_abbrev()`\n> is not correct here.\n> \n> The funny thing is that we _already_ will have called `git rev-parse\n> --verify` for both `p->oid_src` and `p->oid_dst` in the submodule, in the\n> caller of this function! And while we throw away the result, and while we\n> do not pass `--short`, there is no reason why we shouldn't be able to do\n> precisely that.\n\nOkay so you are saying that there is no need of a 'find_unique_abbrev()'\nsince we would be easily able to obtain these values from the caller of\n'print_submodule_summary()' right? Maybe we can pass 'oid_src' or\n'oid_dst' as an argument?\n\n> > +\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n> > +\t\telse\n> > +\t\t\tprintf(_(\"* %s %s(submodule)->%s(blob)\"),\n> > +\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n> > +\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n> > +\t} else {\n> > +\t\tprintf(\"* %s %s...%s\",\n> > +\t\t\tdisplaypath, find_unique_abbrev(&p->oid_src, 7),\n> > +\t\t\tfind_unique_abbrev(&p->oid_dst, 7));\n> > +\t}\n> > +\n> > +\tif (total_commits < 0)\n> > +\t\tprintf(\":\\n\");\n> > +\telse\n> > +\t\tprintf(\" (%d):\\n\", total_commits);\n> > +\n> > +\tif (errmsg) {\n> > +\t\t/*\n> > +\t\t * Don't give error msg for modification whose dst is not\n> > +\t\t * submodule, i.e. deleted or changed to blob\n> > +\t\t */\n> > +\t\tif (S_ISGITLINK(p->mod_src)) {\n> > +\t\t\tif (missing_src && missing_dst) {\n> > +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commits %s and %s\\n\"),\n> > +\t\t\t\t       displaypath, oid_to_hex(&p->oid_src),\n> > +\t\t\t\t       oid_to_hex(&p->oid_dst));\n> > +\t\t\t} else if (missing_src) {\n> > +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n> > +\t\t\t\t       displaypath, oid_to_hex(&p->oid_src));\n> > +\t\t\t} else {\n> > +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n> > +\t\t\t\t       displaypath, oid_to_hex(&p->oid_dst));\n> > +\t\t\t}\n> > +\t\t}\n> > +\t} else if (is_sm_git_dir) {\n> > +\t\tstruct child_process cp_log = CHILD_PROCESS_INIT;\n> > +\n> > +\t\tcp_log.git_cmd = 1;\n> > +\t\tcp_log.dir = p->sm_path;\n> > +\t\tprepare_submodule_repo_env(&cp_log.env_array);\n> \n> Since the working directory is set to the top-level directory of the\n> submodule, and since `prepare_submodule_repo_env()` sets `GIT_DIR` to\n> `.git`, I think that the `is_sm_git_dir` condition is unnecessary. In\n> fact, the entire `is_sm_git_dir` parameter (and local variable in the\n> caller, see more on that below) can go away.\n\nBecause we already set the $GIT_DIR to .git/ so an extra check will not\nbe necessary right?\n\n> > +\t\targv_array_pushl(&cp_log.args, \"log\", NULL);\n> > +\n> > +\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst)) {\n> > +\t\t\tif (info->summary_limit > 0)\n> > +\t\t\t\targv_array_pushf(&cp_log.args, \"-%d\",\n> > +\t\t\t\t\t\t info->summary_limit);\n> > +\n> > +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  %m %s\",\n> > +\t\t\t\t\t \"--first-parent\", NULL);\n> > +\t\t\targv_array_pushf(&cp_log.args, \"%s...%s\",\n> > +\t\t\t\t\t oid_to_hex(&p->oid_src),\n> > +\t\t\t\t\t oid_to_hex(&p->oid_dst));\n> > +\t\t} else if (S_ISGITLINK(p->mod_dst)) {\n> > +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  > %s\",\n> > +\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_dst), NULL);\n> > +\t\t} else {\n> > +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  < %s\",\n> > +\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_src), NULL);\n> > +\t\t}\n> > +\t\trun_command(&cp_log);\n> > +\t}\n> > +\tprintf(\"\\n\");\n> > +}\n> \n> It looks as if there is a whole lot of `oid_to_hex(&p->oid_src)` in that\n> function. Together with the realization that we need the abbreviated\n> version of that at least in one place, and the other realization that we\n> already call `rev-parse --verify` for both `oid_src` and `oid_dst` in the\n> caller of this function, it seems to suggest itself that we would actually\n> want to pass the `--short` option, too, and to capture the output, and\n> pass it down to `print_submodule_summary()` _instead of_ `missing_src` and\n> `missing_dst` (e.g. as `src_abbrev` and `dst_abbrev`).\n\nOh you have mentioned it here too. This seems quite a good approach. I\nwill adopt this.\n\n> > +\n> > +static void generate_submodule_summary(struct summary_cb *info,\n> > +\t\t\t\t       struct module_cb *p)\n> > +{\n> > +\tint missing_src = 0;\n> > +\tint missing_dst = 0;\n> > +\tchar *displaypath;\n> > +\tint errmsg = 0;\n> > +\tint total_commits = -1;\n> > +\tint is_sm_git_dir = 0;\n> > +\tstruct strbuf sm_git_dir_sb = STRBUF_INIT;\n> > +\n> > +\tif (!info->cached && oideq(&p->oid_dst, &null_oid)) {\n> > +\t\tif (S_ISGITLINK(p->mod_dst)) {\n> > +\t\t\t/*\n> > +\t\t\t * NEEDSWORK: avoid using separate process with\n> > +\t\t\t * the help of the function head_ref_submodule()\n> \n> I don't quite understand this comment. There is no `head_ref_submodule()`\n> function.\n> \n> > +\t\t\t */\n> > +\t\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> > +\t\t\tstruct strbuf sb_rev_parse = STRBUF_INIT;\n> > +\n> > +\t\t\tcp_rev_parse.git_cmd = 1;\n> > +\t\t\tcp_rev_parse.no_stderr = 1;\n> > +\t\t\tcp_rev_parse.dir = p->sm_path;\n> > +\t\t\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n> > +\n> > +\t\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n> > +\t\t\t\t\t \"HEAD\", NULL);\n> > +\t\t\tif (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n> > +\t\t\t\tstrbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n> > +\t\t\t\tget_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n> > +\t\t\t}\n> > +\t\t\tstrbuf_release(&sb_rev_parse);\n> > +\t\t} else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n> > +\t\t\tstruct child_process cp_hash_object = CHILD_PROCESS_INIT;\n> > +\t\t\tstruct strbuf sb_hash_object = STRBUF_INIT;\n> > +\n> > +\t\t\tcp_hash_object.git_cmd = 1;\n> > +\t\t\targv_array_pushl(&cp_hash_object.args, \"hash-object\",\n> > +\t\t\t\t\t p->sm_path, NULL);\n> > +\t\t\tif (!capture_command(&cp_hash_object,\n> > +\t\t\t\t\t     &sb_hash_object, 0)) {\n> > +\t\t\t\tstrbuf_strip_suffix(&sb_hash_object, \"\\n\");\n> > +\t\t\t\tget_oid_hex(sb_hash_object.buf, &p->oid_dst);\n> > +\t\t\t}\n> > +\t\t\tstrbuf_release(&sb_hash_object);\n> \n> It would probably be shorter, less error-prone, and quicker to use\n> `index_fd()` directly.\n>\n> BTW I am not quite sure that this code does the correct thing in case of a\n> symlink: it hashes the contents of the symlink target (if it is a file,\n> otherwise it errors out). But that is hardly an issue introduced by the\n> conversion, that's just copied from `git-submodule.sh`.\n> \n> > +\t\t} else {\n> > +\t\t\tif (p->mod_dst)\n> > +\t\t\t\tdie(_(\"unexpected mode %d\\n\"), p->mod_dst);\n> \n> Hmm. This does not match what the shell script version does:\n> \n>                         *)\n>                                 # unexpected type\n>                                 eval_gettextln \"unexpected mode \\$mod_dst\" >&2\n>                                 continue ;;\n> \n> I think we should also just write the message to `stderr` and continue,\n> not `die()`.\n> \n> In addition to that, I am missing the C code for this case:\n> \n>                         000000)\n>                                 ;; # removed\n> \n> It is quite possible that our test suite does not cover this case (or did\n> the test suite fail for you?). If that is indeed the case, it would be\n> really good to add a test case as part of this patch series, to gain\n> confidence in the correctness of the conversion.\n\nThe tests passed for me actually. Whether this is covered by the test\ncases, I am not sure. I will have to check it.\n\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstrbuf_addstr(&sm_git_dir_sb, p->sm_path);\n> \n> I have to admit that I am not loving the name `sm_git_dir_sb`. Why not\n> `submodule_git_dir`? I guess you copied it from elsewhere in\n> `submodule--helper.c`...\n> \n> > +\tif (is_nonbare_repository_dir(&sm_git_dir_sb))\n> > +\t\tis_sm_git_dir = 1;\n> \n> So here, we verify whether there is a repository at `p->sm_path`. I don't\n> see that in the shell script version:\n> \n>                 missing_src=\n>                 missing_dst=\n> \n>                 test $mod_src = 160000 &&\n>                 ! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_src^0 >/dev/null &&\n>                 missing_src=t\n> \n>                 test $mod_dst = 160000 &&\n>                 ! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n>                 missing_dst=t\n> \n> Let's read a bit further.\n> \n> > +\n> > +\tif (is_sm_git_dir && S_ISGITLINK(p->mod_src))\n> > +\t\tmissing_src = verify_submodule_object_name(p->sm_path,\n> > +\t\t\t\t\t\t\t   oid_to_hex(&p->oid_src));\n> \n> Ah, and `verify_submodule_object_name()` uses `p->sm_path` as working\n> directory. But that's not what the shell script version did: it specified\n> the `GIT_DIR` explicitly.\n> \n> And by using the `prepare_submodule_repo_env()` function in\n> `verify_submodule_object_name()`, we specify `GIT_DIR` implicitly, as I\n> pointed out in my comment on that function.\n\nOh so you're saying that it will be better to call\n'prepare_submodule_repo_env()' on some variable since we explicitly want to\nstore the path to GIT_DIR?\n\nIt would be of much help if you could explain this part just a little\nmore (for my own sake).\n\n> So I think that `is_sm_git_dir` might be\n\nI think you missed something here...\n\n> > +\n> > +\tif (is_sm_git_dir && S_ISGITLINK(p->mod_dst))\n> > +\t\tmissing_dst = verify_submodule_object_name(p->sm_path,\n> > +\t\t\t\t\t\t\t   oid_to_hex(&p->oid_dst));\n> > +\n> > +\tdisplaypath = get_submodule_displaypath(p->sm_path, info->prefix);\n> > +\n> > +\tif (!missing_dst && !missing_src) {\n> > +\t\tif (is_sm_git_dir) {\n> > +\t\t\tstruct child_process cp_rev_list = CHILD_PROCESS_INIT;\n> > +\t\t\tstruct strbuf sb_rev_list = STRBUF_INIT;\n> > +\t\t\tchar *range;\n> > +\n> > +\t\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n> > +\t\t\t\trange = xstrfmt(\"%s...%s\", oid_to_hex(&p->oid_src),\n> > +\t\t\t\t\t\toid_to_hex(&p->oid_dst));\n> > +\t\t\telse if (S_ISGITLINK(p->mod_src))\n> > +\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_src));\n> > +\t\t\telse\n> > +\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_dst));\n> > +\n> > +\t\t\tcp_rev_list.git_cmd = 1;\n> > +\t\t\tcp_rev_list.dir = p->sm_path;\n> > +\t\t\tprepare_submodule_repo_env(&cp_rev_list.env_array);\n> \n> Again, due to setting the working directory to `p->sm_path` and\n> (implicitly, via `prepare_submodule_repo_env()`) `GIT_DIR` to `.git`, I do\n> not think that we have to guard this block beind `is_sm_git_dir`.\n\n> > +\n> > +\t\t\targv_array_pushl(&cp_rev_list.args, \"rev-list\",\n> > +\t\t\t\t\t \"--first-parent\", range, \"--\", NULL);\n> \n> Since `argv_array_push()` duplicates the strings, anyway, we can totally\n> avoid the need for the `range` variable:\n> \n> \t\t\tif (IS_GITLINK(p->mod_src) && IS_GITLINK(p->mod_dst))\n> \t\t\t\targv_array_pushf(&cp_rev_list.args, \"%s...%s\",\n> \t\t\t\t\t\t oid_to_hex(&p->oid_src),\n> \t\t\t\t\t\t oid_to_hex(&p->oid_dst));\n> \t\t\telse\n> \t\t\t\targv_array_push(&cp_rev_list.args, IS_GITLINK(p->mod_src) ?\n> \t\t\t\t\t\toid_to_hex(&p->oid_src) :\n> \t\t\t\t\t\toid_to_hex(&p->oid_dst));\n> \n> > +\t\t\tif (!capture_command(&cp_rev_list, &sb_rev_list, 0)) {\n> > +\t\t\t\tif (sb_rev_list.len)\n> > +\t\t\t\t\ttotal_commits = count_lines(sb_rev_list.buf,\n> > +\t\t\t\t\t\t\t\t    sb_rev_list.len);\n> \n> That's actually not necessary. `git rev-list --count` will give you a nice\n> number, no need to capture a potentially large amount of memory only to\n> count the lines.\n> \n> This may also make the patch obsolete that makes `count_lines()` public.\n\nTherefore we eliminate count_lines() from here and instead do a 'git\nrev-list --count'?\n\n> > +\t\t\t\telse\n> > +\t\t\t\t\ttotal_commits = 0;\n> > +\t\t\t}\n> \n> > +\n> > +\t\t\tfree(range);\n> > +\t\t\tstrbuf_release(&sb_rev_list);\n> > +\t\t}\n> > +\t} else {\n> > +\t\terrmsg = 1;\n> > +\t}\n> \n> I am missing the equivalent for these lines here:\n> \n>                 t,)\n>                         errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_src\")\"\n>                         ;;\n>                 ,t)\n>                         errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_dst\")\"\n>                         ;;\n>                 t,t)\n>                         errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commits \\$sha1_src and \\$ ha1_dst\")\"\n>                         ;;\n\nI will add them.\n\n> I am not quite sure whether it is a good idea to leave it to the\n> `print_submodule_summary()` function to generate the `errmsg`. I think I'd\n> rather have it a `char *` than an `int`.\n\nWould it be better to add these error messages in\n'prepare_submodule_summary()'? If we have error messages as integers\nthen we will simply\n\n> > +\n> > +\tprint_submodule_summary(info, errmsg, total_commits,\n> > +\t\t\t\tmissing_src, missing_dst,\n> > +\t\t      \t\tdisplaypath, is_sm_git_dir, p);\n> > +\n> > +\tfree(displaypath);\n> > +\tstrbuf_release(&sm_git_dir_sb);\n> > +}\n> > +\n> > +static void prepare_submodule_summary(struct summary_cb *info,\n> > +\t\t\t\t      struct module_cb_list *list)\n> > +{\n> > +\tint i;\n> > +\tfor (i = 0; i < list->nr; i++) {\n> > +\t\tstruct module_cb *p = list->entries[i];\n> > +\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> > +\n> > +\t\tif (p->status == 'D' || p->status == 'T') {\n> > +\t\t\tgenerate_submodule_summary(info, p);\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tif (info->for_status) {\n> > +\t\t\tchar *config_key;\n> \n> Since the `config_key` is only used within the `if()` block it would be\n> better to declare it within that block.\n> \n> > +\t\t\tconst char *ignore_config = \"none\";\n> \n> Since the only value we ever care about is \"all\", how about turning this\n> into an `int`, setting it to `0` here, and later assigning it to\n> `!strcmp(value, \"all\")` and `!strcmp(sub->ignore, \"all\")`, respectively?\n\nAlright will do!\n\n> I mean, I get it. Unix shell scripts are all about passing around text.\n> And it is much easier to just translate that to C faithfully. But that\n> does not make it good C code. C has data types, and proper C code makes\n> use of that.\n> \n> > +\t\t\tconst char *value;\n> \n> If you want to save on lines, you can cuddle this together with other\n> declarations of the same type. Even so, it could be scoped more narrowly.\n> \n> > +\t\t\tconst struct submodule *sub = submodule_from_path(the_repository,\n> > +\t\t\t\t\t\t\t\t\t  &null_oid,\n> > +\t\t\t\t\t\t\t\t\t  p->sm_path);\n> > +\n> > +\t\t\tif (sub && p->status != 'A') {\n> \n> Good. The shell script version _always_ retrieved the `.ignore` config\n> value, even if the `status` is `A`. Your version is much better.\n> \n> But why bother calling `submodule_from_path()` if the status is `A`?\n\nWhat exactly does a status of 'A' or 'T' mean? I mean I know what we are\ndoing but what exactly do these translate into?\n\n> I could actually see the `const struct submodule *sub;` declaration be\n> pulled out of this scope, and combining the `if (info->for_status &&\n> p->status != 'A'), and the moving the assignment of `sub` into the `else\n> if ((sub = submodule_from_path(r, &null_oid, p->sm_path)) &&\n> sub->ignore)`.\n> \n> That would save us one entire indentation level.\n\nThat seems a good approach! I will try this out.\n\n> > +\t\t\t\tconfig_key = xstrfmt(\"submodule.%s.ignore\",\n> > +\t\t\t\t\t\t     sub->name);\n> > +\t\t\t\tif (!git_config_get_string_const(config_key, &value))\n> > +\t\t\t\t\tignore_config = value;\n> > +\t\t\t\telse if (sub->ignore)\n> > +\t\t\t\t\tignore_config = sub->ignore;\n> > +\n> > +\t\t\t\tfree(config_key);\n> > +\t\t\t\tif (!strcmp(ignore_config, \"all\"))\n> > +\t\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\t/* Also show added or modified modules which are checked out */\n> > +\t\tcp_rev_parse.dir = p->sm_path;\n> > +\t\tcp_rev_parse.git_cmd = 1;\n> > +\t\tcp_rev_parse.no_stderr = 1;\n> > +\t\tcp_rev_parse.no_stdout = 1;\n> > +\n> > +\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n> > +\t\t\t\t \"--git-dir\", NULL);\n> > +\n> > +\t\tif (!run_command(&cp_rev_parse))\n> \n> I wonder whether we really need to waste an entire spawned process on\n> figuring out whether `p->sm_path` refers to an active repository. Wouldn't\n> `is_submodule_active(r, p->sm_path)` fulfill the same purpose?\n\nYep! This is correct. I will change.\n\n> > +\t\t\tgenerate_submodule_summary(info, p);\n> > +\t}\n> > +}\n> > +\n> > +static void submodule_summary_callback(struct diff_queue_struct *q,\n> > +\t\t\t\t       struct diff_options *options,\n> > +\t\t\t\t       void *data)\n> > +{\n> > +\tint i;\n> > +\tstruct module_cb_list *list = data;\n> > +\tfor (i = 0; i < q->nr; i++) {\n> > +\t\tstruct diff_filepair *p = q->queue[i];\n> > +\t\tstruct module_cb *temp;\n> > +\n> > +\t\tif (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode))\n> > +\t\t\tcontinue;\n> > +\t\ttemp = (struct module_cb*)malloc(sizeof(struct module_cb));\n> > +\t\ttemp->mod_src = p->one->mode;\n> > +\t\ttemp->mod_dst = p->two->mode;\n> > +\t\ttemp->oid_src = p->one->oid;\n> > +\t\ttemp->oid_dst = p->two->oid;\n> > +\t\ttemp->status = p->status;\n> > +\t\ttemp->sm_path = xstrdup(p->one->path);\n> > +\n> > +\t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n> > +\t\tlist->entries[list->nr++] = temp;\n> > +\t}\n> > +}\n> > +\n> > +static const char *get_diff_cmd(enum diff_cmd diff_cmd)\n> > +{\n> > +\tswitch (diff_cmd) {\n> > +\tcase DIFF_INDEX: return \"diff-index\";\n> > +\tcase DIFF_FILES: return \"diff-files\";\n> > +\tdefault: BUG(\"bad diff_cmd value %d\", diff_cmd);\n> > +\t}\n> > +}\n> > +\n> > +static int compute_summary_module_list(char *head,\n> > +\t\t\t\t         struct summary_cb *info,\n> > +\t\t\t\t         enum diff_cmd diff_cmd)\n> > +{\n> > +\tstruct argv_array diff_args = ARGV_ARRAY_INIT;\n> > +\tstruct rev_info rev;\n> > +\tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n> > +\n> > +\targv_array_push(&diff_args, get_diff_cmd(diff_cmd));\n> > +\tif (info->cached)\n> > +\t\targv_array_push(&diff_args, \"--cached\");\n> > +\targv_array_pushl(&diff_args, \"--ignore-submodules=dirty\", \"--raw\",\n> > +\t\t\t NULL);\n> > +\tif (head)\n> > +\t\targv_array_push(&diff_args, head);\n> > +\targv_array_push(&diff_args, \"--\");\n> > +\tif (info->argc)\n> > +\t\targv_array_pushv(&diff_args, info->argv);\n> > +\n> > +\tgit_config(git_diff_basic_config, NULL);\n> > +\tinit_revisions(&rev, info->prefix);\n> > +\trev.abbrev = 0;\n> > +\tprecompose_argv(diff_args.argc, diff_args.argv);\n> > +\n> > +\tdiff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,\n> > +\t\t\t\t\t &rev, NULL);\n> > +\trev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n> > +\trev.diffopt.format_callback = submodule_summary_callback;\n> > +\trev.diffopt.format_callback_data = &list;\n> > +\n> > +\tif (!info->cached) {\n> > +\t\tif (diff_cmd ==  DIFF_INDEX)\n> \n> Please substitute the double-space by a single one.\n\nWill do!\n\n> > +\t\t\tsetup_work_tree();\n> > +\t\tif (read_cache_preload(&rev.diffopt.pathspec) < 0) {\n> > +\t\t\tperror(\"read_cache_preload\");\n> > +\t\t\treturn -1;\n> > +\t\t}\n> > +\t} else if (read_cache() < 0) {\n> > +\t\tperror(\"read_cache\");\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\tif (diff_cmd == DIFF_INDEX)\n> > +\t\trun_diff_index(&rev, info->cached);\n> > +\telse\n> > +\t\trun_diff_files(&rev, 0);\n> > +\tprepare_submodule_summary(info, &list);\n> > +\treturn 0;\n> > +}\n> > +\n> > +static int module_summary(int argc, const char **argv, const char *prefix)\n> > +{\n> > +\tstruct summary_cb info = SUMMARY_CB_INIT;\n> > +\tint cached = 0;\n> > +\tint for_status = 0;\n> > +\tint quiet = 0;\n> > +\tint files = 0;\n> > +\tint summary_limit = -1;\n> > +\tstruct child_process cp_rev = CHILD_PROCESS_INIT;\n> > +\tstruct strbuf sb = STRBUF_INIT;\n> > +\tenum diff_cmd diff_cmd = DIFF_INDEX;\n> > +\tint ret;\n> > +\n> > +\tstruct option module_summary_options[] = {\n> > +\t\tOPT__QUIET(&quiet, N_(\"Suppress output of summarising submodules\")),\n> > +\t\tOPT_BOOL(0, \"cached\", &cached,\n> > +\t\t\t N_(\"Use the commit stored in the index instead of the submodule HEAD\")),\n> > +\t\tOPT_BOOL(0, \"files\", &files,\n> > +\t\t\t N_(\"To compare the commit in the index with that in the submodule HEAD\")),\n> > +\t\tOPT_BOOL(0, \"for-status\", &for_status,\n> > +\t\t\t N_(\"Skip submodules with 'ignore_config' value set to 'all'\")),\n> > +\t\tOPT_INTEGER('n', \"summary-limit\", &summary_limit,\n> > +\t\t\t     N_(\"Limit the summary size\")),\n> > +\t\tOPT_END()\n> > +\t};\n> > +\n> > +\tconst char *const git_submodule_helper_usage[] = {\n> > +\t\tN_(\"git submodule--helper summary [<options>] [commit] [--] [<path>]\"),\n> > +\t\tNULL\n> > +\t};\n> > +\n> > +\targc = parse_options(argc, argv, prefix, module_summary_options,\n> > +\t\t\t     git_submodule_helper_usage, 0);\n> > +\n> > +\tif (!summary_limit)\n> > +\t\treturn 0;\n> > +\n> > +\tcp_rev.git_cmd = 1;\n> > +\targv_array_pushl(&cp_rev.args, \"rev-parse\", \"-q\", \"--verify\",\n> > +\t\t\t argc ? argv[0] : \"HEAD\", NULL);\n> \n> Oy. Why not simply call `get_oid()`? No need to spawn a new process.\n\nThen everytime we need 'rev-parse', we simply call 'get_oid()'? That\nwill save us a ton of processes?\n\nBut I think we do need to capture the output of 'git rev-parse --verify\n....' so I think it will backfire to use 'get_oid()' or am I just being\ntoo dumb and not catching on something?\n\n> > +\n> > +\tif (!capture_command(&cp_rev, &sb, 0)) {\n> > +\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n> > +\t\tif (argc) {\n> > +\t\t\targv++;\n> > +\t\t\targc--;\n> > +\t\t}\n> > +\t} else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n> > +\t\t/* before the first commit: compare with an empty tree */\n> > +\t\tstruct stat st;\n> > +\t\tstruct object_id oid;\n> > +\t\tif (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,\n> > +\t\t\t\t\t\t  prefix, 3))\n> > +\t\t\tdie(\"Unable to add %s to database\", oid.hash);\n> \n> Umm. The original reads:\n> \n>                 # before the first commit: compare with an empty tree\n>                 head=$(git hash-object -w -t tree --stdin </dev/null)\n> \n> It does not actually read from `stdin`. It reads from `/dev/null`,\n> redirected to the input. And what it _actually_ does is to generate the\n> OID of the empty tree.\n> \n> But we already _have_ the OID of the empty tree! It's\n> `the_hash_algo->empty_tree`.\n\nI did not know this 'the_hash_algo'. I will use it. Thanks! :)\n\n> I hope that this is covered by the test suite. Please check that. The test\n> would succeed with your version, but only because tests are run with\n> `stdin` redirected from `/dev/null` by default.\n\nI guess yes. My work passed because the tests are written this way.\n\n> > +\t\tstrbuf_addstr(&sb, oid_to_hex(&oid));\n> > +\t\tif (argc) {\n> > +\t\t\targv++;\n> > +\t\t\targc--;\n> > +\t\t}\n> > +\t} else {\n> > +\t\tstrbuf_addstr(&sb, \"HEAD\");\n> > +\t}\n> \n> The conversion to C would make for a fine excuse to simplify the logic.\n\nThis was kind of like the 'shift' in shell. What equivalent do you\nsuggest?\n\n> > +\tif (files) {\n> > +\t\tif (cached)\n> > +\t\t\tdie(_(\"--cached and --files are mutually exclusive\"));\n> > +\t\tdiff_cmd = DIFF_FILES;\n> > +\t}\n> > +\n> > +\tinfo.argc = argc;\n> > +\tinfo.argv = argv;\n> > +\tinfo.prefix = prefix;\n> > +\tinfo.cached = !!cached;\n> > +\tinfo.for_status = !!for_status;\n> > +\tinfo.quiet = quiet;\n> > +\tinfo.files = files;\n> > +\tinfo.summary_limit = summary_limit;\n> > +\n> > +\tret = compute_summary_module_list((diff_cmd == DIFF_FILES) ? NULL : sb.buf,\n> > +\t\t\t\t\t   &info, diff_cmd);\n> \n> It would be better to pass the OID as `struct object_id *`, not as string.\n\nWill do!\n\n> Other than that, this patch nicely follows previous conversions from Unix\n> shell scripts to C.\n> \n> Well done,\n> Johannes\n\nThank you! It was a highly detailed review! I am still learning tons of\nstuff about Git's code and such a review does help a lot! :)\n"},{"id":"401039","messageId":"0a2e53d4-7fc5-183d-663d-7a8f52b2a685@gmail.com","threadId":"53791","inReplyTo":"20200705173458.GA5204@konoha","subject":"Re: [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-07-06T09:16:54Z","receivedAt":"2020-07-06T09:17:03Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Note: I've added some comment but I've not been able to address all the\nparts due to lack of time. I'm sending it sooner hoping it would be\nuseful.\n\nOn 05-07-2020 23:04, Shourya Shukla wrote:\n[...]\n>> [exchanging Stefan Beller's dysfunct @google address for their private\n>> one; I encourage you to do the same in the next iteration, probably\n>> by editing the `Mentored-by:` line.]\n> \n> I think you missed to mention it.\n>\n\nIf you're looking for the private e-mail. The exachange was already done\nand it was right there in the Cc list of the mail sent by Dscho. I've\nadded it again as you seem to have removed it.\n\n>> On Fri, 3 Jul 2020, Shourya Shukla wrote:\n>>\n>>> From: Prathamesh Chavan <pc44800@gmail.com>\n>>>\n>>> The submodule subcommand 'summary' is ported in the process of\n>>> making git-submodule a builtin. The function cmd_summary() from\n>>> git-submodule.sh is ported to functions module_summary(),\n>>> compute_summary_module_list(), prepare_submodule_summary() and\n>>> generate_submodule_summary(), print_submodule_summary().\n>>>\n>>> The first function module_summary() parses the options of submodule\n>>> subcommand and also acts as the front-end of this subcommand.\n>>> After parsing them, it calls the compute_summary_module_list()\n>>\n>> Missing full-stop, and probably the sentence also wanted to say \"function\"\n>> at the end.\n> \n> I will correct. Thanks for pointing out!\n> \n>>> The functions compute_summary_module_list() runs the diff_cmd,\n>>> and generates the modules list, as required by the subcommand.\n>>> The generation of this module list is done by the using the\n>>\n>> s/the using/using/\n> \n> Will amend!\n> \n>>> callback function submodule_summary_callback(), and stored in the\n>>> structure module_cb.\n>>\n>> This explains nicely what the patch does. But the commit message should\n>> not really repeat what can be readily deduced from the patch; It should\n>> focus on the motivation and on interesting background information that is\n>> _not_ readily deduced from the patch.\n> \n> I understand. I will follow your suggestions regarding my patch.\n> \n>> For example, I see that `$diff_cmd` is called twice in the shell script\n>> version, once to \"get modified modules cared by user\" and then _again_,\n>> with that list of modified modules.\n>>\n>> I would have liked to see a reasoning in the commit message that explains\n>> why this has to be so in the C version. I get why it is complicated in a\n>> shell script (which lacks proper objects, after all), but I would have\n>> expected the C version to be able to accumulate the information with a\n>> single pass.\n>>\n>> (Before writing the following paragraph, I actually reviewed the patch\n>> from bottom to top, in the caller->callee direction.)\n>>\n>> Ah. I see that this indeed is the case: there is only one pass in the C\n>> version. That's a useful piece of metadata for the commit message, I\n>> think, much more useful than describing the call tree of the functions.\n> \n> Yup that it worth mentioning.\n> \n>> Another thing worth mentioning in the commit message is that we use the\n>> combination of setting a child_process' working directory to the submodule\n>> path and then calling `prepare_submodule_repo_env()` which also sets\n>> `GIT_DIR` to `.git`, so that we can be certain that those spawned\n>> processes will not access the superproject's ODB by mistake.\n>>\n>> When reading my suggestions, please keep in mind that I reviewed the\n>> functions in caller->callee order, i.e. I started at the end of the patch\n>> and then worked my way up.\n>>\n>> All in all, I like the function structure, but I think there is still a\n>> bit room for improvement in a v2.\n> \n>>> +static int verify_submodule_object_name(const char *sm_path,\n>>> +\t\t\t\t\t  const char *sha1)\n>>> +{\n>>> +\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n>>> + > > +\tcp_rev_parse.git_cmd = 1;\n>>> +\tcp_rev_parse.no_stdout = 1;\n>>> +\tcp_rev_parse.dir = sm_path;\n>>\n>> So here we specify `sm_path` as current working directory.\n>>\n>>> +\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n>>\n>> And this implicitly sets `GIT_DIR=.git`. Good.\n>>\n>>> +\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\", \"-q\",\n>>> +\t\t\t \"--verify\", NULL);\n>>> +\targv_array_pushf(&cp_rev_parse.args, \"%s^0\", sha1);\n>>\n>> After this, we should also append `--` to make sure that we're not parsing\n>> this as a file name.\n> \n> Will do!\n> \n>> Two comments about naming: `sha1` is pretty misleading here, as we do not\n>> require it to be a SHA-1 (especially in the future in which we switch to\n>> SHA-256). Besides, what we're really asking for (via that `^0` suffix) is\n>> a committish. Therefore, I would propose to use `committish` both in the\n>> parameter name as well as the function name.\n> \n> I am not aware of this change. I will take this suggestion into account.\n> \n>>> +\n>>> +\tif (run_command(&cp_rev_parse))\n>>> +\t\treturn 1;\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +static void print_submodule_summary(struct summary_cb *info, int errmsg,\n>>> +\t\t\t\t      int total_commits, int missing_src,\n>>> +\t\t\t\t      int missing_dst, const char *displaypath,\n>>> +\t\t\t\t      int is_sm_git_dir, struct module_cb *p)\n>>> +{\n>>> +\tif (p->status == 'T') {\n>>> +\t\tif (S_ISGITLINK(p->mod_dst))\n>>> +\t\t\tprintf(_(\"* %s %s(blob)->%s(submodule)\"),\n>>> +\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n>>\n>> The shell script version does this:\n>>\n>>                 sha1_abbr_src=$(GIT_DIR=\"$name/.git\" git rev-parse --short $sha1_src 2>/dev/null ||\n>>                         echo $sha1_src | cut -c1-7)\n>>\n>> That is not quite the same, as it looks for the abbreviation _in the\n>> submodule_, not in the current project. So I think `find_unique_abbrev()`\n>> is not correct here.\n>>\n>> The funny thing is that we _already_ will have called `git rev-parse\n>> --verify` for both `p->oid_src` and `p->oid_dst` in the submodule, in the\n>> caller of this function! And while we throw away the result, and while we\n>> do not pass `--short`, there is no reason why we shouldn't be able to do\n>> precisely that.\n> \n> Okay so you are saying that there is no need of a 'find_unique_abbrev()'\n> since we would be easily able to obtain these values from the caller of\n> 'print_submodule_summary()' right?\n\nYes. 'generate_submodule_summary' already does a rev-parse on\np->oid_src and p->oid_dst via 'verify_submodule_object_name'.\nWe should be able to get the short version of them by passing '--short'\nto rev-parse there and make it return the short SHA1. We can then use it\nlike how Dscho mentions below.\n\n> Maybe we can pass 'oid_src' or 'oid_dst' as an argument?\n> \n>>> +\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n>>> +\t\telse\n>>> +\t\t\tprintf(_(\"* %s %s(submodule)->%s(blob)\"),\n>>> +\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n>>> +\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n>>> +\t} else {\n>>> +\t\tprintf(\"* %s %s...%s\",\n>>> +\t\t\tdisplaypath, find_unique_abbrev(&p->oid_src, 7),\n>>> +\t\t\tfind_unique_abbrev(&p->oid_dst, 7));\n>>> +\t}\n>>> +\n>>> +\tif (total_commits < 0)\n>>> +\t\tprintf(\":\\n\");\n>>> +\telse\n>>> +\t\tprintf(\" (%d):\\n\", total_commits);\n>>> +\n>>> +\tif (errmsg) {\n>>> +\t\t/*\n>>> +\t\t * Don't give error msg for modification whose dst is not\n>>> +\t\t * submodule, i.e. deleted or changed to blob\n>>> +\t\t */\n>>> +\t\tif (S_ISGITLINK(p->mod_src)) {\n>>> +\t\t\tif (missing_src && missing_dst) {\n>>> +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commits %s and %s\\n\"),\n>>> +\t\t\t\t       displaypath, oid_to_hex(&p->oid_src),\n>>> +\t\t\t\t       oid_to_hex(&p->oid_dst));\n>>> +\t\t\t} else if (missing_src) {\n>>> +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n>>> +\t\t\t\t       displaypath, oid_to_hex(&p->oid_src));\n>>> +\t\t\t} else {\n>>> +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n>>> +\t\t\t\t       displaypath, oid_to_hex(&p->oid_dst));\n>>> +\t\t\t}\n>>> +\t\t}\n>>> +\t} else if (is_sm_git_dir) {\n>>> +\t\tstruct child_process cp_log = CHILD_PROCESS_INIT;\n>>> +\n>>> +\t\tcp_log.git_cmd = 1;\n>>> +\t\tcp_log.dir = p->sm_path;\n>>> +\t\tprepare_submodule_repo_env(&cp_log.env_array);\n>>\n>> Since the working directory is set to the top-level directory of the\n>> submodule, and since `prepare_submodule_repo_env()` sets `GIT_DIR` to\n>> `.git`, I think that the `is_sm_git_dir` condition is unnecessary. In\n>> fact, the entire `is_sm_git_dir` parameter (and local variable in the\n>> caller, see more on that below) can go away.\n> \n> Because we already set the $GIT_DIR to .git/ so an extra check will not\n> be necessary right?\n> \n\nYes. If we remove that check and we get a p->sm_path that does not point\nto a submodule, I wonder what would happen if we run 'run_command'\non it. I'm also not sure if that's a possible case. Something to\nexplore.\n\n>>> +\t\targv_array_pushl(&cp_log.args, \"log\", NULL);\n>>> +\n>>> +\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst)) {\n>>> +\t\t\tif (info->summary_limit > 0)\n>>> +\t\t\t\targv_array_pushf(&cp_log.args, \"-%d\",\n>>> +\t\t\t\t\t\t info->summary_limit);\n>>> +\n>>> +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  %m %s\",\n>>> +\t\t\t\t\t \"--first-parent\", NULL);\n>>> +\t\t\targv_array_pushf(&cp_log.args, \"%s...%s\",\n>>> +\t\t\t\t\t oid_to_hex(&p->oid_src),\n>>> +\t\t\t\t\t oid_to_hex(&p->oid_dst));\n>>> +\t\t} else if (S_ISGITLINK(p->mod_dst)) {\n>>> +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  > %s\",\n>>> +\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_dst), NULL);\n>>> +\t\t} else {\n>>> +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  < %s\",\n>>> +\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_src), NULL);\n>>> +\t\t}\n>>> +\t\trun_command(&cp_log);\n>>> +\t}\n>>> +\tprintf(\"\\n\");\n>>> +}\n>>\n>> It looks as if there is a whole lot of `oid_to_hex(&p->oid_src)` in that\n>> function. Together with the realization that we need the abbreviated\n>> version of that at least in one place, and the other realization that we\n>> already call `rev-parse --verify` for both `oid_src` and `oid_dst` in the\n>> caller of this function, it seems to suggest itself that we would actually\n>> want to pass the `--short` option, too, and to capture the output, and\n>> pass it down to `print_submodule_summary()` _instead of_ `missing_src` and\n>> `missing_dst` (e.g. as `src_abbrev` and `dst_abbrev`).\n> \n> Oh you have mentioned it here too. This seems quite a good approach. I\n> will adopt this.\n> \n>>> +\n>>> +static void generate_submodule_summary(struct summary_cb *info,\n>>> +\t\t\t\t       struct module_cb *p)\n>>> +{\n>>> +\tint missing_src = 0;\n>>> +\tint missing_dst = 0;\n>>> +\tchar *displaypath;\n>>> +\tint errmsg = 0;\n>>> +\tint total_commits = -1;\n>>> +\tint is_sm_git_dir = 0;\n>>> +\tstruct strbuf sm_git_dir_sb = STRBUF_INIT;\n>>> +\n>>> +\tif (!info->cached && oideq(&p->oid_dst, &null_oid)) {\n>>> +\t\tif (S_ISGITLINK(p->mod_dst)) {\n>>> +\t\t\t/*\n>>> +\t\t\t * NEEDSWORK: avoid using separate process with\n>>> +\t\t\t * the help of the function head_ref_submodule()\n>>\n>> I don't quite understand this comment. There is no `head_ref_submodule()`\n>> function.\n>>\n\nThat NEEDSWORK was added based on Stefan's comment on a previous version\nof Prathamesh's patch. Here it is for reference:\n\n>> +       if (!info->cached && !oidcmp(&p->oid_dst, &null_oid)) {\n>> +               if (S_ISGITLINK(p->mod_dst)) {\n>> +                       struct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n>> +                       struct strbuf sb_rev_parse = STRBUF_INIT;\n>> +\n>> +                       cp_rev_parse.git_cmd = 1;\n>> +                       cp_rev_parse.no_stderr = 1;\n>> +                       cp_rev_parse.dir = p->sm_path;\n>> +                       prepare_submodule_repo_env(&cp_rev_parse.env_array);\n>> +\n>> +                       argv_array_pushl(&cp_rev_parse.args,\n>> +                                        \"rev-parse\", \"HEAD\", NULL);\n>> +                       if (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n>> +                               strbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n>> +\n>> +                               get_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n>> +                       }\n>> +                       strbuf_release(&sb_rev_parse);\n> \n> I think this could be replaced via\n> head_ref_submodule(sub->path, callback function, &where_to_store)\n> or is there some trickery going on, that this also works on\n> non-compliant submodules?\n> (Maybe add that as a NEEDSWORK/TODO)\n\nRef:\nhttps://public-inbox.org/git/CAGZ79kaWn9z47Va=VW4R2Aswws1N5n2u4Kvatn73s0YnV0pVqQ@mail.gmail.com/\n\nA quick search reveals that 'head_ref_submodule' existed during that\nperiod. On further investigation it seems that 'refs_head_ref' was\nintroduced in 62f0b399e0 (refs: add refs_head_ref(), 2017-08-23) and\n'head_ref_submodule' was made to use it. Later on, in 419221c106 (refs:\nremove dead for_each_*_submodule(), 2017-08-23), 'head_ref-submodule'\nwas removed with an advice to use the 'refs_' API for accessing\nsubmodules.\n\n> +* Use `refs_` API for accessing submodules. The submodule ref store could\n> +  be obtained with `get_submodule_ref_store()`\n\nHow it applies to our code is something to be looked into, yet.\n\n>>> +\t\t\t */\n>>> +\t\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n>>> +\t\t\tstruct strbuf sb_rev_parse = STRBUF_INIT;\n>>> +\n>>> +\t\t\tcp_rev_parse.git_cmd = 1;\n>>> +\t\t\tcp_rev_parse.no_stderr = 1;\n>>> +\t\t\tcp_rev_parse.dir = p->sm_path;\n>>> +\t\t\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n>>> +\n>>> +\t\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n>>> +\t\t\t\t\t \"HEAD\", NULL);\n>>> +\t\t\tif (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n>>> +\t\t\t\tstrbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n>>> +\t\t\t\tget_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n>>> +\t\t\t}\n>>> +\t\t\tstrbuf_release(&sb_rev_parse);\n>>> +\t\t} else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n>>> +\t\t\tstruct child_process cp_hash_object = CHILD_PROCESS_INIT;\n>>> +\t\t\tstruct strbuf sb_hash_object = STRBUF_INIT;\n>>> +\n>>> +\t\t\tcp_hash_object.git_cmd = 1;\n>>> +\t\t\targv_array_pushl(&cp_hash_object.args, \"hash-object\",\n>>> +\t\t\t\t\t p->sm_path, NULL);\n>>> +\t\t\tif (!capture_command(&cp_hash_object,\n>>> +\t\t\t\t\t     &sb_hash_object, 0)) {\n>>> +\t\t\t\tstrbuf_strip_suffix(&sb_hash_object, \"\\n\");\n>>> +\t\t\t\tget_oid_hex(sb_hash_object.buf, &p->oid_dst);\n>>> +\t\t\t}\n>>> +\t\t\tstrbuf_release(&sb_hash_object);\n>>\n>> It would probably be shorter, less error-prone, and quicker to use\n>> `index_fd()` directly.\n>>\n>> BTW I am not quite sure that this code does the correct thing in case of a\n>> symlink: it hashes the contents of the symlink target (if it is a file,\n>> otherwise it errors out). But that is hardly an issue introduced by the\n>> conversion, that's just copied from `git-submodule.sh`.\n>>\n>>> +\t\t} else {\n>>> +\t\t\tif (p->mod_dst)\n>>> +\t\t\t\tdie(_(\"unexpected mode %d\\n\"), p->mod_dst);\n>>\n>> Hmm. This does not match what the shell script version does:\n>>\n>>                         *)\n>>                                 # unexpected type\n>>                                 eval_gettextln \"unexpected mode \\$mod_dst\" >&2\n>>                                 continue ;;\n>>\n>> I think we should also just write the message to `stderr` and continue,\n>> not `die()`.\n>>\n>> In addition to that, I am missing the C code for this case:\n>>\n>>                         000000)\n>>                                 ;; # removed\n>>\n>> It is quite possible that our test suite does not cover this case (or did\n>> the test suite fail for you?). If that is indeed the case, it would be\n>> really good to add a test case as part of this patch series, to gain\n>> confidence in the correctness of the conversion.\n> \n> The tests passed for me actually. Whether this is covered by the test\n> cases, I am not sure. I will have to check it.\n> \n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> +\tstrbuf_addstr(&sm_git_dir_sb, p->sm_path);\n>>\n>> I have to admit that I am not loving the name `sm_git_dir_sb`. Why not\n>> `submodule_git_dir`? I guess you copied it from elsewhere in\n>> `submodule--helper.c`...\n>>\n>>> +\tif (is_nonbare_repository_dir(&sm_git_dir_sb))\n>>> +\t\tis_sm_git_dir = 1;\n>>\n>> So here, we verify whether there is a repository at `p->sm_path`. I don't\n>> see that in the shell script version:\n>>\n>>                 missing_src=\n>>                 missing_dst=\n>>\n>>                 test $mod_src = 160000 &&\n>>                 ! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_src^0 >/dev/null &&\n>>                 missing_src=t\n>>\n>>                 test $mod_dst = 160000 &&\n>>                 ! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n>>                 missing_dst=t\n>>\n>> Let's read a bit further.\n>>\n>>> +\n>>> +\tif (is_sm_git_dir && S_ISGITLINK(p->mod_src))\n>>> +\t\tmissing_src = verify_submodule_object_name(p->sm_path,\n>>> +\t\t\t\t\t\t\t   oid_to_hex(&p->oid_src));\n>>\n>> Ah, and `verify_submodule_object_name()` uses `p->sm_path` as working\n>> directory. But that's not what the shell script version did: it specified\n>> the `GIT_DIR` explicitly.\n>>\n>> And by using the `prepare_submodule_repo_env()` function in\n>> `verify_submodule_object_name()`, we specify `GIT_DIR` implicitly, as I\n>> pointed out in my comment on that function.\n> \n> Oh so you're saying that it will be better to call\n> 'prepare_submodule_repo_env()' on some variable since we explicitly want to\n> store the path to GIT_DIR?\n> \n\nWe already call 'prepare_submodule_repo_env' in\n'verify_submodule_object_name'. So, he's likely saying that\n'is_sm_git_dir' is unnecessary here.\n\n> It would be of much help if you could explain this part just a little\n> more (for my own sake).\n> \n>> So I think that `is_sm_git_dir` might be\n> \n\n... unnecessary.\n\n> I think you missed something here...\n> \n\nThat's likely what he meant based on what is mentioned above.\n\n>>> +\n>>> +\tif (is_sm_git_dir && S_ISGITLINK(p->mod_dst))\n>>> +\t\tmissing_dst = verify_submodule_object_name(p->sm_path,\n>>> +\t\t\t\t\t\t\t   oid_to_hex(&p->oid_dst));\n>>> +\n>>> +\tdisplaypath = get_submodule_displaypath(p->sm_path, info->prefix);\n>>> +\n>>> +\tif (!missing_dst && !missing_src) {\n>>> +\t\tif (is_sm_git_dir) {\n>>> +\t\t\tstruct child_process cp_rev_list = CHILD_PROCESS_INIT;\n>>> +\t\t\tstruct strbuf sb_rev_list = STRBUF_INIT;\n>>> +\t\t\tchar *range;\n>>> +\n>>> +\t\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n>>> +\t\t\t\trange = xstrfmt(\"%s...%s\", oid_to_hex(&p->oid_src),\n>>> +\t\t\t\t\t\toid_to_hex(&p->oid_dst));\n>>> +\t\t\telse if (S_ISGITLINK(p->mod_src))\n>>> +\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_src));\n>>> +\t\t\telse\n>>> +\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_dst));\n>>> +\n>>> +\t\t\tcp_rev_list.git_cmd = 1;\n>>> +\t\t\tcp_rev_list.dir = p->sm_path;\n>>> +\t\t\tprepare_submodule_repo_env(&cp_rev_list.env_array);\n>>\n>> Again, due to setting the working directory to `p->sm_path` and\n>> (implicitly, via `prepare_submodule_repo_env()`) `GIT_DIR` to `.git`, I do\n>> not think that we have to guard this block beind `is_sm_git_dir`.\n> \n>>> +\n>>> +\t\t\targv_array_pushl(&cp_rev_list.args, \"rev-list\",\n>>> +\t\t\t\t\t \"--first-parent\", range, \"--\", NULL);\n>>\n>> Since `argv_array_push()` duplicates the strings, anyway, we can totally\n>> avoid the need for the `range` variable:\n>>\n>> \t\t\tif (IS_GITLINK(p->mod_src) && IS_GITLINK(p->mod_dst))\n>> \t\t\t\targv_array_pushf(&cp_rev_list.args, \"%s...%s\",\n>> \t\t\t\t\t\t oid_to_hex(&p->oid_src),\n>> \t\t\t\t\t\t oid_to_hex(&p->oid_dst));\n>> \t\t\telse\n>> \t\t\t\targv_array_push(&cp_rev_list.args, IS_GITLINK(p->mod_src) ?\n>> \t\t\t\t\t\toid_to_hex(&p->oid_src) :\n>> \t\t\t\t\t\toid_to_hex(&p->oid_dst));\n>>\n>>> +\t\t\tif (!capture_command(&cp_rev_list, &sb_rev_list, 0)) {\n>>> +\t\t\t\tif (sb_rev_list.len)\n>>> +\t\t\t\t\ttotal_commits = count_lines(sb_rev_list.buf,\n>>> +\t\t\t\t\t\t\t\t    sb_rev_list.len);\n>>\n>> That's actually not necessary. `git rev-list --count` will give you a nice\n>> number, no need to capture a potentially large amount of memory only to\n>> count the lines.\n>>\n>> This may also make the patch obsolete that makes `count_lines()` public.\n> \n> Therefore we eliminate count_lines() from here and instead do a 'git\n> rev-list --count'?\n> \n\nYes.\n\n>>> +\t\t\t\telse\n>>> +\t\t\t\t\ttotal_commits = 0;\n>>> +\t\t\t}\n>>\n>>> +\n>>> +\t\t\tfree(range);\n>>> +\t\t\tstrbuf_release(&sb_rev_list);\n>>> +\t\t}\n>>> +\t} else {\n>>> +\t\terrmsg = 1;\n>>> +\t}\n>>\n>> I am missing the equivalent for these lines here:\n>>\n>>                 t,)\n>>                         errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_src\")\"\n>>                         ;;\n>>                 ,t)\n>>                         errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_dst\")\"\n>>                         ;;\n>>                 t,t)\n>>                         errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commits \\$sha1_src and \\$ ha1_dst\")\"\n>>                         ;;\n> \n> I will add them.\n> \n\nI think they're already there in the 'print_submodule_summary' function\nabove.\n\n>> I am not quite sure whether it is a good idea to leave it to the\n>> `print_submodule_summary()` function to generate the `errmsg`. I think I'd\n>> rather have it a `char *` than an `int`.\n> \n> Would it be better to add these error messages in\n> 'prepare_submodule_summary()'?\n\nNo. He's likely saying that instead of setting `errmsg` to 1 and\nconstructing the error message in `print_submodule_summary` we should\nbe having the error messages in `generate_submodule_summary` and pass\nthem to `print_submodule_summary` instead of passing an int.\n\n> If we have error messages as integers\n> then we will simply\n> \n\nYou missed something here. ;)\n\n>>> +\n>>> +\tprint_submodule_summary(info, errmsg, total_commits,\n>>> +\t\t\t\tmissing_src, missing_dst,\n>>> +\t\t      \t\tdisplaypath, is_sm_git_dir, p);\n>>> +\n>>> +\tfree(displaypath);\n>>> +\tstrbuf_release(&sm_git_dir_sb);\n>>> +}\n>>> +\n>>> +static void prepare_submodule_summary(struct summary_cb *info,\n>>> +\t\t\t\t      struct module_cb_list *list)\n>>> +{\n>>> +\tint i;\n>>> +\tfor (i = 0; i < list->nr; i++) {\n>>> +\t\tstruct module_cb *p = list->entries[i];\n>>> +\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n>>> +\n>>> +\t\tif (p->status == 'D' || p->status == 'T') {\n>>> +\t\t\tgenerate_submodule_summary(info, p);\n>>> +\t\t\tcontinue;\n>>> +\t\t}\n>>> +\n>>> +\t\tif (info->for_status) {\n>>> +\t\t\tchar *config_key;\n>>\n>> Since the `config_key` is only used within the `if()` block it would be\n>> better to declare it within that block.\n>>\n>>> +\t\t\tconst char *ignore_config = \"none\";\n>>\n>> Since the only value we ever care about is \"all\", how about turning this\n>> into an `int`, setting it to `0` here, and later assigning it to\n>> `!strcmp(value, \"all\")` and `!strcmp(sub->ignore, \"all\")`, respectively?\n> \n> Alright will do!\n> \n>> I mean, I get it. Unix shell scripts are all about passing around text.\n>> And it is much easier to just translate that to C faithfully. But that\n>> does not make it good C code. C has data types, and proper C code makes\n>> use of that.\n>>\n>>> +\t\t\tconst char *value;\n>>\n>> If you want to save on lines, you can cuddle this together with other\n>> declarations of the same type. Even so, it could be scoped more narrowly.\n>>\n>>> +\t\t\tconst struct submodule *sub = submodule_from_path(the_repository,\n>>> +\t\t\t\t\t\t\t\t\t  &null_oid,\n>>> +\t\t\t\t\t\t\t\t\t  p->sm_path);\n>>> +\n>>> +\t\t\tif (sub && p->status != 'A') {\n>>\n>> Good. The shell script version _always_ retrieved the `.ignore` config\n>> value, even if the `status` is `A`. Your version is much better.\n>>\n>> But why bother calling `submodule_from_path()` if the status is `A`?\n> \n> What exactly does a status of 'A' or 'T' mean? I mean I know what we are\n> doing but what exactly do these translate into?\n> \n\nIts interesting you understood it without knowing what 'A' and 'T'\nmeant. Anyways, if you take a look at the documentation of\n'diff-index'[1] which provides us the `status` you'll know that:\n> \n> Possible status letters are:\n> \n>     A: addition of a file\n> \n>     C: copy of a file into a new one\n> \n>     D: deletion of a file\n> \n>     M: modification of the contents or mode of a file\n> \n>     R: renaming of a file\n> \n>     T: change in the type of the file\n> \n>     U: file is unmerged (you must complete the merge before it can be committed)\n> \n>     X: \"unknown\" change type (most probably a bug, please report it)\n> \n\n[1]: https://git-scm.com/docs/git-diff-index\n\n>> I could actually see the `const struct submodule *sub;` declaration be\n>> pulled out of this scope, and combining the `if (info->for_status &&\n>> p->status != 'A'), and the moving the assignment of `sub` into the `else\n>> if ((sub = submodule_from_path(r, &null_oid, p->sm_path)) &&\n>> sub->ignore)`.\n>>\n>> That would save us one entire indentation level.\n> \n> That seems a good approach! I will try this out.\n> \n>>> +\t\t\t\tconfig_key = xstrfmt(\"submodule.%s.ignore\",\n>>> +\t\t\t\t\t\t     sub->name);\n>>> +\t\t\t\tif (!git_config_get_string_const(config_key, &value))\n>>> +\t\t\t\t\tignore_config = value;\n>>> +\t\t\t\telse if (sub->ignore)\n>>> +\t\t\t\t\tignore_config = sub->ignore;\n>>> +\n>>> +\t\t\t\tfree(config_key);\n>>> +\t\t\t\tif (!strcmp(ignore_config, \"all\"))\n>>> +\t\t\t\t\tcontinue;\n>>> +\t\t\t}\n>>> +\t\t}\n>>> +\n>>> +\t\t/* Also show added or modified modules which are checked out */\n>>> +\t\tcp_rev_parse.dir = p->sm_path;\n>>> +\t\tcp_rev_parse.git_cmd = 1;\n>>> +\t\tcp_rev_parse.no_stderr = 1;\n>>> +\t\tcp_rev_parse.no_stdout = 1;\n>>> +\n>>> +\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n>>> +\t\t\t\t \"--git-dir\", NULL);\n>>> +\n>>> +\t\tif (!run_command(&cp_rev_parse))\n>>\n>> I wonder whether we really need to waste an entire spawned process on\n>> figuring out whether `p->sm_path` refers to an active repository. Wouldn't\n>> `is_submodule_active(r, p->sm_path)` fulfill the same purpose?\n> \n> Yep! This is correct. I will change.\n> \n>>> +\t\t\tgenerate_submodule_summary(info, p);\n>>> +\t}\n>>> +}\n>>> +\n>>> +static void submodule_summary_callback(struct diff_queue_struct *q,\n>>> +\t\t\t\t       struct diff_options *options,\n>>> +\t\t\t\t       void *data)\n>>> +{\n>>> +\tint i;\n>>> +\tstruct module_cb_list *list = data;\n>>> +\tfor (i = 0; i < q->nr; i++) {\n>>> +\t\tstruct diff_filepair *p = q->queue[i];\n>>> +\t\tstruct module_cb *temp;\n>>> +\n>>> +\t\tif (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode))\n>>> +\t\t\tcontinue;\n>>> +\t\ttemp = (struct module_cb*)malloc(sizeof(struct module_cb));\n>>> +\t\ttemp->mod_src = p->one->mode;\n>>> +\t\ttemp->mod_dst = p->two->mode;\n>>> +\t\ttemp->oid_src = p->one->oid;\n>>> +\t\ttemp->oid_dst = p->two->oid;\n>>> +\t\ttemp->status = p->status;\n>>> +\t\ttemp->sm_path = xstrdup(p->one->path);\n>>> +\n>>> +\t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n>>> +\t\tlist->entries[list->nr++] = temp;\n>>> +\t}\n>>> +}\n>>> +\n>>> +static const char *get_diff_cmd(enum diff_cmd diff_cmd)\n>>> +{\n>>> +\tswitch (diff_cmd) {\n>>> +\tcase DIFF_INDEX: return \"diff-index\";\n>>> +\tcase DIFF_FILES: return \"diff-files\";\n>>> +\tdefault: BUG(\"bad diff_cmd value %d\", diff_cmd);\n>>> +\t}\n>>> +}\n>>> +\n>>> +static int compute_summary_module_list(char *head,\n>>> +\t\t\t\t         struct summary_cb *info,\n>>> +\t\t\t\t         enum diff_cmd diff_cmd)\n>>> +{\n>>> +\tstruct argv_array diff_args = ARGV_ARRAY_INIT;\n>>> +\tstruct rev_info rev;\n>>> +\tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n>>> +\n>>> +\targv_array_push(&diff_args, get_diff_cmd(diff_cmd));\n>>> +\tif (info->cached)\n>>> +\t\targv_array_push(&diff_args, \"--cached\");\n>>> +\targv_array_pushl(&diff_args, \"--ignore-submodules=dirty\", \"--raw\",\n>>> +\t\t\t NULL);\n>>> +\tif (head)\n>>> +\t\targv_array_push(&diff_args, head);\n>>> +\targv_array_push(&diff_args, \"--\");\n>>> +\tif (info->argc)\n>>> +\t\targv_array_pushv(&diff_args, info->argv);\n>>> +\n>>> +\tgit_config(git_diff_basic_config, NULL);\n>>> +\tinit_revisions(&rev, info->prefix);\n>>> +\trev.abbrev = 0;\n>>> +\tprecompose_argv(diff_args.argc, diff_args.argv);\n>>> +\n>>> +\tdiff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,\n>>> +\t\t\t\t\t &rev, NULL);\n>>> +\trev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n>>> +\trev.diffopt.format_callback = submodule_summary_callback;\n>>> +\trev.diffopt.format_callback_data = &list;\n>>> +\n>>> +\tif (!info->cached) {\n>>> +\t\tif (diff_cmd ==  DIFF_INDEX)\n>>\n>> Please substitute the double-space by a single one.\n> \n> Will do!\n> \n>>> +\t\t\tsetup_work_tree();\n>>> +\t\tif (read_cache_preload(&rev.diffopt.pathspec) < 0) {\n>>> +\t\t\tperror(\"read_cache_preload\");\n>>> +\t\t\treturn -1;\n>>> +\t\t}\n>>> +\t} else if (read_cache() < 0) {\n>>> +\t\tperror(\"read_cache\");\n>>> +\t\treturn -1;\n>>> +\t}\n>>> +\n>>> +\tif (diff_cmd == DIFF_INDEX)\n>>> +\t\trun_diff_index(&rev, info->cached);\n>>> +\telse\n>>> +\t\trun_diff_files(&rev, 0);\n>>> +\tprepare_submodule_summary(info, &list);\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> +static int module_summary(int argc, const char **argv, const char *prefix)\n>>> +{\n>>> +\tstruct summary_cb info = SUMMARY_CB_INIT;\n>>> +\tint cached = 0;\n>>> +\tint for_status = 0;\n>>> +\tint quiet = 0;\n>>> +\tint files = 0;\n>>> +\tint summary_limit = -1;\n>>> +\tstruct child_process cp_rev = CHILD_PROCESS_INIT;\n>>> +\tstruct strbuf sb = STRBUF_INIT;\n>>> +\tenum diff_cmd diff_cmd = DIFF_INDEX;\n>>> +\tint ret;\n>>> +\n>>> +\tstruct option module_summary_options[] = {\n>>> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output of summarising submodules\")),\n>>> +\t\tOPT_BOOL(0, \"cached\", &cached,\n>>> +\t\t\t N_(\"Use the commit stored in the index instead of the submodule HEAD\")),\n>>> +\t\tOPT_BOOL(0, \"files\", &files,\n>>> +\t\t\t N_(\"To compare the commit in the index with that in the submodule HEAD\")),\n>>> +\t\tOPT_BOOL(0, \"for-status\", &for_status,\n>>> +\t\t\t N_(\"Skip submodules with 'ignore_config' value set to 'all'\")),\n>>> +\t\tOPT_INTEGER('n', \"summary-limit\", &summary_limit,\n>>> +\t\t\t     N_(\"Limit the summary size\")),\n>>> +\t\tOPT_END()\n>>> +\t};\n>>> +\n>>> +\tconst char *const git_submodule_helper_usage[] = {\n>>> +\t\tN_(\"git submodule--helper summary [<options>] [commit] [--] [<path>]\"),\n>>> +\t\tNULL\n>>> +\t};\n>>> +\n>>> +\targc = parse_options(argc, argv, prefix, module_summary_options,\n>>> +\t\t\t     git_submodule_helper_usage, 0);\n>>> +\n>>> +\tif (!summary_limit)\n>>> +\t\treturn 0;\n>>> +\n>>> +\tcp_rev.git_cmd = 1;\n>>> +\targv_array_pushl(&cp_rev.args, \"rev-parse\", \"-q\", \"--verify\",\n>>> +\t\t\t argc ? argv[0] : \"HEAD\", NULL);\n>>\n>> Oy. Why not simply call `get_oid()`? No need to spawn a new process.\n> \n> Then everytime we need 'rev-parse', we simply call 'get_oid()'? That\n> will save us a ton of processes?\n> \n> But I think we do need to capture the output of 'git rev-parse --verify\n> ....' so I think it will backfire to use 'get_oid()' or am I just being\n> too dumb and not catching on something?\n> \n\nI'll leave this for others to answer.\n\n>>> +\n>>> +\tif (!capture_command(&cp_rev, &sb, 0)) {\n>>> +\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n>>> +\t\tif (argc) {\n>>> +\t\t\targv++;\n>>> +\t\t\targc--;\n>>> +\t\t}\n>>> +\t} else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n>>> +\t\t/* before the first commit: compare with an empty tree */\n>>> +\t\tstruct stat st;\n>>> +\t\tstruct object_id oid;\n>>> +\t\tif (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,\n>>> +\t\t\t\t\t\t  prefix, 3))\n>>> +\t\t\tdie(\"Unable to add %s to database\", oid.hash);\n>>\n>> Umm. The original reads:\n>>\n>>                 # before the first commit: compare with an empty tree\n>>                 head=$(git hash-object -w -t tree --stdin </dev/null)\n>>\n>> It does not actually read from `stdin`. It reads from `/dev/null`,\n>> redirected to the input. And what it _actually_ does is to generate the\n>> OID of the empty tree.\n>>\n>> But we already _have_ the OID of the empty tree! It's\n>> `the_hash_algo->empty_tree`.\n> \n> I did not know this 'the_hash_algo'. I will use it. Thanks! :)\n> \n>> I hope that this is covered by the test suite. Please check that. The test\n>> would succeed with your version, but only because tests are run with\n>> `stdin` redirected from `/dev/null` by default.\n> \n> I guess yes. My work passed because the tests are written this way.\n> \n>>> +\t\tstrbuf_addstr(&sb, oid_to_hex(&oid));\n>>> +\t\tif (argc) {\n>>> +\t\t\targv++;\n>>> +\t\t\targc--;\n>>> +\t\t}\n>>> +\t} else {\n>>> +\t\tstrbuf_addstr(&sb, \"HEAD\");\n>>> +\t}\n>>\n>> The conversion to C would make for a fine excuse to simplify the logic.\n> \n> This was kind of like the 'shift' in shell. What equivalent do you\n> suggest?\n> \n\nI think that's just a general comment after the other comments found\njust above about simplifying things.\n\n>>> +\tif (files) {\n>>> +\t\tif (cached)\n>>> +\t\t\tdie(_(\"--cached and --files are mutually exclusive\"));\n>>> +\t\tdiff_cmd = DIFF_FILES;\n>>> +\t}\n>>> +\n>>> +\tinfo.argc = argc;\n>>> +\tinfo.argv = argv;\n>>> +\tinfo.prefix = prefix;\n>>> +\tinfo.cached = !!cached;\n>>> +\tinfo.for_status = !!for_status;\n>>> +\tinfo.quiet = quiet;\n>>> +\tinfo.files = files;\n>>> +\tinfo.summary_limit = summary_limit;\n>>> +\n>>> +\tret = compute_summary_module_list((diff_cmd == DIFF_FILES) ? NULL : sb.buf,\n>>> +\t\t\t\t\t   &info, diff_cmd);\n>>\n>> It would be better to pass the OID as `struct object_id *`, not as string.\n> \n> Will do!\n> \n>> Other than that, this patch nicely follows previous conversions from Unix\n>> shell scripts to C.\n>>\n>> Well done,\n>> Johannes\n> \n> Thank you! It was a highly detailed review! I am still learning tons of\n> stuff about Git's code and such a review does help a lot! :)\n> \n\nHope this helps,\nSivaraam\n"},{"id":"401041","messageId":"20200706111559.GA10820@konoha","threadId":"53791","inReplyTo":"0a2e53d4-7fc5-183d-663d-7a8f52b2a685@gmail.com","subject":"Re: [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-06T11:15:59Z","receivedAt":"2020-07-06T11:16:13Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 06/07 02:46, Kaartic Sivaraam wrote:\n> Note: I've added some comment but I've not been able to address all the\n> parts due to lack of time. I'm sending it sooner hoping it would be\n> useful.\n\nSure no problem\n\n> On 05-07-2020 23:04, Shourya Shukla wrote:\n> [...]\n> >> [exchanging Stefan Beller's dysfunct @google address for their private\n> >> one; I encourage you to do the same in the next iteration, probably\n> >> by editing the `Mentored-by:` line.]\n> > \n> > I think you missed to mention it.\n> >\n> \n> If you're looking for the private e-mail. The exachange was already done\n> and it was right there in the Cc list of the mail sent by Dscho. I've\n> added it again as you seem to have removed it.\n\nOh I did not catch that actually. Thanks!\n\n> >> On Fri, 3 Jul 2020, Shourya Shukla wrote:\n> >>\n> >>> From: Prathamesh Chavan <pc44800@gmail.com>\n> >>>\n> >>> The submodule subcommand 'summary' is ported in the process of\n> >>> making git-submodule a builtin. The function cmd_summary() from\n> >>> git-submodule.sh is ported to functions module_summary(),\n> >>> compute_summary_module_list(), prepare_submodule_summary() and\n> >>> generate_submodule_summary(), print_submodule_summary().\n> >>>\n> >>> The first function module_summary() parses the options of submodule\n> >>> subcommand and also acts as the front-end of this subcommand.\n> >>> After parsing them, it calls the compute_summary_module_list()\n> >>\n> >> Missing full-stop, and probably the sentence also wanted to say \"function\"\n> >> at the end.\n> > \n> > I will correct. Thanks for pointing out!\n> > \n> >>> The functions compute_summary_module_list() runs the diff_cmd,\n> >>> and generates the modules list, as required by the subcommand.\n> >>> The generation of this module list is done by the using the\n> >>\n> >> s/the using/using/\n> > \n> > Will amend!\n> > \n> >>> callback function submodule_summary_callback(), and stored in the\n> >>> structure module_cb.\n> >>\n> >> This explains nicely what the patch does. But the commit message should\n> >> not really repeat what can be readily deduced from the patch; It should\n> >> focus on the motivation and on interesting background information that is\n> >> _not_ readily deduced from the patch.\n> > \n> > I understand. I will follow your suggestions regarding my patch.\n> > \n> >> For example, I see that `$diff_cmd` is called twice in the shell script\n> >> version, once to \"get modified modules cared by user\" and then _again_,\n> >> with that list of modified modules.\n> >>\n> >> I would have liked to see a reasoning in the commit message that explains\n> >> why this has to be so in the C version. I get why it is complicated in a\n> >> shell script (which lacks proper objects, after all), but I would have\n> >> expected the C version to be able to accumulate the information with a\n> >> single pass.\n> >>\n> >> (Before writing the following paragraph, I actually reviewed the patch\n> >> from bottom to top, in the caller->callee direction.)\n> >>\n> >> Ah. I see that this indeed is the case: there is only one pass in the C\n> >> version. That's a useful piece of metadata for the commit message, I\n> >> think, much more useful than describing the call tree of the functions.\n> > \n> > Yup that it worth mentioning.\n> > \n> >> Another thing worth mentioning in the commit message is that we use the\n> >> combination of setting a child_process' working directory to the submodule\n> >> path and then calling `prepare_submodule_repo_env()` which also sets\n> >> `GIT_DIR` to `.git`, so that we can be certain that those spawned\n> >> processes will not access the superproject's ODB by mistake.\n> >>\n> >> When reading my suggestions, please keep in mind that I reviewed the\n> >> functions in caller->callee order, i.e. I started at the end of the patch\n> >> and then worked my way up.\n> >>\n> >> All in all, I like the function structure, but I think there is still a\n> >> bit room for improvement in a v2.\n> > \n> >>> +static int verify_submodule_object_name(const char *sm_path,\n> >>> +\t\t\t\t\t  const char *sha1)\n> >>> +{\n> >>> +\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> >>> + > > +\tcp_rev_parse.git_cmd = 1;\n> >>> +\tcp_rev_parse.no_stdout = 1;\n> >>> +\tcp_rev_parse.dir = sm_path;\n> >>\n> >> So here we specify `sm_path` as current working directory.\n> >>\n> >>> +\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n> >>\n> >> And this implicitly sets `GIT_DIR=.git`. Good.\n> >>\n> >>> +\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\", \"-q\",\n> >>> +\t\t\t \"--verify\", NULL);\n> >>> +\targv_array_pushf(&cp_rev_parse.args, \"%s^0\", sha1);\n> >>\n> >> After this, we should also append `--` to make sure that we're not parsing\n> >> this as a file name.\n> > \n> > Will do!\n> > \n> >> Two comments about naming: `sha1` is pretty misleading here, as we do not\n> >> require it to be a SHA-1 (especially in the future in which we switch to\n> >> SHA-256). Besides, what we're really asking for (via that `^0` suffix) is\n> >> a committish. Therefore, I would propose to use `committish` both in the\n> >> parameter name as well as the function name.\n> > \n> > I am not aware of this change. I will take this suggestion into account.\n> > \n> >>> +\n> >>> +\tif (run_command(&cp_rev_parse))\n> >>> +\t\treturn 1;\n> >>> +\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +static void print_submodule_summary(struct summary_cb *info, int errmsg,\n> >>> +\t\t\t\t      int total_commits, int missing_src,\n> >>> +\t\t\t\t      int missing_dst, const char *displaypath,\n> >>> +\t\t\t\t      int is_sm_git_dir, struct module_cb *p)\n> >>> +{\n> >>> +\tif (p->status == 'T') {\n> >>> +\t\tif (S_ISGITLINK(p->mod_dst))\n> >>> +\t\t\tprintf(_(\"* %s %s(blob)->%s(submodule)\"),\n> >>> +\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n> >>\n> >> The shell script version does this:\n> >>\n> >>                 sha1_abbr_src=$(GIT_DIR=\"$name/.git\" git rev-parse --short $sha1_src 2>/dev/null ||\n> >>                         echo $sha1_src | cut -c1-7)\n> >>\n> >> That is not quite the same, as it looks for the abbreviation _in the\n> >> submodule_, not in the current project. So I think `find_unique_abbrev()`\n> >> is not correct here.\n> >>\n> >> The funny thing is that we _already_ will have called `git rev-parse\n> >> --verify` for both `p->oid_src` and `p->oid_dst` in the submodule, in the\n> >> caller of this function! And while we throw away the result, and while we\n> >> do not pass `--short`, there is no reason why we shouldn't be able to do\n> >> precisely that.\n> > \n> > Okay so you are saying that there is no need of a 'find_unique_abbrev()'\n> > since we would be easily able to obtain these values from the caller of\n> > 'print_submodule_summary()' right?\n> \n> Yes. 'generate_submodule_summary' already does a rev-parse on\n> p->oid_src and p->oid_dst via 'verify_submodule_object_name'.\n> We should be able to get the short version of them by passing '--short'\n> to rev-parse there and make it return the short SHA1. We can then use it\n> like how Dscho mentions below.\n\nOkay. That seems good. Yes, it will be cleaner to pass them as\narguments.\n\n> > Maybe we can pass 'oid_src' or 'oid_dst' as an argument?\n> > \n> >>> +\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n> >>> +\t\telse\n> >>> +\t\t\tprintf(_(\"* %s %s(submodule)->%s(blob)\"),\n> >>> +\t\t\t\t displaypath, find_unique_abbrev(&p->oid_src, 7),\n> >>> +\t\t\t\t find_unique_abbrev(&p->oid_dst, 7));\n> >>> +\t} else {\n> >>> +\t\tprintf(\"* %s %s...%s\",\n> >>> +\t\t\tdisplaypath, find_unique_abbrev(&p->oid_src, 7),\n> >>> +\t\t\tfind_unique_abbrev(&p->oid_dst, 7));\n> >>> +\t}\n> >>> +\n> >>> +\tif (total_commits < 0)\n> >>> +\t\tprintf(\":\\n\");\n> >>> +\telse\n> >>> +\t\tprintf(\" (%d):\\n\", total_commits);\n> >>> +\n> >>> +\tif (errmsg) {\n> >>> +\t\t/*\n> >>> +\t\t * Don't give error msg for modification whose dst is not\n> >>> +\t\t * submodule, i.e. deleted or changed to blob\n> >>> +\t\t */\n> >>> +\t\tif (S_ISGITLINK(p->mod_src)) {\n> >>> +\t\t\tif (missing_src && missing_dst) {\n> >>> +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commits %s and %s\\n\"),\n> >>> +\t\t\t\t       displaypath, oid_to_hex(&p->oid_src),\n> >>> +\t\t\t\t       oid_to_hex(&p->oid_dst));\n> >>> +\t\t\t} else if (missing_src) {\n> >>> +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n> >>> +\t\t\t\t       displaypath, oid_to_hex(&p->oid_src));\n> >>> +\t\t\t} else {\n> >>> +\t\t\t\tprintf(_(\"  Warn: %s doesn't contain commit %s\\n\"),\n> >>> +\t\t\t\t       displaypath, oid_to_hex(&p->oid_dst));\n> >>> +\t\t\t}\n> >>> +\t\t}\n> >>> +\t} else if (is_sm_git_dir) {\n> >>> +\t\tstruct child_process cp_log = CHILD_PROCESS_INIT;\n> >>> +\n> >>> +\t\tcp_log.git_cmd = 1;\n> >>> +\t\tcp_log.dir = p->sm_path;\n> >>> +\t\tprepare_submodule_repo_env(&cp_log.env_array);\n> >>\n> >> Since the working directory is set to the top-level directory of the\n> >> submodule, and since `prepare_submodule_repo_env()` sets `GIT_DIR` to\n> >> `.git`, I think that the `is_sm_git_dir` condition is unnecessary. In\n> >> fact, the entire `is_sm_git_dir` parameter (and local variable in the\n> >> caller, see more on that below) can go away.\n> > \n> > Because we already set the $GIT_DIR to .git/ so an extra check will not\n> > be necessary right?\n> > \n> \n> Yes. If we remove that check and we get a p->sm_path that does not point\n> to a submodule, I wonder what would happen if we run 'run_command'\n> on it. I'm also not sure if that's a possible case. Something to\n> explore.\n\nThis might take some time but would be a huge change for this patch if\nsuccessful.\n\n> >>> +\t\targv_array_pushl(&cp_log.args, \"log\", NULL);\n> >>> +\n> >>> +\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst)) {\n> >>> +\t\t\tif (info->summary_limit > 0)\n> >>> +\t\t\t\targv_array_pushf(&cp_log.args, \"-%d\",\n> >>> +\t\t\t\t\t\t info->summary_limit);\n> >>> +\n> >>> +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  %m %s\",\n> >>> +\t\t\t\t\t \"--first-parent\", NULL);\n> >>> +\t\t\targv_array_pushf(&cp_log.args, \"%s...%s\",\n> >>> +\t\t\t\t\t oid_to_hex(&p->oid_src),\n> >>> +\t\t\t\t\t oid_to_hex(&p->oid_dst));\n> >>> +\t\t} else if (S_ISGITLINK(p->mod_dst)) {\n> >>> +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  > %s\",\n> >>> +\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_dst), NULL);\n> >>> +\t\t} else {\n> >>> +\t\t\targv_array_pushl(&cp_log.args, \"--pretty=  < %s\",\n> >>> +\t\t\t\t\t \"-1\", oid_to_hex(&p->oid_src), NULL);\n> >>> +\t\t}\n> >>> +\t\trun_command(&cp_log);\n> >>> +\t}\n> >>> +\tprintf(\"\\n\");\n> >>> +}\n> >>\n> >> It looks as if there is a whole lot of `oid_to_hex(&p->oid_src)` in that\n> >> function. Together with the realization that we need the abbreviated\n> >> version of that at least in one place, and the other realization that we\n> >> already call `rev-parse --verify` for both `oid_src` and `oid_dst` in the\n> >> caller of this function, it seems to suggest itself that we would actually\n> >> want to pass the `--short` option, too, and to capture the output, and\n> >> pass it down to `print_submodule_summary()` _instead of_ `missing_src` and\n> >> `missing_dst` (e.g. as `src_abbrev` and `dst_abbrev`).\n> > \n> > Oh you have mentioned it here too. This seems quite a good approach. I\n> > will adopt this.\n> > \n> >>> +\n> >>> +static void generate_submodule_summary(struct summary_cb *info,\n> >>> +\t\t\t\t       struct module_cb *p)\n> >>> +{\n> >>> +\tint missing_src = 0;\n> >>> +\tint missing_dst = 0;\n> >>> +\tchar *displaypath;\n> >>> +\tint errmsg = 0;\n> >>> +\tint total_commits = -1;\n> >>> +\tint is_sm_git_dir = 0;\n> >>> +\tstruct strbuf sm_git_dir_sb = STRBUF_INIT;\n> >>> +\n> >>> +\tif (!info->cached && oideq(&p->oid_dst, &null_oid)) {\n> >>> +\t\tif (S_ISGITLINK(p->mod_dst)) {\n> >>> +\t\t\t/*\n> >>> +\t\t\t * NEEDSWORK: avoid using separate process with\n> >>> +\t\t\t * the help of the function head_ref_submodule()\n> >>\n> >> I don't quite understand this comment. There is no `head_ref_submodule()`\n> >> function.\n> >>\n> \n> That NEEDSWORK was added based on Stefan's comment on a previous version\n> of Prathamesh's patch. Here it is for reference:\n> \n> >> +       if (!info->cached && !oidcmp(&p->oid_dst, &null_oid)) {\n> >> +               if (S_ISGITLINK(p->mod_dst)) {\n> >> +                       struct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> >> +                       struct strbuf sb_rev_parse = STRBUF_INIT;\n> >> +\n> >> +                       cp_rev_parse.git_cmd = 1;\n> >> +                       cp_rev_parse.no_stderr = 1;\n> >> +                       cp_rev_parse.dir = p->sm_path;\n> >> +                       prepare_submodule_repo_env(&cp_rev_parse.env_array);\n> >> +\n> >> +                       argv_array_pushl(&cp_rev_parse.args,\n> >> +                                        \"rev-parse\", \"HEAD\", NULL);\n> >> +                       if (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n> >> +                               strbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n> >> +\n> >> +                               get_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n> >> +                       }\n> >> +                       strbuf_release(&sb_rev_parse);\n> > \n> > I think this could be replaced via\n> > head_ref_submodule(sub->path, callback function, &where_to_store)\n> > or is there some trickery going on, that this also works on\n> > non-compliant submodules?\n> > (Maybe add that as a NEEDSWORK/TODO)\n> \n> Ref:\n> https://public-inbox.org/git/CAGZ79kaWn9z47Va=VW4R2Aswws1N5n2u4Kvatn73s0YnV0pVqQ@mail.gmail.com/\n> \n> A quick search reveals that 'head_ref_submodule' existed during that\n> period. On further investigation it seems that 'refs_head_ref' was\n> introduced in 62f0b399e0 (refs: add refs_head_ref(), 2017-08-23) and\n> 'head_ref_submodule' was made to use it. Later on, in 419221c106 (refs:\n> remove dead for_each_*_submodule(), 2017-08-23), 'head_ref-submodule'\n> was removed with an advice to use the 'refs_' API for accessing\n> submodules.\n> \n> > +* Use `refs_` API for accessing submodules. The submodule ref store could\n> > +  be obtained with `get_submodule_ref_store()`\n> \n> How it applies to our code is something to be looked into, yet.\n> \n> >>> +\t\t\t */\n> >>> +\t\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> >>> +\t\t\tstruct strbuf sb_rev_parse = STRBUF_INIT;\n> >>> +\n> >>> +\t\t\tcp_rev_parse.git_cmd = 1;\n> >>> +\t\t\tcp_rev_parse.no_stderr = 1;\n> >>> +\t\t\tcp_rev_parse.dir = p->sm_path;\n> >>> +\t\t\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n> >>> +\n> >>> +\t\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n> >>> +\t\t\t\t\t \"HEAD\", NULL);\n> >>> +\t\t\tif (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n> >>> +\t\t\t\tstrbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n> >>> +\t\t\t\tget_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n> >>> +\t\t\t}\n> >>> +\t\t\tstrbuf_release(&sb_rev_parse);\n> >>> +\t\t} else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n> >>> +\t\t\tstruct child_process cp_hash_object = CHILD_PROCESS_INIT;\n> >>> +\t\t\tstruct strbuf sb_hash_object = STRBUF_INIT;\n> >>> +\n> >>> +\t\t\tcp_hash_object.git_cmd = 1;\n> >>> +\t\t\targv_array_pushl(&cp_hash_object.args, \"hash-object\",\n> >>> +\t\t\t\t\t p->sm_path, NULL);\n> >>> +\t\t\tif (!capture_command(&cp_hash_object,\n> >>> +\t\t\t\t\t     &sb_hash_object, 0)) {\n> >>> +\t\t\t\tstrbuf_strip_suffix(&sb_hash_object, \"\\n\");\n> >>> +\t\t\t\tget_oid_hex(sb_hash_object.buf, &p->oid_dst);\n> >>> +\t\t\t}\n> >>> +\t\t\tstrbuf_release(&sb_hash_object);\n> >>\n> >> It would probably be shorter, less error-prone, and quicker to use\n> >> `index_fd()` directly.\n> >>\n> >> BTW I am not quite sure that this code does the correct thing in case of a\n> >> symlink: it hashes the contents of the symlink target (if it is a file,\n> >> otherwise it errors out). But that is hardly an issue introduced by the\n> >> conversion, that's just copied from `git-submodule.sh`.\n> >>\n> >>> +\t\t} else {\n> >>> +\t\t\tif (p->mod_dst)\n> >>> +\t\t\t\tdie(_(\"unexpected mode %d\\n\"), p->mod_dst);\n> >>\n> >> Hmm. This does not match what the shell script version does:\n> >>\n> >>                         *)\n> >>                                 # unexpected type\n> >>                                 eval_gettextln \"unexpected mode \\$mod_dst\" >&2\n> >>                                 continue ;;\n> >>\n> >> I think we should also just write the message to `stderr` and continue,\n> >> not `die()`.\n> >>\n> >> In addition to that, I am missing the C code for this case:\n> >>\n> >>                         000000)\n> >>                                 ;; # removed\n> >>\n> >> It is quite possible that our test suite does not cover this case (or did\n> >> the test suite fail for you?). If that is indeed the case, it would be\n> >> really good to add a test case as part of this patch series, to gain\n> >> confidence in the correctness of the conversion.\n> > \n> > The tests passed for me actually. Whether this is covered by the test\n> > cases, I am not sure. I will have to check it.\n> > \n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\tstrbuf_addstr(&sm_git_dir_sb, p->sm_path);\n> >>\n> >> I have to admit that I am not loving the name `sm_git_dir_sb`. Why not\n> >> `submodule_git_dir`? I guess you copied it from elsewhere in\n> >> `submodule--helper.c`...\n> >>\n> >>> +\tif (is_nonbare_repository_dir(&sm_git_dir_sb))\n> >>> +\t\tis_sm_git_dir = 1;\n> >>\n> >> So here, we verify whether there is a repository at `p->sm_path`. I don't\n> >> see that in the shell script version:\n> >>\n> >>                 missing_src=\n> >>                 missing_dst=\n> >>\n> >>                 test $mod_src = 160000 &&\n> >>                 ! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_src^0 >/dev/null &&\n> >>                 missing_src=t\n> >>\n> >>                 test $mod_dst = 160000 &&\n> >>                 ! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n> >>                 missing_dst=t\n> >>\n> >> Let's read a bit further.\n> >>\n> >>> +\n> >>> +\tif (is_sm_git_dir && S_ISGITLINK(p->mod_src))\n> >>> +\t\tmissing_src = verify_submodule_object_name(p->sm_path,\n> >>> +\t\t\t\t\t\t\t   oid_to_hex(&p->oid_src));\n> >>\n> >> Ah, and `verify_submodule_object_name()` uses `p->sm_path` as working\n> >> directory. But that's not what the shell script version did: it specified\n> >> the `GIT_DIR` explicitly.\n> >>\n> >> And by using the `prepare_submodule_repo_env()` function in\n> >> `verify_submodule_object_name()`, we specify `GIT_DIR` implicitly, as I\n> >> pointed out in my comment on that function.\n> > \n> > Oh so you're saying that it will be better to call\n> > 'prepare_submodule_repo_env()' on some variable since we explicitly want to\n> > store the path to GIT_DIR?\n> > \n> \n> We already call 'prepare_submodule_repo_env' in\n> 'verify_submodule_object_name'. So, he's likely saying that\n> 'is_sm_git_dir' is unnecessary here.\n\nUnderstood.\n\n> > It would be of much help if you could explain this part just a little\n> > more (for my own sake).\n> > \n> >> So I think that `is_sm_git_dir` might be\n> > \n> \n> ... unnecessary.\n> \n> > I think you missed something here...\n> > \n> \n> That's likely what he meant based on what is mentioned above.\n\nOh okay. Thanks!\n\n> >>> +\n> >>> +\tif (is_sm_git_dir && S_ISGITLINK(p->mod_dst))\n> >>> +\t\tmissing_dst = verify_submodule_object_name(p->sm_path,\n> >>> +\t\t\t\t\t\t\t   oid_to_hex(&p->oid_dst));\n> >>> +\n> >>> +\tdisplaypath = get_submodule_displaypath(p->sm_path, info->prefix);\n> >>> +\n> >>> +\tif (!missing_dst && !missing_src) {\n> >>> +\t\tif (is_sm_git_dir) {\n> >>> +\t\t\tstruct child_process cp_rev_list = CHILD_PROCESS_INIT;\n> >>> +\t\t\tstruct strbuf sb_rev_list = STRBUF_INIT;\n> >>> +\t\t\tchar *range;\n> >>> +\n> >>> +\t\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n> >>> +\t\t\t\trange = xstrfmt(\"%s...%s\", oid_to_hex(&p->oid_src),\n> >>> +\t\t\t\t\t\toid_to_hex(&p->oid_dst));\n> >>> +\t\t\telse if (S_ISGITLINK(p->mod_src))\n> >>> +\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_src));\n> >>> +\t\t\telse\n> >>> +\t\t\t\trange = xstrdup(oid_to_hex(&p->oid_dst));\n> >>> +\n> >>> +\t\t\tcp_rev_list.git_cmd = 1;\n> >>> +\t\t\tcp_rev_list.dir = p->sm_path;\n> >>> +\t\t\tprepare_submodule_repo_env(&cp_rev_list.env_array);\n> >>\n> >> Again, due to setting the working directory to `p->sm_path` and\n> >> (implicitly, via `prepare_submodule_repo_env()`) `GIT_DIR` to `.git`, I do\n> >> not think that we have to guard this block beind `is_sm_git_dir`.\n> > \n> >>> +\n> >>> +\t\t\targv_array_pushl(&cp_rev_list.args, \"rev-list\",\n> >>> +\t\t\t\t\t \"--first-parent\", range, \"--\", NULL);\n> >>\n> >> Since `argv_array_push()` duplicates the strings, anyway, we can totally\n> >> avoid the need for the `range` variable:\n> >>\n> >> \t\t\tif (IS_GITLINK(p->mod_src) && IS_GITLINK(p->mod_dst))\n> >> \t\t\t\targv_array_pushf(&cp_rev_list.args, \"%s...%s\",\n> >> \t\t\t\t\t\t oid_to_hex(&p->oid_src),\n> >> \t\t\t\t\t\t oid_to_hex(&p->oid_dst));\n> >> \t\t\telse\n> >> \t\t\t\targv_array_push(&cp_rev_list.args, IS_GITLINK(p->mod_src) ?\n> >> \t\t\t\t\t\toid_to_hex(&p->oid_src) :\n> >> \t\t\t\t\t\toid_to_hex(&p->oid_dst));\n> >>\n> >>> +\t\t\tif (!capture_command(&cp_rev_list, &sb_rev_list, 0)) {\n> >>> +\t\t\t\tif (sb_rev_list.len)\n> >>> +\t\t\t\t\ttotal_commits = count_lines(sb_rev_list.buf,\n> >>> +\t\t\t\t\t\t\t\t    sb_rev_list.len);\n> >>\n> >> That's actually not necessary. `git rev-list --count` will give you a nice\n> >> number, no need to capture a potentially large amount of memory only to\n> >> count the lines.\n> >>\n> >> This may also make the patch obsolete that makes `count_lines()` public.\n> > \n> > Therefore we eliminate count_lines() from here and instead do a 'git\n> > rev-list --count'?\n> > \n> \n> Yes.\n> \n> >>> +\t\t\t\telse\n> >>> +\t\t\t\t\ttotal_commits = 0;\n> >>> +\t\t\t}\n> >>\n> >>> +\n> >>> +\t\t\tfree(range);\n> >>> +\t\t\tstrbuf_release(&sb_rev_list);\n> >>> +\t\t}\n> >>> +\t} else {\n> >>> +\t\terrmsg = 1;\n> >>> +\t}\n> >>\n> >> I am missing the equivalent for these lines here:\n> >>\n> >>                 t,)\n> >>                         errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_src\")\"\n> >>                         ;;\n> >>                 ,t)\n> >>                         errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commit \\$sha1_dst\")\"\n> >>                         ;;\n> >>                 t,t)\n> >>                         errmsg=\"$(eval_gettext \"  Warn: \\$display_name doesn't contain commits \\$sha1_src and \\$ ha1_dst\")\"\n> >>                         ;;\n> > \n> > I will add them.\n> > \n> \n> I think they're already there in the 'print_submodule_summary' function\n> above.\n> \n> >> I am not quite sure whether it is a good idea to leave it to the\n> >> `print_submodule_summary()` function to generate the `errmsg`. I think I'd\n> >> rather have it a `char *` than an `int`.\n> > \n> > Would it be better to add these error messages in\n> > 'prepare_submodule_summary()'?\n> \n> No. He's likely saying that instead of setting `errmsg` to 1 and\n> constructing the error message in `print_submodule_summary` we should\n> be having the error messages in `generate_submodule_summary` and pass\n> them to `print_submodule_summary` instead of passing an int.\n\nSo this means that we will have to generate the above mentioned messages\nin 'generate_submodule_summary()' and then pass them as char into\n'print_submodule_summary()' rather than generating them in the latter.\nThis is what we want right?\n\n> > If we have error messages as integers\n> > then we will simply\n> > \n> \n> You missed something here. ;)\n\nI actually don't know what I was trying to write here.\n\n> >>> +\n> >>> +\tprint_submodule_summary(info, errmsg, total_commits,\n> >>> +\t\t\t\tmissing_src, missing_dst,\n> >>> +\t\t      \t\tdisplaypath, is_sm_git_dir, p);\n> >>> +\n> >>> +\tfree(displaypath);\n> >>> +\tstrbuf_release(&sm_git_dir_sb);\n> >>> +}\n> >>> +\n> >>> +static void prepare_submodule_summary(struct summary_cb *info,\n> >>> +\t\t\t\t      struct module_cb_list *list)\n> >>> +{\n> >>> +\tint i;\n> >>> +\tfor (i = 0; i < list->nr; i++) {\n> >>> +\t\tstruct module_cb *p = list->entries[i];\n> >>> +\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> >>> +\n> >>> +\t\tif (p->status == 'D' || p->status == 'T') {\n> >>> +\t\t\tgenerate_submodule_summary(info, p);\n> >>> +\t\t\tcontinue;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tif (info->for_status) {\n> >>> +\t\t\tchar *config_key;\n> >>\n> >> Since the `config_key` is only used within the `if()` block it would be\n> >> better to declare it within that block.\n> >>\n> >>> +\t\t\tconst char *ignore_config = \"none\";\n> >>\n> >> Since the only value we ever care about is \"all\", how about turning this\n> >> into an `int`, setting it to `0` here, and later assigning it to\n> >> `!strcmp(value, \"all\")` and `!strcmp(sub->ignore, \"all\")`, respectively?\n> > \n> > Alright will do!\n> > \n> >> I mean, I get it. Unix shell scripts are all about passing around text.\n> >> And it is much easier to just translate that to C faithfully. But that\n> >> does not make it good C code. C has data types, and proper C code makes\n> >> use of that.\n> >>\n> >>> +\t\t\tconst char *value;\n> >>\n> >> If you want to save on lines, you can cuddle this together with other\n> >> declarations of the same type. Even so, it could be scoped more narrowly.\n> >>\n> >>> +\t\t\tconst struct submodule *sub = submodule_from_path(the_repository,\n> >>> +\t\t\t\t\t\t\t\t\t  &null_oid,\n> >>> +\t\t\t\t\t\t\t\t\t  p->sm_path);\n> >>> +\n> >>> +\t\t\tif (sub && p->status != 'A') {\n> >>\n> >> Good. The shell script version _always_ retrieved the `.ignore` config\n> >> value, even if the `status` is `A`. Your version is much better.\n> >>\n> >> But why bother calling `submodule_from_path()` if the status is `A`?\n> > \n> > What exactly does a status of 'A' or 'T' mean? I mean I know what we are\n> > doing but what exactly do these translate into?\n> > \n> \n> Its interesting you understood it without knowing what 'A' and 'T'\n> meant. Anyways, if you take a look at the documentation of\n> 'diff-index'[1] which provides us the `status` you'll know that:\n\nI just assumed that they must mean something to something and read on\nthe rest of the code. I meant to ask the full-form of 'A', 'T', etc.\nYou have provided it below and this makes things even more clear.\nThanks!\n\n> > Possible status letters are:\n> > \n> >     A: addition of a file\n> > \n> >     C: copy of a file into a new one\n> > \n> >     D: deletion of a file\n> > \n> >     M: modification of the contents or mode of a file\n> > \n> >     R: renaming of a file\n> > \n> >     T: change in the type of the file\n> > \n> >     U: file is unmerged (you must complete the merge before it can be committed)\n> > \n> >     X: \"unknown\" change type (most probably a bug, please report it)\n> > \n> \n> [1]: https://git-scm.com/docs/git-diff-index\n> \n> >> I could actually see the `const struct submodule *sub;` declaration be\n> >> pulled out of this scope, and combining the `if (info->for_status &&\n> >> p->status != 'A'), and the moving the assignment of `sub` into the `else\n> >> if ((sub = submodule_from_path(r, &null_oid, p->sm_path)) &&\n> >> sub->ignore)`.\n> >>\n> >> That would save us one entire indentation level.\n> > \n> > That seems a good approach! I will try this out.\n> > \n> >>> +\t\t\t\tconfig_key = xstrfmt(\"submodule.%s.ignore\",\n> >>> +\t\t\t\t\t\t     sub->name);\n> >>> +\t\t\t\tif (!git_config_get_string_const(config_key, &value))\n> >>> +\t\t\t\t\tignore_config = value;\n> >>> +\t\t\t\telse if (sub->ignore)\n> >>> +\t\t\t\t\tignore_config = sub->ignore;\n> >>> +\n> >>> +\t\t\t\tfree(config_key);\n> >>> +\t\t\t\tif (!strcmp(ignore_config, \"all\"))\n> >>> +\t\t\t\t\tcontinue;\n> >>> +\t\t\t}\n> >>> +\t\t}\n> >>> +\n> >>> +\t\t/* Also show added or modified modules which are checked out */\n> >>> +\t\tcp_rev_parse.dir = p->sm_path;\n> >>> +\t\tcp_rev_parse.git_cmd = 1;\n> >>> +\t\tcp_rev_parse.no_stderr = 1;\n> >>> +\t\tcp_rev_parse.no_stdout = 1;\n> >>> +\n> >>> +\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n> >>> +\t\t\t\t \"--git-dir\", NULL);\n> >>> +\n> >>> +\t\tif (!run_command(&cp_rev_parse))\n> >>\n> >> I wonder whether we really need to waste an entire spawned process on\n> >> figuring out whether `p->sm_path` refers to an active repository. Wouldn't\n> >> `is_submodule_active(r, p->sm_path)` fulfill the same purpose?\n> > \n> > Yep! This is correct. I will change.\n> > \n> >>> +\t\t\tgenerate_submodule_summary(info, p);\n> >>> +\t}\n> >>> +}\n> >>> +\n> >>> +static void submodule_summary_callback(struct diff_queue_struct *q,\n> >>> +\t\t\t\t       struct diff_options *options,\n> >>> +\t\t\t\t       void *data)\n> >>> +{\n> >>> +\tint i;\n> >>> +\tstruct module_cb_list *list = data;\n> >>> +\tfor (i = 0; i < q->nr; i++) {\n> >>> +\t\tstruct diff_filepair *p = q->queue[i];\n> >>> +\t\tstruct module_cb *temp;\n> >>> +\n> >>> +\t\tif (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode))\n> >>> +\t\t\tcontinue;\n> >>> +\t\ttemp = (struct module_cb*)malloc(sizeof(struct module_cb));\n> >>> +\t\ttemp->mod_src = p->one->mode;\n> >>> +\t\ttemp->mod_dst = p->two->mode;\n> >>> +\t\ttemp->oid_src = p->one->oid;\n> >>> +\t\ttemp->oid_dst = p->two->oid;\n> >>> +\t\ttemp->status = p->status;\n> >>> +\t\ttemp->sm_path = xstrdup(p->one->path);\n> >>> +\n> >>> +\t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n> >>> +\t\tlist->entries[list->nr++] = temp;\n> >>> +\t}\n> >>> +}\n> >>> +\n> >>> +static const char *get_diff_cmd(enum diff_cmd diff_cmd)\n> >>> +{\n> >>> +\tswitch (diff_cmd) {\n> >>> +\tcase DIFF_INDEX: return \"diff-index\";\n> >>> +\tcase DIFF_FILES: return \"diff-files\";\n> >>> +\tdefault: BUG(\"bad diff_cmd value %d\", diff_cmd);\n> >>> +\t}\n> >>> +}\n> >>> +\n> >>> +static int compute_summary_module_list(char *head,\n> >>> +\t\t\t\t         struct summary_cb *info,\n> >>> +\t\t\t\t         enum diff_cmd diff_cmd)\n> >>> +{\n> >>> +\tstruct argv_array diff_args = ARGV_ARRAY_INIT;\n> >>> +\tstruct rev_info rev;\n> >>> +\tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n> >>> +\n> >>> +\targv_array_push(&diff_args, get_diff_cmd(diff_cmd));\n> >>> +\tif (info->cached)\n> >>> +\t\targv_array_push(&diff_args, \"--cached\");\n> >>> +\targv_array_pushl(&diff_args, \"--ignore-submodules=dirty\", \"--raw\",\n> >>> +\t\t\t NULL);\n> >>> +\tif (head)\n> >>> +\t\targv_array_push(&diff_args, head);\n> >>> +\targv_array_push(&diff_args, \"--\");\n> >>> +\tif (info->argc)\n> >>> +\t\targv_array_pushv(&diff_args, info->argv);\n> >>> +\n> >>> +\tgit_config(git_diff_basic_config, NULL);\n> >>> +\tinit_revisions(&rev, info->prefix);\n> >>> +\trev.abbrev = 0;\n> >>> +\tprecompose_argv(diff_args.argc, diff_args.argv);\n> >>> +\n> >>> +\tdiff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,\n> >>> +\t\t\t\t\t &rev, NULL);\n> >>> +\trev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n> >>> +\trev.diffopt.format_callback = submodule_summary_callback;\n> >>> +\trev.diffopt.format_callback_data = &list;\n> >>> +\n> >>> +\tif (!info->cached) {\n> >>> +\t\tif (diff_cmd ==  DIFF_INDEX)\n> >>\n> >> Please substitute the double-space by a single one.\n> > \n> > Will do!\n> > \n> >>> +\t\t\tsetup_work_tree();\n> >>> +\t\tif (read_cache_preload(&rev.diffopt.pathspec) < 0) {\n> >>> +\t\t\tperror(\"read_cache_preload\");\n> >>> +\t\t\treturn -1;\n> >>> +\t\t}\n> >>> +\t} else if (read_cache() < 0) {\n> >>> +\t\tperror(\"read_cache\");\n> >>> +\t\treturn -1;\n> >>> +\t}\n> >>> +\n> >>> +\tif (diff_cmd == DIFF_INDEX)\n> >>> +\t\trun_diff_index(&rev, info->cached);\n> >>> +\telse\n> >>> +\t\trun_diff_files(&rev, 0);\n> >>> +\tprepare_submodule_summary(info, &list);\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +static int module_summary(int argc, const char **argv, const char *prefix)\n> >>> +{\n> >>> +\tstruct summary_cb info = SUMMARY_CB_INIT;\n> >>> +\tint cached = 0;\n> >>> +\tint for_status = 0;\n> >>> +\tint quiet = 0;\n> >>> +\tint files = 0;\n> >>> +\tint summary_limit = -1;\n> >>> +\tstruct child_process cp_rev = CHILD_PROCESS_INIT;\n> >>> +\tstruct strbuf sb = STRBUF_INIT;\n> >>> +\tenum diff_cmd diff_cmd = DIFF_INDEX;\n> >>> +\tint ret;\n> >>> +\n> >>> +\tstruct option module_summary_options[] = {\n> >>> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output of summarising submodules\")),\n> >>> +\t\tOPT_BOOL(0, \"cached\", &cached,\n> >>> +\t\t\t N_(\"Use the commit stored in the index instead of the submodule HEAD\")),\n> >>> +\t\tOPT_BOOL(0, \"files\", &files,\n> >>> +\t\t\t N_(\"To compare the commit in the index with that in the submodule HEAD\")),\n> >>> +\t\tOPT_BOOL(0, \"for-status\", &for_status,\n> >>> +\t\t\t N_(\"Skip submodules with 'ignore_config' value set to 'all'\")),\n> >>> +\t\tOPT_INTEGER('n', \"summary-limit\", &summary_limit,\n> >>> +\t\t\t     N_(\"Limit the summary size\")),\n> >>> +\t\tOPT_END()\n> >>> +\t};\n> >>> +\n> >>> +\tconst char *const git_submodule_helper_usage[] = {\n> >>> +\t\tN_(\"git submodule--helper summary [<options>] [commit] [--] [<path>]\"),\n> >>> +\t\tNULL\n> >>> +\t};\n> >>> +\n> >>> +\targc = parse_options(argc, argv, prefix, module_summary_options,\n> >>> +\t\t\t     git_submodule_helper_usage, 0);\n> >>> +\n> >>> +\tif (!summary_limit)\n> >>> +\t\treturn 0;\n> >>> +\n> >>> +\tcp_rev.git_cmd = 1;\n> >>> +\targv_array_pushl(&cp_rev.args, \"rev-parse\", \"-q\", \"--verify\",\n> >>> +\t\t\t argc ? argv[0] : \"HEAD\", NULL);\n> >>\n> >> Oy. Why not simply call `get_oid()`? No need to spawn a new process.\n> > \n> > Then everytime we need 'rev-parse', we simply call 'get_oid()'? That\n> > will save us a ton of processes?\n> > \n> > But I think we do need to capture the output of 'git rev-parse --verify\n> > ....' so I think it will backfire to use 'get_oid()' or am I just being\n> > too dumb and not catching on something?\n> > \n> \n> I'll leave this for others to answer.\n\nI will resolve this one after Dscho answers then.\n\n> >>> +\n> >>> +\tif (!capture_command(&cp_rev, &sb, 0)) {\n> >>> +\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n> >>> +\t\tif (argc) {\n> >>> +\t\t\targv++;\n> >>> +\t\t\targc--;\n> >>> +\t\t}\n> >>> +\t} else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n> >>> +\t\t/* before the first commit: compare with an empty tree */\n> >>> +\t\tstruct stat st;\n> >>> +\t\tstruct object_id oid;\n> >>> +\t\tif (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,\n> >>> +\t\t\t\t\t\t  prefix, 3))\n> >>> +\t\t\tdie(\"Unable to add %s to database\", oid.hash);\n> >>\n> >> Umm. The original reads:\n> >>\n> >>                 # before the first commit: compare with an empty tree\n> >>                 head=$(git hash-object -w -t tree --stdin </dev/null)\n> >>\n> >> It does not actually read from `stdin`. It reads from `/dev/null`,\n> >> redirected to the input. And what it _actually_ does is to generate the\n> >> OID of the empty tree.\n> >>\n> >> But we already _have_ the OID of the empty tree! It's\n> >> `the_hash_algo->empty_tree`.\n> > \n> > I did not know this 'the_hash_algo'. I will use it. Thanks! :)\n> > \n> >> I hope that this is covered by the test suite. Please check that. The test\n> >> would succeed with your version, but only because tests are run with\n> >> `stdin` redirected from `/dev/null` by default.\n> > \n> > I guess yes. My work passed because the tests are written this way.\n> > \n> >>> +\t\tstrbuf_addstr(&sb, oid_to_hex(&oid));\n> >>> +\t\tif (argc) {\n> >>> +\t\t\targv++;\n> >>> +\t\t\targc--;\n> >>> +\t\t}\n> >>> +\t} else {\n> >>> +\t\tstrbuf_addstr(&sb, \"HEAD\");\n> >>> +\t}\n> >>\n> >> The conversion to C would make for a fine excuse to simplify the logic.\n> > \n> > This was kind of like the 'shift' in shell. What equivalent do you\n> > suggest?\n> > \n> \n> I think that's just a general comment after the other comments found\n> just above about simplifying things.\n\nAlright. But I do have to simplify the logic right?\n\n> >>> +\tif (files) {\n> >>> +\t\tif (cached)\n> >>> +\t\t\tdie(_(\"--cached and --files are mutually exclusive\"));\n> >>> +\t\tdiff_cmd = DIFF_FILES;\n> >>> +\t}\n> >>> +\n> >>> +\tinfo.argc = argc;\n> >>> +\tinfo.argv = argv;\n> >>> +\tinfo.prefix = prefix;\n> >>> +\tinfo.cached = !!cached;\n> >>> +\tinfo.for_status = !!for_status;\n> >>> +\tinfo.quiet = quiet;\n> >>> +\tinfo.files = files;\n> >>> +\tinfo.summary_limit = summary_limit;\n> >>> +\n> >>> +\tret = compute_summary_module_list((diff_cmd == DIFF_FILES) ? NULL : sb.buf,\n> >>> +\t\t\t\t\t   &info, diff_cmd);\n> >>\n> >> It would be better to pass the OID as `struct object_id *`, not as string.\n> > \n> > Will do!\n> > \n> >> Other than that, this patch nicely follows previous conversions from Unix\n> >> shell scripts to C.\n> >>\n> >> Well done,\n> >> Johannes\n> > \n> > Thank you! It was a highly detailed review! I am still learning tons of\n> > stuff about Git's code and such a review does help a lot! :)\n> > \n\n> Hope this helps,\n\nIt surely did! Thanks Kaartic :0\n"},{"id":"401485","messageId":"nycvar.QRO.7.76.6.2007120230400.50@tvgsbejvaqbjf.bet","threadId":"53791","inReplyTo":"20200706111559.GA10820@konoha","subject":"Re: [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-12T00:46:47Z","receivedAt":"2020-07-13T12:05:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\nOn Mon, 6 Jul 2020, Shourya Shukla wrote:\n\n> On 06/07 02:46, Kaartic Sivaraam wrote:\n> > On 05-07-2020 23:04, Shourya Shukla wrote:\n> > >> On Fri, 3 Jul 2020, Shourya Shukla wrote:\n> > >>\n> > >>> +static int module_summary(int argc, const char **argv, const char *prefix)\n> > >>> +{\n> > >>> +\tstruct summary_cb info = SUMMARY_CB_INIT;\n> > >>> +\tint cached = 0;\n> > >>> +\tint for_status = 0;\n> > >>> +\tint quiet = 0;\n> > >>> +\tint files = 0;\n> > >>> +\tint summary_limit = -1;\n> > >>> +\tstruct child_process cp_rev = CHILD_PROCESS_INIT;\n> > >>> +\tstruct strbuf sb = STRBUF_INIT;\n> > >>> +\tenum diff_cmd diff_cmd = DIFF_INDEX;\n> > >>> +\tint ret;\n> > >>> +\n> > >>> +\tstruct option module_summary_options[] = {\n> > >>> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output of summarising submodules\")),\n> > >>> +\t\tOPT_BOOL(0, \"cached\", &cached,\n> > >>> +\t\t\t N_(\"Use the commit stored in the index instead of the submodule HEAD\")),\n> > >>> +\t\tOPT_BOOL(0, \"files\", &files,\n> > >>> +\t\t\t N_(\"To compare the commit in the index with that in the submodule HEAD\")),\n> > >>> +\t\tOPT_BOOL(0, \"for-status\", &for_status,\n> > >>> +\t\t\t N_(\"Skip submodules with 'ignore_config' value set to 'all'\")),\n> > >>> +\t\tOPT_INTEGER('n', \"summary-limit\", &summary_limit,\n> > >>> +\t\t\t     N_(\"Limit the summary size\")),\n> > >>> +\t\tOPT_END()\n> > >>> +\t};\n> > >>> +\n> > >>> +\tconst char *const git_submodule_helper_usage[] = {\n> > >>> +\t\tN_(\"git submodule--helper summary [<options>] [commit] [--] [<path>]\"),\n> > >>> +\t\tNULL\n> > >>> +\t};\n> > >>> +\n> > >>> +\targc = parse_options(argc, argv, prefix, module_summary_options,\n> > >>> +\t\t\t     git_submodule_helper_usage, 0);\n> > >>> +\n> > >>> +\tif (!summary_limit)\n> > >>> +\t\treturn 0;\n> > >>> +\n> > >>> +\tcp_rev.git_cmd = 1;\n> > >>> +\targv_array_pushl(&cp_rev.args, \"rev-parse\", \"-q\", \"--verify\",\n> > >>> +\t\t\t argc ? argv[0] : \"HEAD\", NULL);\n> > >>\n> > >> Oy. Why not simply call `get_oid()`? No need to spawn a new process.\n> > >\n> > > Then everytime we need 'rev-parse', we simply call 'get_oid()'? That\n> > > will save us a ton of processes?\n> > >\n> > > But I think we do need to capture the output of 'git rev-parse --verify\n> > > ....' so I think it will backfire to use 'get_oid()' or am I just being\n> > > too dumb and not catching on something?\n> > >\n> >\n> > I'll leave this for others to answer.\n>\n> I will resolve this one after Dscho answers then.\n\nThe `rev-parse` command _would_ essentially call `get_oid()` and print the\nresult (after converting it to hex, which you don't need, because the\ncaller actually would parse the hex string anyway).\n\nYou can verify my claim by following the code:\nhttps://github.com/git/git/blob/v2.27.0/builtin/rev-parse.c#L956-L982 (it\nis slightly harder to follow because the `--verify` option makes sure that\nonly one rev is shown, which means that the `get_oid_with_context()` call\nis separated from the corresponding `show_rev()` call).\n\nSo there really is no need to capture the output of that `rev-parse`\ncommand.\n\nIt is a different story, of course, when capturing the output of Git\ncommands _that are run in a submodule_. Those currently still need to be\nspawned.\n\n> > >>> +\n> > >>> +\tif (!capture_command(&cp_rev, &sb, 0)) {\n> > >>> +\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n> > >>> +\t\tif (argc) {\n> > >>> +\t\t\targv++;\n> > >>> +\t\t\targc--;\n> > >>> +\t\t}\n> > >>> +\t} else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n> > >>> +\t\t/* before the first commit: compare with an empty tree */\n> > >>> +\t\tstruct stat st;\n> > >>> +\t\tstruct object_id oid;\n> > >>> +\t\tif (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,\n> > >>> +\t\t\t\t\t\t  prefix, 3))\n> > >>> +\t\t\tdie(\"Unable to add %s to database\", oid.hash);\n> > >>\n> > >> Umm. The original reads:\n> > >>\n> > >>                 # before the first commit: compare with an empty tree\n> > >>                 head=$(git hash-object -w -t tree --stdin </dev/null)\n> > >>\n> > >> It does not actually read from `stdin`. It reads from `/dev/null`,\n> > >> redirected to the input. And what it _actually_ does is to generate the\n> > >> OID of the empty tree.\n> > >>\n> > >> But we already _have_ the OID of the empty tree! It's\n> > >> `the_hash_algo->empty_tree`.\n> > >\n> > > I did not know this 'the_hash_algo'. I will use it. Thanks! :)\n> > >\n> > >> I hope that this is covered by the test suite. Please check that. The test\n> > >> would succeed with your version, but only because tests are run with\n> > >> `stdin` redirected from `/dev/null` by default.\n> > >\n> > > I guess yes. My work passed because the tests are written this way.\n> > >\n> > >>> +\t\tstrbuf_addstr(&sb, oid_to_hex(&oid));\n> > >>> +\t\tif (argc) {\n> > >>> +\t\t\targv++;\n> > >>> +\t\t\targc--;\n> > >>> +\t\t}\n> > >>> +\t} else {\n> > >>> +\t\tstrbuf_addstr(&sb, \"HEAD\");\n> > >>> +\t}\n> > >>\n> > >> The conversion to C would make for a fine excuse to simplify the logic.\n> > >\n> > > This was kind of like the 'shift' in shell. What equivalent do you\n> > > suggest?\n> > >\n> >\n> > I think that's just a general comment after the other comments found\n> > just above about simplifying things.\n>\n> Alright. But I do have to simplify the logic right?\n\nYou do not _have_ to. But it would make for a good opportunity to do that,\nI think, as the code is really hard to follow.\n\nThe idea here is actually not at all hard to understand, though: use\nthe speficied rev (falling back to `HEAD`) to compare to, unless it is a\nyet-unborn `HEAD` in which case you compare to the empty tree.\n\nIt is very, very similar in spirit to the code in `do_pick_commit()` in\n`sequencer.c`:\nhttps://github.com/git/git/blob/v2.27.0/sequencer.c#L1765-L1774\n\nThe only difference is that you will also have to fall back to using\n`HEAD` if the argument in question turns out not to refer to a revision\nbut is actually the first pathspec.\n\nCiao,\nJohannes\n"},{"id":"401589","messageId":"20200715145336.GA18071@konoha","threadId":"53791","inReplyTo":"nycvar.QRO.7.76.6.2007120230400.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-15T14:53:36Z","receivedAt":"2020-07-15T14:53:47Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hello Johannes!\n\nOn 12/07 02:46, Johannes Schindelin wrote:\n> > > >>> +\tcp_rev.git_cmd = 1;\n> > > >>> +\targv_array_pushl(&cp_rev.args, \"rev-parse\", \"-q\", \"--verify\",\n> > > >>> +\t\t\t argc ? argv[0] : \"HEAD\", NULL);\n> > > >>\n> > > >> Oy. Why not simply call `get_oid()`? No need to spawn a new process.\n> > > >\n> > > > Then everytime we need 'rev-parse', we simply call 'get_oid()'? That\n> > > > will save us a ton of processes?\n> > > >\n> > > > But I think we do need to capture the output of 'git rev-parse --verify\n> > > > ....' so I think it will backfire to use 'get_oid()' or am I just being\n> > > > too dumb and not catching on something?\n> > > >\n> > >\n> > > I'll leave this for others to answer.\n> >\n> > I will resolve this one after Dscho answers then.\n> \n> The `rev-parse` command _would_ essentially call `get_oid()` and print the\n> result (after converting it to hex, which you don't need, because the\n> caller actually would parse the hex string anyway).\n> \n> You can verify my claim by following the code:\n> https://github.com/git/git/blob/v2.27.0/builtin/rev-parse.c#L956-L982 (it\n> is slightly harder to follow because the `--verify` option makes sure that\n> only one rev is shown, which means that the `get_oid_with_context()` call\n> is separated from the corresponding `show_rev()` call).\n> \n> So there really is no need to capture the output of that `rev-parse`\n> command.\n\nYep.\n\n> It is a different story, of course, when capturing the output of Git\n> commands _that are run in a submodule_. Those currently still need to be\n> spawned.\n\nOh alright, since we don't need to capture the output and rather use the\ncommand for a particular functionality it offers to, in our just fetch\nthe OID of head, we use a helper function which the command actually\nuses to make things happen instead of spawning a process. Is this\ncorrect?\n\n> > > >>> +\n> > > >>> +\tif (!capture_command(&cp_rev, &sb, 0)) {\n> > > >>> +\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n> > > >>> +\t\tif (argc) {\n> > > >>> +\t\t\targv++;\n> > > >>> +\t\t\targc--;\n> > > >>> +\t\t}\n> > > >>> +\t} else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n> > > >>> +\t\t/* before the first commit: compare with an empty tree */\n> > > >>> +\t\tstruct stat st;\n> > > >>> +\t\tstruct object_id oid;\n> > > >>> +\t\tif (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,\n> > > >>> +\t\t\t\t\t\t  prefix, 3))\n> > > >>> +\t\t\tdie(\"Unable to add %s to database\", oid.hash);\n> > > >>\n> > > >> Umm. The original reads:\n> > > >>\n> > > >>                 # before the first commit: compare with an empty tree\n> > > >>                 head=$(git hash-object -w -t tree --stdin </dev/null)\n> > > >>\n> > > >> It does not actually read from `stdin`. It reads from `/dev/null`,\n> > > >> redirected to the input. And what it _actually_ does is to generate the\n> > > >> OID of the empty tree.\n> > > >>\n> > > >> But we already _have_ the OID of the empty tree! It's\n> > > >> `the_hash_algo->empty_tree`.\n> > > >\n> > > > I did not know this 'the_hash_algo'. I will use it. Thanks! :)\n> > > >\n> > > >> I hope that this is covered by the test suite. Please check that. The test\n> > > >> would succeed with your version, but only because tests are run with\n> > > >> `stdin` redirected from `/dev/null` by default.\n> > > >\n> > > > I guess yes. My work passed because the tests are written this way.\n> > > >\n> > > >>> +\t\tstrbuf_addstr(&sb, oid_to_hex(&oid));\n> > > >>> +\t\tif (argc) {\n> > > >>> +\t\t\targv++;\n> > > >>> +\t\t\targc--;\n> > > >>> +\t\t}\n> > > >>> +\t} else {\n> > > >>> +\t\tstrbuf_addstr(&sb, \"HEAD\");\n> > > >>> +\t}\n> > > >>\n> > > >> The conversion to C would make for a fine excuse to simplify the logic.\n> > > >\n> > > > This was kind of like the 'shift' in shell. What equivalent do you\n> > > > suggest?\n> > > >\n> > >\n> > > I think that's just a general comment after the other comments found\n> > > just above about simplifying things.\n> >\n> > Alright. But I do have to simplify the logic right?\n> \n> You do not _have_ to. But it would make for a good opportunity to do that,\n> I think, as the code is really hard to follow.\n> \n> The idea here is actually not at all hard to understand, though: use\n> the speficied rev (falling back to `HEAD`) to compare to, unless it is a\n> yet-unborn `HEAD` in which case you compare to the empty tree.\n> \n> It is very, very similar in spirit to the code in `do_pick_commit()` in\n> `sequencer.c`:\n> https://github.com/git/git/blob/v2.27.0/sequencer.c#L1765-L1774\n> \n> The only difference is that you will also have to fall back to using\n> `HEAD` if the argument in question turns out not to refer to a revision\n> but is actually the first pathspec.\n\nAlright. So do you suggest a code segment like this?\n\n--------8<---------------------\n\nmodule_summary() {\nstruct object_id *head_oid = NULL;\n.....\n.....\nif (get_oid(argc ? argv[0] : \"HEAD\", head_oid)) {\n\t\t\tif (argc) {\n\t\t\targv++;\n\t\t\targc--;\n\t\t}\n\t} else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n\t\t/* before the first commit: compare with an empty tree */\n\t\toidcpy(head_oid, the_hash_algo->empty_tree);\n\t\tif (argc) {\n\t\t\targv++;\n\t\t\targc--;\n\t\t}\n\t} else {\n\t\tget_oid(\"HEAD\", head_oid);\n\t}\n.....\n.....\n}\n\n*Passing head_oid as a pointer into the fucntion below*\n\ncompute_summary_module_list(..., struct object_id *head_oid,...) {\n\t.....\n    .....\n    if (head_oid) {\n\t\targv_array_push(&diff_args, oid_to_hex(head_oid));\n\t}\n    .....\n    .....\n}\n\n----------->8-------------\n\nWhen I try this, I get a:\n\n    BUG: diff-lib.c:526: run_diff_index must be passed exactly one tree\n\nAnd this occurs when we are inside the first if-statement. I do not\nunderstand how we are passing multiple trees or making it seem as if\nthere are multiple trees? Also, even when we do not enter the first if,\nwe still get a segmentation fault. Why so?\n\nIs this logic correct? I took some inspiration from the links you sent\nabove as well as some advice from Christian and Kaartic on this.\n"},{"id":"401600","messageId":"20200715184153.GA52607@konoha","threadId":"53791","inReplyTo":"20200715145336.GA18071@konoha","subject":"Re: [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-07-15T18:41:53Z","receivedAt":"2020-07-15T18:42:03Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Ignore the above mail I sent, I am a big dimwit. I have tried to do this\nand this works\n\n---------------8<--------------\nmodule_summary() {\nstruct object_id head;\n....\n....\nif (!get_oid(argc ? argv[0] : \"HEAD\", &head_oid)) {\n\t\tif (argc) {\n\t\t\targv++;\n\t\t\targc--;\n\t\t}\n\t} else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n\t\t/* before the first commit: compare with an empty tree */\n\t\toidcpy(&head_oid, the_hash_algo->empty_tree);\n\t\tif (argc) {\n\t\t\targv++;\n\t\t\targc--;\n\t\t}\n\t} else {\n\t\tget_oid(\"HEAD\", &head_oid);\n\t}\n....\n....\nret = compute_summary_module_list((diff_cmd == DIFF_FILES) ? NULL : &head_oid,\n\t\t\t\t\t   &info, diff_cmd);\n\treturn ret;\n}\n\ncompute_summary_module_list() {\n....\n....\nif (head_oid)\n\t\targv_array_push(&diff_args, oid_to_hex(head_oid));\n....\n....\n}\n--------------->8--------------\n\nObviousy I was making the grave mistake of declaring the aforementioned\nstruct as 'struct object_id *' which caused all these weird errors.\n\nAll tests pass now.\n\nBTW in your review of my patch,\nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2007031712160.50@tvgsbejvaqbjf.bet/\nyou said this:\n--------------->8--------------\n> +\t\t\t */\n> +\t\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> +\t\t\tstruct strbuf sb_rev_parse = STRBUF_INIT;\n> +\n> +\t\t\tcp_rev_parse.git_cmd = 1;\n> +\t\t\tcp_rev_parse.no_stderr = 1;\n> +\t\t\tcp_rev_parse.dir = p->sm_path;\n> +\t\t\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n> +\n> +\t\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n> +\t\t\t\t\t \"HEAD\", NULL);\n> +\t\t\tif (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n> +\t\t\t\tstrbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n> +\t\t\t\tget_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n> +\t\t\t}\n> +\t\t\tstrbuf_release(&sb_rev_parse);\n> +\t\t} else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n> +\t\t\tstruct child_process cp_hash_object = CHILD_PROCESS_INIT;\n> +\t\t\tstruct strbuf sb_hash_object = STRBUF_INIT;\n> +\n> +\t\t\tcp_hash_object.git_cmd = 1;\n> +\t\t\targv_array_pushl(&cp_hash_object.args, \"hash-object\",\n> +\t\t\t\t\t p->sm_path, NULL);\n> +\t\t\tif (!capture_command(&cp_hash_object,\n> +\t\t\t\t\t     &sb_hash_object, 0)) {\n> +\t\t\t\tstrbuf_strip_suffix(&sb_hash_object, \"\\n\");\n> +\t\t\t\tget_oid_hex(sb_hash_object.buf, &p->oid_dst);\n> +\t\t\t}\n> +\t\t\tstrbuf_release(&sb_hash_object);\n\nIt would probably be shorter, less error-prone, and quicker to use\n`index_fd()` directly.\n--------------->8--------------\n\nWhat exactly did you mean here and where should the index_fd() be used?\n\nRegards,\nShourya Shukla\n"},{"id":"403260","messageId":"nycvar.QRO.7.76.6.2008101519090.50@tvgsbejvaqbjf.bet","threadId":"53791","inReplyTo":"20200715184153.GA52607@konoha","subject":"Re: [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-10T13:24:03Z","receivedAt":"2020-08-10T13:24:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\nOn Thu, 16 Jul 2020, Shourya Shukla wrote:\n\n> BTW in your review of my patch,\n> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2007031712160.50@tvgsbejvaqbjf.bet/\n> you said this:\n> --------------->8--------------\n> > +\t\t\t */\n> > +\t\t\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n> > +\t\t\tstruct strbuf sb_rev_parse = STRBUF_INIT;\n> > +\n> > +\t\t\tcp_rev_parse.git_cmd = 1;\n> > +\t\t\tcp_rev_parse.no_stderr = 1;\n> > +\t\t\tcp_rev_parse.dir = p->sm_path;\n> > +\t\t\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n> > +\n> > +\t\t\targv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n> > +\t\t\t\t\t \"HEAD\", NULL);\n> > +\t\t\tif (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n> > +\t\t\t\tstrbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n> > +\t\t\t\tget_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n> > +\t\t\t}\n> > +\t\t\tstrbuf_release(&sb_rev_parse);\n> > +\t\t} else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n> > +\t\t\tstruct child_process cp_hash_object = CHILD_PROCESS_INIT;\n> > +\t\t\tstruct strbuf sb_hash_object = STRBUF_INIT;\n> > +\n> > +\t\t\tcp_hash_object.git_cmd = 1;\n> > +\t\t\targv_array_pushl(&cp_hash_object.args, \"hash-object\",\n> > +\t\t\t\t\t p->sm_path, NULL);\n> > +\t\t\tif (!capture_command(&cp_hash_object,\n> > +\t\t\t\t\t     &sb_hash_object, 0)) {\n> > +\t\t\t\tstrbuf_strip_suffix(&sb_hash_object, \"\\n\");\n> > +\t\t\t\tget_oid_hex(sb_hash_object.buf, &p->oid_dst);\n> > +\t\t\t}\n> > +\t\t\tstrbuf_release(&sb_hash_object);\n>\n> It would probably be shorter, less error-prone, and quicker to use\n> `index_fd()` directly.\n> --------------->8--------------\n>\n> What exactly did you mean here and where should the index_fd() be used?\n\nWell, `index_fd()` is the function that `git hash-object` uses to\ncalculate the hash of a given file (in this case, specified via the path\n`p->sm_path`).\n\nYou could open an fd to that file directly, use `index_fd()` to calculate\nthe hash, and thereby avoid spawning another (totally unnecessary)\nprocess.\n\nCiao,\nDscho\n"}]}