Re: [GSoC][PATCH v2 2/2] submodule: port submodule subcommand status
- From
Stefan Beller <sbeller@google.com>
- Date
- Jun 5, 2017, 23:12 UTC
- Message-ID
- <CAGZ79kZW=B1kd91y6VCushz5tqk1jYW=WzHheHi1pSiMWMQ4iQ@mail.gmail.com>
- In-Reply-To
- <20170605202529.22959-2-pc44800@gmail.com>
On Mon, Jun 5, 2017 at 1:25 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
Show 36 quoted lines
> This aims to make git-submodule subcommand status a builtin. Here > 'status' is ported to submodule--helper, and submodule--helper is > called from git-submodule.sh. > > For the purpose of porting cmd_status, the code is split up such that > one function obtains all the list of submodules, acting as the front-end > of git-submodule status. This function later calls the second function > for_each_submodule_list,it which basically loops through the list of > submodules and calls function fn, which in this case is status_submodule. > The third function, status submodule returns the status of submodule and > also takes care of the recursive flag. > > The first function module_status parses the options present in argv, > and then with the help of module_list_compute, generates the list of > submodules present in the current working tree. > > The second function for_each_submodule_list traverses through the list, > and calls function fn (which in the case of submodule subcommand status > is status_submodule) is called for each entry. > > The third function status_submodule checks for the various conditions, > and prints the status of the submodule accordingly. Also, this function > takes care of the recursive flag by creating a separate child_process > and running it inside the submodule. 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, function print_status is introduced. > > The functions for_each_submodule_list and get_submodule_displaypath > are found to be the same as those in the ported submodule subcommand > foreach's patches. The reason for doing so is to keep both the patches > independant and on separate branches.
Maybe keep it even in a separate patch, such that the status series becomes: patch 1: introduce for_each_submodule_list and get_submodule_displaypath patch 2: port print_name_rev patch 3: port status
whereas the foreach series (and other series later) could re-use patch 1, and build on top of it.
For reviewing patches, it is fine to have the get_submodule_displaypath is both series, though for applying patches it for less complication/deduplication from the maintainer I would think.
Show 16 quoted lines
> +
> +static void print_status(struct status_cb *info, char state, const char *path,
> + char *sub_sha1, char *displaypath)
> +{
> + 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);... with the suggestion given in the print_name_rev patch, this would become a one liner. :)
The rest looks good to me :)