Re: [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to C

[Brandon Williams <bmwill@google.com>]

On 07/25, Prathamesh Chavan wrote:
> The same mechanism is used even for porting this submodule
> subcommand, as used in the ported subcommands till now.
> The function cmd_deinit in split up after porting into three
> functions: module_deinit(), for_each_submodule_list() and
> deinit_submodule().
> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
>  builtin/submodule--helper.c | 141 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  55 +----------------
>  2 files changed, 142 insertions(+), 54 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 2d1d3984d..5e84fc42d 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -910,6 +910,146 @@ static int module_sync(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct deinit_cb {
> +	const char *prefix;
> +	unsigned int quiet: 1;
> +	unsigned int force: 1;
> +	unsigned int all: 1;
> +};
> +#define DEINIT_CB_INIT { NULL, 0, 0, 0 }
> +
> +static void deinit_submodule(const struct cache_entry *list_item,
> +			     void *cb_data)
> +{
> +	struct deinit_cb *info = cb_data;
> +	const struct submodule *sub;
> +	char *displaypath = NULL;
> +	struct child_process cp_config = CHILD_PROCESS_INIT;
> +	struct strbuf sb_config = STRBUF_INIT;
> +	char *sub_git_dir = xstrfmt("%s/.git", list_item->name);
> +	struct stat st;
> +
> +	sub = submodule_from_path(null_sha1, list_item->name);
> +
> +	if (!sub || !sub->name)
> +		goto cleanup;
> +
> +	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> +	/* remove the submodule work tree (unless the user already did it) */
> +	if (is_directory(list_item->name)) {
> +		/* protect submodules containing a .git directory */
> +		if (is_git_directory(sub_git_dir))

This may be too strict of a test.  The original code simply checks if
'submodule/.git' is a directory and dies if it is.  This adds additional
checks ensuring it is a gitdir.  If we want to have a straight
conversion from the shell code we should have this only check if it is a
directory.

> +			die(_("Submodule work tree '%s' contains a .git "
> +			      "directory use 'rm -rf' if you really want "
> +			      "to remove it including all of its history"),
> +			      displaypath);
> +
> +		if (!info->force) {
> +			struct child_process cp_rm = CHILD_PROCESS_INIT;
> +			cp_rm.git_cmd = 1;
> +			argv_array_pushl(&cp_rm.args, "rm", "-qn",
> +					 list_item->name, NULL);
> +
> +			if (run_command(&cp_rm))
> +				die(_("Submodule work tree '%s' contains local "
> +				      "modifications; use '-f' to discard them"),
> +				      displaypath);
> +		}
> +
> +		if (!lstat(list_item->name, &st)) {

What's the purpose of the lstat call here?

> +			struct strbuf sb_rm = STRBUF_INIT;
> +			const char *format;
> +
> +			strbuf_addstr(&sb_rm, list_item->name);
> +
> +			if (!remove_dir_recursively(&sb_rm, 0))
> +				format = _("Cleared directory '%s'\n");
> +			else
> +				format = _("Could not remove submodule work tree '%s'\n");
> +
> +			if (!info->quiet)
> +				printf(format, displaypath);
> +
> +			strbuf_release(&sb_rm);
> +		}
> +	}
> +
> +	if (mkdir(list_item->name, st.st_mode))

What should the mode be when making the empty directory? Right now you
have the potential for it to be garbage as 'st' has the potential of not
being set by the lstat call above (since it happens inside the above if
statement).  Also we may not want it to depend on what the permissions
were set as on the filesystem assuming the user didn't already remove
the submodule themselves.

> +		die(_("could not create empty submodule directory %s"),
> +		      displaypath);
> +
> +	cp_config.git_cmd = 1;
> +	argv_array_pushl(&cp_config.args, "config", "--get-regexp", NULL);
> +	argv_array_pushf(&cp_config.args, "submodule.%s\\.", sub->name);
> +
> +	/* remove the .git/config entries (unless the user already did it) */
> +	if (!capture_command(&cp_config, &sb_config, 0) && sb_config.len) {
> +		char *sub_key = xstrfmt("submodule.%s", sub->name);
> +		/*
> +		 * remove the whole section so we have a clean state when
> +		 * the user later decides to init this submodule again
> +		 */
> +		git_config_rename_section_in_file(NULL, sub_key, NULL);
> +		if (!info->quiet)
> +			printf(_("Submodule '%s' (%s) unregistered for path '%s'\n"),
> +				 sub->name, sub->url, displaypath);
> +		free(sub_key);
> +	}
> +
> +cleanup:
> +	free(displaypath);
> +	free(sub_git_dir);
> +	strbuf_release(&sb_config);
> +}
> +
> +static int module_deinit(int argc, const char **argv, const char *prefix)
> +{
> +	struct deinit_cb info = DEINIT_CB_INIT;
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int force = 0;
> +	int all = 0;
> +
> +	struct option module_deinit_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress submodule status output")),
> +		OPT__FORCE(&force, N_("Remove submodule working trees even if they contain local changes")),
> +		OPT_BOOL(0, "all", &all, N_("Unregister all submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule deinit [--quiet] [-f | --force] [--all | [--] [<path>...]]"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_deinit_options,
> +			     git_submodule_helper_usage, 0);
> +
> +	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> +		BUG("module_list_compute should not choke on empty pathspec");
> +
> +	info.prefix = prefix;
> +	info.quiet = !!quiet;
> +	info.all = !!all;
> +	info.force = !!force;
> +
> +	if (all && argc) {
> +		error("pathspec and --all are incompatible");
> +		usage_with_options(git_submodule_helper_usage,
> +				   module_deinit_options);
> +	}
> +
> +	if (!argc && !all)
> +		die(_("Use '--all' if you really want to deinitialize all submodules"));
> +
> +	gitmodules_config();
> +	for_each_submodule_list(list, deinit_submodule, &info);
> +
> +	return 0;
> +}
> +
>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>  			   const char *depth, struct string_list *reference,
>  			   int quiet, int progress)
> @@ -1640,6 +1780,7 @@ static struct cmd_struct commands[] = {
>  	{"status", module_status, SUPPORT_SUPER_PREFIX},
>  	{"print-default-remote", print_default_remote, 0},
>  	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
> +	{"deinit", module_deinit, SUPPORT_SUPER_PREFIX},
>  	{"remote-branch", resolve_remote_submodule_branch, 0},
>  	{"push-check", push_check, 0},
>  	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 6bfc5e17d..73e6f093f 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -428,60 +428,7 @@ cmd_deinit()
>  		shift
>  	done
>  
> -	if test -n "$deinit_all" && test "$#" -ne 0
> -	then
> -		echo >&2 "$(eval_gettext "pathspec and --all are incompatible")"
> -		usage
> -	fi
> -	if test $# = 0 && test -z "$deinit_all"
> -	then
> -		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
> -	fi
> -
> -	{
> -		git submodule--helper list --prefix "$wt_prefix" "$@" ||
> -		echo "#unmatched" $?
> -	} |
> -	while read -r mode sha1 stage sm_path
> -	do
> -		die_if_unmatched "$mode" "$sha1"
> -		name=$(git submodule--helper name "$sm_path") || exit
> -
> -		displaypath=$(git submodule--helper relative-path "$sm_path" "$wt_prefix")
> -
> -		# Remove the submodule work tree (unless the user already did it)
> -		if test -d "$sm_path"
> -		then
> -			# Protect submodules containing a .git directory
> -			if test -d "$sm_path/.git"
> -			then
> -				die "$(eval_gettext "\
> -Submodule work tree '\$displaypath' contains a .git directory
> -(use 'rm -rf' if you really want to remove it including all of its history)")"
> -			fi
> -
> -			if test -z "$force"
> -			then
> -				git rm -qn "$sm_path" ||
> -				die "$(eval_gettext "Submodule work tree '\$displaypath' contains local modifications; use '-f' to discard them")"
> -			fi
> -			rm -rf "$sm_path" &&
> -			say "$(eval_gettext "Cleared directory '\$displaypath'")" ||
> -			say "$(eval_gettext "Could not remove submodule work tree '\$displaypath'")"
> -		fi
> -
> -		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$displaypath'")"
> -
> -		# Remove the .git/config entries (unless the user already did it)
> -		if test -n "$(git config --get-regexp submodule."$name\.")"
> -		then
> -			# Remove the whole section so we have a clean state when
> -			# the user later decides to init this submodule again
> -			url=$(git config submodule."$name".url)
> -			git config --remove-section submodule."$name" 2>/dev/null &&
> -			say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
> -		fi
> -	done
> +	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} ${force:+--force} ${deinit_all:+--all} "$@"
>  }
>  
>  is_tip_reachable () (
> -- 
> 2.13.0
> 

-- 
Brandon Williams