Re: [PATCH v2 5/5] submodule: port submodule subcommand 'summary' from shell to C

From: Shourya Shukla <shouryashukla.oo@gmail.com>
Date: Aug 7, 2020, 16:31 UTC
Message-ID: <20200807163135.GA12568@konoha>
In-Reply-To: <xmqq5z9vjsvz.fsf@gitster.c.googlers.com>

On 06/08 03:45, Junio C Hamano wrote:

Show 38 quoted lines

> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> 
> > ...
> 
> > +			argv_array_pushl(&cp_log.args, "--pretty=  %m %s",
> > +					 "--first-parent", NULL);
> > +			argv_array_pushf(&cp_log.args, "%s...%s",
> > +					 src_abbrev,
> > +					 dst_abbrev);
> 
> > ...
> 
> > +	diff_args.argc = setup_revisions(diff_args.argc, diff_args.argv,
> > +					 &rev, NULL);
> 
> Peff's jk/strvec topic will soon be in 'master', and basing the
> series on top of 'master' after that happens would make these lines
> to read like
> 
> 			strvec_pushl(&cp_log.args, "--pretty=  %m %s",
> 				     "--first-parent", NULL);
> 			strvec_pushf(&cp_log.args, "%s...%s",
> 				     src_abbrev,
> 				     dst_abbrev);
> 
> 	diff_args.nr = setup_revisions(diff_args.nr, diff_args.v,
> 				       &rev, NULL);
> 
> We may even be able to reduce line wrapping thanks to shortening a
> few common words:
> 
>     argv_array => strvec
>     argc       => nc
>     argv       => v
> 
> For today's integration, I dealt with these as conflict resolution,
> so let's keep review discussion going, and hope jk/strvec is in
> 'master' by the time this topic becomes ready.

Understood. I will base this patch on the above series. Seems like a great series of great change! BTW, I asked a couple of things in the cover-letter which I think you might have missed. Quoting them here: -----8<----- Also, I want to ask a couple of things:

	1.Whether we can suppress the error message that we get when
	  trying to find the summary of non-existent submodules?
	  For example:

	  fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
	   * my-subm 35b40f1...0000000:

	 Will it be OK to suppress the above error message?

	2.Is it fine to document and expose the 'for-status' option in
	  'git-submodule.txt'?
----->8-----

Regards, Shourya Shukla