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

On 24/08 01:33, Kaartic Sivaraam wrote:

This is what I have tried to implement after your suggestion:

-----8<-----
strbuf_addstr(&sb, p->sm_path);
	if (is_nonbare_repository_dir(&sb) && S_ISGITLINK(p->mod_src)) {
		src_abbrev = verify_submodule_committish(p->sm_path,
							 oid_to_hex(&p->oid_src));
		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);
	}

	if (is_nonbare_repository_dir(&sb) && S_ISGITLINK(p->mod_dst)) {
		dst_abbrev = verify_submodule_committish(p->sm_path,
							 oid_to_hex(&p->oid_dst));
		if (!dst_abbrev) {
			missing_dst = 1;
			/*
			 * As `rev-parse` failed, we fallback to getting
			 * the abbreviated hash using oid_dst. We do
			 * this as we might still need the abbreviated
			 * hash in cases like a submodule type change, etc.
			 */
			dst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);
		}
	} else {
		/*
		 * The destination does not point to a submodule.
		 * So, we fallback to getting the abbreviation using
		 * oid_dst as we might still need the abbreviated
		 * hash in cases like a submodule removal, etc.
		 */
		dst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);
	}
----->8-----

That is, add another check along with the 'S_ISGITLINK()' one. Now, the thing is that 'rev-list' (called just after this part) starts to bother and comes up with its own 'fatal' that the directory rev-list does not exist.

The thing is that 'missing{src,dst}' should be set to 1 in two cases:

    1. If the hash is not found, i.e, when 'verify_submodule..()'
       returns a NULL. Something which is happening right now as well.

    2. If the SM is not reachable for some reason (maybe it does not
       exist like in our case). Something which is NOT happening right
       now.

Or if having the same variable denote two things does not please you, then we can create another variable for the second check BUT, we will have to incorporate checking of that variable in the

-----8<-----
if (!missing_src && !missing_dst) {
		struct child_process cp_rev_list = CHILD_PROCESS_INIT;
		struct strbuf sb_rev_list = STRBUF_INIT;

		strvec_pushl(&cp_rev_list.args, "rev-list",
			     "--first-parent", "--count", NULL);
                 .........
----->8-----

check.

This way, we hit two birds with one stone:

    1. Bypass the 'verify_submodule..()' call when the SM directory does
       not exist. We can then remove the is_directory() test from the
       'verify_submodule_..()' function.

    2. Avoid a 'rev-{parse,list}' fatal error message and thus pass all
       the tests successfully.

Therefore, the final outcome is something like this:

-----8<-----
	if (is_directory(p->sm_path) && S_ISGITLINK(p->mod_src)) {
		src_abbrev = verify_submodule_committish(p->sm_path,
							 oid_to_hex(&p->oid_src));
		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 {
		missing_src = 1;
		/*
		 * 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);
	}

	if (is_directory(p->sm_path) && S_ISGITLINK(p->mod_dst)) {
		dst_abbrev = verify_submodule_committish(p->sm_path,
							 oid_to_hex(&p->oid_dst));
		if (!dst_abbrev) {
			missing_dst = 1;
			/*
			 * As `rev-parse` failed, we fallback to getting
			 * the abbreviated hash using oid_dst. We do
			 * this as we might still need the abbreviated
			 * hash in cases like a submodule type change, etc.
			 */
			dst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);
		}
	} else {
		missing_dst = 1;
		/*
		 * The destination does not point to a submodule.
		 * So, we fallback to getting the abbreviation using
		 * oid_dst as we might still need the abbreviated
		 * hash in cases like a submodule removal, etc.
		 */
		dst_abbrev = xstrndup(oid_to_hex(&p->oid_dst), 7);
	}
----->8-----

Or if is_directory() does not please you then we can make it 'is_nonbare_..()' too. The outcome will be unchanged.

What are your opinions on this?

Yes, this I will do.