git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
BWBrandon Williams <bmwill@google.com>
Date
Jul 24, 2017, 23:03 UTC
Message-ID
<20170724230335.GD92874@google.com>
In-Reply-To
<20170724203454.13947-7-pc44800@gmail.com>
On 07/25, Prathamesh Chavan wrote:
Show 52 quoted lines
> 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.

Show 18 quoted lines
> +			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?
Show 18 quoted lines
> +			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.

Show 154 quoted lines
> +		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
Previous: Prathamesh ChavanNext: Prathamesh Chavan
Message 11 of 41 in “Update: Week 10”
  1. Prathamesh ChavanJul 24, 2017
  2. [GSoC][PATCH 01/13] submodule--helper: introduce get_submodule_displaypath()Prathamesh Chavan, Jul 24, 2017
  3. [GSoC][PATCH 02/13] submodule--helper: introduce for_each_submodule_list()Prathamesh Chavan, Jul 24, 2017
  4. [GSoC][PATCH 03/13] submodule: port set_name_rev() from shell to CPrathamesh Chavan, Jul 24, 2017
  5. Brandon WilliamsJul 24, 2017
  6. [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' from shell to CPrathamesh Chavan, Jul 24, 2017
  7. Brandon WilliamsJul 24, 2017
  8. [GSoC][PATCH 05/13] submodule: port submodule subcommand 'sync' from shell to CPrathamesh Chavan, Jul 24, 2017
  9. Brandon WilliamsJul 24, 2017
  10. [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to CPrathamesh Chavan, Jul 24, 2017
  11. Brandon WilliamsJul 24, 2017
  12. [GSoC][PATCH 07/13] diff: change scope of the function count_lines()Prathamesh Chavan, Jul 24, 2017
  13. [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to CPrathamesh Chavan, Jul 24, 2017
  14. Brandon WilliamsJul 25, 2017
  15. [GSoC][PATCH 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectoryPrathamesh Chavan, Jul 24, 2017
  16. Brandon WilliamsJul 25, 2017
  17. [GSoC][PATCH 10/13] submodule foreach: document '$sm_path' instead of '$path'Prathamesh Chavan, Jul 24, 2017
  18. Brandon WilliamsJul 25, 2017
  19. [GSoC][PATCH 11/13] submodule foreach: clarify the '$toplevel' variable documentationPrathamesh Chavan, Jul 24, 2017
  20. [GSoC][PATCH 12/13] submodule foreach: document variable '$displaypath'Prathamesh Chavan, Jul 24, 2017
  21. Brandon WilliamsJul 25, 2017
  22. [GSoC][PATCH 13/13] submodule: port submodule subcommand 'foreach' from shell to CPrathamesh Chavan, Jul 24, 2017
  23. Brandon WilliamsJul 25, 2017
  24. [GSoC][PATCH v2 00/13] Update: Week 10Prathamesh Chavan, Jul 29, 2017
  25. [GSoC][PATCH v2 01/13] submodule--helper: introduce get_submodule_displaypath()Prathamesh Chavan, Jul 29, 2017
  26. [GSoC][PATCH v2 02/13] submodule--helper: introduce for_each_submodule_list()Prathamesh Chavan, Jul 29, 2017
  27. [GSoC][PATCH v2 03/13] submodule: port set_name_rev() from shell to CPrathamesh Chavan, Jul 29, 2017
  28. [GSoC][PATCH v2 04/13] submodule: port submodule subcommand 'status' from shell to CPrathamesh Chavan, Jul 29, 2017
  29. Christian CouderJul 30, 2017
  30. [GSoC][PATCH v2 05/13] submodule: port submodule subcommand 'sync' from shell to CPrathamesh Chavan, Jul 29, 2017
  31. [GSoC][PATCH v2 06/13] submodule: port submodule subcommand 'deinit' from shell to CPrathamesh Chavan, Jul 29, 2017
  32. [GSoC][PATCH v2 07/13] diff: change scope of the function count_lines()Prathamesh Chavan, Jul 29, 2017
  33. [GSoC][PATCH v2 08/13] submodule: port submodule subcommand 'summary' from shell to CPrathamesh Chavan, Jul 29, 2017
  34. Christian CouderJul 30, 2017
  35. Prathamesh ChavanJul 30, 2017
  36. [GSoC][PATCH v2 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectoryPrathamesh Chavan, Jul 29, 2017
  37. [GSoC][PATCH v2 10/13] submodule foreach: document '$sm_path' instead of '$path'Prathamesh Chavan, Jul 29, 2017
  38. [GSoC][PATCH v2 11/13] submodule foreach: clarify the '$toplevel' variable documentationPrathamesh Chavan, Jul 29, 2017
  39. [GSoC][PATCH v2 12/13] submodule foreach: document variable '$displaypath'Prathamesh Chavan, Jul 29, 2017
  40. [GSoC][PATCH v2 13/13] submodule: port submodule subcommand 'foreach' from shell to CPrathamesh Chavan, Jul 29, 2017
  41. Brandon WilliamsJul 31, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.