{"thread":{"id":"53998","subject":"[PATCH v2 5/5] submodule: port submodule subcommand 'summary' from shell to C","startedAt":"2020-08-06T16:44:11Z","lastAt":"2020-08-24T17:54:17Z","messageCount":32,"participants":["Shourya Shukla","Junio C Hamano","Kaartic Sivaraam","Christian Couder","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"403037","messageId":"20200806164102.6707-6-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200806164102.6707-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 5/5] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-06T16:41:02Z","receivedAt":"2020-08-06T16:44:11Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"From: Prathamesh Chavan \n\nConvert submodule subcommand 'summary' to a builtin and call it via\n'git-submodule.sh'.\n\nThe shell version had to call $diff_cmd twice, once to find the modified\nmodules cared by the user and then again, with that list of modules\nto do various operations for computing the summary of those modules.\nOn the other hand, the C version does not need a second call to\n$diff_cmd since it reuses the module list from the first call to do the\naforementioned tasks.\n\nIn the C version, we use the combination of setting a child process'\nworking directory to the submodule path and then calling\n'prepare_submodule_repo_env()' which also sets the 'GIT_DIR' to '.git',\nso that we can be certain that those spawned processes will not access\nthe superproject's ODB by mistake.\n\nA behavioural difference between the C and the shell version is that the\nshell version outputs two line feeds after the 'git log' output when run\noutside of the tests while the C version outputs one line feed in any\ncase. The reason for this is that the shell version calls log with\n'--pretty=format:' whose output is followed by two echo\ncalls; 'format' does not have \"terminator\" semantics like its 'tformat'\ncounterpart. So, the log output is terminated by a newline only when\ninvoked by the user and not when invoked from the scripts. This results\nin the one & two line feed differences in the shell version.\nOn the other hand, the C version calls log with '--pretty='\nwhich is equivalent to '--pretty:tformat:' which is then\nfollowed by a 'printf(\"\\n\")'. Due to its \"terminator\" semantics the\nlog output is always terminated by newline and hence one line feed in\nany case.\n\nAlso, when we try to pass an option-like argument after a non-option\nargument, for instance:\n\n git submodule summary HEAD --foo-bar\n\n (or)\n\n git submodule summary HEAD --cached\n\nThat argument would be treated like a path to the submodule for which\nthe user is requesting a summary. So, the option ends up having no\neffect. Though, passing '--quiet' is an exception to this:\n\n git submodule summary HEAD --quiet\n\nWhile 'summary' doesn't support '--quiet', we don't get an output for\nthe above command as '--quiet' is treated as a path which means we get\nan output only if a submodule whose path is '--quiet' exists.\n\nThe error message in case of computing a summary for non-existent\nsubmodules in the C version is different from that of the shell version.\nSince the new error message is not marked for translation, change the\n'test_i18ngrep' in t7421.4 to 'grep'.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nMentored-by: Kaartic Sivaraam \nHelped-by: Johannes Schindelin \nSigned-off-by: Prathamesh Chavan \nSigned-off-by: Shourya Shukla \n---\n builtin/submodule--helper.c | 436 +++++++++++++++++++++++++++++++\n git-submodule.sh | 186 +------------\n t/t7421-submodule-summary-add.sh | 2 +-\n 3 files changed, 438 insertions(+), 186 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 3641718d0a..43ed2f645f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -927,6 +927,441 @@ 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 files: 1;\n+\tint summary_limit;\n+};\n+#define SUMMARY_CB_INIT { 0, NULL, NULL, 0, 0, 0, 0 }\n+\n+enum diff_cmd {\n+\tDIFF_INDEX,\n+\tDIFF_FILES\n+};\n+\n+static char* verify_submodule_committish(const char *sm_path,\n+\t\t\t\t\t const char *committish)\n+{\n+\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n+\tstruct strbuf result = STRBUF_INIT;\n+\n+\tcp_rev_parse.git_cmd = 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 \"--short\", NULL);\n+\targv_array_pushf(&cp_rev_parse.args, \"%s^0\", committish);\n+\targv_array_push(&cp_rev_parse.args, \"--\");\n+\n+\tif (capture_command(&cp_rev_parse, &result, 0))\n+\t\treturn NULL;\n+\n+\tstrbuf_trim_trailing_newline(&result);\n+\treturn strbuf_detach(&result, NULL);\n+}\n+\n+static void print_submodule_summary(struct summary_cb *info, char* errmsg,\n+\t\t\t\t int total_commits, const char *displaypath,\n+\t\t\t\t const char *src_abbrev, const char *dst_abbrev,\n+\t\t\t\t int missing_src, int missing_dst,\n+\t\t\t\t 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, src_abbrev, dst_abbrev);\n+\t\telse\n+\t\t\tprintf(_(\"* %s %s(submodule)->%s(blob)\"),\n+\t\t\t\t displaypath, src_abbrev, dst_abbrev);\n+\t} else {\n+\t\tprintf(\"* %s %s...%s\",\n+\t\t\tdisplaypath, src_abbrev, dst_abbrev);\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\tprintf(_(\"%s\"), errmsg);\n+\t} else if (total_commits > 0) {\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 src_abbrev,\n+\t\t\t\t\t dst_abbrev);\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\", dst_abbrev, NULL);\n+\t\t} else {\n+\t\t\targv_array_pushl(&cp_log.args, \"--pretty= < %s\",\n+\t\t\t\t\t \"-1\", src_abbrev, 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+\tchar *displaypath, *src_abbrev, *dst_abbrev;\n+\tint missing_src = 0, missing_dst = 0;\n+\tchar *errmsg = NULL;\n+\tint total_commits = -1;\n+\n+\tif (!info->cached && oideq(&p->oid_dst, &null_oid)) {\n+\t\tif (S_ISGITLINK(p->mod_dst)) {\n+\t\t\tstruct ref_store *refs = get_submodule_ref_store(p->sm_path);\n+\t\t\tif (refs)\n+\t\t\t\trefs_head_ref(refs, handle_submodule_head_ref, &p->oid_dst);\n+\t\t} else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n+\t\t\tstruct stat st;\n+\t\t\tint fd = open(p->sm_path, O_RDONLY);\n+\n+\t\t\tif (fd < 0 || fstat(fd, &st) < 0 ||\n+\t\t\t index_fd(&the_index, &p->oid_dst, fd, &st, OBJ_BLOB,\n+\t\t\t\t p->sm_path, 0))\n+\t\t\t\terror(_(\"couldn't hash object from '%s'\"), p->sm_path);\n+\t\t} else {\n+\t\t\t/* for a submodule removal (mode:0000000), don't warn */\n+\t\t\tif (p->mod_dst)\n+\t\t\t\twarning(_(\"unexpected mode %d\\n\"), p->mod_dst);\n+\t\t}\n+\t}\n+\n+\tif (S_ISGITLINK(p->mod_src)) {\n+\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n+\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n+\t\tif (!src_abbrev) {\n+\t\t\tmissing_src = 1;\n+\t\t\t/*\n+\t\t\t * As `rev-parse` failed, we fallback to getting\n+\t\t\t * the abbreviated hash using oid_src. We do\n+\t\t\t * this as we might still need the abbreviated\n+\t\t\t * hash in cases like a submodule type change, etc.\n+\t\t\t */\n+\t\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n+\t\t}\n+\t} else {\n+\t\t/*\n+\t\t * The source does not point to a submodule.\n+\t\t * So, we fallback to getting the abbreviation using\n+\t\t * oid_src as we might still need the abbreviated\n+\t\t * hash in cases like submodule add, etc.\n+\t\t */\n+\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n+\t}\n+\n+\tif (S_ISGITLINK(p->mod_dst)) {\n+\t\tdst_abbrev = verify_submodule_committish(p->sm_path,\n+\t\t\t\t\t\t\t oid_to_hex(&p->oid_dst));\n+\t\tif (!dst_abbrev) {\n+\t\t\tmissing_dst = 1;\n+\t\t\t/*\n+\t\t\t * As `rev-parse` failed, we fallback to getting\n+\t\t\t * the abbreviated hash using oid_dst. We do\n+\t\t\t * this as we might still need the abbreviated\n+\t\t\t * hash in cases like a submodule type change, etc.\n+\t\t\t */\n+\t\t\tdst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n+\t\t}\n+\t} else {\n+\t\t/*\n+\t\t * The destination does not point to a submodule.\n+\t\t * So, we fallback to getting the abbreviation using\n+\t\t * oid_dst as we might still need the abbreviated\n+\t\t * hash in cases like a submodule removal, etc.\n+\t\t */\n+\t\tdst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n+\t}\n+\n+\tdisplaypath = get_submodule_displaypath(p->sm_path, info->prefix);\n+\n+\tif (!missing_src && !missing_dst) {\n+\t\tstruct child_process cp_rev_list = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf sb_rev_list = STRBUF_INIT;\n+\n+\t\targv_array_pushl(&cp_rev_list.args, \"rev-list\",\n+\t\t\t\t \"--first-parent\", \"--count\", NULL);\n+\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n+\t\t\targv_array_pushf(&cp_rev_list.args, \"%s...%s\",\n+\t\t\t\t\t src_abbrev,\n+\t\t\t\t\t dst_abbrev);\n+\t\telse\n+\t\t\targv_array_push(&cp_rev_list.args,\n+\t\t\t\t\tS_ISGITLINK(p->mod_src) ?\n+\t\t\t\t\tsrc_abbrev :\n+\t\t\t\t\tdst_abbrev);\n+\t\targv_array_push(&cp_rev_list.args, \"--\");\n+\n+\t\tcp_rev_list.git_cmd = 1;\n+\t\tcp_rev_list.dir = p->sm_path;\n+\t\tprepare_submodule_repo_env(&cp_rev_list.env_array);\n+\n+\t\tif (!capture_command(&cp_rev_list, &sb_rev_list, 0))\n+\t\t\ttotal_commits = atoi(sb_rev_list.buf);\n+\n+\t\tstrbuf_release(&sb_rev_list);\n+\t} else {\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_dst)) {\n+\t\t\tstruct strbuf errmsg_str = STRBUF_INIT;\n+\t\t\tif (missing_src && missing_dst) {\n+\t\t\t\tstrbuf_addf(&errmsg_str, \" Warn: %s doesn't contain commits %s and %s\\n\",\n+\t\t\t\t\t displaypath, oid_to_hex(&p->oid_src),\n+\t\t\t\t\t oid_to_hex(&p->oid_dst));\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addf(&errmsg_str, \" Warn: %s doesn't contain commit %s\\n\",\n+\t\t\t\t\t displaypath, missing_src ?\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\t}\n+\t\t\terrmsg = strbuf_detach(&errmsg_str, NULL);\n+\t\t}\n+\t}\n+\n+\tprint_submodule_summary(info, errmsg, total_commits,\n+\t\t\t\tdisplaypath, src_abbrev,\n+\t\t\t\tdst_abbrev, missing_src,\n+\t\t\t\tmissing_dst, p);\n+\n+\tfree(displaypath);\n+\tfree(src_abbrev);\n+\tfree(dst_abbrev);\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\tconst struct submodule *sub;\n+\t\tstruct module_cb *p = list->entries[i];\n+\t\tstruct strbuf sm_gitdir = STRBUF_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 && p->status != 'A' &&\n+\t\t (sub = submodule_from_path(the_repository,\n+\t\t\t\t\t &null_oid, p->sm_path))) {\n+\t\t\tchar *config_key = NULL;\n+\t\t\tconst char *value;\n+\t\t\tint ignore_all = 0;\n+\n+\t\t\tconfig_key = xstrfmt(\"submodule.%s.ignore\",\n+\t\t\t\t\t sub->name);\n+\t\t\tif (!git_config_get_string_const(config_key, &value))\n+\t\t\t\tignore_all = !strcmp(value, \"all\");\n+\t\t\telse if (sub->ignore)\n+\t\t\t\tignore_all = !strcmp(sub->ignore, \"all\");\n+\n+\t\t\tfree(config_key);\n+\t\t\tif (ignore_all)\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* Also show added or modified modules which are checked out */\n+\t\tstrbuf_addstr(&sm_gitdir, p->sm_path);\n+\t\tif (is_nonbare_repository_dir(&sm_gitdir))\n+\t\t\tgenerate_submodule_summary(info, p);\n+\t\tstrbuf_release(&sm_gitdir);\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(struct object_id *head_oid,\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_oid)\n+\t\targv_array_push(&diff_args, oid_to_hex(head_oid));\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 files = 0;\n+\tint summary_limit = -1;\n+\tenum diff_cmd diff_cmd = DIFF_INDEX;\n+\tstruct object_id head_oid;\n+\tint ret;\n+\n+\tstruct option module_summary_options[] = {\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 [] [commit] [--] []\"),\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+\tif (!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\tif (get_oid(\"HEAD\", &head_oid))\n+\t\t\tdie(_(\"could not fetch a revision for 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.files = !!files;\n+\tinfo.for_status = !!for_status;\n+\tinfo.summary_limit = summary_limit;\n+\n+\tret = compute_summary_module_list((diff_cmd == DIFF_INDEX) ? &head_oid : NULL,\n+\t\t\t\t\t &info, diff_cmd);\n+\treturn ret;\n+}\n+\n struct sync_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n@@ -2341,6 +2776,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 dda3fee167..236a08f27d 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 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 ${prefix:+--prefix \"$prefix\"} ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- \"$@\"\n }\n #\n # List all submodules, prefixed with:\ndiff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh\nindex 829fe26d6d..59a9b00467 100755\n--- a/t/t7421-submodule-summary-add.sh\n+++ b/t/t7421-submodule-summary-add.sh\n@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n \tgit commit -m \"change submodule path\" &&\n \trev=$(git -C sm rev-parse --short HEAD^) &&\n \tgit submodule summary HEAD^^ -- my-subm >actual 2>err &&\n-\ttest_i18ngrep \"fatal:.*my-subm\" err &&\n+\tgrep \"fatal:.*my-subm\" err &&\n \tcat >expected <<-EOF &&\n \t* my-subm ${rev}...0000000:\n \n-- \n2.28.0\n\n"},{"id":"403038","messageId":"20200806164102.6707-3-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200806164102.6707-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 2/5] submodule: remove extra line feeds between callback struct and macro","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-06T16:40:59Z","receivedAt":"2020-08-06T16:44:18Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Many `submodule--helper` subcommands follow the convention that a struct\ndefines their callback data, and the declaration of that struct is\nfollowed immediately by a macro to use in static initializers, without\nany separating empty line.\n\nLet's align the `init`, `status` and `sync` subcommands with that convention.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nHelped-by: Johannes Schindelin \nHelped-by: Philip Oakley \nSigned-off-by: Shourya Shukla \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 a1c75607c7..3641718d0a 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.28.0\n\n"},{"id":"403041","messageId":"20200806164102.6707-4-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200806164102.6707-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 3/5] submodule: rename helper functions to avoid ambiguity","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-06T16:41:00Z","receivedAt":"2020-08-06T16:48:50Z","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 \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \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.28.0\n\n"},{"id":"403042","messageId":"20200806164102.6707-5-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200806164102.6707-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 4/5] t7421: introduce a test script for verifying 'summary' output","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-06T16:41:01Z","receivedAt":"2020-08-06T16:49:14Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"'t7401-submodule-summary.sh' uses 'git add' to add submodules. Therefore,\nsome commands such as 'git submodule init' and 'git submodule deinit'\ndo not work as expected.\n\nSo, introduce a test script for verifying the 'summary' output for\nsubmodules added using 'git submodule add'.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n t/t7421-submodule-summary-add.sh | 69 ++++++++++++++++++++++++++++++++\n 1 file changed, 69 insertions(+)\n create mode 100755 t/t7421-submodule-summary-add.sh\n\ndiff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh\nnew file mode 100755\nindex 0000000000..829fe26d6d\n--- /dev/null\n+++ b/t/t7421-submodule-summary-add.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+#\n+# Copyright (C) 2020 Shourya Shukla\n+#\n+\n+test_description='Summary support for submodules, adding them using git submodule add\n+\n+This test script tries to verify the sanity of summary subcommand of git submodule\n+while making sure to add submodules using `git submodule add` instead of\n+`git add` as done in t7401.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'summary test environment setup' '\n+\tgit init sm &&\n+\ttest_commit -C sm \"add file\" file file-content file-tag &&\n+\n+\tgit submodule add ./sm my-subm &&\n+\ttest_tick &&\n+\tgit commit -m \"add submodule\"\n+'\n+\n+test_expect_success 'submodule summary output for initialized submodule' '\n+\ttest_commit -C sm \"add file2\" file2 file2-content file2-tag &&\n+\tgit submodule update --remote &&\n+\ttest_tick &&\n+\tgit commit -m \"update submodule\" my-subm &&\n+\tgit submodule summary HEAD^ >actual &&\n+\trev1=$(git -C sm rev-parse --short HEAD^) &&\n+\trev2=$(git -C sm rev-parse --short HEAD) &&\n+\tcat >expected <<-EOF &&\n+\t* my-subm ${rev1}...${rev2} (1):\n+\t > add file2\n+\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'submodule summary output for deinitialized submodule' '\n+\tgit submodule deinit my-subm &&\n+\tgit submodule summary HEAD^ >actual &&\n+\ttest_must_be_empty actual &&\n+\tgit submodule update --init my-subm &&\n+\tgit submodule summary HEAD^ >actual &&\n+\trev1=$(git -C sm rev-parse --short HEAD^) &&\n+\trev2=$(git -C sm rev-parse --short HEAD) &&\n+\tcat >expected <<-EOF &&\n+\t* my-subm ${rev1}...${rev2} (1):\n+\t > add file2\n+\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'submodule summary output for submodules with changed paths' '\n+\tgit mv my-subm subm &&\n+\tgit commit -m \"change submodule path\" &&\n+\trev=$(git -C sm rev-parse --short HEAD^) &&\n+\tgit submodule summary HEAD^^ -- my-subm >actual 2>err &&\n+\ttest_i18ngrep \"fatal:.*my-subm\" err &&\n+\tcat >expected <<-EOF &&\n+\t* my-subm ${rev}...0000000:\n+\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n2.28.0\n\n"},{"id":"403043","messageId":"20200806164102.6707-2-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200806164102.6707-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 1/5] submodule: expose the '--for-status' option of summary","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-06T16:40:58Z","receivedAt":"2020-08-06T16:50:09Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"The 'for-status' option is used to compute the summary of submodule(s)\nin a superproject by skipping the ignored submdules i.e., those with\n'submodule..ignore' set to 'all' in the '.gitmodules' or\n'.git/config', with the latter taking precedence over the former.\n\nThe option was introduced in d0f64dd44d (git-submodule summary:\n--for-status option, 2008-04-12), refined in 3ba7407b8b (submodule\nsummary: ignore --for-status option, 2013-09-06) and finally perfected\nin 927b26f87a (submodule: don't print status output with ignore=all,\n2013-09-01). But, it was not mentioned in the 'git submodule'\nDocumentation.\n\nExpose the '--for-status' option accepted by the command 'git submodule\nsummary'.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n Documentation/git-submodule.txt | 8 +++++++-\n contrib/completion/git-completion.bash | 2 +-\n git-submodule.sh | 2 +-\n 3 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 7e5f995f77..d944e4c817 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -190,7 +190,7 @@ set-url [--] ::\n \tautomatically synchronize the submodule's new remote URL\n \tconfiguration.\n \n-summary [--cached|--files] [(-n|--summary-limit) ] [commit] [--] [...]::\n+summary [--cached|--files] [--for-status] [(-n|--summary-limit) ] [commit] [--] [...]::\n \tShow commit summary between the given commit (defaults to HEAD) and\n \tworking tree/index. For a submodule in question, a series of commits\n \tin the submodule between the given super project commit and the\n@@ -309,6 +309,12 @@ OPTIONS\n \tcompares the commit in the index with that in the submodule HEAD\n \twhen this option is used.\n \n+--for-status::\n+\tThis option is only valid for the summary command. This command\n+\tskips the submodules with `submodule..ignore` set to `all`\n+\tin the `.gitmodules` or `.git/config`. The configuration in\n+\t`.git/config` overrides the configuration in `.gitmodules`.\n+\n -n::\n --summary-limit::\n \tThis option is only valid for the summary command.\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 0fdb5da83b..2b7b033c17 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3059,7 +3059,7 @@ _git_submodule ()\n \t\t__gitcomp \"--default --branch\"\n \t\t;;\n \tsummary,--*)\n-\t\t__gitcomp \"--cached --files --summary-limit\"\n+\t\t__gitcomp \"--cached --files --for-status --summary-limit\"\n \t\t;;\n \tforeach,--*|sync,--*)\n \t\t__gitcomp \"--recursive\"\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 43eb6051d2..dda3fee167 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -13,7 +13,7 @@ USAGE=\"[--quiet] [--cached]\n or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference ] [--recursive] [--[no-]single-branch] [--] [...]\n or: $dashless [--quiet] set-branch (--default|--branch ) [--] \n or: $dashless [--quiet] set-url [--] \n- or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] [commit] [--] [...]\n+ or: $dashless [--quiet] summary [--cached|--files] [--for-status] [--summary-limit ] [commit] [--] [...]\n or: $dashless [--quiet] foreach [--recursive] \n or: $dashless [--quiet] sync [--recursive] [--] [...]\n or: $dashless [--quiet] absorbgitdirs [--] [...]\"\n-- \n2.28.0\n\n"},{"id":"403049","messageId":"20200806164102.6707-1-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":null,"subject":"[GSoC][PATCH v2 0/5] submodule: port subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-06T16:40:57Z","receivedAt":"2020-08-06T16:56:51Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Greetings,\n\nAfter a long list of changes proposed by Johannes in the v1, here I\npresent the v2 of the patch series porting 'summary' from shell to C.\nApart from requested changes in 'submodule--helper.c' and dropping the\ncommit changing the scope of the 'count_lines()' function, there are\ntwo new commits: one documents the 'for-status' option and one\nintroduces the test 't7421-submodule-summary-add.sh'. Link to the v1:\nhttps://lore.kernel.org/git/20200702192409.21865-1-shouryashukla.oo@gmail.com/\n\nThe reasons for the introduction of the test are covered in a previous\npatch series delivered by me last night regarding t7401:\nhttps://lore.kernel.org/git/20200805174921.16000-1-shouryashukla.oo@gmail.com/\n\nBut I will touch upon it again in this patch. The test script\n't7401-submodule-summary.sh' adds submodules using 'git add' instead of\n'git submodule add'. Therefore, some commands such as\n'git submodule init' and 'git submodule deinit' do not give the desired\neffects since there is no .gitmodules file. Note that, 'summary'\nworks without issues as it does not depend on the existence of\n.gitmodules.\n\nI will tag along the 'range-diff' between this v2 and the v1 at the end\nof the cover letter for ease of review.\n\nThe changelog:\n\n- Introduce commit [PATCH 1/5] 4f0e4f6ace (submodule: document the\n '--for-status' option, 2020-08-02).\n\n- Improve commit message of [PATCH 2/5] 6e42cc99a7 (submodule: remove\n extra line feeds between callback struct and macro, 2020-06-25).\n\n- Drop commit (Previously [PATCH 3/4]) 745f2bd92e (diff: change scope\n of the function count_lines(), 2017-07-11).\n\n- Introduce commit [PATCH 4/5] ef4cf4be0f (t7421: introduce a test\n script for verifying 'summary' output, 2020-07-23).\n\n- Improve the commit [PATCH 5/5] 331cc1a487 (submodule: port submodule\n subcommand 'summary' from shell to C, 2017-07-11) which includes:\n -> Improve the commit message, focusing more on the specialities of\n the commit rather than describing the flow of the subcommand.\n\n -> Rename the function 'verify_submodule_object_name()' to\n 'verify_submodule_committish()' as the latter is more accurate.\n While at it, rename its argument 'const char *sha1' to 'const\n char *committish' since Git is shifting to SHA256 (covered in the\n patch series by Brian M. Carlson) and we look for a committish in\n the 'rev-parse' called in the function. Also, pass a '--short' in\n the call to 'rev-parse' as this option does the job of '--verify'\n and also shortens the hash the way we desire.\n\n -> Reuse the hash obtained from the function\n 'verify_submodule_committish()' instead of incorrectly calling\n 'find_unique_abbrev()' multiple times. Also eliminate the usage\n of 'oid_to_hex()' and instead reuse the aforementioned hash.\n\n -> Eliminate the variable 'is_sm_git_dir' and therefore, simplify\n the checks involving it since the GIT_DIR is already set to\n '.git/' and therefore we need to check it again.\n\n -> Eliminate the NEEDSWORK in 'generate_submodule_summary()' and\n instead call the function 'refs_head_ref()' (successor of\n 'head_ref_submodule()').\n\n -> Use 'index_fd()' instead of spawning a process to run 'git\n hash-object' to find the 'oid_dst' in case of a regular file or\n a symbolic link.\n\n -> In case of an unexpected file mode, give a warning() and continue\n instead of die()ing.\n\n -> Simplify the 'range' computation by cuddling up a couple of lines\n and thus removing the 'range' variable since it will be useless.\n The 'range' is the range of commits whose summary will be\n computed.\n\n -> Eliminate the usage of 'count_lines()' since 'git rev-list\n --count' does the job anyway.\n\n -> Compute the error messages in 'generate_submodule_summary()'\n itself instead of 'print_submodule_summary()' and therefore, pass\n the err_msg into the latter as a 'char *'.\n\n -> Simplify the if-statement in case of 'for-status' by cuddling up\n various lines and thus removing one whole level of indentation.\n\n -> Remove the OPT__QUIET from the option struct since 'summary' does\n not support 'quiet'.\n\n -> Decapitalise the first letter of the option descriptions since\n the guidelines mention so.\n\n -> Instead of spawning a child process, use\n 'is_nonbare_repository_dir()' to check if the submodules are\n checked out or not.\n\n -> Call 'get_oid()' to find the hash of 'HEAD' instead of spawning a\n process to call 'rev-parse' in 'module_summary()'. While at it,\n simplify the set of if-else statements which fetch the revision\n of the superproject to calculate the summary of.\n\n -> Pass the revision computed above to\n 'compute_summary_module_list()' as 'struct object_id *' instead of\n passing it as a string.\n\nThere is a behavioural difference between the shell and the C version.\nThe shell version printed two newlines at the end of the output\nwhen executed outside the tests but one newline when executed in the\ntests. On the other hand, the C version printed only one newline\nirrespective of where it was executed. This is due to the difference\nin the way they call 'git log'. The shell version does a\n'--pretty=format:' whereas the C version does a '--pretty='.\nThe latter is indirectly a '--pretty=tformat:' which causes the\ndifference between the two.\n\nAlso, when we try to pass an option-like argument after a non-option\nargument:\n\n\tgit submodule summary HEAD --foo-bar\n\nThat argument would be treated like a path to the submodule for which\nthe user is requesting a summary. So, the option ends up having no\neffect. Though, passing `--quiet` is an exception to this:\n\n\tgit submodule summary HEAD --quiet\n\nWhile 'summary' doesn't support '--quiet', we don't get an output for\nthe above command as '--quiet' is treated as a path which means we get\nan output only if a submodule whose path is '--quiet' exists.\n\nAlso, I want to ask a couple of things:\n\n\t1.Whether we can suppress the error message that we get when\n\t trying to find the summary of non-existent submodules?\n\t For example:\n\n\t fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n\t * my-subm 35b40f1...0000000:\n\n\t Will it be OK to suppress the above error message?\n\n\t2.Is it fine to document and expose the 'for-status' option in\n\t 'git-submodule.txt'?\n\nFeedback and reviews are appreciated.\n\nRegards,\nShourya Shukla\n-----\n\n-: ---------- > 1: c063390a94 submodule: expose the '--for-status' option of summary\n1: add55c1d22 ! 2: 561d03351b submodule: amend extra line feed between callback struct and macro\n @@ Metadata\n Author: Shourya Shukla \n \n ## Commit message ##\n - submodule: amend extra line feed between callback struct and macro\n + submodule: remove extra line feeds between callback struct and macro\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 + Many `submodule--helper` subcommands follow the convention that a struct\n + defines their callback data, and the declaration of that struct is\n + followed immediately by a macro to use in static initializers, without\n + any separating empty line.\n +\n + Let's align the `init`, `status` and `sync` subcommands with that convention.\n \n Mentored-by: Christian Couder \n Mentored-by: Kaartic Sivaraam \n + Helped-by: Johannes Schindelin \n + Helped-by: Philip Oakley \n Signed-off-by: Shourya Shukla \n \n ## builtin/submodule--helper.c ##\n2: 75c986a5e0 = 3: e0e0c3ba4b submodule: rename helper functions to avoid ambiguity\n3: 745f2bd92e < -: ---------- diff: change scope of the function count_lines()\n-: ---------- > 4: ec6e6c9e64 t7421: introduce a test script for verifying 'summary' output\n4: c5baf68ecf ! 5: 9222b4168b submodule: port submodule subcommand 'summary' from shell to C\n @@ Metadata\n ## Commit message ##\n submodule: port submodule subcommand 'summary' from shell to C\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 + Convert submodule subcommand 'summary' to a builtin and call it via\n + 'git-submodule.sh'.\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 + The shell version had to call $diff_cmd twice, once to find the modified\n + modules cared by the user and then again, with that list of modules\n + to do various operations for computing the summary of those modules.\n + On the other hand, the C version does not need a second call to\n + $diff_cmd since it reuses the module list from the first call to do the\n + aforementioned tasks.\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 - callback function submodule_summary_callback(), and stored in the\n - structure module_cb.\n + In the C version, we use the combination of setting a child process'\n + working directory to the submodule path and then calling\n + 'prepare_submodule_repo_env()' which also sets the 'GIT_DIR' to '.git',\n + so that we can be certain that those spawned processes will not access\n + the superproject's ODB by mistake.\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 + A behavioural difference between the C and the shell version is that the\n + shell version outputs two line feeds after the 'git log' output when run\n + outside of the tests while the C version outputs one line feed in any\n + case. The reason for this is that the shell version calls log with\n + '--pretty=format:' whose output is followed by two echo\n + calls; 'format' does not have \"terminator\" semantics like its 'tformat'\n + counterpart. So, the log output is terminated by a newline only when\n + invoked by the user and not when invoked from the scripts. This results\n + in the one & two line feed differences in the shell version.\n + On the other hand, the C version calls log with '--pretty='\n + which is equivalent to '--pretty:tformat:' which is then\n + followed by a 'printf(\"\\n\")'. Due to its \"terminator\" semantics the\n + log output is always terminated by newline and hence one line feed in\n + any case.\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 + Also, when we try to pass an option-like argument after a non-option\n + argument, for instance:\n \n - Mentored-by: Christian Couder \n - Mentored-by: Stefan Beller \n + git submodule summary HEAD --foo-bar\n +\n + (or)\n +\n + git submodule summary HEAD --cached\n +\n + That argument would be treated like a path to the submodule for which\n + the user is requesting a summary. So, the option ends up having no\n + effect. Though, passing '--quiet' is an exception to this:\n +\n + git submodule summary HEAD --quiet\n +\n + While 'summary' doesn't support '--quiet', we don't get an output for\n + the above command as '--quiet' is treated as a path which means we get\n + an output only if a submodule whose path is '--quiet' exists.\n +\n + The error message in case of computing a summary for non-existent\n + submodules in the C version is different from that of the shell version.\n + Since the new error message is not marked for translation, change the\n + 'test_i18ngrep' in t7421.4 to 'grep'.\n +\n + Mentored-by: Christian Couder \n + Mentored-by: Stefan Beller \n Mentored-by: Kaartic Sivaraam \n + Helped-by: Johannes Schindelin \n Signed-off-by: Prathamesh Chavan \n Signed-off-by: Shourya Shukla \n \n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + const char *prefix;\n + unsigned int cached: 1;\n + unsigned int for_status: 1;\n -+ unsigned int quiet: 1;\n + unsigned int files: 1;\n + int summary_limit;\n +};\n -+#define SUMMARY_CB_INIT { 0, NULL, NULL, 0, 0, 0, 0, 0 }\n ++#define SUMMARY_CB_INIT { 0, NULL, NULL, 0, 0, 0, 0 }\n +\n +enum diff_cmd {\n + DIFF_INDEX,\n + DIFF_FILES\n +};\n +\n -+static int verify_submodule_object_name(const char *sm_path,\n -+ const char *sha1)\n ++static char* verify_submodule_committish(const char *sm_path,\n ++ const char *committish)\n +{\n + struct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n ++ struct strbuf result = STRBUF_INIT;\n +\n + cp_rev_parse.git_cmd = 1;\n -+ cp_rev_parse.no_stdout = 1;\n + cp_rev_parse.dir = sm_path;\n + prepare_submodule_repo_env(&cp_rev_parse.env_array);\n + argv_array_pushl(&cp_rev_parse.args, \"rev-parse\", \"-q\",\n -+ \"--verify\", NULL);\n -+ argv_array_pushf(&cp_rev_parse.args, \"%s^0\", sha1);\n ++ \"--short\", NULL);\n ++ argv_array_pushf(&cp_rev_parse.args, \"%s^0\", committish);\n ++ argv_array_push(&cp_rev_parse.args, \"--\");\n +\n -+ if (run_command(&cp_rev_parse))\n -+ return 1;\n ++ if (capture_command(&cp_rev_parse, &result, 0))\n ++ return NULL;\n +\n -+ return 0;\n ++ strbuf_trim_trailing_newline(&result);\n ++ return strbuf_detach(&result, NULL);\n +}\n +\n -+static void print_submodule_summary(struct summary_cb *info, int errmsg,\n -+ int total_commits, int missing_src,\n -+ int missing_dst, const char *displaypath,\n -+ int is_sm_git_dir, struct module_cb *p)\n ++static void print_submodule_summary(struct summary_cb *info, char* errmsg,\n ++ int total_commits, const char *displaypath,\n ++ const char *src_abbrev, const char *dst_abbrev,\n ++ int missing_src, int missing_dst,\n ++ struct module_cb *p)\n +{\n + if (p->status == 'T') {\n + if (S_ISGITLINK(p->mod_dst))\n + printf(_(\"* %s %s(blob)->%s(submodule)\"),\n -+ displaypath, find_unique_abbrev(&p->oid_src, 7),\n -+ find_unique_abbrev(&p->oid_dst, 7));\n ++ displaypath, src_abbrev, dst_abbrev);\n + else\n + printf(_(\"* %s %s(submodule)->%s(blob)\"),\n -+ displaypath, find_unique_abbrev(&p->oid_src, 7),\n -+ find_unique_abbrev(&p->oid_dst, 7));\n ++ displaypath, src_abbrev, dst_abbrev);\n + } else {\n + printf(\"* %s %s...%s\",\n -+ displaypath, find_unique_abbrev(&p->oid_src, 7),\n -+ find_unique_abbrev(&p->oid_dst, 7));\n ++ displaypath, src_abbrev, dst_abbrev);\n + }\n +\n + if (total_commits < 0)\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + printf(\" (%d):\\n\", total_commits);\n +\n + if (errmsg) {\n -+ /*\n -+ * Don't give error msg for modification whose dst is not\n -+ * submodule, i.e. deleted or changed to blob\n -+ */\n -+ if (S_ISGITLINK(p->mod_src)) {\n -+ if (missing_src && missing_dst) {\n -+ printf(_(\" Warn: %s doesn't contain commits %s and %s\\n\"),\n -+ displaypath, oid_to_hex(&p->oid_src),\n -+ oid_to_hex(&p->oid_dst));\n -+ } else if (missing_src) {\n -+ printf(_(\" Warn: %s doesn't contain commit %s\\n\"),\n -+ displaypath, oid_to_hex(&p->oid_src));\n -+ } else {\n -+ printf(_(\" Warn: %s doesn't contain commit %s\\n\"),\n -+ displaypath, oid_to_hex(&p->oid_dst));\n -+ }\n -+ }\n -+ } else if (is_sm_git_dir) {\n ++ printf(_(\"%s\"), errmsg);\n ++ } else if (total_commits > 0) {\n + struct child_process cp_log = CHILD_PROCESS_INIT;\n +\n + cp_log.git_cmd = 1;\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + argv_array_pushl(&cp_log.args, \"--pretty= %m %s\",\n + \"--first-parent\", NULL);\n + argv_array_pushf(&cp_log.args, \"%s...%s\",\n -+ oid_to_hex(&p->oid_src),\n -+ oid_to_hex(&p->oid_dst));\n ++ src_abbrev,\n ++ dst_abbrev);\n + } else if (S_ISGITLINK(p->mod_dst)) {\n + argv_array_pushl(&cp_log.args, \"--pretty= > %s\",\n -+ \"-1\", oid_to_hex(&p->oid_dst), NULL);\n ++ \"-1\", dst_abbrev, NULL);\n + } else {\n + argv_array_pushl(&cp_log.args, \"--pretty= < %s\",\n -+ \"-1\", oid_to_hex(&p->oid_src), NULL);\n ++ \"-1\", src_abbrev, NULL);\n + }\n + run_command(&cp_log);\n + }\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n +static void generate_submodule_summary(struct summary_cb *info,\n + struct module_cb *p)\n +{\n -+ int missing_src = 0;\n -+ int missing_dst = 0;\n -+ char *displaypath;\n -+ int errmsg = 0;\n ++ char *displaypath, *src_abbrev, *dst_abbrev;\n ++ int missing_src = 0, missing_dst = 0;\n ++ char *errmsg = NULL;\n + int total_commits = -1;\n -+ int is_sm_git_dir = 0;\n -+ struct strbuf sm_git_dir_sb = STRBUF_INIT;\n +\n + if (!info->cached && oideq(&p->oid_dst, &null_oid)) {\n + if (S_ISGITLINK(p->mod_dst)) {\n -+ /*\n -+ * NEEDSWORK: avoid using separate process with\n -+ * the help of the function head_ref_submodule()\n -+ */\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, \"rev-parse\",\n -+ \"HEAD\", NULL);\n -+ if (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {\n -+ strbuf_strip_suffix(&sb_rev_parse, \"\\n\");\n -+ get_oid_hex(sb_rev_parse.buf, &p->oid_dst);\n -+ }\n -+ strbuf_release(&sb_rev_parse);\n ++ struct ref_store *refs = get_submodule_ref_store(p->sm_path);\n ++ if (refs)\n ++ refs_head_ref(refs, handle_submodule_head_ref, &p->oid_dst);\n + } else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n -+ struct child_process cp_hash_object = CHILD_PROCESS_INIT;\n -+ struct strbuf sb_hash_object = STRBUF_INIT;\n -+\n -+ cp_hash_object.git_cmd = 1;\n -+ argv_array_pushl(&cp_hash_object.args, \"hash-object\",\n -+ p->sm_path, NULL);\n -+ if (!capture_command(&cp_hash_object,\n -+ &sb_hash_object, 0)) {\n -+ strbuf_strip_suffix(&sb_hash_object, \"\\n\");\n -+ get_oid_hex(sb_hash_object.buf, &p->oid_dst);\n -+ }\n -+ strbuf_release(&sb_hash_object);\n ++ struct stat st;\n ++ int fd = open(p->sm_path, O_RDONLY);\n ++\n ++ if (fd < 0 || fstat(fd, &st) < 0 ||\n ++ index_fd(&the_index, &p->oid_dst, fd, &st, OBJ_BLOB,\n ++ p->sm_path, 0))\n ++ error(_(\"couldn't hash object from '%s'\"), p->sm_path);\n + } else {\n ++ /* for a submodule removal (mode:0000000), don't warn */\n + if (p->mod_dst)\n -+ die(_(\"unexpected mode %d\\n\"), p->mod_dst);\n ++ warning(_(\"unexpected mode %d\\n\"), p->mod_dst);\n + }\n + }\n +\n -+ strbuf_addstr(&sm_git_dir_sb, p->sm_path);\n -+ if (is_nonbare_repository_dir(&sm_git_dir_sb))\n -+ is_sm_git_dir = 1;\n -+\n -+ if (is_sm_git_dir && S_ISGITLINK(p->mod_src))\n -+ missing_src = verify_submodule_object_name(p->sm_path,\n -+ oid_to_hex(&p->oid_src));\n ++ if (S_ISGITLINK(p->mod_src)) {\n ++ src_abbrev = verify_submodule_committish(p->sm_path,\n ++ oid_to_hex(&p->oid_src));\n ++ if (!src_abbrev) {\n ++ missing_src = 1;\n ++ /*\n ++ * As `rev-parse` failed, we fallback to getting\n ++ * the abbreviated hash using oid_src. We do\n ++ * this as we might still need the abbreviated\n ++ * hash in cases like a submodule type change, etc.\n ++ */\n ++ src_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n ++ }\n ++ } else {\n ++ /*\n ++ * The source does not point to a submodule.\n ++ * So, we fallback to getting the abbreviation using\n ++ * oid_src as we might still need the abbreviated\n ++ * hash in cases like submodule add, etc.\n ++ */\n ++ src_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n ++ }\n +\n -+ if (is_sm_git_dir && S_ISGITLINK(p->mod_dst))\n -+ missing_dst = verify_submodule_object_name(p->sm_path,\n -+ oid_to_hex(&p->oid_dst));\n ++ if (S_ISGITLINK(p->mod_dst)) {\n ++ dst_abbrev = verify_submodule_committish(p->sm_path,\n ++ oid_to_hex(&p->oid_dst));\n ++ if (!dst_abbrev) {\n ++ missing_dst = 1;\n ++ /*\n ++ * As `rev-parse` failed, we fallback to getting\n ++ * the abbreviated hash using oid_dst. We do\n ++ * this as we might still need the abbreviated\n ++ * hash in cases like a submodule type change, etc.\n ++ */\n ++ dst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n ++ }\n ++ } else {\n ++ /*\n ++ * The destination does not point to a submodule.\n ++ * So, we fallback to getting the abbreviation using\n ++ * oid_dst as we might still need the abbreviated\n ++ * hash in cases like a submodule removal, etc.\n ++ */\n ++ dst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n ++ }\n +\n + displaypath = get_submodule_displaypath(p->sm_path, info->prefix);\n +\n -+ if (!missing_dst && !missing_src) {\n -+ if (is_sm_git_dir) {\n -+ struct child_process cp_rev_list = CHILD_PROCESS_INIT;\n -+ struct strbuf sb_rev_list = STRBUF_INIT;\n -+ char *range;\n -+\n -+ if (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n -+ range = xstrfmt(\"%s...%s\", oid_to_hex(&p->oid_src),\n -+ oid_to_hex(&p->oid_dst));\n -+ else if (S_ISGITLINK(p->mod_src))\n -+ range = xstrdup(oid_to_hex(&p->oid_src));\n -+ else\n -+ range = xstrdup(oid_to_hex(&p->oid_dst));\n -+\n -+ cp_rev_list.git_cmd = 1;\n -+ cp_rev_list.dir = p->sm_path;\n -+ prepare_submodule_repo_env(&cp_rev_list.env_array);\n -+\n -+ argv_array_pushl(&cp_rev_list.args, \"rev-list\",\n -+ \"--first-parent\", range, \"--\", NULL);\n -+ if (!capture_command(&cp_rev_list, &sb_rev_list, 0)) {\n -+ if (sb_rev_list.len)\n -+ total_commits = count_lines(sb_rev_list.buf,\n -+ sb_rev_list.len);\n -+ else\n -+ total_commits = 0;\n -+ }\n ++ if (!missing_src && !missing_dst) {\n ++ struct child_process cp_rev_list = CHILD_PROCESS_INIT;\n ++ struct strbuf sb_rev_list = STRBUF_INIT;\n +\n -+ free(range);\n -+ strbuf_release(&sb_rev_list);\n -+ }\n ++ argv_array_pushl(&cp_rev_list.args, \"rev-list\",\n ++ \"--first-parent\", \"--count\", NULL);\n ++ if (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n ++ argv_array_pushf(&cp_rev_list.args, \"%s...%s\",\n ++ src_abbrev,\n ++ dst_abbrev);\n ++ else\n ++ argv_array_push(&cp_rev_list.args,\n ++ S_ISGITLINK(p->mod_src) ?\n ++ src_abbrev :\n ++ dst_abbrev);\n ++ argv_array_push(&cp_rev_list.args, \"--\");\n ++\n ++ cp_rev_list.git_cmd = 1;\n ++ cp_rev_list.dir = p->sm_path;\n ++ prepare_submodule_repo_env(&cp_rev_list.env_array);\n ++\n ++ if (!capture_command(&cp_rev_list, &sb_rev_list, 0))\n ++ total_commits = atoi(sb_rev_list.buf);\n ++\n ++ strbuf_release(&sb_rev_list);\n + } else {\n -+ errmsg = 1;\n ++ /*\n ++ * Don't give error msg for modification whose dst is not\n ++ * submodule, i.e., deleted or changed to blob\n ++ */\n ++ if (S_ISGITLINK(p->mod_dst)) {\n ++ struct strbuf errmsg_str = STRBUF_INIT;\n ++ if (missing_src && missing_dst) {\n ++ strbuf_addf(&errmsg_str, \" Warn: %s doesn't contain commits %s and %s\\n\",\n ++ displaypath, oid_to_hex(&p->oid_src),\n ++ oid_to_hex(&p->oid_dst));\n ++ } else {\n ++ strbuf_addf(&errmsg_str, \" Warn: %s doesn't contain commit %s\\n\",\n ++ displaypath, missing_src ?\n ++ oid_to_hex(&p->oid_src) :\n ++ oid_to_hex(&p->oid_dst));\n ++ }\n ++ errmsg = strbuf_detach(&errmsg_str, NULL);\n ++ }\n + }\n +\n + print_submodule_summary(info, errmsg, total_commits,\n -+ missing_src, missing_dst,\n -+ displaypath, is_sm_git_dir, p);\n ++ displaypath, src_abbrev,\n ++ dst_abbrev, missing_src,\n ++ missing_dst, p);\n +\n + free(displaypath);\n -+ strbuf_release(&sm_git_dir_sb);\n ++ free(src_abbrev);\n ++ free(dst_abbrev);\n +}\n +\n +static void prepare_submodule_summary(struct summary_cb *info,\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n +{\n + int i;\n + for (i = 0; i < list->nr; i++) {\n ++ const struct submodule *sub;\n + struct module_cb *p = list->entries[i];\n -+ struct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n ++ struct strbuf sm_gitdir = STRBUF_INIT;\n +\n + if (p->status == 'D' || p->status == 'T') {\n + generate_submodule_summary(info, p);\n + continue;\n + }\n +\n -+ if (info->for_status) {\n -+ char *config_key;\n -+ const char *ignore_config = \"none\";\n ++ if (info->for_status && p->status != 'A' &&\n ++ (sub = submodule_from_path(the_repository,\n ++ &null_oid, p->sm_path))) {\n ++ char *config_key = NULL;\n + const char *value;\n -+ const struct submodule *sub = submodule_from_path(the_repository,\n -+ &null_oid,\n -+ p->sm_path);\n -+\n -+ if (sub && p->status != 'A') {\n -+ config_key = xstrfmt(\"submodule.%s.ignore\",\n -+ sub->name);\n -+ if (!git_config_get_string_const(config_key, &value))\n -+ ignore_config = value;\n -+ else if (sub->ignore)\n -+ ignore_config = sub->ignore;\n -+\n -+ free(config_key);\n -+ if (!strcmp(ignore_config, \"all\"))\n -+ continue;\n -+ }\n ++ int ignore_all = 0;\n ++\n ++ config_key = xstrfmt(\"submodule.%s.ignore\",\n ++ sub->name);\n ++ if (!git_config_get_string_const(config_key, &value))\n ++ ignore_all = !strcmp(value, \"all\");\n ++ else if (sub->ignore)\n ++ ignore_all = !strcmp(sub->ignore, \"all\");\n ++\n ++ free(config_key);\n ++ if (ignore_all)\n ++ continue;\n + }\n +\n + /* Also show added or modified modules which are checked out */\n -+ cp_rev_parse.dir = p->sm_path;\n -+ cp_rev_parse.git_cmd = 1;\n -+ cp_rev_parse.no_stderr = 1;\n -+ cp_rev_parse.no_stdout = 1;\n -+\n -+ argv_array_pushl(&cp_rev_parse.args, \"rev-parse\",\n -+ \"--git-dir\", NULL);\n -+\n -+ if (!run_command(&cp_rev_parse))\n ++ strbuf_addstr(&sm_gitdir, p->sm_path);\n ++ if (is_nonbare_repository_dir(&sm_gitdir))\n + generate_submodule_summary(info, p);\n ++ strbuf_release(&sm_gitdir);\n + }\n +}\n +\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + }\n +}\n +\n -+static int compute_summary_module_list(char *head,\n -+ struct summary_cb *info,\n -+ enum diff_cmd diff_cmd)\n ++static int compute_summary_module_list(struct object_id *head_oid,\n ++ struct summary_cb *info,\n ++ enum diff_cmd diff_cmd)\n +{\n + struct argv_array diff_args = ARGV_ARRAY_INIT;\n + struct rev_info rev;\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + argv_array_push(&diff_args, \"--cached\");\n + argv_array_pushl(&diff_args, \"--ignore-submodules=dirty\", \"--raw\",\n + NULL);\n -+ if (head)\n -+ argv_array_push(&diff_args, head);\n ++ if (head_oid)\n ++ argv_array_push(&diff_args, oid_to_hex(head_oid));\n + argv_array_push(&diff_args, \"--\");\n + if (info->argc)\n + argv_array_pushv(&diff_args, info->argv);\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + rev.diffopt.format_callback_data = &list;\n +\n + if (!info->cached) {\n -+ if (diff_cmd == DIFF_INDEX)\n ++ if (diff_cmd == DIFF_INDEX)\n + setup_work_tree();\n + if (read_cache_preload(&rev.diffopt.pathspec) < 0) {\n + perror(\"read_cache_preload\");\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + struct summary_cb info = SUMMARY_CB_INIT;\n + int cached = 0;\n + int for_status = 0;\n -+ int quiet = 0;\n + int files = 0;\n + int summary_limit = -1;\n -+ struct child_process cp_rev = CHILD_PROCESS_INIT;\n -+ struct strbuf sb = STRBUF_INIT;\n + enum diff_cmd diff_cmd = DIFF_INDEX;\n ++ struct object_id head_oid;\n + int ret;\n +\n + struct option module_summary_options[] = {\n -+ OPT__QUIET(&quiet, N_(\"Suppress output of summarising submodules\")),\n + OPT_BOOL(0, \"cached\", &cached,\n -+ N_(\"Use the commit stored in the index instead of the submodule HEAD\")),\n ++ N_(\"use the commit stored in the index instead of the submodule HEAD\")),\n + OPT_BOOL(0, \"files\", &files,\n -+ N_(\"To compare the commit in the index with that in the submodule HEAD\")),\n ++ N_(\"to compare the commit in the index with that in the submodule HEAD\")),\n + OPT_BOOL(0, \"for-status\", &for_status,\n -+ N_(\"Skip submodules with 'ignore_config' value set to 'all'\")),\n ++ N_(\"skip submodules with 'ignore_config' value set to 'all'\")),\n + OPT_INTEGER('n', \"summary-limit\", &summary_limit,\n -+ N_(\"Limit the summary size\")),\n ++ N_(\"limit the summary size\")),\n + OPT_END()\n + };\n +\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + if (!summary_limit)\n + return 0;\n +\n -+ cp_rev.git_cmd = 1;\n -+ argv_array_pushl(&cp_rev.args, \"rev-parse\", \"-q\", \"--verify\",\n -+ argc ? argv[0] : \"HEAD\", NULL);\n -+\n -+ if (!capture_command(&cp_rev, &sb, 0)) {\n -+ strbuf_strip_suffix(&sb, \"\\n\");\n ++ if (!get_oid(argc ? argv[0] : \"HEAD\", &head_oid)) {\n + if (argc) {\n + argv++;\n + argc--;\n + }\n + } else if (!argc || !strcmp(argv[0], \"HEAD\")) {\n + /* before the first commit: compare with an empty tree */\n -+ struct stat st;\n -+ struct object_id oid;\n -+ if (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,\n -+ prefix, 3))\n -+ die(\"Unable to add %s to database\", oid.hash);\n -+ strbuf_addstr(&sb, oid_to_hex(&oid));\n ++ oidcpy(&head_oid, the_hash_algo->empty_tree);\n + if (argc) {\n + argv++;\n + argc--;\n + }\n + } else {\n -+ strbuf_addstr(&sb, \"HEAD\");\n ++ if (get_oid(\"HEAD\", &head_oid))\n ++ die(_(\"could not fetch a revision for HEAD\"));\n + }\n +\n + if (files) {\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + info.argv = argv;\n + info.prefix = prefix;\n + info.cached = !!cached;\n ++ info.files = !!files;\n + info.for_status = !!for_status;\n -+ info.quiet = quiet;\n -+ info.files = files;\n + info.summary_limit = summary_limit;\n +\n -+ ret = compute_summary_module_list((diff_cmd == DIFF_FILES) ? NULL : sb.buf,\n -+ &info, diff_cmd);\n -+ strbuf_release(&sb);\n ++ ret = compute_summary_module_list((diff_cmd == DIFF_INDEX) ? &head_oid : NULL,\n ++ &info, diff_cmd);\n + return ret;\n +}\n +\n @@ git-submodule.sh: cmd_summary() {\n - fi\n - echo\n - done\n -+ git ${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 ++ git ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper summary ${prefix:+--prefix \"$prefix\"} ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- \"$@\"\n }\n #\n # List all submodules, prefixed with:\n +\n + ## t/t7421-submodule-summary-add.sh ##\n +@@ t/t7421-submodule-summary-add.sh: test_expect_success 'submodule summary output for submodules with changed paths'\n + git commit -m \"change submodule path\" &&\n + rev=$(git -C sm rev-parse --short HEAD^) &&\n + git submodule summary HEAD^^ -- my-subm >actual 2>err &&\n +- test_i18ngrep \"fatal:.*my-subm\" err &&\n ++ grep \"fatal:.*my-subm\" err &&\n + cat >expected <<-EOF &&\n + * my-subm ${rev}...0000000:\n + \n\nPrathamesh Chavan (1):\n submodule: port submodule subcommand 'summary' from shell to C\n\nShourya Shukla (4):\n submodule: expose the '--for-status' option of summary\n submodule: remove extra line feeds between callback struct and macro\n submodule: rename helper functions to avoid ambiguity\n t7421: introduce a test script for verifying 'summary' output\n\n Documentation/git-submodule.txt | 8 +-\n builtin/submodule--helper.c | 439 ++++++++++++++++++++++++-\n contrib/completion/git-completion.bash | 2 +-\n diff.c | 2 +-\n git-submodule.sh | 188 +----------\n submodule.c | 10 +-\n submodule.h | 2 +-\n t/t7421-submodule-summary-add.sh | 69 ++++\n 8 files changed, 522 insertions(+), 198 deletions(-)\n create mode 100755 t/t7421-submodule-summary-add.sh\n\n-- \n2.28.0\n\n"},{"id":"403102","messageId":"xmqq5z9vjsvz.fsf@gitster.c.googlers.com","threadId":"53998","inReplyTo":"20200806164102.6707-6-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 5/5] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-06T22:45:20Z","receivedAt":"2020-08-06T22:45:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> ...\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 src_abbrev,\n> +\t\t\t\t\t dst_abbrev);\n\n> ...\n\n> +\tdiff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,\n> +\t\t\t\t\t &rev, NULL);\n\nPeff's jk/strvec topic will soon be in 'master', and basing the\nseries on top of 'master' after that happens would make these lines\nto read like\n\n\t\t\tstrvec_pushl(&cp_log.args, \"--pretty= %m %s\",\n\t\t\t\t \"--first-parent\", NULL);\n\t\t\tstrvec_pushf(&cp_log.args, \"%s...%s\",\n\t\t\t\t src_abbrev,\n\t\t\t\t dst_abbrev);\n\n\tdiff_args.nr = setup_revisions(diff_args.nr, diff_args.v,\n\t\t\t\t &rev, NULL);\n\nWe may even be able to reduce line wrapping thanks to shortening a\nfew common words:\n\n argv_array => strvec\n argc => nc\n argv => v\n\nFor today's integration, I dealt with these as conflict resolution,\nso let's keep review discussion going, and hope jk/strvec is in\n'master' by the time this topic becomes ready.\n\nThanks.\n"},{"id":"403133","messageId":"20200807163135.GA12568@konoha","threadId":"53998","inReplyTo":"xmqq5z9vjsvz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 5/5] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-07T16:31:35Z","receivedAt":"2020-08-07T16:31:47Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 06/08 03:45, Junio C Hamano wrote:\n> Shourya Shukla writes:\n> \n> > ...\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 src_abbrev,\n> > +\t\t\t\t\t dst_abbrev);\n> \n> > ...\n> \n> > +\tdiff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,\n> > +\t\t\t\t\t &rev, NULL);\n> \n> Peff's jk/strvec topic will soon be in 'master', and basing the\n> series on top of 'master' after that happens would make these lines\n> to read like\n> \n> \t\t\tstrvec_pushl(&cp_log.args, \"--pretty= %m %s\",\n> \t\t\t\t \"--first-parent\", NULL);\n> \t\t\tstrvec_pushf(&cp_log.args, \"%s...%s\",\n> \t\t\t\t src_abbrev,\n> \t\t\t\t dst_abbrev);\n> \n> \tdiff_args.nr = setup_revisions(diff_args.nr, diff_args.v,\n> \t\t\t\t &rev, NULL);\n> \n> We may even be able to reduce line wrapping thanks to shortening a\n> few common words:\n> \n> argv_array => strvec\n> argc => nc\n> argv => v\n> \n> For today's integration, I dealt with these as conflict resolution,\n> so let's keep review discussion going, and hope jk/strvec is in\n> 'master' by the time this topic becomes ready.\n\nUnderstood. I will base this patch on the above series. Seems like a\ngreat series of great change! BTW, I asked a couple of things in the\ncover-letter which I think you might have missed. Quoting them here:\n-----8<-----\nAlso, I want to ask a couple of things:\n\n\t1.Whether we can suppress the error message that we get when\n\t trying to find the summary of non-existent submodules?\n\t For example:\n\n\t fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n\t * my-subm 35b40f1...0000000:\n\n\t Will it be OK to suppress the above error message?\n\n\t2.Is it fine to document and expose the 'for-status' option in\n\t 'git-submodule.txt'?\n----->8-----\n\nRegards,\nShourya Shukla\n"},{"id":"403134","messageId":"xmqq8seqidi1.fsf@gitster.c.googlers.com","threadId":"53998","inReplyTo":"20200807163135.GA12568@konoha","subject":"Re: [PATCH v2 5/5] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T17:15:18Z","receivedAt":"2020-08-07T17:15:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> Understood. I will base this patch on the above series. Seems like a\n> great series of great change! BTW, I asked a couple of things in the\n> cover-letter which I think you might have missed.\n\nNo I didn't miss. I didn't resopnd because I didn't have particular\nanswers to them.\n"},{"id":"403195","messageId":"831df9f2-0663-0dfc-0871-d34864d1ecde@gmail.com","threadId":"53998","inReplyTo":"20200806164102.6707-2-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 1/5] submodule: expose the '--for-status' option of summary","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-08T14:40:07Z","receivedAt":"2020-08-08T14:41:34Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 06-08-2020 22:10, Shourya Shukla wrote:\n> The 'for-status' option is used to compute the summary of submodule(s)\n> in a superproject by skipping the ignored submdules i.e., those with\n> 'submodule..ignore' set to 'all' in the '.gitmodules' or\n> '.git/config', with the latter taking precedence over the former.\n> \n> The option was introduced in d0f64dd44d (git-submodule summary:\n> --for-status option, 2008-04-12), refined in 3ba7407b8b (submodule\n> summary: ignore --for-status option, 2013-09-06) and finally perfected\n> in 927b26f87a (submodule: don't print status output with ignore=all,\n> 2013-09-01). But, it was not mentioned in the 'git submodule'\n> Documentation.\n> \n> Expose the '--for-status' option accepted by the command 'git submodule\n> summary'.\n>\n\nI've had one concern about exposing '--for-status'. As of now, the name\nof the option has no relation with the behaviour that we get as a\nconsequence. So long, the option has been internal and this wasn't a\nproblem. Now that we're considering to expose it in the docs, usage and\nautocomplete, I would say it should be done after renaming it\nappropriately given that it's easy to do now than later. As to name\nsuggestions, I really don't have any.\n\nAlso, as to whether exposing this would be useful at all, I really don't\nknow.\n\n-- \nSivaraam\n"},{"id":"403210","messageId":"CAP8UFD20ORozywSAV+Qayuf_vwve9A21ySAtTZVphwhv5nYWXg@mail.gmail.com","threadId":"53998","inReplyTo":"831df9f2-0663-0dfc-0871-d34864d1ecde@gmail.com","subject":"Re: [PATCH v2 1/5] submodule: expose the '--for-status' option of summary","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-08-08T20:25:10Z","receivedAt":"2020-08-08T20:25:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le sam. 8 août 2020 à 16:40, Kaartic Sivaraam\n a écrit :\n>\n> On 06-08-2020 22:10, Shourya Shukla wrote:\n> > The 'for-status' option is used to compute the summary of submodule(s)\n> > in a superproject by skipping the ignored submdules i.e., those with\n> > 'submodule..ignore' set to 'all' in the '.gitmodules' or\n> > '.git/config', with the latter taking precedence over the former.\n\nThe above seems to suggest that a name like --skip-ignored could fit,\nif we wanted to rename --for-status.\n\n> > The option was introduced in d0f64dd44d (git-submodule summary:\n> > --for-status option, 2008-04-12), refined in 3ba7407b8b (submodule\n> > summary: ignore --for-status option, 2013-09-06) and finally perfected\n> > in 927b26f87a (submodule: don't print status output with ignore=all,\n> > 2013-09-01). But, it was not mentioned in the 'git submodule'\n> > Documentation.\n\nAfter this we would need to tell why it's a good idea to actually\ndocument this option (and perhaps rename it if we are going to do\nthat). It could be a good idea, if it could help users to see a\nsummary without the ignored submodules.\n\nSo for example a possibly good justification could be that in a repo\nwith many ignored submodules it might be interesting for users to get\na summary that contains information only about the non-ignored\nsubmodules.\n\nAn example output of `git submodule summary` both with and without\n--for-status (or --skip-ignored) in an interesting case (where there\nare many ignored submodule) could help convince people that it's a\npossibly useful option, and that it's worth documenting.\n\n> > Expose the '--for-status' option accepted by the command 'git submodule\n> > summary'.\n> >\n>\n> I've had one concern about exposing '--for-status'. As of now, the name\n> of the option has no relation with the behaviour that we get as a\n> consequence. So long, the option has been internal and this wasn't a\n> problem. Now that we're considering to expose it in the docs, usage and\n> autocomplete, I would say it should be done after renaming it\n> appropriately given that it's easy to do now than later. As to name\n> suggestions, I really don't have any.\n\nYeah, I agree that finding a good name and a good use case for the\noption would surely help.\n\n> Also, as to whether exposing this would be useful at all, I really don't\n> know.\n"},{"id":"403212","messageId":"xmqqv9hsen3d.fsf@gitster.c.googlers.com","threadId":"53998","inReplyTo":"CAP8UFD20ORozywSAV+Qayuf_vwve9A21ySAtTZVphwhv5nYWXg@mail.gmail.com","subject":"Re: [PATCH v2 1/5] submodule: expose the '--for-status' option of summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-08T23:26:14Z","receivedAt":"2020-08-08T23:26:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Yeah, I agree that finding a good name and a good use case for the\n> option would surely help.\n\nThat makes it sound like a solution looking for a problem.\n\nThe option was added and discussed in [*1*], and it was quite clear\nthat it was merely an implementation detail to show the same info as\n\"git submodule summary\" in a format that would fit better in the\ncontext of \"git status\". I doubt that anything changed since then\nin the past 12 years to make the option deserve more attention by\nthe end users, but what this patch (which is the first in a 5-patch\nseries) does may be worth doing if a later patch in the series\nserves as that \"good use case\".\n\nOn the other hand, if there is no such \"good use case\" example in\nthe other changes in this series, the option can and should be kept\nas an implementation detail of \"git status\", I would think.\n\nThanks.\n\n\n[Reference]\n\n*1*\nhttps://lore.kernel.org/git/1205416085-23431-1-git-send-email-pkufranky@gmail.com/\n\n"},{"id":"403494","messageId":"20200812194404.17028-1-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200806164102.6707-1-shouryashukla.oo@gmail.com","subject":"[GSoC][PATCH v3 0/4] submodule: port subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-12T19:44:00Z","receivedAt":"2020-08-12T19:44:24Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Greetings,\n\nThis is the v3 of the patch series titled 'submodule: port subcommand\n'summary' from shell to C':\nhttps://lore.kernel.org/git/20200806164102.6707-1-shouryashukla.oo@gmail.com/\n\nAfter suggestions from Junio, Kaartic and Christian I made some\nchanges:\n\n-> Drop the commit c063390a94 (submodule: expose the '--for-status'\n option of summary, 2020-08-02) since it felt a bit unnecessary to\n expose the option given its lack of major use cases. Hence, after\n feedback from Junio, Kaartic and Christian I dropped this patch\n for now.\n\n-> Adapt the patch to jk/strvec. It is a big change for Git hence it was\n necessary to make the port as per this patch.\n\nFeedback and reviews are appreciated. I am tagging along a range-diff\nbetween the v3 and v2 for ease of review.\n\nRegards,\nShourya Shukla\n\n-----\nrange-diff:\n\n1: c063390a94 < -: ---------- submodule: expose the '--for-status' option of summary\n2: 561d03351b = 1: 2a66d8c2bb submodule: remove extra line feeds between callback struct and macro\n3: e0e0c3ba4b = 2: 0ae3b92e31 submodule: rename helper functions to avoid ambiguity\n4: ec6e6c9e64 ! 3: e2912a042f t7421: introduce a test script for verifying 'summary' output\n @@ Commit message\n do not work as expected.\n\n So, introduce a test script for verifying the 'summary' output for\n - submodules added using 'git submodule add'.\n + submodules added using 'git submodule add' and notify regarding the\n + above mentioned behaviour in t7401 itself.\n\n Mentored-by: Christian Couder \n Mentored-by: Kaartic Sivaraam \n Signed-off-by: Shourya Shukla \n\n + ## t/t7401-submodule-summary.sh ##\n +@@ t/t7401-submodule-summary.sh: test_description='Summary support for submodules\n +\n + This test tries to verify the sanity of summary subcommand of git submodule.\n + '\n ++# NOTE: This test script uses 'git add' instead of 'git submodule add' to add\n ++# submodules to the superproject. Some submodule subcommands such as init and\n ++# deinit might not work as expected in this script. t7421 does not have this\n ++# caveat.\n +\n + . ./test-lib.sh\n +\n +\n ## t/t7421-submodule-summary-add.sh (new) ##\n @@\n +#!/bin/sh\n5: 9222b4168b ! 4: 879cb902b2 submodule: port submodule subcommand 'summary' from shell to C\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + cp_rev_parse.git_cmd = 1;\n + cp_rev_parse.dir = sm_path;\n + prepare_submodule_repo_env(&cp_rev_parse.env_array);\n -+ argv_array_pushl(&cp_rev_parse.args, \"rev-parse\", \"-q\",\n -+ \"--short\", NULL);\n -+ argv_array_pushf(&cp_rev_parse.args, \"%s^0\", committish);\n -+ argv_array_push(&cp_rev_parse.args, \"--\");\n ++ strvec_pushl(&cp_rev_parse.args, \"rev-parse\", \"-q\", \"--short\", NULL);\n ++ strvec_pushf(&cp_rev_parse.args, \"%s^0\", committish);\n ++ strvec_push(&cp_rev_parse.args, \"--\");\n +\n + if (capture_command(&cp_rev_parse, &result, 0))\n + return NULL;\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + cp_log.git_cmd = 1;\n + cp_log.dir = p->sm_path;\n + prepare_submodule_repo_env(&cp_log.env_array);\n -+ argv_array_pushl(&cp_log.args, \"log\", NULL);\n ++ strvec_pushl(&cp_log.args, \"log\", NULL);\n +\n + if (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst)) {\n + if (info->summary_limit > 0)\n -+ argv_array_pushf(&cp_log.args, \"-%d\",\n -+ info->summary_limit);\n -+\n -+ argv_array_pushl(&cp_log.args, \"--pretty= %m %s\",\n -+ \"--first-parent\", NULL);\n -+ argv_array_pushf(&cp_log.args, \"%s...%s\",\n -+ src_abbrev,\n -+ dst_abbrev);\n ++ strvec_pushf(&cp_log.args, \"-%d\",\n ++ info->summary_limit);\n ++\n ++ strvec_pushl(&cp_log.args, \"--pretty= %m %s\",\n ++ \"--first-parent\", NULL);\n ++ strvec_pushf(&cp_log.args, \"%s...%s\",\n ++ src_abbrev, dst_abbrev);\n + } else if (S_ISGITLINK(p->mod_dst)) {\n -+ argv_array_pushl(&cp_log.args, \"--pretty= > %s\",\n -+ \"-1\", dst_abbrev, NULL);\n ++ strvec_pushl(&cp_log.args, \"--pretty= > %s\",\n ++ \"-1\", dst_abbrev, NULL);\n + } else {\n -+ argv_array_pushl(&cp_log.args, \"--pretty= < %s\",\n -+ \"-1\", src_abbrev, NULL);\n ++ strvec_pushl(&cp_log.args, \"--pretty= < %s\",\n ++ \"-1\", src_abbrev, NULL);\n + }\n + run_command(&cp_log);\n + }\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + struct child_process cp_rev_list = CHILD_PROCESS_INIT;\n + struct strbuf sb_rev_list = STRBUF_INIT;\n +\n -+ argv_array_pushl(&cp_rev_list.args, \"rev-list\",\n -+ \"--first-parent\", \"--count\", NULL);\n ++ strvec_pushl(&cp_rev_list.args, \"rev-list\",\n ++ \"--first-parent\", \"--count\", NULL);\n + if (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n -+ argv_array_pushf(&cp_rev_list.args, \"%s...%s\",\n -+ src_abbrev,\n -+ dst_abbrev);\n ++ strvec_pushf(&cp_rev_list.args, \"%s...%s\",\n ++ src_abbrev, dst_abbrev);\n + else\n -+ argv_array_push(&cp_rev_list.args,\n -+ S_ISGITLINK(p->mod_src) ?\n -+ src_abbrev :\n -+ dst_abbrev);\n -+ argv_array_push(&cp_rev_list.args, \"--\");\n ++ strvec_push(&cp_rev_list.args, S_ISGITLINK(p->mod_src) ?\n ++ src_abbrev : dst_abbrev);\n ++ strvec_push(&cp_rev_list.args, \"--\");\n +\n + cp_rev_list.git_cmd = 1;\n + cp_rev_list.dir = p->sm_path;\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + struct summary_cb *info,\n + enum diff_cmd diff_cmd)\n +{\n -+ struct argv_array diff_args = ARGV_ARRAY_INIT;\n ++ struct strvec diff_args = STRVEC_INIT;\n + struct rev_info rev;\n + struct module_cb_list list = MODULE_CB_LIST_INIT;\n +\n -+ argv_array_push(&diff_args, get_diff_cmd(diff_cmd));\n ++ strvec_push(&diff_args, get_diff_cmd(diff_cmd));\n + if (info->cached)\n -+ argv_array_push(&diff_args, \"--cached\");\n -+ argv_array_pushl(&diff_args, \"--ignore-submodules=dirty\", \"--raw\",\n -+ NULL);\n ++ strvec_push(&diff_args, \"--cached\");\n ++ strvec_pushl(&diff_args, \"--ignore-submodules=dirty\", \"--raw\", NULL);\n + if (head_oid)\n -+ argv_array_push(&diff_args, oid_to_hex(head_oid));\n -+ argv_array_push(&diff_args, \"--\");\n ++ strvec_push(&diff_args, oid_to_hex(head_oid));\n ++ strvec_push(&diff_args, \"--\");\n + if (info->argc)\n -+ argv_array_pushv(&diff_args, info->argv);\n ++ strvec_pushv(&diff_args, info->argv);\n +\n + git_config(git_diff_basic_config, NULL);\n + init_revisions(&rev, info->prefix);\n + rev.abbrev = 0;\n -+ precompose_argv(diff_args.argc, diff_args.argv);\n -+\n -+ diff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,\n -+ &rev, NULL);\n ++ precompose_argv(diff_args.nr, diff_args.v);\n ++ setup_revisions(diff_args.nr, diff_args.v, &rev, NULL);\n + rev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n + rev.diffopt.format_callback = submodule_summary_callback;\n + rev.diffopt.format_callback_data = &list;\n @@ builtin/submodule--helper.c: static int module_name(int argc, const char **argv,\n + else\n + run_diff_files(&rev, 0);\n + prepare_submodule_summary(info, &list);\n ++ strvec_clear(&diff_args);\n + return 0;\n +}\n +\n\n-----\n\nPrathamesh Chavan (1):\n submodule: port submodule subcommand 'summary' from shell to C\n\nShourya Shukla (3):\n submodule: remove extra line feeds between callback struct and macro\n submodule: rename helper functions to avoid ambiguity\n t7421: introduce a test script for verifying 'summary' output\n\n builtin/submodule--helper.c | 432 ++++++++++++++++++++++++++++++-\n diff.c | 2 +-\n git-submodule.sh | 186 +------------\n submodule.c | 10 +-\n submodule.h | 2 +-\n t/t7401-submodule-summary.sh | 4 +\n t/t7421-submodule-summary-add.sh | 69 +++++\n 7 files changed, 510 insertions(+), 195 deletions(-)\n create mode 100755 t/t7421-submodule-summary-add.sh\n\n-- \n2.28.0\n\n"},{"id":"403495","messageId":"20200812194404.17028-2-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200812194404.17028-1-shouryashukla.oo@gmail.com","subject":"[PATCH v3 1/4] submodule: remove extra line feeds between callback struct and macro","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-12T19:44:01Z","receivedAt":"2020-08-12T19:44:27Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Many `submodule--helper` subcommands follow the convention that a struct\ndefines their callback data, and the declaration of that struct is\nfollowed immediately by a macro to use in static initializers, without\nany separating empty line.\n\nLet's align the `init`, `status` and `sync` subcommands with that convention.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nHelped-by: Johannes Schindelin \nHelped-by: Philip Oakley \nSigned-off-by: Shourya Shukla \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 df135abbf1..a03dc84ea4 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.28.0\n\n"},{"id":"403496","messageId":"20200812194404.17028-3-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200812194404.17028-1-shouryashukla.oo@gmail.com","subject":"[PATCH v3 2/4] submodule: rename helper functions to avoid ambiguity","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-12T19:44:02Z","receivedAt":"2020-08-12T19:44:29Z","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 \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \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 f9709de7b4..46175e40a6 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 a52b93a87f..8273647b73 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 9ce85c03fe..4ac6e31cf1 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.28.0\n\n"},{"id":"403497","messageId":"20200812194404.17028-4-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200812194404.17028-1-shouryashukla.oo@gmail.com","subject":"[PATCH v3 3/4] t7421: introduce a test script for verifying 'summary' output","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-12T19:44:03Z","receivedAt":"2020-08-12T19:44:33Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"'t7401-submodule-summary.sh' uses 'git add' to add submodules. Therefore,\nsome commands such as 'git submodule init' and 'git submodule deinit'\ndo not work as expected.\n\nSo, introduce a test script for verifying the 'summary' output for\nsubmodules added using 'git submodule add' and notify regarding the\nabove mentioned behaviour in t7401 itself.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n t/t7401-submodule-summary.sh | 4 ++\n t/t7421-submodule-summary-add.sh | 69 ++++++++++++++++++++++++++++++++\n 2 files changed, 73 insertions(+)\n create mode 100755 t/t7421-submodule-summary-add.sh\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 9bc841d085..45c5d2424e 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -7,6 +7,10 @@ test_description='Summary support for submodules\n \n This test tries to verify the sanity of summary subcommand of git submodule.\n '\n+# NOTE: This test script uses 'git add' instead of 'git submodule add' to add\n+# submodules to the superproject. Some submodule subcommands such as init and\n+# deinit might not work as expected in this script. t7421 does not have this\n+# caveat.\n \n . ./test-lib.sh\n \ndiff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh\nnew file mode 100755\nindex 0000000000..829fe26d6d\n--- /dev/null\n+++ b/t/t7421-submodule-summary-add.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+#\n+# Copyright (C) 2020 Shourya Shukla\n+#\n+\n+test_description='Summary support for submodules, adding them using git submodule add\n+\n+This test script tries to verify the sanity of summary subcommand of git submodule\n+while making sure to add submodules using `git submodule add` instead of\n+`git add` as done in t7401.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'summary test environment setup' '\n+\tgit init sm &&\n+\ttest_commit -C sm \"add file\" file file-content file-tag &&\n+\n+\tgit submodule add ./sm my-subm &&\n+\ttest_tick &&\n+\tgit commit -m \"add submodule\"\n+'\n+\n+test_expect_success 'submodule summary output for initialized submodule' '\n+\ttest_commit -C sm \"add file2\" file2 file2-content file2-tag &&\n+\tgit submodule update --remote &&\n+\ttest_tick &&\n+\tgit commit -m \"update submodule\" my-subm &&\n+\tgit submodule summary HEAD^ >actual &&\n+\trev1=$(git -C sm rev-parse --short HEAD^) &&\n+\trev2=$(git -C sm rev-parse --short HEAD) &&\n+\tcat >expected <<-EOF &&\n+\t* my-subm ${rev1}...${rev2} (1):\n+\t > add file2\n+\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'submodule summary output for deinitialized submodule' '\n+\tgit submodule deinit my-subm &&\n+\tgit submodule summary HEAD^ >actual &&\n+\ttest_must_be_empty actual &&\n+\tgit submodule update --init my-subm &&\n+\tgit submodule summary HEAD^ >actual &&\n+\trev1=$(git -C sm rev-parse --short HEAD^) &&\n+\trev2=$(git -C sm rev-parse --short HEAD) &&\n+\tcat >expected <<-EOF &&\n+\t* my-subm ${rev1}...${rev2} (1):\n+\t > add file2\n+\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'submodule summary output for submodules with changed paths' '\n+\tgit mv my-subm subm &&\n+\tgit commit -m \"change submodule path\" &&\n+\trev=$(git -C sm rev-parse --short HEAD^) &&\n+\tgit submodule summary HEAD^^ -- my-subm >actual 2>err &&\n+\ttest_i18ngrep \"fatal:.*my-subm\" err &&\n+\tcat >expected <<-EOF &&\n+\t* my-subm ${rev}...0000000:\n+\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n2.28.0\n\n"},{"id":"403498","messageId":"20200812194404.17028-5-shouryashukla.oo@gmail.com","threadId":"53998","inReplyTo":"20200812194404.17028-1-shouryashukla.oo@gmail.com","subject":"[PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-12T19:44:04Z","receivedAt":"2020-08-12T19:44:38Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"From: Prathamesh Chavan \n\nConvert submodule subcommand 'summary' to a builtin and call it via\n'git-submodule.sh'.\n\nThe shell version had to call $diff_cmd twice, once to find the modified\nmodules cared by the user and then again, with that list of modules\nto do various operations for computing the summary of those modules.\nOn the other hand, the C version does not need a second call to\n$diff_cmd since it reuses the module list from the first call to do the\naforementioned tasks.\n\nIn the C version, we use the combination of setting a child process'\nworking directory to the submodule path and then calling\n'prepare_submodule_repo_env()' which also sets the 'GIT_DIR' to '.git',\nso that we can be certain that those spawned processes will not access\nthe superproject's ODB by mistake.\n\nA behavioural difference between the C and the shell version is that the\nshell version outputs two line feeds after the 'git log' output when run\noutside of the tests while the C version outputs one line feed in any\ncase. The reason for this is that the shell version calls log with\n'--pretty=format:' whose output is followed by two echo\ncalls; 'format' does not have \"terminator\" semantics like its 'tformat'\ncounterpart. So, the log output is terminated by a newline only when\ninvoked by the user and not when invoked from the scripts. This results\nin the one & two line feed differences in the shell version.\nOn the other hand, the C version calls log with '--pretty='\nwhich is equivalent to '--pretty:tformat:' which is then\nfollowed by a 'printf(\"\\n\")'. Due to its \"terminator\" semantics the\nlog output is always terminated by newline and hence one line feed in\nany case.\n\nAlso, when we try to pass an option-like argument after a non-option\nargument, for instance:\n\n git submodule summary HEAD --foo-bar\n\n (or)\n\n git submodule summary HEAD --cached\n\nThat argument would be treated like a path to the submodule for which\nthe user is requesting a summary. So, the option ends up having no\neffect. Though, passing '--quiet' is an exception to this:\n\n git submodule summary HEAD --quiet\n\nWhile 'summary' doesn't support '--quiet', we don't get an output for\nthe above command as '--quiet' is treated as a path which means we get\nan output only if a submodule whose path is '--quiet' exists.\n\nThe error message in case of computing a summary for non-existent\nsubmodules in the C version is different from that of the shell version.\nSince the new error message is not marked for translation, change the\n'test_i18ngrep' in t7421.4 to 'grep'.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nMentored-by: Kaartic Sivaraam \nHelped-by: Johannes Schindelin \nSigned-off-by: Prathamesh Chavan \nSigned-off-by: Shourya Shukla \n---\n builtin/submodule--helper.c | 429 +++++++++++++++++++++++++++++++\n git-submodule.sh | 186 +-------------\n t/t7421-submodule-summary-add.sh | 2 +-\n 3 files changed, 431 insertions(+), 186 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a03dc84ea4..63ea39025d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -927,6 +927,434 @@ 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 files: 1;\n+\tint summary_limit;\n+};\n+#define SUMMARY_CB_INIT { 0, NULL, NULL, 0, 0, 0, 0 }\n+\n+enum diff_cmd {\n+\tDIFF_INDEX,\n+\tDIFF_FILES\n+};\n+\n+static char* verify_submodule_committish(const char *sm_path,\n+\t\t\t\t\t const char *committish)\n+{\n+\tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n+\tstruct strbuf result = STRBUF_INIT;\n+\n+\tcp_rev_parse.git_cmd = 1;\n+\tcp_rev_parse.dir = sm_path;\n+\tprepare_submodule_repo_env(&cp_rev_parse.env_array);\n+\tstrvec_pushl(&cp_rev_parse.args, \"rev-parse\", \"-q\", \"--short\", NULL);\n+\tstrvec_pushf(&cp_rev_parse.args, \"%s^0\", committish);\n+\tstrvec_push(&cp_rev_parse.args, \"--\");\n+\n+\tif (capture_command(&cp_rev_parse, &result, 0))\n+\t\treturn NULL;\n+\n+\tstrbuf_trim_trailing_newline(&result);\n+\treturn strbuf_detach(&result, NULL);\n+}\n+\n+static void print_submodule_summary(struct summary_cb *info, char* errmsg,\n+\t\t\t\t int total_commits, const char *displaypath,\n+\t\t\t\t const char *src_abbrev, const char *dst_abbrev,\n+\t\t\t\t int missing_src, int missing_dst,\n+\t\t\t\t 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, src_abbrev, dst_abbrev);\n+\t\telse\n+\t\t\tprintf(_(\"* %s %s(submodule)->%s(blob)\"),\n+\t\t\t\t displaypath, src_abbrev, dst_abbrev);\n+\t} else {\n+\t\tprintf(\"* %s %s...%s\",\n+\t\t\tdisplaypath, src_abbrev, dst_abbrev);\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\tprintf(_(\"%s\"), errmsg);\n+\t} else if (total_commits > 0) {\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\tstrvec_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\tstrvec_pushf(&cp_log.args, \"-%d\",\n+\t\t\t\t\t info->summary_limit);\n+\n+\t\t\tstrvec_pushl(&cp_log.args, \"--pretty= %m %s\",\n+\t\t\t\t \"--first-parent\", NULL);\n+\t\t\tstrvec_pushf(&cp_log.args, \"%s...%s\",\n+\t\t\t\t src_abbrev, dst_abbrev);\n+\t\t} else if (S_ISGITLINK(p->mod_dst)) {\n+\t\t\tstrvec_pushl(&cp_log.args, \"--pretty= > %s\",\n+\t\t\t\t \"-1\", dst_abbrev, NULL);\n+\t\t} else {\n+\t\t\tstrvec_pushl(&cp_log.args, \"--pretty= < %s\",\n+\t\t\t\t \"-1\", src_abbrev, 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+\tchar *displaypath, *src_abbrev, *dst_abbrev;\n+\tint missing_src = 0, missing_dst = 0;\n+\tchar *errmsg = NULL;\n+\tint total_commits = -1;\n+\n+\tif (!info->cached && oideq(&p->oid_dst, &null_oid)) {\n+\t\tif (S_ISGITLINK(p->mod_dst)) {\n+\t\t\tstruct ref_store *refs = get_submodule_ref_store(p->sm_path);\n+\t\t\tif (refs)\n+\t\t\t\trefs_head_ref(refs, handle_submodule_head_ref, &p->oid_dst);\n+\t\t} else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {\n+\t\t\tstruct stat st;\n+\t\t\tint fd = open(p->sm_path, O_RDONLY);\n+\n+\t\t\tif (fd < 0 || fstat(fd, &st) < 0 ||\n+\t\t\t index_fd(&the_index, &p->oid_dst, fd, &st, OBJ_BLOB,\n+\t\t\t\t p->sm_path, 0))\n+\t\t\t\terror(_(\"couldn't hash object from '%s'\"), p->sm_path);\n+\t\t} else {\n+\t\t\t/* for a submodule removal (mode:0000000), don't warn */\n+\t\t\tif (p->mod_dst)\n+\t\t\t\twarning(_(\"unexpected mode %d\\n\"), p->mod_dst);\n+\t\t}\n+\t}\n+\n+\tif (S_ISGITLINK(p->mod_src)) {\n+\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n+\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n+\t\tif (!src_abbrev) {\n+\t\t\tmissing_src = 1;\n+\t\t\t/*\n+\t\t\t * As `rev-parse` failed, we fallback to getting\n+\t\t\t * the abbreviated hash using oid_src. We do\n+\t\t\t * this as we might still need the abbreviated\n+\t\t\t * hash in cases like a submodule type change, etc.\n+\t\t\t */\n+\t\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n+\t\t}\n+\t} else {\n+\t\t/*\n+\t\t * The source does not point to a submodule.\n+\t\t * So, we fallback to getting the abbreviation using\n+\t\t * oid_src as we might still need the abbreviated\n+\t\t * hash in cases like submodule add, etc.\n+\t\t */\n+\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n+\t}\n+\n+\tif (S_ISGITLINK(p->mod_dst)) {\n+\t\tdst_abbrev = verify_submodule_committish(p->sm_path,\n+\t\t\t\t\t\t\t oid_to_hex(&p->oid_dst));\n+\t\tif (!dst_abbrev) {\n+\t\t\tmissing_dst = 1;\n+\t\t\t/*\n+\t\t\t * As `rev-parse` failed, we fallback to getting\n+\t\t\t * the abbreviated hash using oid_dst. We do\n+\t\t\t * this as we might still need the abbreviated\n+\t\t\t * hash in cases like a submodule type change, etc.\n+\t\t\t */\n+\t\t\tdst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n+\t\t}\n+\t} else {\n+\t\t/*\n+\t\t * The destination does not point to a submodule.\n+\t\t * So, we fallback to getting the abbreviation using\n+\t\t * oid_dst as we might still need the abbreviated\n+\t\t * hash in cases like a submodule removal, etc.\n+\t\t */\n+\t\tdst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n+\t}\n+\n+\tdisplaypath = get_submodule_displaypath(p->sm_path, info->prefix);\n+\n+\tif (!missing_src && !missing_dst) {\n+\t\tstruct child_process cp_rev_list = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf sb_rev_list = STRBUF_INIT;\n+\n+\t\tstrvec_pushl(&cp_rev_list.args, \"rev-list\",\n+\t\t\t \"--first-parent\", \"--count\", NULL);\n+\t\tif (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))\n+\t\t\tstrvec_pushf(&cp_rev_list.args, \"%s...%s\",\n+\t\t\t\t src_abbrev, dst_abbrev);\n+\t\telse\n+\t\t\tstrvec_push(&cp_rev_list.args, S_ISGITLINK(p->mod_src) ?\n+\t\t\t\t src_abbrev : dst_abbrev);\n+\t\tstrvec_push(&cp_rev_list.args, \"--\");\n+\n+\t\tcp_rev_list.git_cmd = 1;\n+\t\tcp_rev_list.dir = p->sm_path;\n+\t\tprepare_submodule_repo_env(&cp_rev_list.env_array);\n+\n+\t\tif (!capture_command(&cp_rev_list, &sb_rev_list, 0))\n+\t\t\ttotal_commits = atoi(sb_rev_list.buf);\n+\n+\t\tstrbuf_release(&sb_rev_list);\n+\t} else {\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_dst)) {\n+\t\t\tstruct strbuf errmsg_str = STRBUF_INIT;\n+\t\t\tif (missing_src && missing_dst) {\n+\t\t\t\tstrbuf_addf(&errmsg_str, \" Warn: %s doesn't contain commits %s and %s\\n\",\n+\t\t\t\t\t displaypath, oid_to_hex(&p->oid_src),\n+\t\t\t\t\t oid_to_hex(&p->oid_dst));\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addf(&errmsg_str, \" Warn: %s doesn't contain commit %s\\n\",\n+\t\t\t\t\t displaypath, missing_src ?\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\t}\n+\t\t\terrmsg = strbuf_detach(&errmsg_str, NULL);\n+\t\t}\n+\t}\n+\n+\tprint_submodule_summary(info, errmsg, total_commits,\n+\t\t\t\tdisplaypath, src_abbrev,\n+\t\t\t\tdst_abbrev, missing_src,\n+\t\t\t\tmissing_dst, p);\n+\n+\tfree(displaypath);\n+\tfree(src_abbrev);\n+\tfree(dst_abbrev);\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\tconst struct submodule *sub;\n+\t\tstruct module_cb *p = list->entries[i];\n+\t\tstruct strbuf sm_gitdir = STRBUF_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 && p->status != 'A' &&\n+\t\t (sub = submodule_from_path(the_repository,\n+\t\t\t\t\t &null_oid, p->sm_path))) {\n+\t\t\tchar *config_key = NULL;\n+\t\t\tconst char *value;\n+\t\t\tint ignore_all = 0;\n+\n+\t\t\tconfig_key = xstrfmt(\"submodule.%s.ignore\",\n+\t\t\t\t\t sub->name);\n+\t\t\tif (!git_config_get_string_const(config_key, &value))\n+\t\t\t\tignore_all = !strcmp(value, \"all\");\n+\t\t\telse if (sub->ignore)\n+\t\t\t\tignore_all = !strcmp(sub->ignore, \"all\");\n+\n+\t\t\tfree(config_key);\n+\t\t\tif (ignore_all)\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* Also show added or modified modules which are checked out */\n+\t\tstrbuf_addstr(&sm_gitdir, p->sm_path);\n+\t\tif (is_nonbare_repository_dir(&sm_gitdir))\n+\t\t\tgenerate_submodule_summary(info, p);\n+\t\tstrbuf_release(&sm_gitdir);\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(struct object_id *head_oid,\n+\t\t\t\t struct summary_cb *info,\n+\t\t\t\t enum diff_cmd diff_cmd)\n+{\n+\tstruct strvec diff_args = STRVEC_INIT;\n+\tstruct rev_info rev;\n+\tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n+\n+\tstrvec_push(&diff_args, get_diff_cmd(diff_cmd));\n+\tif (info->cached)\n+\t\tstrvec_push(&diff_args, \"--cached\");\n+\tstrvec_pushl(&diff_args, \"--ignore-submodules=dirty\", \"--raw\", NULL);\n+\tif (head_oid)\n+\t\tstrvec_push(&diff_args, oid_to_hex(head_oid));\n+\tstrvec_push(&diff_args, \"--\");\n+\tif (info->argc)\n+\t\tstrvec_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.nr, diff_args.v);\n+\tsetup_revisions(diff_args.nr, diff_args.v, &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+\tstrvec_clear(&diff_args);\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 files = 0;\n+\tint summary_limit = -1;\n+\tenum diff_cmd diff_cmd = DIFF_INDEX;\n+\tstruct object_id head_oid;\n+\tint ret;\n+\n+\tstruct option module_summary_options[] = {\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 [] [commit] [--] []\"),\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+\tif (!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\tif (get_oid(\"HEAD\", &head_oid))\n+\t\t\tdie(_(\"could not fetch a revision for 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.files = !!files;\n+\tinfo.for_status = !!for_status;\n+\tinfo.summary_limit = summary_limit;\n+\n+\tret = compute_summary_module_list((diff_cmd == DIFF_INDEX) ? &head_oid : NULL,\n+\t\t\t\t\t &info, diff_cmd);\n+\treturn ret;\n+}\n+\n struct sync_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n@@ -2341,6 +2769,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..6fb12585cb 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 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 ${prefix:+--prefix \"$prefix\"} ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- \"$@\"\n }\n #\n # List all submodules, prefixed with:\ndiff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh\nindex 829fe26d6d..59a9b00467 100755\n--- a/t/t7421-submodule-summary-add.sh\n+++ b/t/t7421-submodule-summary-add.sh\n@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n \tgit commit -m \"change submodule path\" &&\n \trev=$(git -C sm rev-parse --short HEAD^) &&\n \tgit submodule summary HEAD^^ -- my-subm >actual 2>err &&\n-\ttest_i18ngrep \"fatal:.*my-subm\" err &&\n+\tgrep \"fatal:.*my-subm\" err &&\n \tcat >expected <<-EOF &&\n \t* my-subm ${rev}...0000000:\n \n-- \n2.28.0\n\n"},{"id":"403911","messageId":"20200818020838.GA1872632@coredump.intra.peff.net","threadId":"53998","inReplyTo":"20200812194404.17028-5-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-18T02:08:38Z","receivedAt":"2020-08-18T02:08:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 13, 2020 at 01:14:04AM +0530, Shourya Shukla wrote:\n\n> +static void print_submodule_summary(struct summary_cb *info, char* errmsg,\n> +\t\t\t\t int total_commits, const char *displaypath,\n> +\t\t\t\t const char *src_abbrev, const char *dst_abbrev,\n> +\t\t\t\t int missing_src, int missing_dst,\n> +\t\t\t\t struct module_cb *p)\n\nThe \"missing_src\" and \"missing_dst\" parameters in this function are\nunused.\n\nI _think_ they can be safely removed, and are not a sign of a bug. We\nseem to fully handle them in the calling function. But this is the first\ntime I looked at the code, and I didn't dig too deeply.\n\n-Peff\n"},{"id":"404121","messageId":"20200821052235.GA84497@konoha","threadId":"53998","inReplyTo":"20200818020838.GA1872632@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-21T05:22:35Z","receivedAt":"2020-08-21T05:22:49Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"> The \"missing_src\" and \"missing_dst\" parameters in this function are\n> unused.\n\nYes, they are unused.\n\n> I _think_ they can be safely removed, and are not a sign of a bug. We\n> seem to fully handle them in the calling function. But this is the first\n> time I looked at the code, and I didn't dig too deeply.\n\nSure. They an be removed. I am sending a patch for the same.\n\n"},{"id":"404142","messageId":"nycvar.QRO.7.76.6.2008211708280.56@tvgsbejvaqbjf.bet","threadId":"53998","inReplyTo":"20200812194404.17028-5-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-21T15:17:42Z","receivedAt":"2020-08-21T15:19:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\nOn Thu, 13 Aug 2020, Shourya Shukla wrote:\n\n> [...]\n> diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh\n> index 829fe26d6d..59a9b00467 100755\n> --- a/t/t7421-submodule-summary-add.sh\n> +++ b/t/t7421-submodule-summary-add.sh\n> @@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n> \tgit commit -m \"change submodule path\" &&\n> \trev=$(git -C sm rev-parse --short HEAD^) &&\n> \tgit submodule summary HEAD^^ -- my-subm >actual 2>err &&\n> -\ttest_i18ngrep \"fatal:.*my-subm\" err &&\n> +\tgrep \"fatal:.*my-subm\" err &&\n\nSadly, this breaks on Windows: on Linux (and before this patch, also on\nWindows), the error message reads somewhat like this:\n\n\tfatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n\nHowever, with the built-in `git submodule summary`, on Windows the error\nmessage reads like this:\n\n\terror: cannot spawn git: No such file or directory\n\nNow, this is of course not the best way to present this error message, but\nplease note that even providing a better error message does not fix the\nerroneous expectation of the `fatal:` prefix (Git typically produces this\nwhen `die()`ing, which can be done in the POSIX version that uses `fork()`\nand `exec()` but not in the Windows version that needs to use\n`CreateProcessW()` instead).\n\nTherefore, I propose this patch on top:\n\n-- snipsnap --\n[PATCH] mingw: mention if `mingw_spawnve()` failed due to a missing directory\n\nWhen we recently converted the `summary` subcommand of `git submodule`\nto be mostly built-in, a bug was uncovered where a very unhelpful error\nmessage was produced when a process could not be spawned because the\ndirectory in which it was supposed to be run does not exist.\n\nEven so, we _still_ have to adjust the `git submodule summary` test, to\naccommodate for the fact that the `mingw_spawnve()` function will return\nwith an error instead of `die()`ing.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 4 ++++\n t/t7421-submodule-summary-add.sh | 2 +-\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 1a64d4efb26b..3c30d0cab589 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1850,6 +1850,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n \t/* Make sure to override previous errors, if any */\n \terrno = 0;\n\n+\tif (dir && !is_directory(dir))\n+\t\treturn error_errno(_(\"could not exec '%s' in '%s'\"),\n+\t\t\t\t argv[0], dir);\n+\n \tif (restrict_handle_inheritance < 0)\n \t\trestrict_handle_inheritance = core_restrict_inherited_handles;\n \t/*\ndiff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh\nindex 59a9b00467dc..f00d69ca29ea 100755\n--- a/t/t7421-submodule-summary-add.sh\n+++ b/t/t7421-submodule-summary-add.sh\n@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n \tgit commit -m \"change submodule path\" &&\n \trev=$(git -C sm rev-parse --short HEAD^) &&\n \tgit submodule summary HEAD^^ -- my-subm >actual 2>err &&\n-\tgrep \"fatal:.*my-subm\" err &&\n+\tgrep \"my-subm\" err &&\n \tcat >expected <<-EOF &&\n \t* my-subm ${rev}...0000000:\n\n--\n2.28.0.windows.1\n\n"},{"id":"404147","messageId":"xmqqimdc9cuc.fsf@gitster.c.googlers.com","threadId":"53998","inReplyTo":"nycvar.QRO.7.76.6.2008211708280.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-21T16:35:07Z","receivedAt":"2020-08-21T16:35:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Sadly, this breaks on Windows: on Linux (and before this patch, also on\n> Windows), the error message reads somewhat like this:\n>\n> \tfatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n>\n> However, with the built-in `git submodule summary`, on Windows the error\n> message reads like this:\n>\n> \terror: cannot spawn git: No such file or directory\n\nI think a test that relies on platform-specific error string is a\nbug. It's like expecting an exact string out of strerror(), which\nwe had to fix a few times.\n\nSo I am not sure we would want to butcher compat/mingw.c only to\nmatch such an expectation by a (buggy) test.\n\n"},{"id":"404165","messageId":"20200821171705.GA16484@konoha","threadId":"53998","inReplyTo":"xmqqimdc9cuc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-21T17:17:05Z","receivedAt":"2020-08-21T17:19:04Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"> I think a test that relies on platform-specific error string is a\n> bug. It's like expecting an exact string out of strerror(), which\n> we had to fix a few times.\n\n> So I am not sure we would want to butcher compat/mingw.c only to\n> match such an expectation by a (buggy) test.\n\nAlright. That is understandable. What alternative do you suggest? Should\nwe change the check in the test?\n\n"},{"id":"404175","messageId":"xmqq5z9ban27.fsf@gitster.c.googlers.com","threadId":"53998","inReplyTo":"20200821171705.GA16484@konoha","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-21T18:09:04Z","receivedAt":"2020-08-21T18:09:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n>> I think a test that relies on platform-specific error string is a\n>> bug. It's like expecting an exact string out of strerror(), which\n>> we had to fix a few times.\n>\n>> So I am not sure we would want to butcher compat/mingw.c only to\n>> match such an expectation by a (buggy) test.\n>\n> Alright. That is understandable. What alternative do you suggest? Should\n> we change the check in the test?\n\nA buggy check should of course be changed.\n\nIt should be sufficient to ensure \"git submodule summary\" fails,\nregardless of what exact error message it issues, no?\n\nIf the command does not exit with non-zero exit status, when it\ngives a \"fatal\" error message, that may indicate another bug,\nthough.\n"},{"id":"404189","messageId":"377b1a2ad60c5ca30864f48c5921ff89b5aca65b.camel@gmail.com","threadId":"53998","inReplyTo":"xmqq5z9ban27.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-21T18:54:01Z","receivedAt":"2020-08-21T18:55:15Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Fri, 2020-08-21 at 11:09 -0700, Junio C Hamano wrote:\n> Shourya Shukla writes:\n> \n> > > I think a test that relies on platform-specific error string is a\n> > > bug. It's like expecting an exact string out of strerror(),\n> > > which\n> > > we had to fix a few times.\n> > > So I am not sure we would want to butcher compat/mingw.c only to\n> > > match such an expectation by a (buggy) test.\n> > \n> > Alright. That is understandable. What alternative do you suggest?\n> > Should\n> > we change the check in the test?\n> \n> A buggy check should of course be changed.\n> \n> It should be sufficient to ensure \"git submodule summary\" fails,\n> regardless of what exact error message it issues, no?\n> \n\nUnfortunately, we can't do that here. See below.\n\n> If the command does not exit with non-zero exit status, when it\n> gives a \"fatal\" error message, that may indicate another bug,\n> though.\n\nHere's the error message with context of the trash directory of that\ntest:\n\n-- 8< --\n$ cd t\n$ ./t7421-submodule-summary-add.sh -d\n$ cd trash\\ directory.t7421-submodule-summary-add/\n\n$ git submodule summary HEAD^^\nfatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n* my-subm 35b40f1...0000000:\n\n* subm 0000000...dbd5fc8 (2):\n > add file2\n\n-- >8 --\n\nThat 'fatal' is a consequence of spawning a process in\n`verify_submodule_committish` of `builtin/submodule--helper.c` even for\nnon-existent submodules. I don't think that 'fatal: ' message is giving\nany useful information here. The fact that submodule 'my-subm' doesn't\nexist can easily be inferred just by looking at the destination mode\n(0000000). If anything that 'fatal' message is just confusing and\nunnecessary, IMO.\n\nSo, we could easily suppress it by doing something like this (while\nalso fixing the test):\n\n-- 8< --\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 63ea39025d..d45be7fbdf 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -972,7 +972,7 @@ static char* verify_submodule_committish(const char *sm_path,\n strvec_pushf(&cp_rev_parse.args, \"%s^0\", committish);\n strvec_push(&cp_rev_parse.args, \"--\");\n \n- if (capture_command(&cp_rev_parse, &result, 0))\n+ if (!is_directory(sm_path) || capture_command(&cp_rev_parse, &result, 0))\n return NULL;\n \n strbuf_trim_trailing_newline(&result);\ndiff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh\nindex 59a9b00467..8a2c2b38b6 100755\n--- a/t/t7421-submodule-summary-add.sh\n+++ b/t/t7421-submodule-summary-add.sh\n@@ -58,7 +58,6 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n git commit -m \"change submodule path\" &&\n rev=$(git -C sm rev-parse --short HEAD^) &&\n git submodule summary HEAD^^ -- my-subm >actual 2>err &&\n- grep \"fatal:.*my-subm\" err &&\n cat >expected <<-EOF &&\n * my-subm ${rev}...0000000:\n \n-- >8 --\n\nBTW, I noted that `print_submodule_summary` has the following\ndefinition:\n\n static void print_submodule_summary(struct summary_cb *info, char* errmsg\n \t\t\t\t ...\n\nNote how '*' is placed near 'char' for 'errmsg' with an incorrect style. Ditto\nfor the return type of `verify_submodule_committish`. This might make\nfor a nice cleanup patch.\n\n-- \nSivaraam\n\n\n"},{"id":"404197","messageId":"xmqqa6yn93ll.fsf@gitster.c.googlers.com","threadId":"53998","inReplyTo":"377b1a2ad60c5ca30864f48c5921ff89b5aca65b.camel@gmail.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-21T19:54:46Z","receivedAt":"2020-08-21T19:55:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n> Here's the error message with context of the trash directory of that\n> test:\n>\n> -- 8< --\n> $ cd t\n> $ ./t7421-submodule-summary-add.sh -d\n> $ cd trash\\ directory.t7421-submodule-summary-add/\n>\n> $ git submodule summary HEAD^^\n> fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n> * my-subm 35b40f1...0000000:\n>\n> * subm 0000000...dbd5fc8 (2):\n> > add file2\n>\n> -- >8 --\n>\n> That 'fatal' is a consequence of spawning a process in\n> `verify_submodule_committish` of `builtin/submodule--helper.c` even for\n> non-existent submodules.\n\nOh, so doing something that would cause the error message to be\nemitted itself is a bug.\n\n> I don't think that 'fatal: ' message is giving\n> any useful information here. The fact that submodule 'my-subm' doesn't\n> exist can easily be inferred just by looking at the destination mode\n> (0000000). If anything that 'fatal' message is just confusing and\n> unnecessary, IMO.\n\nYes, I 100% agree.\n\n> So, we could easily suppress it by doing something like this (while\n> also fixing the test):\n\nYup. That is a very good idea. \n\nOr the caller of verify_submodule_committish() should refrain from\ncalling it for the path? After all, it is checking sm_path is a\npath to where a submodule should be before calling the function\n(instead of calling it for every random path), iow its criteria to\nmake the call currently may be \"the path in the index says it is a\nsubmodule\", but it should easily be updated to \"the path in the\nindex says it is a submodule, and the submodule actually is\npopulated\", right?\n\n> @@ -972,7 +972,7 @@ static char* verify_submodule_committish(const char *sm_path,\n> strvec_pushf(&cp_rev_parse.args, \"%s^0\", committish);\n> ...\n> BTW, I noted that `print_submodule_summary` has the following\n> definition:\n>\n> static void print_submodule_summary(struct summary_cb *info, char* errmsg\n> \t\t\t\t ...\n>\n> Note how '*' is placed near 'char' for 'errmsg' with an incorrect style. Ditto\n> for the return type of `verify_submodule_committish`. This might make\n> for a nice cleanup patch.\n\nYup. It would have been nicer to catch these before the topic hit\n'next'.\n\nThanks.\n"},{"id":"404299","messageId":"5b6ed82f3ab58a194bf51c0e2905214f64246ad8.camel@gmail.com","threadId":"53998","inReplyTo":"xmqqa6yn93ll.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-23T20:03:17Z","receivedAt":"2020-08-23T20:03:27Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Fri, 2020-08-21 at 12:54 -0700, Junio C Hamano wrote:\n> Kaartic Sivaraam writes:\n> \n> > Here's the error message with context of the trash directory of that\n> > test:\n> > \n> > -- 8< --\n> > $ cd t\n> > $ ./t7421-submodule-summary-add.sh -d\n> > $ cd trash\\ directory.t7421-submodule-summary-add/\n> > \n> > $ git submodule summary HEAD^^\n> > fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n> > * my-subm 35b40f1...0000000:\n> > \n> > * subm 0000000...dbd5fc8 (2):\n> > > add file2\n> > \n> > -- >8 --\n> > \n> > That 'fatal' is a consequence of spawning a process in\n> > `verify_submodule_committish` of `builtin/submodule--helper.c` even for\n> > non-existent submodules.\n> \n> Oh, so doing something that would cause the error message to be\n> emitted itself is a bug.\n> \n\nExactly.\n\n> > I don't think that 'fatal: ' message is giving\n> > any useful information here. The fact that submodule 'my-subm' doesn't\n> > exist can easily be inferred just by looking at the destination mode\n> > (0000000). If anything that 'fatal' message is just confusing and\n> > unnecessary, IMO.\n> \n> Yes, I 100% agree.\n> \n> > So, we could easily suppress it by doing something like this (while\n> > also fixing the test):\n> \n> Yup. That is a very good idea. \n> \n> Or the caller of verify_submodule_committish() should refrain from\n> calling it for the path? After all, it is checking sm_path is a\n> path to where a submodule should be before calling the function\n> (instead of calling it for every random path), iow its criteria to\n> make the call currently may be \"the path in the index says it is a\n> submodule\", but it should easily be updated to \"the path in the\n> index says it is a submodule, and the submodule actually is\n> populated\", right?\n> \n\nAh, this reminds me of the initial version of the patch which did\nexactly that. Quoting it here for reference:\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\nNote: `verify_submodule_object_name` is now renamed to\n`verify_submodule_committish`.\n\nThat does sound like a sane approach to me. There's not much point in\ninvoking `rev-parse` in a non-populated (a.k.a. de-initialized) or non-\nexistent submodule but we removed that check as we thought it was\nunnecessary redundant because `capture_command` would fail anyway.\nLooks like we failed to notice the additional `fatal` message fallout\nthen.\n\nAlso, I think it would be better to something like the following in\nt7421 to ensure that `fatal` doesn't sneak up accidentally in the\nfuture:\n\n-- 8< --\ndiff --git t/t7421-submodule-summary-add.sh t/t7421-submodule-summary-add.sh\nindex 59a9b00467..b070f13714 100755\n--- t/t7421-submodule-summary-add.sh\n+++ t/t7421-submodule-summary-add.sh\n@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n git commit -m \"change submodule path\" &&\n rev=$(git -C sm rev-parse --short HEAD^) &&\n git submodule summary HEAD^^ -- my-subm >actual 2>err &&\n- grep \"fatal:.*my-subm\" err &&\n+ test_must_be_empty err &&\n cat >expected <<-EOF &&\n * my-subm ${rev}...0000000:\n \n-- >8 --\n\n-- \nSivaraam\n\n\n"},{"id":"404300","messageId":"450b1ca28419afec12dc81a04f2c1ce6edfb3943.camel@gmail.com","threadId":"53998","inReplyTo":"5b6ed82f3ab58a194bf51c0e2905214f64246ad8.camel@gmail.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-23T20:12:01Z","receivedAt":"2020-08-23T20:12:10Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Mon, 2020-08-24 at 01:33 +0530, Kaartic Sivaraam wrote:\n> On Fri, 2020-08-21 at 12:54 -0700, Junio C Hamano wrote:\n> > Kaartic Sivaraam writes:\n> > \n> > > So, we could easily suppress it by doing something like this (while\n> > > also fixing the test):\n> > \n> > Yup. That is a very good idea. \n> > \n> > Or the caller of verify_submodule_committish() should refrain from\n> > calling it for the path? After all, it is checking sm_path is a\n> > path to where a submodule should be before calling the function\n> > (instead of calling it for every random path), iow its criteria to\n> > make the call currently may be \"the path in the index says it is a\n> > submodule\", but it should easily be updated to \"the path in the\n> > index says it is a submodule, and the submodule actually is\n> > populated\", right?\n> > \n> \n> Ah, this reminds me of the initial version of the patch which did\n> exactly that. Quoting it here for reference:\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> \n> Note: `verify_submodule_object_name` is now renamed to\n> `verify_submodule_committish`.\n> \n> That does sound like a sane approach to me. There's not much point in\n> invoking `rev-parse` in a non-populated (a.k.a. de-initialized) or non-\n> existent submodule but we removed that check as we thought it was\n> unnecessary redundant because `capture_command` would fail anyway.\n> Looks like we failed to notice the additional `fatal` message fallout\n> then.\n> \n\nHere's a link to the start of the relevant discussion, just in case:\n\nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2007031712160.50@tvgsbejvaqbjf.bet/\n\n> Also, I think it would be better to something like the following in\n> t7421 to ensure that `fatal` doesn't sneak up accidentally in the\n> future:\n> \n> -- 8< --\n> diff --git t/t7421-submodule-summary-add.sh t/t7421-submodule-summary-add.sh\n> index 59a9b00467..b070f13714 100755\n> --- t/t7421-submodule-summary-add.sh\n> +++ t/t7421-submodule-summary-add.sh\n> @@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n> git commit -m \"change submodule path\" &&\n> rev=$(git -C sm rev-parse --short HEAD^) &&\n> git submodule summary HEAD^^ -- my-subm >actual 2>err &&\n> - grep \"fatal:.*my-subm\" err &&\n> + test_must_be_empty err &&\n> cat >expected <<-EOF &&\n> * my-subm ${rev}...0000000:\n> \n> -- >8 --\n> \n\n\n-- \nSivaraam\n\n\n"},{"id":"404307","messageId":"20200824072633.GA38870@konoha","threadId":"53998","inReplyTo":"5b6ed82f3ab58a194bf51c0e2905214f64246ad8.camel@gmail.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-24T07:26:33Z","receivedAt":"2020-08-24T07:26:44Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 24/08 01:33, Kaartic Sivaraam wrote:\n> > Or the caller of verify_submodule_committish() should refrain from\n> > calling it for the path? After all, it is checking sm_path is a\n> > path to where a submodule should be before calling the function\n> > (instead of calling it for every random path), iow its criteria to\n> > make the call currently may be \"the path in the index says it is a\n> > submodule\", but it should easily be updated to \"the path in the\n> > index says it is a submodule, and the submodule actually is\n> > populated\", right?\n> > \n> \n> Ah, this reminds me of the initial version of the patch which did\n> exactly that. Quoting it here for reference:\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> \n> Note: `verify_submodule_object_name` is now renamed to\n> `verify_submodule_committish`.\n> \n> That does sound like a sane approach to me. There's not much point in\n> invoking `rev-parse` in a non-populated (a.k.a. de-initialized) or non-\n> existent submodule but we removed that check as we thought it was\n> unnecessary redundant because `capture_command` would fail anyway.\n> Looks like we failed to notice the additional `fatal` message fallout\n> then.\n\nThis is what I have tried to implement after your suggestion:\n\n-----8<-----\nstrbuf_addstr(&sb, p->sm_path);\n\tif (is_nonbare_repository_dir(&sb) && S_ISGITLINK(p->mod_src)) {\n\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n\t\tif (!src_abbrev) {\n\t\t\tmissing_src = 1;\n\t\t\t/*\n\t\t\t * As `rev-parse` failed, we fallback to getting\n\t\t\t * the abbreviated hash using oid_src. We do\n\t\t\t * this as we might still need the abbreviated\n\t\t\t * hash in cases like a submodule type change, etc.\n\t\t\t */\n\t\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n\t\t}\n\t} else {\n\t\t/*\n\t\t * The source does not point to a submodule.\n\t\t * So, we fallback to getting the abbreviation using\n\t\t * oid_src as we might still need the abbreviated\n\t\t * hash in cases like submodule add, etc.\n\t\t */\n\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n\t}\n\n\tif (is_nonbare_repository_dir(&sb) && S_ISGITLINK(p->mod_dst)) {\n\t\tdst_abbrev = verify_submodule_committish(p->sm_path,\n\t\t\t\t\t\t\t oid_to_hex(&p->oid_dst));\n\t\tif (!dst_abbrev) {\n\t\t\tmissing_dst = 1;\n\t\t\t/*\n\t\t\t * As `rev-parse` failed, we fallback to getting\n\t\t\t * the abbreviated hash using oid_dst. We do\n\t\t\t * this as we might still need the abbreviated\n\t\t\t * hash in cases like a submodule type change, etc.\n\t\t\t */\n\t\t\tdst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n\t\t}\n\t} else {\n\t\t/*\n\t\t * The destination does not point to a submodule.\n\t\t * So, we fallback to getting the abbreviation using\n\t\t * oid_dst as we might still need the abbreviated\n\t\t * hash in cases like a submodule removal, etc.\n\t\t */\n\t\tdst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n\t}\n----->8-----\n\nThat is, add another check along with the 'S_ISGITLINK()' one. Now, the\nthing is that 'rev-list' (called just after this part) starts to bother\nand comes up with its own 'fatal' that the directory rev-list does not\nexist.\n\nThe thing is that 'missing{src,dst}' should be set to 1 in two cases:\n\n 1. If the hash is not found, i.e, when 'verify_submodule..()'\n returns a NULL. Something which is happening right now as well.\n\n 2. If the SM is not reachable for some reason (maybe it does not\n exist like in our case). Something which is NOT happening right\n now.\n\nOr if having the same variable denote two things does not please you,\nthen we can create another variable for the second check BUT, we will\nhave to incorporate checking of that variable in the \n\n-----8<-----\nif (!missing_src && !missing_dst) {\n\t\tstruct child_process cp_rev_list = CHILD_PROCESS_INIT;\n\t\tstruct strbuf sb_rev_list = STRBUF_INIT;\n\n\t\tstrvec_pushl(&cp_rev_list.args, \"rev-list\",\n\t\t\t \"--first-parent\", \"--count\", NULL);\n .........\n----->8-----\n\ncheck.\n\nThis way, we hit two birds with one stone:\n\n 1. Bypass the 'verify_submodule..()' call when the SM directory does\n not exist. We can then remove the is_directory() test from the\n 'verify_submodule_..()' function.\n\n 2. Avoid a 'rev-{parse,list}' fatal error message and thus pass all\n the tests successfully.\n\nTherefore, the final outcome is something like this:\n\n-----8<-----\n\tif (is_directory(p->sm_path) && S_ISGITLINK(p->mod_src)) {\n\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n\t\tif (!src_abbrev) {\n\t\t\tmissing_src = 1;\n\t\t\t/*\n\t\t\t * As `rev-parse` failed, we fallback to getting\n\t\t\t * the abbreviated hash using oid_src. We do\n\t\t\t * this as we might still need the abbreviated\n\t\t\t * hash in cases like a submodule type change, etc.\n\t\t\t */\n\t\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n\t\t}\n\t} else {\n\t\tmissing_src = 1;\n\t\t/*\n\t\t * The source does not point to a submodule.\n\t\t * So, we fallback to getting the abbreviation using\n\t\t * oid_src as we might still need the abbreviated\n\t\t * hash in cases like submodule add, etc.\n\t\t */\n\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n\t}\n\n\tif (is_directory(p->sm_path) && S_ISGITLINK(p->mod_dst)) {\n\t\tdst_abbrev = verify_submodule_committish(p->sm_path,\n\t\t\t\t\t\t\t oid_to_hex(&p->oid_dst));\n\t\tif (!dst_abbrev) {\n\t\t\tmissing_dst = 1;\n\t\t\t/*\n\t\t\t * As `rev-parse` failed, we fallback to getting\n\t\t\t * the abbreviated hash using oid_dst. We do\n\t\t\t * this as we might still need the abbreviated\n\t\t\t * hash in cases like a submodule type change, etc.\n\t\t\t */\n\t\t\tdst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n\t\t}\n\t} else {\n\t\tmissing_dst = 1;\n\t\t/*\n\t\t * The destination does not point to a submodule.\n\t\t * So, we fallback to getting the abbreviation using\n\t\t * oid_dst as we might still need the abbreviated\n\t\t * hash in cases like a submodule removal, etc.\n\t\t */\n\t\tdst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);\n\t}\n----->8-----\n\nOr if is_directory() does not please you then we can make it\n'is_nonbare_..()' too. The outcome will be unchanged.\n\nWhat are your opinions on this?\n\n> Also, I think it would be better to something like the following in\n> t7421 to ensure that `fatal` doesn't sneak up accidentally in the\n> future:\n> \n> -- 8< --\n> diff --git t/t7421-submodule-summary-add.sh t/t7421-submodule-summary-add.sh\n> index 59a9b00467..b070f13714 100755\n> --- t/t7421-submodule-summary-add.sh\n> +++ t/t7421-submodule-summary-add.sh\n> @@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n> git commit -m \"change submodule path\" &&\n> rev=$(git -C sm rev-parse --short HEAD^) &&\n> git submodule summary HEAD^^ -- my-subm >actual 2>err &&\n> - grep \"fatal:.*my-subm\" err &&\n> + test_must_be_empty err &&\n> cat >expected <<-EOF &&\n> * my-subm ${rev}...0000000:\n> \n> -- >8 --\n\nYes, this I will do.\n\n"},{"id":"404311","messageId":"20200824084634.GA377527@konoha","threadId":"53998","inReplyTo":"20200824072633.GA38870@konoha","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-24T08:46:34Z","receivedAt":"2020-08-24T09:39:26Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Or rather, we can do this:\n\n-----8<-----\nif (S_ISGITLINK(p->mod_src)) {\n\t\tstruct strbuf sb = STRBUF_INIT;\n\t\tstrbuf_addstr(&sb, p->sm_path);\n\t\tif (is_nonbare_repository_dir(&sb))\n\t\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n\t\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n\t\tstrbuf_release(&sb);\n\t\tif (!src_abbrev) {\n\t\t\tmissing_src = 1;\n\t\t\t/*\n\t\t\t * As `rev-parse` failed, we fallback to getting\n\t\t\t * the abbreviated hash using oid_src. We do\n\t\t\t * this as we might still need the abbreviated\n\t\t\t * hash in cases like a submodule type change, etc.\n\t\t\t */\n\t\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n\t\t}\n\t} else {\n\t\t/*\n\t\t * The source does not point to a submodule.\n\t\t * So, we fallback to getting the abbreviation using\n\t\t * oid_src as we might still need the abbreviated\n\t\t * hash in cases like submodule add, etc.\n\t\t */\n\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n\t}\n----->8-----\n\nSimilarly for dst as well. This solution passes all the tests and does\nnot call 'verify_submodule_committish()' all the time. The previous\napproach failed a couple of tests, this one seems fine to me.\n\nHow is this one?\n\n"},{"id":"404312","messageId":"a30f43ecbbc5fa64fe62eb5903d81bce7440986c.camel@gmail.com","threadId":"53998","inReplyTo":"20200824084634.GA377527@konoha","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-24T11:08:39Z","receivedAt":"2020-08-24T11:08:52Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Mon, 2020-08-24 at 14:16 +0530, Shourya Shukla wrote:\n> Or rather, we can do this:\n> \n> -----8<-----\n> if (S_ISGITLINK(p->mod_src)) {\n> \t\tstruct strbuf sb = STRBUF_INIT;\n> \t\tstrbuf_addstr(&sb, p->sm_path);\n> \t\tif (is_nonbare_repository_dir(&sb))\n> \t\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n> \t\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n> \t\tstrbuf_release(&sb);\n> \t\tif (!src_abbrev) {\n> \t\t\tmissing_src = 1;\n> \t\t\t/*\n> \t\t\t * As `rev-parse` failed, we fallback to getting\n> \t\t\t * the abbreviated hash using oid_src. We do\n> \t\t\t * this as we might still need the abbreviated\n> \t\t\t * hash in cases like a submodule type change, etc.\n> \t\t\t */\n> \t\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n> \t\t}\n> \t} else {\n> \t\t/*\n> \t\t * The source does not point to a submodule.\n> \t\t * So, we fallback to getting the abbreviation using\n> \t\t * oid_src as we might still need the abbreviated\n> \t\t * hash in cases like submodule add, etc.\n> \t\t */\n> \t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n> \t}\n> ----->8-----\n> \n> Similarly for dst as well. This solution passes all the tests and does\n> not call 'verify_submodule_committish()' all the time. The previous\n> approach failed a couple of tests, this one seems fine to me.\n> \n> How is this one?\n> \n\nThis is more or less what I had in mind initially. But later after\nbeing reminded about the fact that there's a code path which calls\n`generate_submodule_summary` only when `is_nonbare_repository_dir`\nsucceeds, I realized any conditional that uses\n`is_nonbare_repository_dir` or the likes of it would be confusing. So,\nI think a better approach would be something like:\n\n-- 8< --\ndiff --git builtin/submodule--helper.c builtin/submodule--helper.c\nindex 63ea39025d..b490108cd9 100644\n--- builtin/submodule--helper.c\n+++ builtin/submodule--helper.c\n@@ -1036,7 +1036,7 @@ static void print_submodule_summary(struct summary_cb *info, char* errmsg,\n static void generate_submodule_summary(struct summary_cb *info,\n struct module_cb *p)\n {\n- char *displaypath, *src_abbrev, *dst_abbrev;\n+ char *displaypath, *src_abbrev = NULL, *dst_abbrev;\n int missing_src = 0, missing_dst = 0;\n char *errmsg = NULL;\n int total_commits = -1;\n@@ -1062,8 +1062,9 @@ static void generate_submodule_summary(struct summary_cb *info,\n }\n \n if (S_ISGITLINK(p->mod_src)) {\n- src_abbrev = verify_submodule_committish(p->sm_path,\n- oid_to_hex(&p->oid_src));\n+ if (p->status != 'D')\n+ src_abbrev = verify_submodule_committish(p->sm_path,\n+ oid_to_hex(&p->oid_src));\n if (!src_abbrev) {\n missing_src = 1;\n /*\ndiff --git t/t7421-submodule-summary-add.sh t/t7421-submodule-summary-add.sh\nindex 59a9b00467..b070f13714 100755\n--- t/t7421-submodule-summary-add.sh\n+++ t/t7421-submodule-summary-add.sh\n@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n git commit -m \"change submodule path\" &&\n rev=$(git -C sm rev-parse --short HEAD^) &&\n git submodule summary HEAD^^ -- my-subm >actual 2>err &&\n- grep \"fatal:.*my-subm\" err &&\n+ test_must_be_empty err &&\n cat >expected <<-EOF &&\n * my-subm ${rev}...0000000:\n \n-- >8 --\n\nI suggest this as the other code path that calls\n`generate_submodule_summary` without going through the\n`is_nonbare_repository_dir` condition is the one where we get\n`p->status` as 'T' (typechange) or 'D' (deleted). We don't have to\nworry about 'T' as we would want the hash for the new object anyway.\nThat leaves us with 'D' which we indeed have to handle.\n\nNote that no such handling is required for the similar portion\ncorresponding to `dst_abbrev` as the conditional `if (S_ISGITLINK(p-\n>mod_dst))` already guards the `verify_submodule_committish` when we\nhave a status of 'D'.\n\n-- \nSivaraam\n\n"},{"id":"404343","messageId":"20200824175029.GB531246@konoha","threadId":"53998","inReplyTo":"a30f43ecbbc5fa64fe62eb5903d81bce7440986c.camel@gmail.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-24T17:50:29Z","receivedAt":"2020-08-24T17:50:39Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 24/08 04:38, Kaartic Sivaraam wrote:\n> On Mon, 2020-08-24 at 14:16 +0530, Shourya Shukla wrote:\n> > Or rather, we can do this:\n> > \n> > -----8<-----\n> > if (S_ISGITLINK(p->mod_src)) {\n> > \t\tstruct strbuf sb = STRBUF_INIT;\n> > \t\tstrbuf_addstr(&sb, p->sm_path);\n> > \t\tif (is_nonbare_repository_dir(&sb))\n> > \t\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n> > \t\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n> > \t\tstrbuf_release(&sb);\n> > \t\tif (!src_abbrev) {\n> > \t\t\tmissing_src = 1;\n> > \t\t\t/*\n> > \t\t\t * As `rev-parse` failed, we fallback to getting\n> > \t\t\t * the abbreviated hash using oid_src. We do\n> > \t\t\t * this as we might still need the abbreviated\n> > \t\t\t * hash in cases like a submodule type change, etc.\n> > \t\t\t */\n> > \t\t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n> > \t\t}\n> > \t} else {\n> > \t\t/*\n> > \t\t * The source does not point to a submodule.\n> > \t\t * So, we fallback to getting the abbreviation using\n> > \t\t * oid_src as we might still need the abbreviated\n> > \t\t * hash in cases like submodule add, etc.\n> > \t\t */\n> > \t\tsrc_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);\n> > \t}\n> > ----->8-----\n> > \n> > Similarly for dst as well. This solution passes all the tests and does\n> > not call 'verify_submodule_committish()' all the time. The previous\n> > approach failed a couple of tests, this one seems fine to me.\n> > \n> > How is this one?\n> > \n> \n> This is more or less what I had in mind initially. But later after\n> being reminded about the fact that there's a code path which calls\n> `generate_submodule_summary` only when `is_nonbare_repository_dir`\n> succeeds, I realized any conditional that uses\n> `is_nonbare_repository_dir` or the likes of it would be confusing. So,\n> I think a better approach would be something like:\n\nAlright. I understand. The case for which we faced the problem got\ncalled using this part:\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\nBut I understand your concern. I will change this.\n\n> -- 8< --\n> diff --git builtin/submodule--helper.c builtin/submodule--helper.c\n> index 63ea39025d..b490108cd9 100644\n> --- builtin/submodule--helper.c\n> +++ builtin/submodule--helper.c\n> @@ -1036,7 +1036,7 @@ static void print_submodule_summary(struct summary_cb *info, char* errmsg,\n> static void generate_submodule_summary(struct summary_cb *info,\n> struct module_cb *p)\n> {\n> - char *displaypath, *src_abbrev, *dst_abbrev;\n> + char *displaypath, *src_abbrev = NULL, *dst_abbrev;\n> int missing_src = 0, missing_dst = 0;\n> char *errmsg = NULL;\n> int total_commits = -1;\n> @@ -1062,8 +1062,9 @@ static void generate_submodule_summary(struct summary_cb *info,\n> }\n> \n> if (S_ISGITLINK(p->mod_src)) {\n> - src_abbrev = verify_submodule_committish(p->sm_path,\n> - oid_to_hex(&p->oid_src));\n> + if (p->status != 'D')\n> + src_abbrev = verify_submodule_committish(p->sm_path,\n> + oid_to_hex(&p->oid_src));\n> if (!src_abbrev) {\n> missing_src = 1;\n> /*\n> diff --git t/t7421-submodule-summary-add.sh t/t7421-submodule-summary-add.sh\n> index 59a9b00467..b070f13714 100755\n> --- t/t7421-submodule-summary-add.sh\n> +++ t/t7421-submodule-summary-add.sh\n> @@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n> git commit -m \"change submodule path\" &&\n> rev=$(git -C sm rev-parse --short HEAD^) &&\n> git submodule summary HEAD^^ -- my-subm >actual 2>err &&\n> - grep \"fatal:.*my-subm\" err &&\n> + test_must_be_empty err &&\n> cat >expected <<-EOF &&\n> * my-subm ${rev}...0000000:\n> \n> -- >8 --\n> \n> I suggest this as the other code path that calls\n> `generate_submodule_summary` without going through the\n> `is_nonbare_repository_dir` condition is the one where we get\n> `p->status` as 'T' (typechange) or 'D' (deleted). We don't have to\n> worry about 'T' as we would want the hash for the new object anyway.\n> That leaves us with 'D' which we indeed have to handle.\n\nOh you did mention it here. Yeah, this is perfect.\n\n> Note that no such handling is required for the similar portion\n> corresponding to `dst_abbrev` as the conditional `if (S_ISGITLINK(p-\n> >mod_dst))` already guards the `verify_submodule_committish` when we\n> have a status of 'D'.\n\nSure I will keep this in mind.\n\n"},{"id":"404344","messageId":"xmqqo8n053r6.fsf@gitster.c.googlers.com","threadId":"53998","inReplyTo":"20200812194404.17028-5-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-24T17:54:05Z","receivedAt":"2020-08-24T17:54:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"(A few miniscule things I noticed that are irrelevant to the code\nstructure discussion).\n\n> +static char* verify_submodule_committish(const char *sm_path,\n\nStyle: in C, asterisk sticks to the identifier, not type, i.e.\n\n static char *verify_submodule_committish(const char *sm_path, ...);\n\n> +static void print_submodule_summary(struct summary_cb *info, char* errmsg,\n\nLikewise; \"char *errmsg\".\n\n> +static void generate_submodule_summary(struct summary_cb *info,\n> +\t\t\t\t struct module_cb *p)\n> +{\n> +\tchar *displaypath, *src_abbrev, *dst_abbrev;\n> +\tint missing_src = 0, missing_dst = 0;\n> +\tchar *errmsg = NULL;\n> +\tint total_commits = -1;\n> +\n> +\tif (!info->cached && oideq(&p->oid_dst, &null_oid)) {\n> +\t\tif (S_ISGITLINK(p->mod_dst)) {\n> +...\n> +\t\t} else {\n> +\t\t\t/* for a submodule removal (mode:0000000), don't warn */\n> +\t\t\tif (p->mod_dst)\n> +\t\t\t\twarning(_(\"unexpected mode %d\\n\"), p->mod_dst);\n> +\t\t}\n> +\t}\n\nNobody can read mode bits written in decimal. Use \"%o\" instead,\nperhaps?\n\nThanks.\n"}]}