Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Aug 24, 2020, 11:08 UTC
Message-ID: <a30f43ecbbc5fa64fe62eb5903d81bce7440986c.camel@gmail.com>
In-Reply-To: <20200824084634.GA377527@konoha>

On Mon, 2020-08-24 at 14:16 +0530, Shourya Shukla wrote:

Show 37 quoted lines

> Or rather, we can do this:
> 
> -----8<-----
> if (S_ISGITLINK(p->mod_src)) {
> 		struct strbuf sb = STRBUF_INIT;
> 		strbuf_addstr(&sb, p->sm_path);
> 		if (is_nonbare_repository_dir(&sb))
> 			src_abbrev = verify_submodule_committish(p->sm_path,
> 								                     oid_to_hex(&p->oid_src));
> 		strbuf_release(&sb);
> 		if (!src_abbrev) {
> 			missing_src = 1;
> 			/*
> 			 * As `rev-parse` failed, we fallback to getting
> 			 * the abbreviated hash using oid_src. We do
> 			 * this as we might still need the abbreviated
> 			 * hash in cases like a submodule type change, etc.
> 			 */
> 			src_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);
> 		}
> 	} else {
> 		/*
> 		 * The source does not point to a submodule.
> 		 * So, we fallback to getting the abbreviation using
> 		 * oid_src as we might still need the abbreviated
> 		 * hash in cases like submodule add, etc.
> 		 */
> 		src_abbrev = xstrndup(oid_to_hex(&p->oid_src), 7);
> 	}
> ----->8-----
> 
> Similarly for dst as well. This solution passes all the tests and does
> not call 'verify_submodule_committish()' all the time. The previous
> approach failed a couple of tests, this one seems fine to me.
> 
> How is this one?
>

This is more or less what I had in mind initially. But later after being reminded about the fact that there's a code path which calls `generate_submodule_summary` only when `is_nonbare_repository_dir` succeeds, I realized any conditional that uses `is_nonbare_repository_dir` or the likes of it would be confusing. So, I think a better approach would be something like:

-- 8< --

diff --git builtin/submodule--helper.c builtin/submodule--helper.c
index 63ea39025d..b490108cd9 100644
--- builtin/submodule--helper.c
+++ builtin/submodule--helper.c
@@ -1036,7 +1036,7 @@ static void print_submodule_summary(struct summary_cb *info, char* errmsg,
 static void generate_submodule_summary(struct summary_cb *info,
                                       struct module_cb *p)
 {
-       char *displaypath, *src_abbrev, *dst_abbrev;
+       char *displaypath, *src_abbrev = NULL, *dst_abbrev;
        int missing_src = 0, missing_dst = 0;
        char *errmsg = NULL;
        int total_commits = -1;
@@ -1062,8 +1062,9 @@ static void generate_submodule_summary(struct summary_cb *info,
        }
 
        if (S_ISGITLINK(p->mod_src)) {
-               src_abbrev = verify_submodule_committish(p->sm_path,
-                                                        oid_to_hex(&p->oid_src));
+               if (p->status != 'D')
+                       src_abbrev = verify_submodule_committish(p->sm_path,
+                                                                oid_to_hex(&p->oid_src));
                if (!src_abbrev) {
                        missing_src = 1;
                        /*
diff --git t/t7421-submodule-summary-add.sh t/t7421-submodule-summary-add.sh
index 59a9b00467..b070f13714 100755
--- t/t7421-submodule-summary-add.sh
+++ t/t7421-submodule-summary-add.sh
@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'
        git commit -m "change submodule path" &&
        rev=$(git -C sm rev-parse --short HEAD^) &&
        git submodule summary HEAD^^ -- my-subm >actual 2>err &&
-       grep "fatal:.*my-subm" err &&
+       test_must_be_empty err &&
        cat >expected <<-EOF &&
        * my-subm ${rev}...0000000:
 
-- >8 --

I suggest this as the other code path that calls
`generate_submodule_summary` without going through the
`is_nonbare_repository_dir` condition is the one where we get
`p->status` as 'T' (typechange) or 'D' (deleted). We don't have to
worry about 'T' as we would want the hash for the new object anyway.
That leaves us with 'D' which we indeed have to handle.

Note that no such handling is required for the similar portion
corresponding to `dst_abbrev` as the conditional `if (S_ISGITLINK(p-
>mod_dst))` already guards the `verify_submodule_committish` when we
have a status of 'D'.

-- 
Sivaraam