Re: [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

On 2021-06-09 16:01:40+0530, Atharva Raykar <raykar.ath@gmail.com> wrote:
> On 08-Jun-2021, at 18:02, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> > 
> >> [...]
> >> +static char *get_next_line(char *const begin, const char *const end)
> >> +{
> >> +	char *pos = begin;
> >> +	while (pos != end && *pos++ != '\n');
> >> +	return pos;
> >> +}
> > 
> > On my first glance, this function looks like a reinvention of strchr(3).
> > Except that, this function also has a second parameter for "end".
> > Maybe it has a specical use-case?
> > 
> >> +
> >> +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))) {
> > 
> > And this is the only use-case.  Because you also want to check if you
> > reached the last token or not.  I guess you really meant to write:
> > 
> > 	while ((line = strchr(begin, '\n')) != NULL) {
> > 
> > Anyway, I would name the "line" variable as "nextline"
> > 
> >> +			int namelen = 0, urllen = 0, taillen = 0;
> >> +			char *name = parse_token(&begin, line, &namelen);
> >> +			char *url = parse_token(&begin, line, &urllen);
> >> +			char *tail = parse_token(&begin, line, &taillen);
> >> +			if (!memcmp(tail, "(fetch)", 7))
> >> +				fprintf(output, "  %.*s\t%.*s\n",
> >> +					namelen, name, urllen, url);
> > 
> > I think this whole block is better replaced with strip_suffix_mem and
> > fprintf.
> > 
> > Overral I would replace the block inside capture_command with:
> > 
> > -----8<-----
> > 	char *nextline;
> > 	char *line = sb_remote_out.buf;
> > 	while ((nextline = strchr(line, '\n')) != NULL) {
> > 		size_t len = nextline - line;
> > 		if (strip_suffix_mem(line, &len, "(fetch)"))
> > 			fprintf(output, "  %.*s\n", (int)len, line);

Fix-up for my suggestion:

To be bug-for-bug with shell implementation, it should be:

		if (strip_suffix_mem(line, &len, " (fetch)"))

> > 		line = nextline + 1;
> > 	}
> > ---->8-----
> > 
> > And get rid of parse_token and get_next_line functions.
> 
> That looks much simpler. Thanks!
> 
> I realised that all the token parsing I do is not really necessary.
> What I really want to do is "If this line ends with '(fetch)',
> print it, but without the '(fetch)'", and I think your version
> captures that succinctly.
> 
> >> +		}
> >> +	}
> >> +
> >> +	strbuf_release(&sb_remote_out);
> >> +}
> >> +
> >> +static int add_submodule(const struct add_data *add_data)
> >> +{
> >> +	char *submod_gitdir_path;
> >> +	/* perhaps the path already exists and is already a git repo, else clone it */
> >> +	if (is_directory(add_data->sm_path)) {
> >> +		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
> >> +		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
> >> +			printf(_("Adding existing path at '%s' to index\n"),
> >> +			       add_data->sm_path);
> >> +		else
> >> +			die(_("'%s' already exists and is not a valid git repo"),
> >> +			    add_data->sm_path);
> >> +		free(submod_gitdir_path);
> >> +	} else {
> >> +		struct strvec clone_args = STRVEC_INIT;
> >> +		struct child_process cp = CHILD_PROCESS_INIT;
> >> +		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
> >> +
> >> +		if (is_directory(submod_gitdir_path)) {
> >> +			if (!add_data->force) {
> >> +				error(_("A git directory for '%s' is found "
> >> +					"locally with remote(s):"), add_data->sm_name);
> > 
> > We don't capitalise first character of error message.
> > IOW, downcase "A git".
> 
> Got it.
> 
> > Well, it's bug-for-bug with shell implementation, so it doesn't matter much, anyway.
> 
> While it is meant to be a faithful implementation, I think this
> is a good opportunity to make minor style fixes.
> 
> >> +				show_fetch_remotes(stderr, add_data->sm_name,
> >> +						   submod_gitdir_path);
> >> +				fprintf(stderr,
> >> +					_("If you want to reuse this local git "
> >> +					  "directory instead of cloning again from\n"
> >> +					  "  %s\n"
> >> +					  "use the '--force' option. If the local git "
> >> +					  "directory is not the correct repo\n"
> >> +					  "or if you are unsure what this means, choose "
> >> +					  "another name with the '--name' option.\n"),
> >> +					add_data->realrepo);
> > 
> > Is there any reason we can't use "error" here?
> 
> The message in its entirety looks like this:
> 
> error: A git directory for 'test' is found locally with remote(s):
>   origin	git@github.com:tfidfwastaken/abc.git
> If you want to reuse this local git directory instead of cloning again from
>   git@github.com:tfidfwastaken/test.git
> use the '--force' option. If the local git directory is not the correct repo
> or if you are unsure what this means, choose another name with the '--name' option.
> 
> Since the 'error:' is already there in the first line, having it
> prepended before 'If you want to reuse...' felt redundant to me.
> 
> Besides, it's more of an informational message about what a user
> can do next, rather than a message that signifies an error.
> 
> If there is a preferred convention or label for such messages,
> I can use that. The shell version did not have any such thing though.
> 
> >> [...]
> >> +	struct option options[] = {
> >> +		OPT_STRING('b', "branch", &add_data.branch,
> >> +			   N_("branch"),
> >> +			   N_("branch of repository to checkout on cloning")),
> >> +		OPT_STRING(0, "prefix", &add_data.prefix,
> >> +			   N_("path"),
> >> +			   N_("alternative anchor for relative paths")),
> >> +		OPT_STRING(0, "path", &add_data.sm_path,
> >> +			   N_("path"),
> >> +			   N_("where the new submodule will be cloned to")),
> >> +		OPT_STRING(0, "name", &add_data.sm_name,
> >> +			   N_("string"),
> >> +			   N_("name of the new submodule")),
> >> +		OPT_STRING(0, "url", &add_data.realrepo,
> >> +			   N_("string"),
> >> +			   N_("url where to clone the submodule from")),
> >> +		OPT_STRING(0, "reference", &add_data.reference_path,
> >> +			   N_("repo"),
> >> +			   N_("reference repository")),
> >> +		OPT_BOOL(0, "dissociate", &dissociate,
> >> +			 N_("use --reference only while cloning")),
> >> +		OPT_INTEGER(0, "depth", &add_data.depth,
> >> +			    N_("depth for shallow clones")),
> >> +		OPT_BOOL(0, "progress", &progress,
> >> +			 N_("force cloning progress")),
> >> +		OPT_BOOL('f', "force", &force,
> >> +			 N_("allow adding an otherwise ignored submodule path")),
> > 
> > We have OPT__FORCE, too.
> 
> Will switch over to that.
> 
> >> +		OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
> > 
> > And, please downcase "Suppress".
> 
> OK.
> 
> >> +		OPT_END()
> >> +	};
> >> +
> >> +	const char *const usage[] = {
> >> +		N_("git submodule--helper add-clone [--prefix=<path>] [--quiet] [--force] "
> >> +		   "[--reference <repository>] [--depth <depth>] [-b|--branch <branch>]"
> >> +		   "[--progress] [--dissociate] --url <url> --path <path> --name <name>"),
> > 
> > I think it's too crowded here, I guess it's better to write:
> > 
> > 	N_("git submodule--helper add-clone [<options>...] --url <url> --path <path> --name <name>"),
> 
> OK. It shouldn't be an issue to shorten it, because this is not
> user-facing, and is only ever used within 'cmd_add()'.
> 
> >> +		NULL
> >> +	};
> >> +
> >> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> > 
> > From above usage, I think url, path, name is required, should we have a check for them, here?
> 
> We could. The reason why I was not too rigorous about this is
> because I plan to eliminate the shell interface for this helper
> eventually and call add-clone from within C, in the next few
> patches.
> 
> But this is a small ask, and I can just add a quick check just
> to be extra safe, so I'll do it.
> 
> >> +
> >> +	add_data.progress = !!progress;
> >> +	add_data.dissociate = !!dissociate;
> >> +	add_data.force = !!force;
> >> +	add_data.quiet = !!quiet;
> >> +
> >> +	if (add_submodule(&add_data))
> >> +		return 1;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> #define SUPPORT_SUPER_PREFIX (1<<0)
> >> 
> >> struct cmd_struct {
> >> @@ -2757,6 +2955,7 @@ static struct cmd_struct commands[] = {
> >> 	{"list", module_list, 0},
> >> 	{"name", module_name, 0},
> >> 	{"clone", module_clone, 0},
> >> +	{"add-clone", add_clone, 0},
> >> 	{"update-module-mode", module_update_module_mode, 0},
> >> 	{"update-clone", update_clone, 0},
> >> 	{"ensure-core-worktree", ensure_core_worktree, 0},
> >> diff --git a/git-submodule.sh b/git-submodule.sh
> >> index 4678378424..f71e1e5495 100755
> >> --- a/git-submodule.sh
> >> +++ b/git-submodule.sh
> >> @@ -241,43 +241,7 @@ cmd_add()
> >> 		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
> >> 	fi
> >> 
> >> -	# perhaps the path exists and is already a git repo, else clone it
> >> -	if test -e "$sm_path"
> >> -	then
> >> -		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
> >> -		then
> >> -			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
> >> -		else
> >> -			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
> >> -		fi
> >> -
> >> -	else
> >> -		if test -d ".git/modules/$sm_name"
> >> -		then
> >> -			if test -z "$force"
> >> -			then
> >> -				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
> >> -				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
> >> -				die "$(eval_gettextln "\
> >> -If you want to reuse this local git directory instead of cloning again from
> >> -  \$realrepo
> >> -use the '--force' option. If the local git directory is not the correct repo
> >> -or you are unsure what this means choose another name with the '--name' option.")"
> >> -			else
> >> -				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
> >> -			fi
> >> -		fi
> >> -		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
> >> -		(
> >> -			sanitize_submodule_env
> >> -			cd "$sm_path" &&
> >> -			# ash fails to wordsplit ${branch:+-b "$branch"...}
> >> -			case "$branch" in
> >> -			'') git checkout -f -q ;;
> >> -			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
> >> -			esac
> >> -		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
> >> -	fi
> >> +	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
> >> 	git config submodule."$sm_name".url "$realrepo"
> >> 
> >> 	git add --no-warn-embedded-repo $force "$sm_path" ||
> >> -- 
> >> 2.31.1
> >> 
> > 
> > -- 
> > Danh
> 
> Thanks for the comments!

-- 
Danh