Hello Dscho!
Before replying, one thing I want to clarify is that I ask various
things for double-checking since if I get even the slightest confusion I
start overthinking and destroying it all for myself. Please bear with me
and confirm/clarify stuff wherever possible. Would be of great help! :)
> Hi Shourya,
>
> [exchanging Stefan Beller's dysfunct @google address for their private
> one; I encourage you to do the same in the next iteration, probably
> by editing the `Mentored-by:` line.]
I think you missed to mention it.
> On Fri, 3 Jul 2020, Shourya Shukla wrote:
>
> > From: Prathamesh Chavan <pc44800@xxxxxxxxx>
> >
> > The submodule subcommand 'summary' is ported in the process of
> > making git-submodule a builtin. The function cmd_summary() from
> > git-submodule.sh is ported to functions module_summary(),
> > compute_summary_module_list(), prepare_submodule_summary() and
> > generate_submodule_summary(), print_submodule_summary().
> >
> > The first function module_summary() parses the options of submodule
> > subcommand and also acts as the front-end of this subcommand.
> > After parsing them, it calls the compute_summary_module_list()
>
> Missing full-stop, and probably the sentence also wanted to say "function"
> at the end.
I will correct. Thanks for pointing out!
> > The functions compute_summary_module_list() runs the diff_cmd,
> > and generates the modules list, as required by the subcommand.
> > The generation of this module list is done by the using the
>
> s/the using/using/
Will amend!
> > callback function submodule_summary_callback(), and stored in the
> > structure module_cb.
>
> This explains nicely what the patch does. But the commit message should
> not really repeat what can be readily deduced from the patch; It should
> focus on the motivation and on interesting background information that is
> _not_ readily deduced from the patch.
I understand. I will follow your suggestions regarding my patch.
> For example, I see that `$diff_cmd` is called twice in the shell script
> version, once to "get modified modules cared by user" and then _again_,
> with that list of modified modules.
>
> I would have liked to see a reasoning in the commit message that explains
> why this has to be so in the C version. I get why it is complicated in a
> shell script (which lacks proper objects, after all), but I would have
> expected the C version to be able to accumulate the information with a
> single pass.
>
> (Before writing the following paragraph, I actually reviewed the patch
> from bottom to top, in the caller->callee direction.)
>
> Ah. I see that this indeed is the case: there is only one pass in the C
> version. That's a useful piece of metadata for the commit message, I
> think, much more useful than describing the call tree of the functions.
Yup that it worth mentioning.
> Another thing worth mentioning in the commit message is that we use the
> combination of setting a child_process' working directory to the submodule
> path and then calling `prepare_submodule_repo_env()` which also sets
> `GIT_DIR` to `.git`, so that we can be certain that those spawned
> processes will not access the superproject's ODB by mistake.
>
> When reading my suggestions, please keep in mind that I reviewed the
> functions in caller->callee order, i.e. I started at the end of the patch
> and then worked my way up.
>
> All in all, I like the function structure, but I think there is still a
> bit room for improvement in a v2.
> > +static int verify_submodule_object_name(const char *sm_path,
> > + const char *sha1)
> > +{
> > + struct child_process cp_rev_parse = CHILD_PROCESS_INIT;
> > + > > + cp_rev_parse.git_cmd = 1;
> > + cp_rev_parse.no_stdout = 1;
> > + cp_rev_parse.dir = sm_path;
>
> So here we specify `sm_path` as current working directory.
>
> > + prepare_submodule_repo_env(&cp_rev_parse.env_array);
>
> And this implicitly sets `GIT_DIR=.git`. Good.
>
> > + argv_array_pushl(&cp_rev_parse.args, "rev-parse", "-q",
> > + "--verify", NULL);
> > + argv_array_pushf(&cp_rev_parse.args, "%s^0", sha1);
>
> After this, we should also append `--` to make sure that we're not parsing
> this as a file name.
Will do!
> Two comments about naming: `sha1` is pretty misleading here, as we do not
> require it to be a SHA-1 (especially in the future in which we switch to
> SHA-256). Besides, what we're really asking for (via that `^0` suffix) is
> a committish. Therefore, I would propose to use `committish` both in the
> parameter name as well as the function name.
I am not aware of this change. I will take this suggestion into account.
> > +
> > + if (run_command(&cp_rev_parse))
> > + return 1;
> > +
> > + return 0;
> > +}
> > +
> > +static void print_submodule_summary(struct summary_cb *info, int errmsg,
> > + int total_commits, int missing_src,
> > + int missing_dst, const char *displaypath,
> > + int is_sm_git_dir, struct module_cb *p)
> > +{
> > + if (p->status == 'T') {
> > + if (S_ISGITLINK(p->mod_dst))
> > + printf(_("* %s %s(blob)->%s(submodule)"),
> > + displaypath, find_unique_abbrev(&p->oid_src, 7),
>
> The shell script version does this:
>
> sha1_abbr_src=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_src 2>/dev/null ||
> echo $sha1_src | cut -c1-7)
>
> That is not quite the same, as it looks for the abbreviation _in the
> submodule_, not in the current project. So I think `find_unique_abbrev()`
> is not correct here.
>
> The funny thing is that we _already_ will have called `git rev-parse
> --verify` for both `p->oid_src` and `p->oid_dst` in the submodule, in the
> caller of this function! And while we throw away the result, and while we
> do not pass `--short`, there is no reason why we shouldn't be able to do
> precisely that.
Okay so you are saying that there is no need of a 'find_unique_abbrev()'
since we would be easily able to obtain these values from the caller of
'print_submodule_summary()' right? Maybe we can pass 'oid_src' or
'oid_dst' as an argument?
> > + find_unique_abbrev(&p->oid_dst, 7));
> > + else
> > + printf(_("* %s %s(submodule)->%s(blob)"),
> > + displaypath, find_unique_abbrev(&p->oid_src, 7),
> > + find_unique_abbrev(&p->oid_dst, 7));
> > + } else {
> > + printf("* %s %s...%s",
> > + displaypath, find_unique_abbrev(&p->oid_src, 7),
> > + find_unique_abbrev(&p->oid_dst, 7));
> > + }
> > +
> > + if (total_commits < 0)
> > + printf(":\n");
> > + else
> > + printf(" (%d):\n", total_commits);
> > +
> > + if (errmsg) {
> > + /*
> > + * Don't give error msg for modification whose dst is not
> > + * submodule, i.e. deleted or changed to blob
> > + */
> > + if (S_ISGITLINK(p->mod_src)) {
> > + if (missing_src && missing_dst) {
> > + printf(_(" Warn: %s doesn't contain commits %s and %s\n"),
> > + displaypath, oid_to_hex(&p->oid_src),
> > + oid_to_hex(&p->oid_dst));
> > + } else if (missing_src) {
> > + printf(_(" Warn: %s doesn't contain commit %s\n"),
> > + displaypath, oid_to_hex(&p->oid_src));
> > + } else {
> > + printf(_(" Warn: %s doesn't contain commit %s\n"),
> > + displaypath, oid_to_hex(&p->oid_dst));
> > + }
> > + }
> > + } else if (is_sm_git_dir) {
> > + struct child_process cp_log = CHILD_PROCESS_INIT;
> > +
> > + cp_log.git_cmd = 1;
> > + cp_log.dir = p->sm_path;
> > + prepare_submodule_repo_env(&cp_log.env_array);
>
> Since the working directory is set to the top-level directory of the
> submodule, and since `prepare_submodule_repo_env()` sets `GIT_DIR` to
> `.git`, I think that the `is_sm_git_dir` condition is unnecessary. In
> fact, the entire `is_sm_git_dir` parameter (and local variable in the
> caller, see more on that below) can go away.
Because we already set the $GIT_DIR to .git/ so an extra check will not
be necessary right?
> > + argv_array_pushl(&cp_log.args, "log", NULL);
> > +
> > + if (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst)) {
> > + if (info->summary_limit > 0)
> > + argv_array_pushf(&cp_log.args, "-%d",
> > + info->summary_limit);
> > +
> > + argv_array_pushl(&cp_log.args, "--pretty= %m %s",
> > + "--first-parent", NULL);
> > + argv_array_pushf(&cp_log.args, "%s...%s",
> > + oid_to_hex(&p->oid_src),
> > + oid_to_hex(&p->oid_dst));
> > + } else if (S_ISGITLINK(p->mod_dst)) {
> > + argv_array_pushl(&cp_log.args, "--pretty= > %s",
> > + "-1", oid_to_hex(&p->oid_dst), NULL);
> > + } else {
> > + argv_array_pushl(&cp_log.args, "--pretty= < %s",
> > + "-1", oid_to_hex(&p->oid_src), NULL);
> > + }
> > + run_command(&cp_log);
> > + }
> > + printf("\n");
> > +}
>
> It looks as if there is a whole lot of `oid_to_hex(&p->oid_src)` in that
> function. Together with the realization that we need the abbreviated
> version of that at least in one place, and the other realization that we
> already call `rev-parse --verify` for both `oid_src` and `oid_dst` in the
> caller of this function, it seems to suggest itself that we would actually
> want to pass the `--short` option, too, and to capture the output, and
> pass it down to `print_submodule_summary()` _instead of_ `missing_src` and
> `missing_dst` (e.g. as `src_abbrev` and `dst_abbrev`).
Oh you have mentioned it here too. This seems quite a good approach. I
will adopt this.
> > +
> > +static void generate_submodule_summary(struct summary_cb *info,
> > + struct module_cb *p)
> > +{
> > + int missing_src = 0;
> > + int missing_dst = 0;
> > + char *displaypath;
> > + int errmsg = 0;
> > + int total_commits = -1;
> > + int is_sm_git_dir = 0;
> > + struct strbuf sm_git_dir_sb = STRBUF_INIT;
> > +
> > + if (!info->cached && oideq(&p->oid_dst, &null_oid)) {
> > + if (S_ISGITLINK(p->mod_dst)) {
> > + /*
> > + * NEEDSWORK: avoid using separate process with
> > + * the help of the function head_ref_submodule()
>
> I don't quite understand this comment. There is no `head_ref_submodule()`
> function.
>
> > + */
> > + struct child_process cp_rev_parse = CHILD_PROCESS_INIT;
> > + struct strbuf sb_rev_parse = STRBUF_INIT;
> > +
> > + cp_rev_parse.git_cmd = 1;
> > + cp_rev_parse.no_stderr = 1;
> > + cp_rev_parse.dir = p->sm_path;
> > + prepare_submodule_repo_env(&cp_rev_parse.env_array);
> > +
> > + argv_array_pushl(&cp_rev_parse.args, "rev-parse",
> > + "HEAD", NULL);
> > + if (!capture_command(&cp_rev_parse, &sb_rev_parse, 0)) {
> > + strbuf_strip_suffix(&sb_rev_parse, "\n");
> > + get_oid_hex(sb_rev_parse.buf, &p->oid_dst);
> > + }
> > + strbuf_release(&sb_rev_parse);
> > + } else if (S_ISLNK(p->mod_dst) || S_ISREG(p->mod_dst)) {
> > + struct child_process cp_hash_object = CHILD_PROCESS_INIT;
> > + struct strbuf sb_hash_object = STRBUF_INIT;
> > +
> > + cp_hash_object.git_cmd = 1;
> > + argv_array_pushl(&cp_hash_object.args, "hash-object",
> > + p->sm_path, NULL);
> > + if (!capture_command(&cp_hash_object,
> > + &sb_hash_object, 0)) {
> > + strbuf_strip_suffix(&sb_hash_object, "\n");
> > + get_oid_hex(sb_hash_object.buf, &p->oid_dst);
> > + }
> > + strbuf_release(&sb_hash_object);
>
> It would probably be shorter, less error-prone, and quicker to use
> `index_fd()` directly.
>
> BTW I am not quite sure that this code does the correct thing in case of a
> symlink: it hashes the contents of the symlink target (if it is a file,
> otherwise it errors out). But that is hardly an issue introduced by the
> conversion, that's just copied from `git-submodule.sh`.
>
> > + } else {
> > + if (p->mod_dst)
> > + die(_("unexpected mode %d\n"), p->mod_dst);
>
> Hmm. This does not match what the shell script version does:
>
> *)
> # unexpected type
> eval_gettextln "unexpected mode \$mod_dst" >&2
> continue ;;
>
> I think we should also just write the message to `stderr` and continue,
> not `die()`.
>
> In addition to that, I am missing the C code for this case:
>
> 000000)
> ;; # removed
>
> It is quite possible that our test suite does not cover this case (or did
> the test suite fail for you?). If that is indeed the case, it would be
> really good to add a test case as part of this patch series, to gain
> confidence in the correctness of the conversion.
The tests passed for me actually. Whether this is covered by the test
cases, I am not sure. I will have to check it.
> > + }
> > + }
> > +
> > + strbuf_addstr(&sm_git_dir_sb, p->sm_path);
>
> I have to admit that I am not loving the name `sm_git_dir_sb`. Why not
> `submodule_git_dir`? I guess you copied it from elsewhere in
> `submodule--helper.c`...
>
> > + if (is_nonbare_repository_dir(&sm_git_dir_sb))
> > + is_sm_git_dir = 1;
>
> So here, we verify whether there is a repository at `p->sm_path`. I don't
> see that in the shell script version:
>
> missing_src=
> missing_dst=
>
> test $mod_src = 160000 &&
> ! GIT_DIR="$name/.git" git rev-parse -q --verify $sha1_src^0 >/dev/null &&
> missing_src=t
>
> test $mod_dst = 160000 &&
> ! GIT_DIR="$name/.git" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&
> missing_dst=t
>
> Let's read a bit further.
>
> > +
> > + if (is_sm_git_dir && S_ISGITLINK(p->mod_src))
> > + missing_src = verify_submodule_object_name(p->sm_path,
> > + oid_to_hex(&p->oid_src));
>
> Ah, and `verify_submodule_object_name()` uses `p->sm_path` as working
> directory. But that's not what the shell script version did: it specified
> the `GIT_DIR` explicitly.
>
> And by using the `prepare_submodule_repo_env()` function in
> `verify_submodule_object_name()`, we specify `GIT_DIR` implicitly, as I
> pointed out in my comment on that function.
Oh so you're saying that it will be better to call
'prepare_submodule_repo_env()' on some variable since we explicitly want to
store the path to GIT_DIR?
It would be of much help if you could explain this part just a little
more (for my own sake).
> So I think that `is_sm_git_dir` might be
I think you missed something here...
> > +
> > + if (is_sm_git_dir && S_ISGITLINK(p->mod_dst))
> > + missing_dst = verify_submodule_object_name(p->sm_path,
> > + oid_to_hex(&p->oid_dst));
> > +
> > + displaypath = get_submodule_displaypath(p->sm_path, info->prefix);
> > +
> > + if (!missing_dst && !missing_src) {
> > + if (is_sm_git_dir) {
> > + struct child_process cp_rev_list = CHILD_PROCESS_INIT;
> > + struct strbuf sb_rev_list = STRBUF_INIT;
> > + char *range;
> > +
> > + if (S_ISGITLINK(p->mod_src) && S_ISGITLINK(p->mod_dst))
> > + range = xstrfmt("%s...%s", oid_to_hex(&p->oid_src),
> > + oid_to_hex(&p->oid_dst));
> > + else if (S_ISGITLINK(p->mod_src))
> > + range = xstrdup(oid_to_hex(&p->oid_src));
> > + else
> > + range = xstrdup(oid_to_hex(&p->oid_dst));
> > +
> > + cp_rev_list.git_cmd = 1;
> > + cp_rev_list.dir = p->sm_path;
> > + prepare_submodule_repo_env(&cp_rev_list.env_array);
>
> Again, due to setting the working directory to `p->sm_path` and
> (implicitly, via `prepare_submodule_repo_env()`) `GIT_DIR` to `.git`, I do
> not think that we have to guard this block beind `is_sm_git_dir`.
> > +
> > + argv_array_pushl(&cp_rev_list.args, "rev-list",
> > + "--first-parent", range, "--", NULL);
>
> Since `argv_array_push()` duplicates the strings, anyway, we can totally
> avoid the need for the `range` variable:
>
> if (IS_GITLINK(p->mod_src) && IS_GITLINK(p->mod_dst))
> argv_array_pushf(&cp_rev_list.args, "%s...%s",
> oid_to_hex(&p->oid_src),
> oid_to_hex(&p->oid_dst));
> else
> argv_array_push(&cp_rev_list.args, IS_GITLINK(p->mod_src) ?
> oid_to_hex(&p->oid_src) :
> oid_to_hex(&p->oid_dst));
>
> > + if (!capture_command(&cp_rev_list, &sb_rev_list, 0)) {
> > + if (sb_rev_list.len)
> > + total_commits = count_lines(sb_rev_list.buf,
> > + sb_rev_list.len);
>
> That's actually not necessary. `git rev-list --count` will give you a nice
> number, no need to capture a potentially large amount of memory only to
> count the lines.
>
> This may also make the patch obsolete that makes `count_lines()` public.
Therefore we eliminate count_lines() from here and instead do a 'git
rev-list --count'?
> > + else
> > + total_commits = 0;
> > + }
>
> > +
> > + free(range);
> > + strbuf_release(&sb_rev_list);
> > + }
> > + } else {
> > + errmsg = 1;
> > + }
>
> I am missing the equivalent for these lines here:
>
> t,)
> errmsg="$(eval_gettext " Warn: \$display_name doesn't contain commit \$sha1_src")"
> ;;
> ,t)
> errmsg="$(eval_gettext " Warn: \$display_name doesn't contain commit \$sha1_dst")"
> ;;
> t,t)
> errmsg="$(eval_gettext " Warn: \$display_name doesn't contain commits \$sha1_src and \$ ha1_dst")"
> ;;
I will add them.
> I am not quite sure whether it is a good idea to leave it to the
> `print_submodule_summary()` function to generate the `errmsg`. I think I'd
> rather have it a `char *` than an `int`.
Would it be better to add these error messages in
'prepare_submodule_summary()'? If we have error messages as integers
then we will simply
> > +
> > + print_submodule_summary(info, errmsg, total_commits,
> > + missing_src, missing_dst,
> > + displaypath, is_sm_git_dir, p);
> > +
> > + free(displaypath);
> > + strbuf_release(&sm_git_dir_sb);
> > +}
> > +
> > +static void prepare_submodule_summary(struct summary_cb *info,
> > + struct module_cb_list *list)
> > +{
> > + int i;
> > + for (i = 0; i < list->nr; i++) {
> > + struct module_cb *p = list->entries[i];
> > + struct child_process cp_rev_parse = CHILD_PROCESS_INIT;
> > +
> > + if (p->status == 'D' || p->status == 'T') {
> > + generate_submodule_summary(info, p);
> > + continue;
> > + }
> > +
> > + if (info->for_status) {
> > + char *config_key;
>
> Since the `config_key` is only used within the `if()` block it would be
> better to declare it within that block.
>
> > + const char *ignore_config = "none";
>
> Since the only value we ever care about is "all", how about turning this
> into an `int`, setting it to `0` here, and later assigning it to
> `!strcmp(value, "all")` and `!strcmp(sub->ignore, "all")`, respectively?
Alright will do!
> I mean, I get it. Unix shell scripts are all about passing around text.
> And it is much easier to just translate that to C faithfully. But that
> does not make it good C code. C has data types, and proper C code makes
> use of that.
>
> > + const char *value;
>
> If you want to save on lines, you can cuddle this together with other
> declarations of the same type. Even so, it could be scoped more narrowly.
>
> > + const struct submodule *sub = submodule_from_path(the_repository,
> > + &null_oid,
> > + p->sm_path);
> > +
> > + if (sub && p->status != 'A') {
>
> Good. The shell script version _always_ retrieved the `.ignore` config
> value, even if the `status` is `A`. Your version is much better.
>
> But why bother calling `submodule_from_path()` if the status is `A`?
What exactly does a status of 'A' or 'T' mean? I mean I know what we are
doing but what exactly do these translate into?
> I could actually see the `const struct submodule *sub;` declaration be
> pulled out of this scope, and combining the `if (info->for_status &&
> p->status != 'A'), and the moving the assignment of `sub` into the `else
> if ((sub = submodule_from_path(r, &null_oid, p->sm_path)) &&
> sub->ignore)`.
>
> That would save us one entire indentation level.
That seems a good approach! I will try this out.
> > + config_key = xstrfmt("submodule.%s.ignore",
> > + sub->name);
> > + if (!git_config_get_string_const(config_key, &value))
> > + ignore_config = value;
> > + else if (sub->ignore)
> > + ignore_config = sub->ignore;
> > +
> > + free(config_key);
> > + if (!strcmp(ignore_config, "all"))
> > + continue;
> > + }
> > + }
> > +
> > + /* Also show added or modified modules which are checked out */
> > + cp_rev_parse.dir = p->sm_path;
> > + cp_rev_parse.git_cmd = 1;
> > + cp_rev_parse.no_stderr = 1;
> > + cp_rev_parse.no_stdout = 1;
> > +
> > + argv_array_pushl(&cp_rev_parse.args, "rev-parse",
> > + "--git-dir", NULL);
> > +
> > + if (!run_command(&cp_rev_parse))
>
> I wonder whether we really need to waste an entire spawned process on
> figuring out whether `p->sm_path` refers to an active repository. Wouldn't
> `is_submodule_active(r, p->sm_path)` fulfill the same purpose?
Yep! This is correct. I will change.
> > + generate_submodule_summary(info, p);
> > + }
> > +}
> > +
> > +static void submodule_summary_callback(struct diff_queue_struct *q,
> > + struct diff_options *options,
> > + void *data)
> > +{
> > + int i;
> > + struct module_cb_list *list = data;
> > + for (i = 0; i < q->nr; i++) {
> > + struct diff_filepair *p = q->queue[i];
> > + struct module_cb *temp;
> > +
> > + if (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode))
> > + continue;
> > + temp = (struct module_cb*)malloc(sizeof(struct module_cb));
> > + temp->mod_src = p->one->mode;
> > + temp->mod_dst = p->two->mode;
> > + temp->oid_src = p->one->oid;
> > + temp->oid_dst = p->two->oid;
> > + temp->status = p->status;
> > + temp->sm_path = xstrdup(p->one->path);
> > +
> > + ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
> > + list->entries[list->nr++] = temp;
> > + }
> > +}
> > +
> > +static const char *get_diff_cmd(enum diff_cmd diff_cmd)
> > +{
> > + switch (diff_cmd) {
> > + case DIFF_INDEX: return "diff-index";
> > + case DIFF_FILES: return "diff-files";
> > + default: BUG("bad diff_cmd value %d", diff_cmd);
> > + }
> > +}
> > +
> > +static int compute_summary_module_list(char *head,
> > + struct summary_cb *info,
> > + enum diff_cmd diff_cmd)
> > +{
> > + struct argv_array diff_args = ARGV_ARRAY_INIT;
> > + struct rev_info rev;
> > + struct module_cb_list list = MODULE_CB_LIST_INIT;
> > +
> > + argv_array_push(&diff_args, get_diff_cmd(diff_cmd));
> > + if (info->cached)
> > + argv_array_push(&diff_args, "--cached");
> > + argv_array_pushl(&diff_args, "--ignore-submodules=dirty", "--raw",
> > + NULL);
> > + if (head)
> > + argv_array_push(&diff_args, head);
> > + argv_array_push(&diff_args, "--");
> > + if (info->argc)
> > + argv_array_pushv(&diff_args, info->argv);
> > +
> > + git_config(git_diff_basic_config, NULL);
> > + init_revisions(&rev, info->prefix);
> > + rev.abbrev = 0;
> > + precompose_argv(diff_args.argc, diff_args.argv);
> > +
> > + diff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,
> > + &rev, NULL);
> > + rev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;
> > + rev.diffopt.format_callback = submodule_summary_callback;
> > + rev.diffopt.format_callback_data = &list;
> > +
> > + if (!info->cached) {
> > + if (diff_cmd == DIFF_INDEX)
>
> Please substitute the double-space by a single one.
Will do!
> > + setup_work_tree();
> > + if (read_cache_preload(&rev.diffopt.pathspec) < 0) {
> > + perror("read_cache_preload");
> > + return -1;
> > + }
> > + } else if (read_cache() < 0) {
> > + perror("read_cache");
> > + return -1;
> > + }
> > +
> > + if (diff_cmd == DIFF_INDEX)
> > + run_diff_index(&rev, info->cached);
> > + else
> > + run_diff_files(&rev, 0);
> > + prepare_submodule_summary(info, &list);
> > + return 0;
> > +}
> > +
> > +static int module_summary(int argc, const char **argv, const char *prefix)
> > +{
> > + struct summary_cb info = SUMMARY_CB_INIT;
> > + int cached = 0;
> > + int for_status = 0;
> > + int quiet = 0;
> > + int files = 0;
> > + int summary_limit = -1;
> > + struct child_process cp_rev = CHILD_PROCESS_INIT;
> > + struct strbuf sb = STRBUF_INIT;
> > + enum diff_cmd diff_cmd = DIFF_INDEX;
> > + int ret;
> > +
> > + struct option module_summary_options[] = {
> > + OPT__QUIET(&quiet, N_("Suppress output of summarising submodules")),
> > + OPT_BOOL(0, "cached", &cached,
> > + N_("Use the commit stored in the index instead of the submodule HEAD")),
> > + OPT_BOOL(0, "files", &files,
> > + N_("To compare the commit in the index with that in the submodule HEAD")),
> > + OPT_BOOL(0, "for-status", &for_status,
> > + N_("Skip submodules with 'ignore_config' value set to 'all'")),
> > + OPT_INTEGER('n', "summary-limit", &summary_limit,
> > + N_("Limit the summary size")),
> > + OPT_END()
> > + };
> > +
> > + const char *const git_submodule_helper_usage[] = {
> > + N_("git submodule--helper summary [<options>] [commit] [--] [<path>]"),
> > + NULL
> > + };
> > +
> > + argc = parse_options(argc, argv, prefix, module_summary_options,
> > + git_submodule_helper_usage, 0);
> > +
> > + if (!summary_limit)
> > + return 0;
> > +
> > + cp_rev.git_cmd = 1;
> > + argv_array_pushl(&cp_rev.args, "rev-parse", "-q", "--verify",
> > + argc ? argv[0] : "HEAD", NULL);
>
> Oy. Why not simply call `get_oid()`? No need to spawn a new process.
Then everytime we need 'rev-parse', we simply call 'get_oid()'? That
will save us a ton of processes?
But I think we do need to capture the output of 'git rev-parse --verify
....' so I think it will backfire to use 'get_oid()' or am I just being
too dumb and not catching on something?
> > +
> > + if (!capture_command(&cp_rev, &sb, 0)) {
> > + strbuf_strip_suffix(&sb, "\n");
> > + if (argc) {
> > + argv++;
> > + argc--;
> > + }
> > + } else if (!argc || !strcmp(argv[0], "HEAD")) {
> > + /* before the first commit: compare with an empty tree */
> > + struct stat st;
> > + struct object_id oid;
> > + if (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,
> > + prefix, 3))
> > + die("Unable to add %s to database", oid.hash);
>
> Umm. The original reads:
>
> # before the first commit: compare with an empty tree
> head=$(git hash-object -w -t tree --stdin </dev/null)
>
> It does not actually read from `stdin`. It reads from `/dev/null`,
> redirected to the input. And what it _actually_ does is to generate the
> OID of the empty tree.
>
> But we already _have_ the OID of the empty tree! It's
> `the_hash_algo->empty_tree`.
I did not know this 'the_hash_algo'. I will use it. Thanks! :)
> I hope that this is covered by the test suite. Please check that. The test
> would succeed with your version, but only because tests are run with
> `stdin` redirected from `/dev/null` by default.
I guess yes. My work passed because the tests are written this way.
> > + strbuf_addstr(&sb, oid_to_hex(&oid));
> > + if (argc) {
> > + argv++;
> > + argc--;
> > + }
> > + } else {
> > + strbuf_addstr(&sb, "HEAD");
> > + }
>
> The conversion to C would make for a fine excuse to simplify the logic.
This was kind of like the 'shift' in shell. What equivalent do you
suggest?
> > + if (files) {
> > + if (cached)
> > + die(_("--cached and --files are mutually exclusive"));
> > + diff_cmd = DIFF_FILES;
> > + }
> > +
> > + info.argc = argc;
> > + info.argv = argv;
> > + info.prefix = prefix;
> > + info.cached = !!cached;
> > + info.for_status = !!for_status;
> > + info.quiet = quiet;
> > + info.files = files;
> > + info.summary_limit = summary_limit;
> > +
> > + ret = compute_summary_module_list((diff_cmd == DIFF_FILES) ? NULL : sb.buf,
> > + &info, diff_cmd);
>
> It would be better to pass the OID as `struct object_id *`, not as string.
Will do!
> Other than that, this patch nicely follows previous conversions from Unix
> shell scripts to C.
>
> Well done,
> Johannes
Thank you! It was a highly detailed review! I am still learning tons of
stuff about Git's code and such a review does help a lot! :)
