git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Atharva Raykar <raykar.ath@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org,
	Christian Couder <christian.couder@gmail.com>,
	Shourya Shukla <shouryashukla.oo@gmail.com>,
	Prathamesh Chavan <pc44800@gmail.com>
Subject: Re: [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand
Date: Wed, 9 Jun 2021 16:01:42 +0530	[thread overview]
Message-ID: <25A3EBAB-AD9A-4FF4-8E6E-63E8ACFF2739@gmail.com> (raw)
In-Reply-To: <xmqqh7i7ll6h.fsf@gitster.g>

On 09-Jun-2021, at 09:54, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Just a bit of random comments, leaving the full review to mentors.
> 
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index d55f6262e9..c9cb535312 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -2745,6 +2745,204 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>> 	return !!ret;
>> }
>> 
>> +struct add_data {
>> +	const char *prefix;
>> +	const char *branch;
>> +	const char *reference_path;
>> +	const char *sm_path;
>> +	const char *sm_name;
>> +	const char *repo;
>> +	const char *realrepo;
>> +	int depth;
>> +	unsigned int force: 1;
>> +	unsigned int quiet: 1;
>> +	unsigned int progress: 1;
>> +	unsigned int dissociate: 1;
>> +};
>> +#define ADD_DATA_INIT { .depth = -1 }
>> +
>> +static char *parse_token(char **begin, const char *end, int *tok_len)
>> +{
>> +	char *tok_start, *pos = *begin;
> 
> Make it a habit to have a blank line between the initial block
> of declarations and the first statement.
> 
>> +	while (pos != end && (*pos != ' ' && *pos != '\t' && *pos != '\n'))
>> +		pos++;
>> +	tok_start = *begin;
>> +	*tok_len = pos - *begin;
>> +	*begin = pos + 1;
>> +	return tok_start;
>> +}
>> +static char *get_next_line(char *const begin, const char *const end)
>> +{
>> +	char *pos = begin;
>> +	while (pos != end && *pos++ != '\n');
> 
> Write an empty loop on two lines, like this:
> 
> 	while (... condition ...)
> 		; /* keep scanning */

OK.

> If there is a NUL byte between begin and end, this keeps going and
> the resulting string will contain one.  Is that a problem?
> 
>> +	return pos;
>> +}
> 
> In general, this project is mature enough that we should question
> ourselves if there is already a suitable line parser we can reuse
> when tempted to write another one.

I will keep this in mind.

>> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
>> +{
>> +	struct child_process cp_remote = CHILD_PROCESS_INIT;
>> +	struct strbuf sb_remote_out = STRBUF_INIT;
>> +
>> +	cp_remote.git_cmd = 1;
>> +	strvec_pushf(&cp_remote.env_array,
>> +		     "GIT_DIR=%s", git_dir_path);
>> +	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
>> +	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
>> +	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
>> +		char *line;
>> +		char *begin = sb_remote_out.buf;
>> +		char *end = sb_remote_out.buf + sb_remote_out.len;
>> +		while (begin != end && (line = get_next_line(begin, end))) {
> 
> OK, so this tries to parse output from "git remote -v", so NUL will
> not be an issue at all.  We will get a string that is NUL terminated
> and has zero or more lines, terminated with LFs.
> 
> If that is the case, I think it is far easier to read without
> a custom get-next-line wrapper, e.g.
> 
> 	for (this_line = begin;
> 	     *this_line;
> 	     this_line = next_line) {
> 		next_line = strchrnul(this_line, '\n');
> 		... process bytes between this_line..next_line ...
> 	}                
> 
>> +			int namelen = 0, urllen = 0, taillen = 0;
>> +			char *name = parse_token(&begin, line, &namelen);
> 
> Similarly, consider if strcspn() is useful in implementing
> parse_token().  See how existing code uses the standard system
> function with
> 
> 	$ git grep strcspn \*.c
> 
>> +			char *url = parse_token(&begin, line, &urllen);
>> +			char *tail = parse_token(&begin, line, &taillen);
>> +			if (!memcmp(tail, "(fetch)", 7))
> 
> At this point do we know there are enough number of bytes after
> tail[0] to allow us to do this comparison safely?  Otherwise,
> 
> 			if (starts_with(tail, "(fetch)")
> 
> may be preferrable.

This solution is definitely an improvement over what I was doing.

That said, I like Danh's suggestion[1] more, because it eliminates the
need for parsing tokens entirely.

The fundamental thing that piece of code was meant to do is:

"If this line ends with '(fetch)', print the line, but without the '(fetch)'"

Parsing tokens only to put them back together through fprintf() may
not be necessary for this usage, so using strchr() with
strip_suffix_mem() should do the trick.

[1] https://lore.kernel.org/git/YL9jTFAoEBP+mDA2@danh.dev/

Thanks for the comments :^)

  reply	other threads:[~2021-06-09 10:33 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-05 11:39 [GSoC] [PATCH 0/2] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-05 11:39 ` [GSoC] [PATCH 1/2] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-06  3:38   ` Bagas Sanjaya
2021-06-06  9:06     ` Christian Couder
2021-06-05 11:39 ` [GSoC] [PATCH 2/2] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-07  9:24   ` Christian Couder
2021-06-07 11:24     ` Atharva Raykar
2021-06-08  9:56 ` [GSoC] [PATCH v2 0/2] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-08  9:56   ` [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-08 12:32     ` Đoàn Trần Công Danh
2021-06-09 10:31       ` Atharva Raykar
2021-06-09 13:06         ` Đoàn Trần Công Danh
2021-06-09 13:10           ` Atharva Raykar
2021-06-09  4:24     ` Junio C Hamano
2021-06-09 10:31       ` Atharva Raykar [this message]
2021-06-08  9:56   ` [GSoC] [PATCH v2 2/2] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-10  8:39   ` [GSoC] [PATCH v3 0/2] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-10  8:39     ` [PATCH v3 1/2] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-11  6:10       ` Junio C Hamano
2021-06-11  7:32         ` Atharva Raykar
2021-06-11  7:59           ` Junio C Hamano
2021-06-10  8:39     ` [PATCH v3 2/2] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-14 12:51     ` [GSoC] [PATCH v4 0/3] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-14 12:51       ` [PATCH v4 1/3] submodule--helper: refactor module_clone() Atharva Raykar
2021-06-15  3:51         ` Junio C Hamano
2021-06-15  9:03           ` Atharva Raykar
2021-06-14 12:51       ` [PATCH v4 2/3] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-14 12:51       ` [PATCH v4 3/3] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-14 19:51         ` Rafael Silva
2021-06-14 20:12           ` Eric Sunshine
2021-06-15  9:37             ` Rafael Silva
2021-06-15  7:09           ` Atharva Raykar
2021-06-15  9:38       ` [GSoC] [PATCH v5 0/3] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-15  9:38         ` [PATCH v5 1/3] submodule--helper: refactor module_clone() Atharva Raykar
2021-06-15  9:38         ` [PATCH v5 2/3] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-15  9:38         ` [PATCH v5 3/3] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-15 14:57         ` [GSoC] [PATCH v6 0/3] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-15 14:57           ` [PATCH v6 1/3] submodule--helper: refactor module_clone() Atharva Raykar
2021-06-15 14:57           ` [PATCH v6 2/3] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-15 14:57           ` [PATCH v6 3/3] submodule--helper: introduce add-config subcommand Atharva Raykar

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=25A3EBAB-AD9A-4FF4-8E6E-63E8ACFF2739@gmail.com \
    --to=raykar.ath@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pc44800@gmail.com \
    --cc=shouryashukla.oo@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).