From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: Johannes.Schindelin@gmx.de, christian.couder@gmail.com,
git@vger.kernel.org, gitster@pobox.com, liu.denton@gmail.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 16:38:39 +0530 [thread overview]
Message-ID: <a30f43ecbbc5fa64fe62eb5903d81bce7440986c.camel@gmail.com> (raw)
In-Reply-To: <20200824084634.GA377527@konoha>
On Mon, 2020-08-24 at 14:16 +0530, Shourya Shukla wrote:
> 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
next prev parent reply other threads:[~2020-08-24 11:09 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 subcommand 'summary' from shell to C 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
2020-08-24 8:46 ` Shourya Shukla
2020-08-24 11:08 ` Kaartic Sivaraam [this message]
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a30f43ecbbc5fa64fe62eb5903d81bce7440986c.camel@gmail.com \
--to=kaartic.sivaraam@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=liu.denton@gmail.com \
--cc=pc44800@gmail.com \
--cc=shouryashukla.oo@gmail.com \
--cc=stefanbeller@gmail.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).