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

From: Junio C Hamano <gitster@pobox.com>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: christian.couder@gmail.com, git@vger.kernel.org,
	Johannes.Schindelin@gmx.de, kaartic.sivaraam@gmail.com,
	liu.denton@gmail.com, Prathamesh Chavan <pc44800@gmail.com>,
	Christian Couder <chriscool@tuxfamily.org>,
	Stefan Beller <stefanbeller@gmail.com>
Subject: Re: [PATCH v3 4/4] submodule: port submodule subcommand 'summary' from shell to C
Date: Mon, 24 Aug 2020 10:54:05 -0700	[thread overview]
Message-ID: <xmqqo8n053r6.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200812194404.17028-5-shouryashukla.oo@gmail.com> (Shourya Shukla's message of "Thu, 13 Aug 2020 01:14:04 +0530")

(A few miniscule things I noticed that are irrelevant to the code
structure discussion).

> +static char* verify_submodule_committish(const char *sm_path,

Style: in C, asterisk sticks to the identifier, not type, i.e.

    static char *verify_submodule_committish(const char *sm_path, ...);

> +static void print_submodule_summary(struct summary_cb *info, char* errmsg,

Likewise; "char *errmsg".

> +static void generate_submodule_summary(struct summary_cb *info,
> +				       struct module_cb *p)
> +{
> +	char *displaypath, *src_abbrev, *dst_abbrev;
> +	int missing_src = 0, missing_dst = 0;
> +	char *errmsg = NULL;
> +	int total_commits = -1;
> +
> +	if (!info->cached && oideq(&p->oid_dst, &null_oid)) {
> +		if (S_ISGITLINK(p->mod_dst)) {
> +...
> +		} else {
> +			/* for a submodule removal (mode:0000000), don't warn */
> +			if (p->mod_dst)
> +				warning(_("unexpected mode %d\n"), p->mod_dst);
> +		}
> +	}

Nobody can read mode bits written in decimal.  Use "%o" instead,
perhaps?

Thanks.