git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Brandon Williams <bmwill@google.com>
To: Prathamesh Chavan <pc44800@gmail.com>
Cc: git@vger.kernel.org, sbeller@google.com, christian.couder@gmail.com
Subject: Re: [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to C
Date: Mon, 24 Jul 2017 16:03:35 -0700
Message-ID: <20170724230335.GD92874@google.com> (raw)
In-Reply-To: <20170724203454.13947-7-pc44800@gmail.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

  reply index

Thread overview: 45+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-07-24 20:34 [GSoC][PATCH 00/13] Update: Week 10 Prathamesh Chavan
2017-07-24 20:34 ` [GSoC][PATCH 01/13] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-07-24 20:34 ` [GSoC][PATCH 02/13] submodule--helper: introduce for_each_submodule_list() Prathamesh Chavan
2017-07-24 20:34 ` [GSoC][PATCH 03/13] submodule: port set_name_rev() from shell to C Prathamesh Chavan
2017-07-24 20:54   ` Brandon Williams
2017-07-24 20:34 ` [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' " Prathamesh Chavan
2017-07-24 21:30   ` Brandon Williams
2017-07-24 20:34 ` [GSoC][PATCH 05/13] submodule: port submodule subcommand 'sync' " Prathamesh Chavan
2017-07-24 21:52   ` Brandon Williams
2017-07-24 20:34 ` [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' " Prathamesh Chavan
2017-07-24 23:03   ` Brandon Williams [this message]
2017-07-24 20:34 ` [GSoC][PATCH 07/13] diff: change scope of the function count_lines() Prathamesh Chavan
2017-07-24 20:34 ` [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C Prathamesh Chavan
2017-07-25  0:09   ` Brandon Williams
2017-07-24 20:34 ` [GSoC][PATCH 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectory Prathamesh Chavan
2017-07-25  0:13   ` Brandon Williams
2017-07-24 20:34 ` [GSoC][PATCH 10/13] submodule foreach: document '$sm_path' instead of '$path' Prathamesh Chavan
2017-07-25  0:15   ` Brandon Williams
2017-07-24 20:34 ` [GSoC][PATCH 11/13] submodule foreach: clarify the '$toplevel' variable documentation Prathamesh Chavan
2017-07-24 20:34 ` [GSoC][PATCH 12/13] submodule foreach: document variable '$displaypath' Prathamesh Chavan
2017-07-25  0:16   ` Brandon Williams
2017-07-24 20:34 ` [GSoC][PATCH 13/13] submodule: port submodule subcommand 'foreach' from shell to C Prathamesh Chavan
2017-07-25  0:29   ` Brandon Williams
2017-07-29 22:23 ` [GSoC][PATCH v2 00/13] Update: Week 10 Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 01/13] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 02/13] submodule--helper: introduce for_each_submodule_list() Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 03/13] submodule: port set_name_rev() from shell to C Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 04/13] submodule: port submodule subcommand 'status' " Prathamesh Chavan
2017-07-30  5:35     ` Christian Couder
2017-07-29 22:23   ` [GSoC][PATCH v2 05/13] submodule: port submodule subcommand 'sync' " Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 06/13] submodule: port submodule subcommand 'deinit' " Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 07/13] diff: change scope of the function count_lines() Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 08/13] submodule: port submodule subcommand 'summary' from shell to C Prathamesh Chavan
2017-07-30  5:28     ` Christian Couder
2017-07-30  6:33       ` Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectory Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 10/13] submodule foreach: document '$sm_path' instead of '$path' Prathamesh Chavan
2017-07-29 22:23   ` [GSoC][PATCH v2 11/13] submodule foreach: clarify the '$toplevel' variable documentation Prathamesh Chavan
2017-07-29 22:24   ` [GSoC][PATCH v2 12/13] submodule foreach: document variable '$displaypath' Prathamesh Chavan
2017-07-29 22:24   ` [GSoC][PATCH v2 13/13] submodule: port submodule subcommand 'foreach' from shell to C Prathamesh Chavan
2017-07-31 20:28   ` [GSoC][PATCH v2 00/13] Update: Week 10 Brandon Williams
2017-07-31 20:56 [GSoC][PATCH 00/13] Update: Week-11 Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to C Prathamesh Chavan
2017-07-31 21:42   ` Stefan Beller
2017-08-01 21:19     ` Prathamesh Chavan
2017-08-07 21:18 [GSoC][PATCH 00/13] Update: Week-12 Prathamesh Chavan
2017-08-07 21:18 ` [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to C Prathamesh Chavan

Reply instructions:

You may reply publically 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 to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170724230335.GD92874@google.com \
    --to=bmwill@google.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pc44800@gmail.com \
    --cc=sbeller@google.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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox