git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Shourya Shukla <shouryashukla.oo@gmail.com>
To: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Cc: git@vger.kernel.org, christian.couder@gmail.com,
	liu.denton@gmail.com, kaartic.sivaraam@gmail.com,
	Johannes.Schindelin@gmx.de, gitster@pobox.com, pc44800@gmail.com,
	stefanbeller@gmail.com
Subject: Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C
Date: Mon, 24 Aug 2020 12:56:33 +0530	[thread overview]
Message-ID: <20200824072633.GA38870@konoha> (raw)
In-Reply-To: <5b6ed82f3ab58a194bf51c0e2905214f64246ad8.camel@gmail.com>

On 24/08 01:33, Kaartic Sivaraam wrote:
> > Or the caller of verify_submodule_committish() should refrain from
> > calling it for the path?  After all, it is checking sm_path is a
> > path to where a submodule should be before calling the function
> > (instead of calling it for every random path), iow its criteria to
> > make the call currently may be "the path in the index says it is a
> > submodule", but it should easily be updated to "the path in the
> > index says it is a submodule, and the submodule actually is
> > populated", right?
> > 
> 
> Ah, this reminds me of the initial version of the patch which did
> exactly that. Quoting it here for reference:
> 
> +	strbuf_addstr(&sm_git_dir_sb, p->sm_path);
> +	if (is_nonbare_repository_dir(&sm_git_dir_sb))
> +		is_sm_git_dir = 1;
> +
> +	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));
> +
> +	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));
> +
> 
> Note: `verify_submodule_object_name` is now renamed to
> `verify_submodule_committish`.
> 
> That does sound like a sane approach to me. There's not much point in
> invoking `rev-parse` in a non-populated (a.k.a. de-initialized) or non-
> existent submodule but we removed that check as we thought it was
> unnecessary redundant because `capture_command` would fail anyway.
> Looks like we failed to notice the additional `fatal` message fallout
> then.

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?

> Also, I think it would be better to something like the following in
> t7421 to ensure that `fatal` doesn't sneak up accidentally in the
> future:
> 
> -- 8< --
> 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 --

Yes, this I will do.


  parent reply	other threads:[~2020-08-24  7:26 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-06 16:40 [GSoC][PATCH v2 0/5] submodule: port " Shourya Shukla
2020-08-06 16:40 ` [PATCH v2 1/5] submodule: expose the '--for-status' option of summary Shourya Shukla
2020-08-08 14:40   ` Kaartic Sivaraam
2020-08-08 20:25     ` Christian Couder
2020-08-08 23:26       ` Junio C Hamano
2020-08-06 16:40 ` [PATCH v2 2/5] submodule: remove extra line feeds between callback struct and macro Shourya Shukla
2020-08-06 16:41 ` [PATCH v2 3/5] submodule: rename helper functions to avoid ambiguity Shourya Shukla
2020-08-06 16:41 ` [PATCH v2 4/5] t7421: introduce a test script for verifying 'summary' output Shourya Shukla
2020-08-06 16:41 ` [PATCH v2 5/5] submodule: port submodule subcommand 'summary' from shell to C Shourya Shukla
2020-08-06 22:45   ` Junio C Hamano
2020-08-07 16:31     ` Shourya Shukla
2020-08-07 17:15       ` Junio C Hamano
2020-08-12 19:44 ` [GSoC][PATCH v3 0/4] submodule: port " Shourya Shukla
2020-08-12 19:44   ` [PATCH v3 1/4] submodule: remove extra line feeds between callback struct and macro Shourya Shukla
2020-08-12 19:44   ` [PATCH v3 2/4] submodule: rename helper functions to avoid ambiguity Shourya Shukla
2020-08-12 19:44   ` [PATCH v3 3/4] t7421: introduce a test script for verifying 'summary' output Shourya Shukla
2020-08-12 19:44   ` [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C Shourya Shukla
2020-08-18  2:08     ` Jeff King
2020-08-21  5:22       ` Shourya Shukla
2020-08-21 15:17     ` Johannes Schindelin
2020-08-21 16:35       ` Junio C Hamano
2020-08-21 17:17         ` Shourya Shukla
2020-08-21 18:09           ` Junio C Hamano
2020-08-21 18:54             ` Kaartic Sivaraam
2020-08-21 19:54               ` Junio C Hamano
2020-08-23 20:03                 ` Kaartic Sivaraam
2020-08-23 20:12                   ` Kaartic Sivaraam
2020-08-24  7:26                   ` Shourya Shukla [this message]
2020-08-24  8:46                     ` Shourya Shukla
2020-08-24 11:08                       ` Kaartic Sivaraam
2020-08-24 17:50                         ` Shourya Shukla
2020-08-24 17:54     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200824072633.GA38870@konoha \
    --to=shouryashukla.oo@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=liu.denton@gmail.com \
    --cc=pc44800@gmail.com \
    --cc=stefanbeller@gmail.com \
    --subject='Re: [PATCH v3 4/4] submodule: port submodule subcommand '\''summary'\'' from shell to C' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).