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 04/13] submodule: port submodule subcommand 'status' from shell to C
Date: Mon, 24 Jul 2017 14:30:28 -0700
Message-ID: <20170724213028.GB92874@google.com> (raw)
In-Reply-To: <20170724203454.13947-5-pc44800@gmail.com>

On 07/25, Prathamesh Chavan wrote:
> This aims to make git-submodule 'status' a built-in. Hence, the function
> cmd_status() is ported from shell to C. This is done by introducing
> three functions: module_status(), submodule_status() and print_status().
> 
> The function module_status() acts as the front-end of the subcommand.
> It parses subcommand's options and then calls the function
> module_list_compute() for computing the list of submodules. Then
> this functions calls for_each_submodule_list() looping through the
> list obtained.
> 
> Then for_each_submodule_list() calls submodule_status() for each of the
> submodule in its list. The function submodule_status() is responsible
> for generating the status each submodule it is called for, and
> then calls print_status().
> 
> Finally, the function print_status() handles the printing of submodule's
> status.
> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> In this new version of patch, following changes were made:
> * instead of using the ce_match_stat(), cmd_diff_files is used.
> * currently, no comment about future scope of optimization wrt the
>   cmd_diff_files() usage was added as currently, I'm not fully sure of
>   the way to optimize the function.
> 
>  builtin/submodule--helper.c | 151 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  49 +-------------
>  2 files changed, 152 insertions(+), 48 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 80f744407..b39828174 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -560,6 +560,156 @@ static int module_init(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct status_cb {
> +	const char *prefix;
> +	unsigned int quiet: 1;
> +	unsigned int recursive: 1;
> +	unsigned int cached: 1;
> +};
> +#define STATUS_CB_INIT { NULL, 0, 0, 0 }
> +
> +static void print_status(struct status_cb *info, char state, const char *path,
> +			 char *sub_sha1, char *displaypath)

Lets mark these strings as 'const char *' since you aren't modifying
them in the function, only using them.  Also it may make more sense to
pass the 'struct object_id' instead of a hex string of the object id.
Then you can call the necessary 'oid_to_hex' function when you are
adding the object id to the argv array.  That would also eliminate an
allocation in 'status_submodule'.

Also eliminates the need to use the word 'sha1' as we want to eliminate
its usage in the codebase.

> +{
> +	if (info->quiet)
> +		return;
> +
> +	printf("%c%s %s", state, sub_sha1, displaypath);
> +
> +	if (state == ' ' || state == '+') {
> +		struct argv_array name_rev_args = ARGV_ARRAY_INIT;
> +
> +		argv_array_pushl(&name_rev_args, "print-name-rev",
> +				 path, sub_sha1, NULL);
> +		print_name_rev(name_rev_args.argc, name_rev_args.argv,
> +			       info->prefix);
> +	} else {
> +		printf("\n");
> +	}
> +}
> +
> +static int handle_submodule_head_ref(const char *refname,
> +				     const struct object_id *oid, int flags,
> +				     void *cb_data)
> +{
> +	struct strbuf *output = cb_data;
> +	if (oid)
> +		strbuf_addstr(output, oid_to_hex(oid));

Since we're going to be working with 'struct object_id' instead of
strings this would need to change slightly.  Instead of copying into a
strbuf we could just pass a pointer to a 'struct object_id' and use
'oidcpy' to copy the contents of oid to output.

  struct object_id *output = cb_data;
  if (oid)
    oidcpy(output, oid);

> +	return 0;
> +}
> +
> +static void status_submodule(const struct cache_entry *list_item, void *cb_data)
> +{
> +	struct status_cb *info = cb_data;
> +	char *sub_sha1 = xstrdup(oid_to_hex(&list_item->oid));
> +	char *displaypath;
> +	struct argv_array diff_files_args = ARGV_ARRAY_INIT;

'diff_files_args' needs to be cleared at the end of the function when
doing cleanup to prevent a memory leak.

> +
> +	if (!submodule_from_path(null_sha1, list_item->name))
> +		die(_("no submodule mapping found in .gitmodules for path '%s'"),
> +		      list_item->name);
> +
> +	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> +	if (list_item->ce_flags) {

Is there a particular flag we are interested in here or only that a flag
is set?

> +		print_status(info, 'U', list_item->name,
> +			     sha1_to_hex(null_sha1), displaypath);

Since we are already using OID's in other parts of this code lets be
consistant and use null_oid instead like 'oid_to_hex(&null_oid)'.

> +		goto cleanup;
> +	}
> +
> +	if (!is_submodule_active(the_repository, list_item->name)) {
> +		print_status(info, '-', list_item->name, sub_sha1, displaypath);
> +		goto cleanup;
> +	}
> +
> +	argv_array_pushl(&diff_files_args, "diff-files",
> +			 "--ignore-submodules=dirty", "--quiet", "--",
> +			 list_item->name, NULL);
> +
> +	if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
> +			    info->prefix)) {
> +		print_status(info, ' ', list_item->name, sub_sha1, displaypath);
> +	} else {
> +		if (!info->cached) {
> +			struct strbuf sb = STRBUF_INIT;
> +			if (head_ref_submodule(list_item->name,
> +					       handle_submodule_head_ref, &sb))
> +				die(_("could not resolve HEAD ref inside the"
> +				      "submodule '%s'"), list_item->name);
> +			print_status(info, '+', list_item->name, sb.buf,
> +				     displaypath);
> +			strbuf_release(&sb);

Like i mentioned above this would change into using a 'struct object_id'
which can be allocated on the stack, eliminating the need for the
allocation and releasing of the strbuf too.

> +		} else {
> +			print_status(info, '+', list_item->name, sub_sha1,
> +				     displaypath);
> +		}
> +	}
> +
> +	if (info->recursive) {
> +		struct child_process cpr = CHILD_PROCESS_INIT;
> +
> +		cpr.git_cmd = 1;
> +		cpr.dir = list_item->name;
> +		prepare_submodule_repo_env(&cpr.env_array);
> +
> +		argv_array_pushl(&cpr.args, "--super-prefix", displaypath,

This bit here doesn't seem right.  'displaypath' can include relative
'..'s if you are not at the root of the project but rather in a
subdirectory.  'super_prefix' has traditionally been defined as the path
from the root of the superproject down to the root of the submodule.
This means that there should not ever be any relative '..'s.

> +				 "submodule--helper", "status", "--recursive",
> +				 NULL);
> +
> +		if (info->cached)
> +			argv_array_push(&cpr.args, "--cached");
> +
> +		if (info->quiet)
> +			argv_array_push(&cpr.args, "--quiet");
> +
> +		if (run_command(&cpr))
> +			die(_("failed to recurse into submodule '%s'"),
> +			      list_item->name);
> +	}
> +
> +cleanup:
> +	free(displaypath);
> +	free(sub_sha1);
> +}
> +
> +static int module_status(int argc, const char **argv, const char *prefix)
> +{
> +	struct status_cb info = STATUS_CB_INIT;
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int cached = 0;
> +	int recursive = 0;
> +
> +	struct option module_status_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress submodule status output")),
> +		OPT_BOOL(0, "cached", &cached, N_("Use commit stored in the index instead of the one stored in the submodule HEAD")),
> +		OPT_BOOL(0, "recursive", &recursive, N_("Recurse into nested submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule status [--quiet] [--cached] [--recursive] [<path>]"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_status_options,
> +			     git_submodule_helper_usage, 0);
> +
> +	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> +		return 1;
> +
> +	info.prefix = prefix;
> +	info.quiet = !!quiet;
> +	info.recursive = !!recursive;
> +	info.cached = !!cached;
> +
> +	gitmodules_config();
> +	for_each_submodule_list(list, status_submodule, &info);
> +
> +	return 0;
> +}
> +
>  static int module_name(int argc, const char **argv, const char *prefix)
>  {
>  	const struct submodule *sub;
> @@ -1306,6 +1456,7 @@ static struct cmd_struct commands[] = {
>  	{"resolve-relative-url-test", resolve_relative_url_test, 0},
>  	{"print-name-rev", print_name_rev, 0},
>  	{"init", module_init, SUPPORT_SUPER_PREFIX},
> +	{"status", module_status, 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 e988167e0..51b057d82 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1005,54 +1005,7 @@ cmd_status()
>  		shift
>  	done
>  
> -	{
> -		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 "$prefix$sm_path" "$wt_prefix")
> -		if test "$stage" = U
> -		then
> -			say "U$sha1 $displaypath"
> -			continue
> -		fi
> -		if ! git submodule--helper is-active "$sm_path" ||
> -		{
> -			! test -d "$sm_path"/.git &&
> -			! test -f "$sm_path"/.git
> -		}
> -		then
> -			say "-$sha1 $displaypath"
> -			continue;
> -		fi
> -		if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path"
> -		then
> -			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
> -			say " $sha1 $displaypath$revname"
> -		else
> -			if test -z "$cached"
> -			then
> -				sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD)
> -			fi
> -			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
> -			say "+$sha1 $displaypath$revname"
> -		fi
> -
> -		if test -n "$recursive"
> -		then
> -			(
> -				prefix="$displaypath/"
> -				sanitize_submodule_env
> -				wt_prefix=
> -				cd "$sm_path" &&
> -				eval cmd_status
> -			) ||
> -			die "$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")"
> -		fi
> -	done
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} "$@"
>  }
>  #
>  # Sync remote urls for submodules
> -- 
> 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 [this message]
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
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
     [not found] <20170731205621.24305-1-pc44800@gmail.com>
2017-07-31 20:56 ` [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' from shell to C Prathamesh Chavan
2017-07-31 21:12   ` Stefan Beller
2017-08-01 21:14     ` Prathamesh Chavan
     [not found] <20170807211900.15001-1-pc44800@gmail.com>
2017-08-07 21:18 ` 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=20170724213028.GB92874@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