submodule--helper: introduce get_submodule_displaypath()
PATCH v2 1/4 submodule--helper: introduce get_submodule_displaypath()
Introduce function get_submodule_displaypath() to replace the code occurring in submodule_init() for generating displaypath of the submodule with a call to it.
This new function will also be used in other parts of the system in later patches.
---
As said in the previous update,
a short patch series is floated for the maintainer's review, and is consisting of the following changes:
* introduce function get_submodule_displaypath()
* introduce function for_each_submodule_list()
* port function set_name_rev() from shell to C
* port submodule subcommand 'status' from shell to C
The complete build report for the above series of patches is available at:
https://travis-ci.org/pratham-pc/git/builds
Branch: week-14-1
Build #158
The above changes are also push on github and are available at:
https://github.com/pratham-pc/git/commits/week-14-1
The above changes were based on master branch.
But along with the above changes, the same series was also applied to a separate branch based on the 'next' branch. The changes were applicable without any additional changes to the patches. Complete build report of that is also available at:
https://travis-ci.org/pratham-pc/git/builds
Branch: week-14-1-next
Build #159
The above changes are also push on github and are available at:
https://github.com/pratham-pc/git/commits/week-14-1-next
builtin/submodule--helper.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)
builtin/submodule--helper.c +22 -11
@@ -220,6 +220,27 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *prreturn 0 ; } +static char *get_submodule_displaypath (const char *path,const char *prefix) +{ +const char *super_prefix =get_super_prefix (); + +if (prefix && super_prefix) { +BUG ("cannot have prefix '%s' and superprefix '%s'" , + prefix, super_prefix); + }else if (prefix) { +struct strbuf sb =STRBUF_INIT ; +char *displaypath =xstrdup (relative_path (path, prefix, &sb)); +strbuf_release (&sb); +return displaypath; + }else if (super_prefix) { +int len =strlen (super_prefix); +const char *format =is_dir_sep (super_prefix[len -1 ]) ?"%s%s" :"%s/%s" ; +return xstrfmt (format, super_prefix, path); + }else { +return xstrdup (path); + } +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -339,16 +360,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)/* Only loads from .gitmodules, no overlay with .git/config */ gitmodules_config (); - -if (prefix &&get_super_prefix ()) -die ("BUG: cannot have prefix and superprefix" ); -else if (prefix) - displaypath =xstrdup (relative_path (path, prefix, &sb)); -else if (get_super_prefix ()) { -strbuf_addf (&sb,"%s%s" ,get_super_prefix (), path); - displaypath =strbuf_detach (&sb,NULL ); - }else - displaypath =xstrdup (path); + displaypath =get_submodule_displaypath (path, prefix); sub =submodule_from_path (&null_oid, path);
@@ -363,7 +375,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); }
Signature · 1 line
2.13.0
PATCH v2 2/4 submodule--helper: introduce for_each_submodule_list()
Introduce function for_each_submodule_list() and replace a loop in module_init() with a call to it.
The new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-)
builtin/submodule--helper.c +29 -10
@@ -14,6 +14,9 @@#include "refs.h" #include "connect.h" +typedef void (*submodule_list_func_t )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;
@@ -352,17 +355,30 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_submodule_list (const struct module_list list, +submodule_list_func_t fn,void *cb_data) { +int i; +for (i =0 ; i < list.nr; i++) +fn (list.entries[i], cb_data); +} + +struct init_cb { +const char *prefix; +unsigned int quiet:1 ; +}; +#define INIT_CB_INIT { NULL, 0 } + +static void init_submodule (const struct cache_entry *list_item,void *cb_data) +{ +struct init_cb *info = cb_data;const struct submodule *sub;struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; -/* Only loads from .gitmodules, no overlay with .git/config */ -gitmodules_config (); - displaypath =get_submodule_displaypath (path, prefix); + displaypath =get_submodule_displaypath (list_item->name, info->prefix); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (&null_oid, list_item->name);if (!sub) die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -374,7 +390,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* * Set active flag for the submodule being initialized */ -if (!is_submodule_active (the_repository, path)) { +if (!is_submodule_active (the_repository, list_item->name)) {strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); }
@@ -416,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)if (git_config_set_gently (sb.buf, url))die (_ ("Failed to register url for submodule path '%s'" ), displaypath); -if (!quiet) +if (!info->quiet)fprintf (stderr,_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath);
@@ -445,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)static int module_init (int argc,const char **argv,const char *prefix) { +struct init_cb info =INIT_CB_INIT ;struct pathspec pathspec;struct module_list list =MODULE_LIST_INIT ;int quiet =0 ; -int i;struct option module_init_options[] = {OPT__QUIET (&quiet,N_ ("Suppress output for initializing a submodule" )),
@@ -473,8 +489,11 @@ static int module_init(int argc, const char **argv, const char *prefix)if (!argc &&git_config_get_value_multi ("submodule.active" ))module_list_active (&list); -for (i =0 ; i < list.nr; i++) -init_submodule (list.entries[i]->name, prefix, quiet); + info.prefix = prefix; + info.quiet = !!quiet; + +gitmodules_config (); +for_each_submodule_list (list, init_submodule, &info);return 0 ; }
Signature · 1 line
2.13.0
Re: [GSoC][PATCH 2/4] submodule--helper: introduce for_each_submodule_list()
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 359
Quoted 3 lines · Prathamesh Chavan
builtin/submodule--helper.c +2
-1-static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_submodule_list (const struct module_list list, +submodule_list_func_t fn,void *cb_data)
It may not be wrong per-se, but can't this just be for_each_submodule()?
Your "justification" may be that this makes it clear that you are iterating over module_list and not other kind of group of submodules, but I would say the design of the subsystem is broken if some places use a list of submodules while some other places use an array of submodules to represent a group of submodules. Especially when there is a dedicated type to hold a group of submodules, i.e. struct module-list, that type should be used consistently throughout the subsystem and API, no?
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 364
Quoted 5 lines
{
builtin/submodule--helper.c +4
-0+int i; +for (i =0 ; i < list.nr; i++) +fn (list.entries[i], cb_data); +}
Also, did you really want to pass the structure by value? At least in C, it is more customary to pass these things by pointer, i.e.
for_each_submodule(struct module_list *list,
for_each_submodule_fn fn,
void *cb_data)
{
for (i = 0; i < list->nr; i++)
...Otherwise you'd be making a copy on stack unnecessarily (ok, "const" might hint a smart compiler to turn this inefficient code to pass it by pointer, but I do not think it is a particulary good to rely on such things).
PATCH v2 3/4 submodule: port set_name_rev() from shell to C
In reply to Prathamesh ChavanFunction set_name_rev() is ported from git-submodule to the submodule--helper builtin. The function get_name_rev() generates the value of the revision name as required, and the function print_name_rev() handles the formating and printing of the obtained revision name.
--- builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 16 ++---------- 2 files changed, 65 insertions(+), 14 deletions(-)
builtin/submodule--helper.c +63 -0
@@ -244,6 +244,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix) } } +static char *get_name_rev (const char *sub_path,const char * object_id) +{ +struct strbuf sb =STRBUF_INIT ; +const char ***d; + +static const char *describe_bare[] = { +NULL + }; + +static const char *describe_tags[] = { +"--tags" ,NULL + }; + +static const char *describe_contains[] = { +"--contains" ,NULL + }; + +static const char *describe_all_always[] = { +"--all" ,"--always" ,NULL + }; + +static const char **describe_argv[] = { + describe_bare, describe_tags, describe_contains, + describe_all_always,NULL + }; + +for (d = describe_argv; *d; d++) { +struct child_process cp =CHILD_PROCESS_INIT ; +prepare_submodule_repo_env (&cp.env_array); + cp.dir = sub_path; + cp.git_cmd =1 ; + cp.no_stderr =1 ; + +argv_array_push (&cp.args,"describe" ); +argv_array_pushv (&cp.args, *d); +argv_array_push (&cp.args, object_id); + +if (!capture_command (&cp, &sb,0 ) && sb.len) { +strbuf_strip_suffix (&sb,"\n" ); +return strbuf_detach (&sb,NULL ); + } + } + +strbuf_release (&sb); +return NULL ; +} + +static int print_name_rev (int argc,const char **argv,const char *prefix) +{ +char *namerev; +if (argc !=3 ) +die ("print-name-rev only accepts two arguments: <path> <sha1>" ); + + namerev =get_name_rev (argv[1 ], argv[2 ]); +if (namerev && namerev[0 ]) +printf (" (%s)" , namerev); +printf ("\n" ); + +free (namerev); +return 0 ; +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = { {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, + {"print-name-rev", print_name_rev, 0}, {"init", module_init, SUPPORT_SUPER_PREFIX}, {"remote-branch", resolve_remote_submodule_branch, 0}, {"push-check", push_check, 0},
git-submodule.sh +2 -14
@@ -759,18 +759,6 @@ cmd_update() } } -set_name_rev () { - revname=$( ( - sanitize_submodule_env - cd "$1" && { - git describe "$2" 2>/dev/null || - git describe --tags "$2" 2>/dev/null || - git describe --contains "$2" 2>/dev/null || - git describe --all --always "$2" - } - ) ) - test -z "$revname" || revname=" ($revname)" -} # # Show commit summary for submodules in index or working tree #
@@ -1042,14 +1030,14 @@ cmd_status() fi if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path" then - set_name_rev "$sm_path" "$sha1" + 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 - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1") say "+$sha1 $displaypath$revname" fi
Signature · 1 line
2.13.0
Re: [GSoC][PATCH 3/4] submodule: port set_name_rev() from shell to C
Quoted 18 lines · Prathamesh Chavan
Function
set_name_rev()is ported from git-submodule to the submodule--helper builtin. The functionget_name_rev()generates the value of the revision name as required, and the functionprint_name_rev()handles the formating and printing of the obtained revision name.Mentored-by Christian Couder <[email protected]>Mentored-by Stefan Beller <[email protected]>Signed-off-by Prathamesh Chavan <[email protected]>--- builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 16 ++---------- 2 files changed, 65 insertions(+), 14 deletions(-)builtin/submodule--helper.c +0
-0
[...]
builtin/submodule--helper.c static struct cmd_struct commands[] = { · line 1307
Quoted 5 lines
builtin/submodule--helper.c +1
-0@@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = { {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, + {"print-name-rev", print_name_rev, 0},
I see the function in git-submodule.sh was named kind of reverse. How about we do it more naturally here and call this 'rev-name' instead? That makes is more clear to me and is also similar to the used variable name 'revname'.
I would also prefix it differently like 'get' or 'calculate' instead of 'print' since it tries to find a name and is not just a simple lookup.
So in summary I would prefer something like 'get-rev-name' as a name for the subcommand here.
Quoted 46 lines
builtin/submodule--helper.c +0
-0{"init", module_init, SUPPORT_SUPER_PREFIX}, {"remote-branch", resolve_remote_submodule_branch, 0}, {"push-check", push_check, 0},git-submodule.sh +2
-14@@ -759,18 +759,6 @@ cmd_update() } } -set_name_rev () { - revname=$( ( - sanitize_submodule_env - cd "$1" && { - git describe "$2" 2>/dev/null || - git describe --tags "$2" 2>/dev/null || - git describe --contains "$2" 2>/dev/null || - git describe --all --always "$2" - } - ) ) - test -z "$revname" || revname=" ($revname)" -} # # Show commit summary for submodules in index or working tree #@@ -1042,14 +1030,14 @@ cmd_status() fi if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path" then - set_name_rev "$sm_path" "$sha1" + 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 - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1") say "+$sha1 $displaypath$revname" fiSignature · 1 line
2.13.0
Re: [GSoC][PATCH 3/4] submodule: port set_name_rev() from shell to C
Quoted 22 lines · Heiko Voigt
Quoted 18 lines · Prathamesh Chavan
Function
set_name_rev()is ported from git-submodule to the submodule--helper builtin. The functionget_name_rev()generates the value of the revision name as required, and the functionprint_name_rev()handles the formating and printing of the obtained revision name.Mentored-by Christian Couder <[email protected]>Mentored-by Stefan Beller <[email protected]>Signed-off-by Prathamesh Chavan <[email protected]>--- builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 16 ++---------- 2 files changed, 65 insertions(+), 14 deletions(-)builtin/submodule--helper.c +0
-0[...]
Quoted 10 lines
builtin/submodule--helper.c static struct cmd_struct commands[] = { · line 1307
Quoted 5 lines
builtin/submodule--helper.c +1
-0@@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = { {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, + {"print-name-rev", print_name_rev, 0},I see the function in
git-submodule.shwas named kind of reverse. How about we do it more naturally here and call this 'rev-name' instead? That makes is more clear to me and is also similar to the used variable name 'revname'.
The functions 'print_name_rev()' and 'get_name_rev()' are the ported C functions of the function 'set_name_rev()'
Their names were assigned so due to the existing function's name, and hence to faithfully port the functions.
But, thanks for the above suggestion, and also for reviewing the patch. I will update the names as print_rev_name() and get_rev_name() respectively.
Quoted 3 lines
I would also prefix it differently like 'get' or 'calculate' instead of 'print' since it tries to find a name and is not just a simple lookup.
The former function from the shell script, 'set_name_rev()' is split into two functions, namely: 'print_name_rev()' and 'get_name_rev()' The function print_name_rev() is just the front_end of the function, and exists to printf the return value of the get_name_rev() function is the required format.
Calculation of the value is actually done by the function get_name_rev().
Hence, I named the functions the way they are.
Thanks,
Prathamesh Chavan
PATCH v2 4/4 submodule: port submodule subcommand 'status' from shell to C
In reply to Prathamesh ChavanThis 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.
---
The patch below underwent some changes after its previous version.
In the shell script, the submodules, which had merge-conflicts are identified by checking if the $stage variable has the value 'U' Till the last patch series, this was done by checking if ce_flags have a non-zero value. In this new patch series, this was changed and instead ce_stage() was called on the list_item(cache_entry), and then we check whether it has a non-zero value.
builtin/submodule--helper.c | 156 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 49 +------------- 2 files changed, 157 insertions(+), 48 deletions(-)
builtin/submodule--helper.c +156 -0
@@ -560,6 +560,161 @@ 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, +const struct object_id *oid,const char *displaypath) +{ +if (info->quiet) +return ; + +printf ("%c%s %s" , state,oid_to_hex (oid), displaypath); + +if (state ==' ' || state =='+' ) { +struct argv_array name_rev_args =ARGV_ARRAY_INIT ; + +argv_array_pushl (&name_rev_args,"print-name-rev" , + path,oid_to_hex (oid),NULL ); +print_name_rev (name_rev_args.argc, name_rev_args.argv, + info->prefix); + +argv_array_clear (&name_rev_args); + }else { +printf ("\n" ); + } +} + +static int handle_submodule_head_ref (const char *refname, +const struct object_id *oid,int flags, +void *cb_data) +{ +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 *displaypath; +struct argv_array diff_files_args =ARGV_ARRAY_INIT ; + +if (!submodule_from_path (&null_oid, 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 (ce_stage (list_item)) { +print_status (info,'U' , list_item->name, + &null_oid, displaypath); + goto cleanup; + } + +if (!is_submodule_active (the_repository, list_item->name)) { +print_status (info,'-' , list_item->name, &list_item->oid, + 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, &list_item->oid, + displaypath); + }else { +if (!info->cached) { +struct object_id oid; + +if (head_ref_submodule (list_item->name, + handle_submodule_head_ref, &oid)) +die (_ ("could not resolve HEAD ref inside the" +"submodule '%s'" ), list_item->name); + +print_status (info,'+' , list_item->name, &oid, + displaypath); + }else { +print_status (info,'+' , list_item->name, + &list_item->oid, 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, +"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: +argv_array_clear (&diff_files_args); +free (displaypath); +} + +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 +1461,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},
git-submodule.sh +1 -48
55 lines not on this page · raw message
Signature · 1 line
2.13.0
Quoted 3 lines · Prathamesh Chavan
Introduce function
get_submodule_displaypath()to replace the code occurring insubmodule_init()for generating displaypath of the submodule with a call to it.
Looks like a quite straight-forward refactoring.
Review
Quoted 12 lines
(quoted diff) +12
-0+static char *get_submodule_displaypath(const char *path, const char *prefix) +{ + const char *super_prefix = get_super_prefix(); + + if (prefix && super_prefix) { + BUG("cannot have prefix '%s' and superprefix '%s'", + prefix, super_prefix); + } else if (prefix) { + struct strbuf sb = STRBUF_INIT; + char *displaypath = xstrdup(relative_path(path, prefix, &sb)); + strbuf_release(&sb); + return displaypath;
Use of xstrdup() would waste one extra allocation and copy, no? In other words, wouldn't this do the same thing?
... {
struct strbuf sb = STRBUF_INIT;
relative_path(path, prefix, &sb);
return strbuf_detach(&sb, NULL);Review static void init_submodule(const char *path, const char *prefix, int quiet) · line 380
Quoted 8 lines
(quoted diff) +0
-1@@ -363,7 +375,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet) * Set active flag for the submodule being initialized */ if (!is_submodule_active(the_repository, path)) { - strbuf_reset(&sb); strbuf_addf(&sb, "submodule.%s.active", sub->name); git_config_set_gently(sb.buf, "true"); }
This is because sb will stay untouched with the use of the new helper. With the way this single strbuf is reused throughout this function, I cannot help wondering if the prevailing pattern of resetting and then using is a mistake. If the strbuf is mostly used as a scratchpad, wouldn't it make more sense to use it and then clean up when you are done with it?
I.e. something along this line that shows only two uses...
builtin/submodule--helper.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
builtin/submodule--helper.c +2 -3
@@ -363,9 +363,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); +strbuf_reset (&sb); }/*
@@ -373,7 +373,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* To look up the url in .git/config, we must not fall back to * .gitmodules, so look it up directly. */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.url" , sub->name);if (git_config_get_string (sb.buf, &url)) {if (!sub->url)
@@ -410,9 +409,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath); } +strbuf_reset (&sb);/* Copy "update" setting when it is not set yet */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.update" , sub->name);if (git_config_get_string (sb.buf, &upd) && sub->update_strategy.type !=SM_UPDATE_UNSPECIFIED ) {
v2 cover submodule: Incremental rewrite of git-submodules
Changes in v2:
* In the function get_submodule_displaypath(), I tried avoiding the extra memory allocation, but it rather invoked test 5 from t7406-submodule-update. The details of the test failiure can be seen in the build report as well. (Build #162)
This failure occured as even though the function relative_path(), returned the value correctly, NULL was stored in sb.buf
Hence currently the function is still using the old way of allocating extra memory and releasing the strbuf first, and return the extra allocated memory.
* It was observed that strbuf_reset was being done just before the strbuf was being reused. It made more sense to reset them just after their use instead. Hence, these function calls of strbuf_reset() were moved accordingly.
(The above change was limited to the function init_submodule() only,
since already there was a deletion of one such funciton call, and IMO, it won't be suitable to carryout the operation for the complete file in the same commit.)
* The function name for_each_submodule_list() was changed to for_each_submodule().
* Instead of passing the complete list to the function for_each_submodule(), only its pointer is passed to avoid the compiler from making copy of the list, and to make the code more efficient.
* The function names of print_name_rev() and get_name_rev() were changed to get_rev_name() and compute_rev_name(). Now the function get_rev_name() acts as the front end of the subcommand and calls the function compute_rev_name() for generating and receiving the value of revision name.
* The above change was also accompanied by the change in names of some variables used internally in the functions.
As before you can find this series at:
https://github.com/pratham-pc/git/commits/week-14-1
And the build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: week-14-1
Build #163
Prathamesh Chavan (4):
submodule--helper: introduce get_submodule_displaypath()
submodule--helper: introduce for_each_submodule()
submodule: port set_name_rev() from shell to C
submodule: port submodule subcommand 'status' from shell to C
builtin/submodule--helper.c | 294 ++++++++++++++++++++++++++++++++++++++++---- git-submodule.sh | 61 +-------- 2 files changed, 273 insertions(+), 82 deletions(-)
Signature · 1 line
2.13.0
PATCH v2 1/4 submodule--helper: introduce get_submodule_displaypath()
Introduce function get_submodule_displaypath() to replace the code occurring in submodule_init() for generating displaypath of the submodule with a call to it.
This new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)
builtin/submodule--helper.c +25 -13
@@ -220,6 +220,28 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *prreturn 0 ; } +static char *get_submodule_displaypath (const char *path,const char *prefix) +{ +const char *super_prefix =get_super_prefix (); + +if (prefix && super_prefix) { +BUG ("cannot have prefix '%s' and superprefix '%s'" , + prefix, super_prefix); + }else if (prefix) { +struct strbuf sb =STRBUF_INIT ; +char *displaypath =xstrdup (relative_path (path, prefix, &sb)); +strbuf_release (&sb); +return displaypath; + }else if (super_prefix) { +int len =strlen (super_prefix); +const char *format =is_dir_sep (super_prefix[len -1 ]) ?"%s%s" :"%s/%s" ; + +return xstrfmt (format, super_prefix, path); + }else { +return xstrdup (path); + } +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -339,16 +361,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)/* Only loads from .gitmodules, no overlay with .git/config */ gitmodules_config (); - -if (prefix &&get_super_prefix ()) -die ("BUG: cannot have prefix and superprefix" ); -else if (prefix) - displaypath =xstrdup (relative_path (path, prefix, &sb)); -else if (get_super_prefix ()) { -strbuf_addf (&sb,"%s%s" ,get_super_prefix (), path); - displaypath =strbuf_detach (&sb,NULL ); - }else - displaypath =xstrdup (path); + displaypath =get_submodule_displaypath (path, prefix); sub =submodule_from_path (&null_oid, path);
@@ -363,9 +376,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); +strbuf_reset (&sb); }/*
@@ -373,7 +386,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* To look up the url in .git/config, we must not fall back to * .gitmodules, so look it up directly. */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.url" , sub->name);if (git_config_get_string (sb.buf, &url)) {if (!sub->url)
@@ -410,9 +422,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath); } +strbuf_reset (&sb);/* Copy "update" setting when it is not set yet */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.update" , sub->name);if (git_config_get_string (sb.buf, &upd) && sub->update_strategy.type !=SM_UPDATE_UNSPECIFIED ) {
Signature · 1 line
2.13.0
PATCH v2 2/4 submodule--helper: introduce for_each_submodule()
In reply to Prathamesh ChavanIntroduce function for_each_submodule() and replace a loop in module_init() with a call to it.
The new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-)
builtin/submodule--helper.c +29 -10
@@ -14,6 +14,9 @@#include "refs.h" #include "connect.h" +typedef void (*submodule_list_func_t )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;
@@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_submodule (const struct module_list *list, +submodule_list_func_t fn,void *cb_data) +{ +int i; +for (i =0 ; i < list->nr; i++) +fn (list->entries[i], cb_data); +} + +struct init_cb { +const char *prefix; +unsigned int quiet:1 ; +}; +#define INIT_CB_INIT { NULL, 0 } + +static void init_submodule (const struct cache_entry *list_item,void *cb_data) { +struct init_cb *info = cb_data;const struct submodule *sub;struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; -/* Only loads from .gitmodules, no overlay with .git/config */ -gitmodules_config (); - displaypath =get_submodule_displaypath (path, prefix); + displaypath =get_submodule_displaypath (list_item->name, info->prefix); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (&null_oid, list_item->name);if (!sub) die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -375,7 +391,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* * Set active flag for the submodule being initialized */ -if (!is_submodule_active (the_repository, path)) { +if (!is_submodule_active (the_repository, list_item->name)) {strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" );strbuf_reset (&sb);
@@ -417,7 +433,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)if (git_config_set_gently (sb.buf, url))die (_ ("Failed to register url for submodule path '%s'" ), displaypath); -if (!quiet) +if (!info->quiet)fprintf (stderr,_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath);
@@ -446,10 +462,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)static int module_init (int argc,const char **argv,const char *prefix) { +struct init_cb info =INIT_CB_INIT ;struct pathspec pathspec;struct module_list list =MODULE_LIST_INIT ;int quiet =0 ; -int i;struct option module_init_options[] = {OPT__QUIET (&quiet,N_ ("Suppress output for initializing a submodule" )),
@@ -474,8 +490,11 @@ static int module_init(int argc, const char **argv, const char *prefix)if (!argc &&git_config_get_value_multi ("submodule.active" ))module_list_active (&list); -for (i =0 ; i < list.nr; i++) -init_submodule (list.entries[i]->name, prefix, quiet); + info.prefix = prefix; + info.quiet = !!quiet; + +gitmodules_config (); +for_each_submodule (&list, init_submodule, &info);return 0 ; }
Signature · 1 line
2.13.0
Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 360
Quoted 13 lines · Prathamesh Chavan
builtin/submodule--helper.c +5
-1+typedef void (*submodule_list_func_t )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;@@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_submodule (const struct module_list *list, +submodule_list_func_t fn,void *cb_data)
In the output from
$ git grep for_each \*.h
we find that the convention is that an interator over a group of X is for_each_X, the callback function that is given to for_each_X is of type each_X_fn. An interator over a subset of group of X that has trait Y, for_each_Y_X() iterates and calls back a function of type each_X_fn (e.g. for_each_tag_ref() still calls each_ref_fn).
I do not offhand think of a reason why the above code need to deviate from that pattern.
Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()
Quoted 18 lines · Junio C Hamano
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 360
Quoted 13 lines · Prathamesh Chavan
builtin/submodule--helper.c +5
-1+typedef void (*submodule_list_func_t )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;@@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_submodule (const struct module_list *list, +submodule_list_func_t fn,void *cb_data)In the output from
Quoted 4 lines
$ git grep for_each \*.hwe find that the convention is that an interator over a group of X is
for_each_X,
... which this is...
Quoted 2 lines
the callback function that is given to
for_each_Xis of typeeach_X_fn.
So you suggest s/submodule_list_func_t/each_submodule_fn/
Quoted 3 lines
An interator over a subset of group of X that
has trait Y,for_each_Y_X()iterates and calls back a function of typeeach_X_fn(e.g.for_each_tag_ref()still callseach_ref_fn).
This reads as a suggestion for for_each_listed_submodule as the name.
Quoted 2 lines
I do not offhand think of a reason why the above code need to deviate from that pattern.
Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()
Quoted 24 lines · Stefan Beller
Quoted 18 lines · Junio C Hamano
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 360
Quoted 13 lines · Prathamesh Chavan
builtin/submodule--helper.c +5
-1+typedef void (*submodule_list_func_t )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;@@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_submodule (const struct module_list *list, +submodule_list_func_t fn,void *cb_data)In the output from
Quoted 4 lines
$ git grep for_each \*.hwe find that the convention is that an interator over a group of X is
for_each_X,
Quoted 6 lines
... which this is...
Quoted 2 lines
the callback function that is given to
for_each_Xis of typeeach_X_fn.So you suggest s/submodule_list_func_t/each_submodule_fn/
It's not _I_ suggest---the remainder of the codebase screams that the above name is wrong ;-). Didn't it for the mentors while they were reading this code?
Quoted 6 lines
Quoted 3 lines
An interator over a subset of group of X that
has trait Y,for_each_Y_X()iterates and calls back a function of typeeach_X_fn(e.g.for_each_tag_ref()still callseach_ref_fn).This reads as a suggestion for
for_each_listed_submoduleas the name.
If you need to have two ways to iterate over them, i.e. (1) over all submodules and (2) over only the listed ones (whatever that means), then yes, for_each_listed_submodule() would be a good name for the latter which would be a complement to for_each_submodule() that is the former.
Quoted 3 lines
Quoted 3 lines
I do not offhand think of a reason why the above code need to deviate from that pattern.
v3 cover Incremental rewrite of git-submodules
Changes in v3:
* The name of the iterator function for_each_submodule() was changed to the for_each_listed_submodule(), as the function fits the naming pattern for_each_Y_X(), as here we iterate over group of listed submodules (X) which are listed (Y) by the function module_list_compute()
* The name of the call back function type for the above pattern for_each_Y_X() is each_X_fn. Hence, this pattern was followed and the name of the call back function type for for_each_listed_submodule() was changed from submodule_list_func_t to each_submodule_fn.
As before you can find this series at:
https://github.com/pratham-pc/git/commits/week-14-1
And its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: week-14-1
Build #164
Prathamesh Chavan (4):
submodule--helper: introduce get_submodule_displaypath()
submodule--helper: introduce for_each_listed_submodule()
submodule: port set_name_rev() from shell to C
submodule: port submodule subcommand 'status' from shell to C
builtin/submodule--helper.c | 294 ++++++++++++++++++++++++++++++++++++++++---- git-submodule.sh | 61 +-------- 2 files changed, 273 insertions(+), 82 deletions(-)
Signature · 1 line
2.13.0
PATCH v3 1/4 submodule--helper: introduce get_submodule_displaypath()
Introduce function get_submodule_displaypath() to replace the code occurring in submodule_init() for generating displaypath of the submodule with a call to it.
This new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)
builtin/submodule--helper.c +25 -13
@@ -220,6 +220,28 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *prreturn 0 ; } +static char *get_submodule_displaypath (const char *path,const char *prefix) +{ +const char *super_prefix =get_super_prefix (); + +if (prefix && super_prefix) { +BUG ("cannot have prefix '%s' and superprefix '%s'" , + prefix, super_prefix); + }else if (prefix) { +struct strbuf sb =STRBUF_INIT ; +char *displaypath =xstrdup (relative_path (path, prefix, &sb)); +strbuf_release (&sb); +return displaypath; + }else if (super_prefix) { +int len =strlen (super_prefix); +const char *format =is_dir_sep (super_prefix[len -1 ]) ?"%s%s" :"%s/%s" ; + +return xstrfmt (format, super_prefix, path); + }else { +return xstrdup (path); + } +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -339,16 +361,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)/* Only loads from .gitmodules, no overlay with .git/config */ gitmodules_config (); - -if (prefix &&get_super_prefix ()) -die ("BUG: cannot have prefix and superprefix" ); -else if (prefix) - displaypath =xstrdup (relative_path (path, prefix, &sb)); -else if (get_super_prefix ()) { -strbuf_addf (&sb,"%s%s" ,get_super_prefix (), path); - displaypath =strbuf_detach (&sb,NULL ); - }else - displaypath =xstrdup (path); + displaypath =get_submodule_displaypath (path, prefix); sub =submodule_from_path (&null_oid, path);
@@ -363,9 +376,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); +strbuf_reset (&sb); }/*
@@ -373,7 +386,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* To look up the url in .git/config, we must not fall back to * .gitmodules, so look it up directly. */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.url" , sub->name);if (git_config_get_string (sb.buf, &url)) {if (!sub->url)
@@ -410,9 +422,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath); } +strbuf_reset (&sb);/* Copy "update" setting when it is not set yet */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.update" , sub->name);if (git_config_get_string (sb.buf, &upd) && sub->update_strategy.type !=SM_UPDATE_UNSPECIFIED ) {
Signature · 1 line
2.13.0
PATCH v3 2/4 submodule--helper: introduce for_each_listed_submodule()
In reply to Prathamesh ChavanIntroduce function for_each_listed_submodule() and replace a loop in module_init() with a call to it.
The new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-)
builtin/submodule--helper.c +29 -10
@@ -14,6 +14,9 @@#include "refs.h" #include "connect.h" +typedef void (*each_submodule_fn )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;
@@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_listed_submodule (const struct module_list *list, +each_submodule_fn fn,void *cb_data) +{ +int i; +for (i =0 ; i < list->nr; i++) +fn (list->entries[i], cb_data); +} + +struct init_cb { +const char *prefix; +unsigned int quiet:1 ; +}; +#define INIT_CB_INIT { NULL, 0 } + +static void init_submodule (const struct cache_entry *list_item,void *cb_data) { +struct init_cb *info = cb_data;const struct submodule *sub;struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; -/* Only loads from .gitmodules, no overlay with .git/config */ -gitmodules_config (); - displaypath =get_submodule_displaypath (path, prefix); + displaypath =get_submodule_displaypath (list_item->name, info->prefix); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (&null_oid, list_item->name);if (!sub) die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -375,7 +391,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* * Set active flag for the submodule being initialized */ -if (!is_submodule_active (the_repository, path)) { +if (!is_submodule_active (the_repository, list_item->name)) {strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" );strbuf_reset (&sb);
@@ -417,7 +433,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)if (git_config_set_gently (sb.buf, url))die (_ ("Failed to register url for submodule path '%s'" ), displaypath); -if (!quiet) +if (!info->quiet)fprintf (stderr,_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath);
@@ -446,10 +462,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)static int module_init (int argc,const char **argv,const char *prefix) { +struct init_cb info =INIT_CB_INIT ;struct pathspec pathspec;struct module_list list =MODULE_LIST_INIT ;int quiet =0 ; -int i;struct option module_init_options[] = {OPT__QUIET (&quiet,N_ ("Suppress output for initializing a submodule" )),
@@ -474,8 +490,11 @@ static int module_init(int argc, const char **argv, const char *prefix)if (!argc &&git_config_get_value_multi ("submodule.active" ))module_list_active (&list); -for (i =0 ; i < list.nr; i++) -init_submodule (list.entries[i]->name, prefix, quiet); + info.prefix = prefix; + info.quiet = !!quiet; + +gitmodules_config (); +for_each_listed_submodule (&list, init_submodule, &info);return 0 ; }
Signature · 1 line
2.13.0
PATCH v3 3/4 submodule: port set_name_rev() from shell to C
In reply to Prathamesh ChavanFunction set_name_rev() is ported from git-submodule to the submodule--helper builtin. The function compute_rev_name() generates the value of the revision name as required.
The function get_rev_name() calls compute_rev_name() and receives the revision name, and later handles its formating and printing.
--- builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 16 ++---------- 2 files changed, 65 insertions(+), 14 deletions(-)
builtin/submodule--helper.c +63 -0
@@ -245,6 +245,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix) } } +static char *compute_rev_name (const char *sub_path,const char * object_id) +{ +struct strbuf sb =STRBUF_INIT ; +const char ***d; + +static const char *describe_bare[] = { +NULL + }; + +static const char *describe_tags[] = { +"--tags" ,NULL + }; + +static const char *describe_contains[] = { +"--contains" ,NULL + }; + +static const char *describe_all_always[] = { +"--all" ,"--always" ,NULL + }; + +static const char **describe_argv[] = { + describe_bare, describe_tags, describe_contains, + describe_all_always,NULL + }; + +for (d = describe_argv; *d; d++) { +struct child_process cp =CHILD_PROCESS_INIT ; +prepare_submodule_repo_env (&cp.env_array); + cp.dir = sub_path; + cp.git_cmd =1 ; + cp.no_stderr =1 ; + +argv_array_push (&cp.args,"describe" ); +argv_array_pushv (&cp.args, *d); +argv_array_push (&cp.args, object_id); + +if (!capture_command (&cp, &sb,0 ) && sb.len) { +strbuf_strip_suffix (&sb,"\n" ); +return strbuf_detach (&sb,NULL ); + } + } + +strbuf_release (&sb); +return NULL ; +} + +static int get_rev_name (int argc,const char **argv,const char *prefix) +{ +char *revname; +if (argc !=3 ) +die ("get-rev-name only accepts two arguments: <path> <sha1>" ); + + revname =compute_rev_name (argv[1 ], argv[2 ]); +if (revname && revname[0 ]) +printf (" (%s)" , revname); +printf ("\n" ); + +free (revname); +return 0 ; +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -1243,6 +1305,7 @@ static struct cmd_struct commands[] = { {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, + {"get-rev-name", get_rev_name, 0}, {"init", module_init, SUPPORT_SUPER_PREFIX}, {"remote-branch", resolve_remote_submodule_branch, 0}, {"push-check", push_check, 0},
git-submodule.sh +2 -14
@@ -759,18 +759,6 @@ cmd_update() } } -set_name_rev () { - revname=$( ( - sanitize_submodule_env - cd "$1" && { - git describe "$2" 2>/dev/null || - git describe --tags "$2" 2>/dev/null || - git describe --contains "$2" 2>/dev/null || - git describe --all --always "$2" - } - ) ) - test -z "$revname" || revname=" ($revname)" -} # # Show commit summary for submodules in index or working tree #
@@ -1042,14 +1030,14 @@ cmd_status() fi if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path" then - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$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 - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$sm_path" "$sha1") say "+$sha1 $displaypath$revname" fi
Signature · 1 line
2.13.0
PATCH v3 4/4 submodule: port submodule subcommand 'status' from shell to C
In reply to Prathamesh ChavanThis 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_listed_submodule() looping through the list obtained.
Then for_each_listed_submodule() 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.
--- builtin/submodule--helper.c | 156 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 49 +------------- 2 files changed, 157 insertions(+), 48 deletions(-)
builtin/submodule--helper.c +156 -0
@@ -561,6 +561,161 @@ 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, +const struct object_id *oid,const char *displaypath) +{ +if (info->quiet) +return ; + +printf ("%c%s %s" , state,oid_to_hex (oid), displaypath); + +if (state ==' ' || state =='+' ) { +struct argv_array get_rev_args =ARGV_ARRAY_INIT ; + +argv_array_pushl (&get_rev_args,"get-rev-name" , + path,oid_to_hex (oid),NULL ); +get_rev_name (get_rev_args.argc, get_rev_args.argv, + info->prefix); + +argv_array_clear (&get_rev_args); + }else { +printf ("\n" ); + } +} + +static int handle_submodule_head_ref (const char *refname, +const struct object_id *oid,int flags, +void *cb_data) +{ +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 *displaypath; +struct argv_array diff_files_args =ARGV_ARRAY_INIT ; + +if (!submodule_from_path (&null_oid, 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 (ce_stage (list_item)) { +print_status (info,'U' , list_item->name, + &null_oid, displaypath); + goto cleanup; + } + +if (!is_submodule_active (the_repository, list_item->name)) { +print_status (info,'-' , list_item->name, &list_item->oid, + 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, &list_item->oid, + displaypath); + }else { +if (!info->cached) { +struct object_id oid; + +if (head_ref_submodule (list_item->name, + handle_submodule_head_ref, &oid)) +die (_ ("could not resolve HEAD ref inside the" +"submodule '%s'" ), list_item->name); + +print_status (info,'+' , list_item->name, &oid, + displaypath); + }else { +print_status (info,'+' , list_item->name, + &list_item->oid, 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, +"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: +argv_array_clear (&diff_files_args); +free (displaypath); +} + +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_listed_submodule (&list, status_submodule, &info); + +return 0 ; +} +static int module_name (int argc,const char **argv,const char *prefix) {const struct submodule *sub;
@@ -1307,6 +1462,7 @@ static struct cmd_struct commands[] = { {"resolve-relative-url-test", resolve_relative_url_test, 0}, {"get-rev-name", get_rev_name, 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},
git-submodule.sh +1 -48
55 lines not on this page · raw message
Signature · 1 line
2.13.0
Re: [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules
In reply to Prathamesh ChavanThanks. I'll try to queue these before I'll go offline.
Mentors may want to help the student further in adjusting the patch series to the more recent codebase; unfortunately the area the GSoC project touches is a bit fluid these days. I resolved the conflicts with nd/pune-in-worktree and bw/submodule-config-cleanup topics so that the result would compile, but I am not sure if the resolution is correct (e.g. there may be a new leak I introduced while doing so).
Thanks.
Re: [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules
Quoted 11 lines · Junio C Hamano
Thanks. I'll try to queue these before I'll go offline.
Mentors may want to help the student further in adjusting the patch series to the more recent codebase; unfortunately the area the GSoC project touches is a bit fluid these days. I resolved the conflicts with nd/pune-in-worktree and bw/submodule-config-cleanup topics so that the result would compile, but I am not sure if the resolution is correct (e.g. there may be a new leak I introduced while doing so).
Thanks.
Ok, noted.
Presumably I'll review your dirty merge then for this series? (And later parts might go on top of the new dirty merge)
Re: [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules
Quoted 11 lines · Stefan Beller
Quoted 10 lines · Junio C Hamano
Thanks. I'll try to queue these before I'll go offline.
Mentors may want to help the student further in adjusting the patch series to the more recent codebase; unfortunately the area the GSoC project touches is a bit fluid these days. I resolved the conflicts with nd/pune-in-worktree and bw/submodule-config-cleanup topics so that the result would compile, but I am not sure if the resolution is correct (e.g. there may be a new leak I introduced while doing so).
Quoted 6 lines
Quoted 1 line
Thanks.
Ok, noted.
Presumably I'll review your dirty merge then for this series? (And later parts might go on top of the new dirty merge)
In my mind, GSoC mentors are there to help the student improve as a developer, more than they are to help improving the immediate output of the student. So in that sense, I was hoping that you'd teach how to work well together with other developers, when one's topic has interactions with topics by others. Making sure an inevitable evil merge is made correctly is of course needed for the current topic, but a more important skill to learn is to avoid the need to ask an evil merge to be made by the integrator in the first place.
Re: [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules
In reply to Junio C HamanoQuoted 10 lines · Junio C Hamano
Thanks. I'll try to queue these before I'll go offline.
Mentors may want to help the student further in adjusting the patch series to the more recent codebase; unfortunately the area the GSoC project touches is a bit fluid these days. I resolved the conflicts with nd/pune-in-worktree and bw/submodule-config-cleanup topics so that the result would compile, but I am not sure if the resolution is correct (e.g. there may be a new leak I introduced while doing so).
Thanks.
I'll see the dirty merges and will resend the whole series after reviewing the dirty merge and sending a new one with/without changes as required.
Thanks,
Prathamesh Chavan
v4 cover Incremental rewrite of git-submodules
In reply to Junio C HamanoChanges in v4:
* The patches were adjusted to recent codebase. Also, the gitmodules_config() was call was removed from the functions module_init() and module_status() which was essential after the merge of the branch bw/submodule-config-cleanup.
Since it was mentioned earlier, I even tried basing the patch series on the branch nd/prune-in-worktree. Conflicts occured while doing so, since the later branch is based on the master branch with the HEAD pointing to the commit: The fourth batch post 2.14 I think there won't be any conflicts, if that is changed.
As before you can find this series at:
https://github.com/pratham-pc/git/commits/patch-series-1
And its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-1
Build #167
The above changes were based on master branch.
Another branch, similar to the above, was created, but was based on the 'next' branch.
Complete build report of that is also available at:
https://travis-ci.org/pratham-pc/git/builds
Branch: patch-series-1-next
Build #168
The above changes are also push on github and are available at:
https://github.com/pratham-pc/git/commits/patch-series-1-next
Prathamesh Chavan (4):
submodule--helper: introduce get_submodule_displaypath()
submodule--helper: introduce for_each_listed_submodule()
submodule: port set_name_rev() from shell to C
submodule: port submodule subcommand 'status' from shell to C
builtin/submodule--helper.c | 289 +++++++++++++++++++++++++++++++++++++++++--- git-submodule.sh | 61 +--------- 2 files changed, 271 insertions(+), 79 deletions(-)
Signature · 1 line
2.13.0
PATCH v4 1/4 submodule--helper: introduce get_submodule_displaypath()
Introduce function get_submodule_displaypath() to replace the code occurring in submodule_init() for generating displaypath of the submodule with a call to it.
This new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)
builtin/submodule--helper.c +25 -12
@@ -220,6 +220,28 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *prreturn 0 ; } +static char *get_submodule_displaypath (const char *path,const char *prefix) +{ +const char *super_prefix =get_super_prefix (); + +if (prefix && super_prefix) { +BUG ("cannot have prefix '%s' and superprefix '%s'" , + prefix, super_prefix); + }else if (prefix) { +struct strbuf sb =STRBUF_INIT ; +char *displaypath =xstrdup (relative_path (path, prefix, &sb)); +strbuf_release (&sb); +return displaypath; + }else if (super_prefix) { +int len =strlen (super_prefix); +const char *format =is_dir_sep (super_prefix[len -1 ]) ?"%s%s" :"%s/%s" ; + +return xstrfmt (format, super_prefix, path); + }else { +return xstrdup (path); + } +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -335,15 +357,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; -if (prefix &&get_super_prefix ()) -die ("BUG: cannot have prefix and superprefix" ); -else if (prefix) - displaypath =xstrdup (relative_path (path, prefix, &sb)); -else if (get_super_prefix ()) { -strbuf_addf (&sb,"%s%s" ,get_super_prefix (), path); - displaypath =strbuf_detach (&sb,NULL ); - }else - displaypath =xstrdup (path); + displaypath =get_submodule_displaypath (path, prefix); sub =submodule_from_path (&null_oid, path);
@@ -358,9 +372,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); +strbuf_reset (&sb); }/*
@@ -368,7 +382,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* To look up the url in .git/config, we must not fall back to * .gitmodules, so look it up directly. */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.url" , sub->name);if (git_config_get_string (sb.buf, &url)) {if (!sub->url)
@@ -405,9 +418,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath); } +strbuf_reset (&sb);/* Copy "update" setting when it is not set yet */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.update" , sub->name);if (git_config_get_string (sb.buf, &upd) && sub->update_strategy.type !=SM_UPDATE_UNSPECIFIED ) {
Signature · 1 line
2.13.0
PATCH v4 1/4 submodule--helper: introduce get_submodule_displaypath()
LGTM with nits.
+static char *get_submodule_displaypath(const char *path, const char *prefix)
this could do with a comment
/* the result should be freed by the caller. */
+ } else if (super_prefix) {
+ int len = strlen(super_prefix);
+ const char *format = is_dir_sep(super_prefix[len - 1]) ? "%s%s" : "%s/%s";
what if len == 0? The handling of '/' looks like a change from the original.
PATCH v4 2/4 submodule--helper: introduce for_each_listed_submodule()
In reply to Prathamesh ChavanIntroduce function for_each_listed_submodule() and replace a loop in module_init() with a call to it.
The new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-)
builtin/submodule--helper.c +28 -8
@@ -14,6 +14,9 @@#include "refs.h" #include "connect.h" +typedef void (*each_submodule_fn )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;
@@ -351,15 +354,30 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_listed_submodule (const struct module_list *list, +each_submodule_fn fn,void *cb_data) +{ +int i; +for (i =0 ; i < list->nr; i++) +fn (list->entries[i], cb_data); +} + +struct init_cb { +const char *prefix; +unsigned int quiet:1 ; +}; +#define INIT_CB_INIT { NULL, 0 } + +static void init_submodule (const struct cache_entry *list_item,void *cb_data) { +struct init_cb *info = cb_data;const struct submodule *sub;struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; - displaypath =get_submodule_displaypath (path, prefix); + displaypath =get_submodule_displaypath (list_item->name, info->prefix); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (&null_oid, list_item->name);if (!sub) die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -371,7 +389,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* * Set active flag for the submodule being initialized */ -if (!is_submodule_active (the_repository, path)) { +if (!is_submodule_active (the_repository, list_item->name)) {strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" );strbuf_reset (&sb);
@@ -413,7 +431,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)if (git_config_set_gently (sb.buf, url))die (_ ("Failed to register url for submodule path '%s'" ), displaypath); -if (!quiet) +if (!info->quiet)fprintf (stderr,_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath);
@@ -442,10 +460,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)static int module_init (int argc,const char **argv,const char *prefix) { +struct init_cb info =INIT_CB_INIT ;struct pathspec pathspec;struct module_list list =MODULE_LIST_INIT ;int quiet =0 ; -int i;struct option module_init_options[] = {OPT__QUIET (&quiet,N_ ("Suppress output for initializing a submodule" )),
@@ -470,8 +488,10 @@ static int module_init(int argc, const char **argv, const char *prefix)if (!argc &&git_config_get_value_multi ("submodule.active" ))module_list_active (&list); -for (i =0 ; i < list.nr; i++) -init_submodule (list.entries[i]->name, prefix, quiet); + info.prefix = prefix; + info.quiet = !!quiet; + +for_each_listed_submodule (&list, init_submodule, &info);return 0 ; }
Signature · 1 line
2.13.0
PATCH v4 3/4 submodule: port set_name_rev() from shell to C
In reply to Prathamesh ChavanFunction set_name_rev() is ported from git-submodule to the submodule--helper builtin. The function compute_rev_name() generates the value of the revision name as required.
The function get_rev_name() calls compute_rev_name() and receives the revision name, and later handles its formating and printing.
--- builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 16 ++---------- 2 files changed, 65 insertions(+), 14 deletions(-)
builtin/submodule--helper.c +63 -0
@@ -245,6 +245,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix) } } +static char *compute_rev_name (const char *sub_path,const char * object_id) +{ +struct strbuf sb =STRBUF_INIT ; +const char ***d; + +static const char *describe_bare[] = { +NULL + }; + +static const char *describe_tags[] = { +"--tags" ,NULL + }; + +static const char *describe_contains[] = { +"--contains" ,NULL + }; + +static const char *describe_all_always[] = { +"--all" ,"--always" ,NULL + }; + +static const char **describe_argv[] = { + describe_bare, describe_tags, describe_contains, + describe_all_always,NULL + }; + +for (d = describe_argv; *d; d++) { +struct child_process cp =CHILD_PROCESS_INIT ; +prepare_submodule_repo_env (&cp.env_array); + cp.dir = sub_path; + cp.git_cmd =1 ; + cp.no_stderr =1 ; + +argv_array_push (&cp.args,"describe" ); +argv_array_pushv (&cp.args, *d); +argv_array_push (&cp.args, object_id); + +if (!capture_command (&cp, &sb,0 ) && sb.len) { +strbuf_strip_suffix (&sb,"\n" ); +return strbuf_detach (&sb,NULL ); + } + } + +strbuf_release (&sb); +return NULL ; +} + +static int get_rev_name (int argc,const char **argv,const char *prefix) +{ +char *revname; +if (argc !=3 ) +die ("get-rev-name only accepts two arguments: <path> <sha1>" ); + + revname =compute_rev_name (argv[1 ], argv[2 ]); +if (revname && revname[0 ]) +printf (" (%s)" , revname); +printf ("\n" ); + +free (revname); +return 0 ; +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -1292,6 +1354,7 @@ static struct cmd_struct commands[] = { {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, + {"get-rev-name", get_rev_name, 0}, {"init", module_init, SUPPORT_SUPER_PREFIX}, {"remote-branch", resolve_remote_submodule_branch, 0}, {"push-check", push_check, 0},
git-submodule.sh +2 -14
@@ -758,18 +758,6 @@ cmd_update() } } -set_name_rev () { - revname=$( ( - sanitize_submodule_env - cd "$1" && { - git describe "$2" 2>/dev/null || - git describe --tags "$2" 2>/dev/null || - git describe --contains "$2" 2>/dev/null || - git describe --all --always "$2" - } - ) ) - test -z "$revname" || revname=" ($revname)" -} # # Show commit summary for submodules in index or working tree #
@@ -1041,14 +1029,14 @@ cmd_status() fi if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path" then - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$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 - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$sm_path" "$sha1") say "+$sha1 $displaypath$revname" fi
Signature · 1 line
2.13.0
PATCH v4 3/4 submodule: port set_name_rev() from shell to C
LGTM with nits
commit message:
"revision name, and later handles its formating and printing."
typo: formatting
+ if (!capture_command(&cp, &sb, 0) && sb.len) {
+ strbuf_strip_suffix(&sb, "\n");
+ return strbuf_detach(&sb, NULL);
+ }
you discard all output if these commands fail, so if the argument is a not a submodule, or the other is not a sha1, it will just print nothing without error message. Maybe that is OK, though? I don't see documentation for these commands, so maybe this is not meant to be usable?
PATCH v4 4/4 submodule: port submodule subcommand 'status' from shell to C
In reply to Prathamesh ChavanThis 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_listed_submodule() looping through the list obtained.
Then for_each_listed_submodule() 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.
--- builtin/submodule--helper.c | 155 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 49 +------------- 2 files changed, 156 insertions(+), 48 deletions(-)
builtin/submodule--helper.c +155 -0
@@ -558,6 +558,160 @@ 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, +const struct object_id *oid,const char *displaypath) +{ +if (info->quiet) +return ; + +printf ("%c%s %s" , state,oid_to_hex (oid), displaypath); + +if (state ==' ' || state =='+' ) { +struct argv_array get_rev_args =ARGV_ARRAY_INIT ; + +argv_array_pushl (&get_rev_args,"get-rev-name" , + path,oid_to_hex (oid),NULL ); +get_rev_name (get_rev_args.argc, get_rev_args.argv, + info->prefix); + +argv_array_clear (&get_rev_args); + }else { +printf ("\n" ); + } +} + +static int handle_submodule_head_ref (const char *refname, +const struct object_id *oid,int flags, +void *cb_data) +{ +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 *displaypath; +struct argv_array diff_files_args =ARGV_ARRAY_INIT ; + +if (!submodule_from_path (&null_oid, 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 (ce_stage (list_item)) { +print_status (info,'U' , list_item->name, + &null_oid, displaypath); + goto cleanup; + } + +if (!is_submodule_active (the_repository, list_item->name)) { +print_status (info,'-' , list_item->name, &list_item->oid, + 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, &list_item->oid, + displaypath); + }else { +if (!info->cached) { +struct object_id oid; + +if (head_ref_submodule (list_item->name, + handle_submodule_head_ref, &oid)) +die (_ ("could not resolve HEAD ref inside the" +"submodule '%s'" ), list_item->name); + +print_status (info,'+' , list_item->name, &oid, + displaypath); + }else { +print_status (info,'+' , list_item->name, + &list_item->oid, 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, +"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: +argv_array_clear (&diff_files_args); +free (displaypath); +} + +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; + +for_each_listed_submodule (&list, status_submodule, &info); + +return 0 ; +} +static int module_name (int argc,const char **argv,const char *prefix) {const struct submodule *sub;
@@ -1356,6 +1510,7 @@ static struct cmd_struct commands[] = { {"resolve-relative-url-test", resolve_relative_url_test, 0}, {"get-rev-name", get_rev_name, 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},
git-submodule.sh +1 -48
55 lines not on this page · raw message
Signature · 1 line
2.13.0
PATCH v4 4/4 submodule: port submodule subcommand 'status' from shell to C
+ const char *const git_submodule_helper_usage[] = {
+ N_("git submodule status [--quiet] [--cached] [--recursive] [<path>]"),
+ NULL
the manpage over here says
git submodule [--quiet] status [--cached] [--recursive] [--] [<path>...]
ie. multiple path arguments. Should this usage string be tweaked?
+static void print_status(struct status_cb *info, char state, const char *path,
+ const struct object_id *oid, const char *displaypath)
+{
could do with a comment. What are the options for the state char?
+ if (state == ' ' || state == '+') {
+ struct argv_array get_rev_args = ARGV_ARRAY_INIT;
+
+ argv_array_pushl(&get_rev_args, "get-rev-name",
+ path, oid_to_hex(oid), NULL);
+ get_rev_name(get_rev_args.argc, get_rev_args.argv,
+ info->prefix);
since you're not really subprocessing, can't you simply have a
do_print_rev_name(char *path, char *sha) {
..
printf("\n");
}and call that directly? Or call compute_rev_name directly. Then you don't have to do argv setup here.
Also, the name get_rev_name() is a little unfortunate, since it doesn't return a name, but rather prints it. Maybe the functions implementing helper commands could be named like:
command_get_rev_name
or similar.
v5 cover Incremental rewrite of git-submodules
Changes in v5:
* in print_status() function, we now call compute_rev_name() directly instead of doing the argv setup and calling get_rev_name() function.
* Since we no longer use the helper function get_rev_name(), we have removed it from the code base after porting submodule subcommand 'status'.
* function get_submodule_displaypath() has been modified, and now handles the case of super_prefix being non-null and of length zero.
* I have still kept the name of the function get_rev_name() unchanged, since in the function cmd_status(), it is used to get the value of the variable revname. And in the next commit since we do not require the function anymore, we reomve it from the code base.
As before you can find this series at:
https://github.com/pratham-pc/git/commits/patch-series-1
And its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-1
Build #179
Thanks, Han-Wen Nienhuys for reviewing the previous patch series.
Prathamesh Chavan (4):
submodule--helper: introduce get_submodule_displaypath()
submodule--helper: introduce for_each_listed_submodule()
submodule: port set_name_rev() from shell to C
submodule: port submodule subcommand 'status' from shell to C
builtin/submodule--helper.c | 265 ++++++++++++++++++++++++++++++++++++++++---- git-submodule.sh | 61 +--------- 2 files changed, 247 insertions(+), 79 deletions(-)
Signature · 1 line
2.13.0
PATCH v5 1/4 submodule--helper: introduce get_submodule_displaypath()
Introduce function get_submodule_displaypath() to replace the code occurring in submodule_init() for generating displaypath of the submodule with a call to it.
This new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)
builtin/submodule--helper.c +26 -12
@@ -220,6 +220,29 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *prreturn 0 ; } +/* the result should be freed by the caller. */ +static char *get_submodule_displaypath (const char *path,const char *prefix) +{ +const char *super_prefix =get_super_prefix (); + +if (prefix && super_prefix) { +BUG ("cannot have prefix '%s' and superprefix '%s'" , + prefix, super_prefix); + }else if (prefix) { +struct strbuf sb =STRBUF_INIT ; +char *displaypath =xstrdup (relative_path (path, prefix, &sb)); +strbuf_release (&sb); +return displaypath; + }else if (super_prefix) { +int len =strlen (super_prefix); +const char *format = (len >0 &&is_dir_sep (super_prefix[len -1 ])) ?"%s%s" :"%s/%s" ; + +return xstrfmt (format, super_prefix, path); + }else { +return xstrdup (path); + } +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -335,15 +358,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; -if (prefix &&get_super_prefix ()) -die ("BUG: cannot have prefix and superprefix" ); -else if (prefix) - displaypath =xstrdup (relative_path (path, prefix, &sb)); -else if (get_super_prefix ()) { -strbuf_addf (&sb,"%s%s" ,get_super_prefix (), path); - displaypath =strbuf_detach (&sb,NULL ); - }else - displaypath =xstrdup (path); + displaypath =get_submodule_displaypath (path, prefix); sub =submodule_from_path (&null_oid, path);
@@ -358,9 +373,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); +strbuf_reset (&sb); }/*
@@ -368,7 +383,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* To look up the url in .git/config, we must not fall back to * .gitmodules, so look it up directly. */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.url" , sub->name);if (git_config_get_string (sb.buf, &url)) {if (!sub->url)
@@ -405,9 +419,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath); } +strbuf_reset (&sb);/* Copy "update" setting when it is not set yet */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.update" , sub->name);if (git_config_get_string (sb.buf, &upd) && sub->update_strategy.type !=SM_UPDATE_UNSPECIFIED ) {
Signature · 1 line
2.13.0
Quoted 34 lines · Prathamesh Chavan
Introduce function
get_submodule_displaypath()to replace the code occurring insubmodule_init()for generating displaypath of the submodule with a call to it.This new function will also be used in other parts of the system in later patches.
Mentored-by Christian Couder <[email protected]>Mentored-by Stefan Beller <[email protected]>Signed-off-by Prathamesh Chavan <[email protected]>--- builtin/submodule--helper.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)builtin/submodule--helper.c +12
-0@@ -220,6 +220,29 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *prreturn 0 ; } +/* the result should be freed by the caller. */ +static char *get_submodule_displaypath (const char *path,const char *prefix) +{ +const char *super_prefix =get_super_prefix (); + +if (prefix && super_prefix) { +BUG ("cannot have prefix '%s' and superprefix '%s'" , + prefix, super_prefix); + }else if (prefix) { +struct strbuf sb =STRBUF_INIT ; +char *displaypath =xstrdup (relative_path (path, prefix, &sb)); +strbuf_release (&sb);
builtin/submodule--helper.c static int resolve_relative_url_test(int argc, const char **argv, const char *pr · line 244
Quoted 10 lines
builtin/submodule--helper.c +10
-0+return displaypath; + }else if (super_prefix) { +int len =strlen (super_prefix); +const char *format = (len >0 &&is_dir_sep (super_prefix[len -1 ])) ?"%s%s" :"%s/%s" ; + +return xstrfmt (format, super_prefix, path); + }else { +return xstrdup (path); + } +}
Looks like a fairly faithful rewrite of the original below, with a reasonable clean-up (e.g. use of xstrfmt() instead of addf()).
One thing I noticed is that future callers of this function, unlike init_submodule() which is its original caller, are allowed to have super-prefix that does not end in a slash, because the helper automatically supplies one when it is missing.
I am not sure the added leniency is desirable [*1*], but in any case, the added leniency deserves a mention in the log message, I would think.
[Footnote]
*1* Often it is harder to introduce bugs when the internal rules are stricter, e.g. "prefix must end with slash", than when they are looser, e.g. "prefix may or may not end with slash", because allowing different codepaths to have the same thing in different representations would hide bugs that is only uncovered when these codepaths eventually meet (e.g. one codepath has a path with and the other without trailing slash and they both think that is the prefix---then they use strcmp() to see if they have the same prefix, which would be a bug, which may not be noticed for a long time).
Quoted 50 lines
builtin/submodule--helper.c +3
-12struct module_list {const struct cache_entry **entries;int alloc, nr;@@ -335,15 +358,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; -if (prefix &&get_super_prefix ()) -die ("BUG: cannot have prefix and superprefix" ); -else if (prefix) - displaypath =xstrdup (relative_path (path, prefix, &sb)); -else if (get_super_prefix ()) { -strbuf_addf (&sb,"%s%s" ,get_super_prefix (), path); - displaypath =strbuf_detach (&sb,NULL ); - }else - displaypath =xstrdup (path); + displaypath =get_submodule_displaypath (path, prefix); sub =submodule_from_path (&null_oid, path);@@ -358,9 +373,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); +strbuf_reset (&sb); }/* @@ -368,7 +383,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* To look up the url in .git/config, we must not fall back to * .gitmodules, so look it up directly. */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.url" , sub->name);if (git_config_get_string (sb.buf, &url)) {if (!sub->url)@@ -405,9 +419,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath); } +strbuf_reset (&sb);/* Copy "update" setting when it is not set yet */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.update" , sub->name);if (git_config_get_string (sb.buf, &upd) && sub->update_strategy.type !=SM_UPDATE_UNSPECIFIED ) {
PATCH v5 2/4 submodule--helper: introduce for_each_listed_submodule()
In reply to Prathamesh ChavanIntroduce function for_each_listed_submodule() and replace a loop in module_init() with a call to it.
The new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-)
builtin/submodule--helper.c +28 -8
@@ -14,6 +14,9 @@#include "refs.h" #include "connect.h" +typedef void (*each_submodule_fn )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;
@@ -352,15 +355,30 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_listed_submodule (const struct module_list *list, +each_submodule_fn fn,void *cb_data) +{ +int i; +for (i =0 ; i < list->nr; i++) +fn (list->entries[i], cb_data); +} + +struct init_cb { +const char *prefix; +unsigned int quiet:1 ; +}; +#define INIT_CB_INIT { NULL, 0 } + +static void init_submodule (const struct cache_entry *list_item,void *cb_data) { +struct init_cb *info = cb_data;const struct submodule *sub;struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; - displaypath =get_submodule_displaypath (path, prefix); + displaypath =get_submodule_displaypath (list_item->name, info->prefix); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (&null_oid, list_item->name);if (!sub) die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -372,7 +390,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* * Set active flag for the submodule being initialized */ -if (!is_submodule_active (the_repository, path)) { +if (!is_submodule_active (the_repository, list_item->name)) {strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" );strbuf_reset (&sb);
@@ -414,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)if (git_config_set_gently (sb.buf, url))die (_ ("Failed to register url for submodule path '%s'" ), displaypath); -if (!quiet) +if (!info->quiet)fprintf (stderr,_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath);
@@ -443,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)static int module_init (int argc,const char **argv,const char *prefix) { +struct init_cb info =INIT_CB_INIT ;struct pathspec pathspec;struct module_list list =MODULE_LIST_INIT ;int quiet =0 ; -int i;struct option module_init_options[] = {OPT__QUIET (&quiet,N_ ("Suppress output for initializing a submodule" )),
@@ -471,8 +489,10 @@ static int module_init(int argc, const char **argv, const char *prefix)if (!argc &&git_config_get_value_multi ("submodule.active" ))module_list_active (&list); -for (i =0 ; i < list.nr; i++) -init_submodule (list.entries[i]->name, prefix, quiet); + info.prefix = prefix; + info.quiet = !!quiet; + +for_each_listed_submodule (&list, init_submodule, &info);return 0 ; }
Signature · 1 line
2.13.0
Re: [PATCH v5 2/4] submodule--helper: introduce for_each_listed_submodule()
Quoted 6 lines · Prathamesh Chavan
Introduce function
for_each_listed_submodule()and replace a loop inmodule_init()with a call to it.The new function will also be used in other parts of the system in later patches.
Quoted 6 lines
Mentored-by Christian Couder <[email protected]>Mentored-by Stefan Beller <[email protected]>Signed-off-by Prathamesh Chavan <[email protected]>--- builtin/submodule--helper.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-)
This looks more or less perfectly done, but I say this basing it on an assumption that nobody ever would want to initialize a single submodule without going through module_init() interface.
If there will be a caller that would have made a direct call to init_submodule() if this patch did not exist, then I think this patch is better done by keeping the external interface to the init_submodule() function intact, and introducing an intermediate helper init_submodule_cb() function that is to be used as the callback used in for_each_listed_submodule() API. In general, we should make sure that these callback functions that take "void *cb_data" parameters are called _only_ by these iterators that defined the callback (in this case, for_each_listed_submodule()) and not other functions.
Quoted 91 lines
builtin/submodule--helper.c +28
-8@@ -14,6 +14,9 @@#include "refs.h" #include "connect.h" +typedef void (*each_submodule_fn )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;@@ -352,15 +355,30 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_listed_submodule (const struct module_list *list, +each_submodule_fn fn,void *cb_data) +{ +int i; +for (i =0 ; i < list->nr; i++) +fn (list->entries[i], cb_data); +} + +struct init_cb { +const char *prefix; +unsigned int quiet:1 ; +}; +#define INIT_CB_INIT { NULL, 0 } + +static void init_submodule (const struct cache_entry *list_item,void *cb_data) { +struct init_cb *info = cb_data;const struct submodule *sub;struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; - displaypath =get_submodule_displaypath (path, prefix); + displaypath =get_submodule_displaypath (list_item->name, info->prefix); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (&null_oid, list_item->name);if (!sub) die(_("No url found for submodule path '%s' in .gitmodules"),@@ -372,7 +390,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* * Set active flag for the submodule being initialized */ -if (!is_submodule_active (the_repository, path)) { +if (!is_submodule_active (the_repository, list_item->name)) {strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" );strbuf_reset (&sb);@@ -414,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)if (git_config_set_gently (sb.buf, url))die (_ ("Failed to register url for submodule path '%s'" ), displaypath); -if (!quiet) +if (!info->quiet)fprintf (stderr,_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath);@@ -443,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)static int module_init (int argc,const char **argv,const char *prefix) { +struct init_cb info =INIT_CB_INIT ;struct pathspec pathspec;struct module_list list =MODULE_LIST_INIT ;int quiet =0 ; -int i;struct option module_init_options[] = {OPT__QUIET (&quiet,N_ ("Suppress output for initializing a submodule" )),@@ -471,8 +489,10 @@ static int module_init(int argc, const char **argv, const char *prefix)if (!argc &&git_config_get_value_multi ("submodule.active" ))module_list_active (&list); -for (i =0 ; i < list.nr; i++) -init_submodule (list.entries[i]->name, prefix, quiet); + info.prefix = prefix; + info.quiet = !!quiet; + +for_each_listed_submodule (&list, init_submodule, &info);return 0 ; }
PATCH v5 3/4 submodule: port set_name_rev() from shell to C
In reply to Prathamesh ChavanFunction set_name_rev() is ported from git-submodule to the submodule--helper builtin. The function compute_rev_name() generates the value of the revision name as required.
The function get_rev_name() calls compute_rev_name() and receives the revision name, and later handles its formatting and printing.
--- builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 16 ++---------- 2 files changed, 65 insertions(+), 14 deletions(-)
builtin/submodule--helper.c +63 -0
@@ -246,6 +246,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix) } } +static char *compute_rev_name (const char *sub_path,const char * object_id) +{ +struct strbuf sb =STRBUF_INIT ; +const char ***d; + +static const char *describe_bare[] = { +NULL + }; + +static const char *describe_tags[] = { +"--tags" ,NULL + }; + +static const char *describe_contains[] = { +"--contains" ,NULL + }; + +static const char *describe_all_always[] = { +"--all" ,"--always" ,NULL + }; + +static const char **describe_argv[] = { + describe_bare, describe_tags, describe_contains, + describe_all_always,NULL + }; + +for (d = describe_argv; *d; d++) { +struct child_process cp =CHILD_PROCESS_INIT ; +prepare_submodule_repo_env (&cp.env_array); + cp.dir = sub_path; + cp.git_cmd =1 ; + cp.no_stderr =1 ; + +argv_array_push (&cp.args,"describe" ); +argv_array_pushv (&cp.args, *d); +argv_array_push (&cp.args, object_id); + +if (!capture_command (&cp, &sb,0 ) && sb.len) { +strbuf_strip_suffix (&sb,"\n" ); +return strbuf_detach (&sb,NULL ); + } + } + +strbuf_release (&sb); +return NULL ; +} + +static int get_rev_name (int argc,const char **argv,const char *prefix) +{ +char *revname; +if (argc !=3 ) +die ("get-rev-name only accepts two arguments: <path> <sha1>" ); + + revname =compute_rev_name (argv[1 ], argv[2 ]); +if (revname && revname[0 ]) +printf (" (%s)" , revname); +printf ("\n" ); + +free (revname); +return 0 ; +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -1293,6 +1355,7 @@ static struct cmd_struct commands[] = { {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, + {"get-rev-name", get_rev_name, 0}, {"init", module_init, SUPPORT_SUPER_PREFIX}, {"remote-branch", resolve_remote_submodule_branch, 0}, {"push-check", push_check, 0},
git-submodule.sh +2 -14
@@ -758,18 +758,6 @@ cmd_update() } } -set_name_rev () { - revname=$( ( - sanitize_submodule_env - cd "$1" && { - git describe "$2" 2>/dev/null || - git describe --tags "$2" 2>/dev/null || - git describe --contains "$2" 2>/dev/null || - git describe --all --always "$2" - } - ) ) - test -z "$revname" || revname=" ($revname)" -} # # Show commit summary for submodules in index or working tree #
@@ -1041,14 +1029,14 @@ cmd_status() fi if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path" then - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$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 - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$sm_path" "$sha1") say "+$sha1 $displaypath$revname" fi
Signature · 1 line
2.13.0
Re: [PATCH v5 3/4] submodule: port set_name_rev() from shell to C
Quoted 14 lines · Prathamesh Chavan
builtin/submodule--helper.c +14
-0+static char *compute_rev_name (const char *sub_path,const char * object_id) +{ +struct strbuf sb =STRBUF_INIT ; +const char ***d; + +static const char *describe_bare[] = { +NULL + }; +... +static const char **describe_argv[] = { + describe_bare, describe_tags, describe_contains, + describe_all_always,NULL + }; +
builtin/submodule--helper.c static char *get_submodule_displaypath(const char *path, const char *prefix) · line 284
Quoted 10 lines
builtin/submodule--helper.c +10
-0+for (d = describe_argv; *d; d++) { +struct child_process cp =CHILD_PROCESS_INIT ; +prepare_submodule_repo_env (&cp.env_array); + cp.dir = sub_path; + cp.git_cmd =1 ; + cp.no_stderr =1 ; + +argv_array_push (&cp.args,"describe" ); +argv_array_pushv (&cp.args, *d); +argv_array_push (&cp.args, object_id);
Nicely done.
builtin/submodule--helper.c static char *get_submodule_displaypath(const char *path, const char *prefix) · line 286
Quoted 1 line
builtin/submodule--helper.c +1
-0+if (!capture_command (&cp, &sb,0 ) && sb.len) {
Observation: even if the command did not fail, if it returned an empty string, then you would reject that result and loop on here. This is different from the original scripted version, but it should be OK.
builtin/submodule--helper.c static char *get_submodule_displaypath(const char *path, const char *prefix) · line 293
Quoted 7 lines
builtin/submodule--helper.c +7
-0+strbuf_strip_suffix (&sb,"\n" ); +return strbuf_detach (&sb,NULL ); + } + } + +strbuf_release (&sb); +return NULL ;
Observation: and if you do not find any non-empty-string answer, you return NULL.
builtin/submodule--helper.c static char *get_submodule_displaypath(const char *path, const char *prefix) · line 304
Quoted 11 lines
builtin/submodule--helper.c +11
-0+} + +static int get_rev_name (int argc,const char **argv,const char *prefix) +{ +char *revname; +if (argc !=3 ) +die ("get-rev-name only accepts two arguments: <path> <sha1>" ); + + revname =compute_rev_name (argv[1 ], argv[2 ]); +if (revname && revname[0 ]) +printf (" (%s)" , revname);
So, while it is not wrong per-se, I do not think we need to check revname[0] here. The helper never returns a non-NULL pointer that points at an empty string, right?
On the other hand, if we dropped the "&& sb.len" check in the helper function to be more faithful to the original, then we must check revname[0] for an empty string.
Quoted 56 lines
builtin/submodule--helper.c +7
-0+printf ("\n" ); + +free (revname); +return 0 ; +} +struct module_list {const struct cache_entry **entries;int alloc, nr;@@ -1293,6 +1355,7 @@ static struct cmd_struct commands[] = { {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, + {"get-rev-name", get_rev_name, 0}, {"init", module_init, SUPPORT_SUPER_PREFIX}, {"remote-branch", resolve_remote_submodule_branch, 0}, {"push-check", push_check, 0},git-submodule.sh +2
-14@@ -758,18 +758,6 @@ cmd_update() } } -set_name_rev () { - revname=$( ( - sanitize_submodule_env - cd "$1" && { - git describe "$2" 2>/dev/null || - git describe --tags "$2" 2>/dev/null || - git describe --contains "$2" 2>/dev/null || - git describe --all --always "$2" - } - ) ) - test -z "$revname" || revname=" ($revname)" -} # # Show commit summary for submodules in index or working tree #@@ -1041,14 +1029,14 @@ cmd_status() fi if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path" then - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$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 - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$sm_path" "$sha1") say "+$sha1 $displaypath$revname" fi
Re: [PATCH v5 3/4] submodule: port set_name_rev() from shell to C
Quoted 11 lines · Junio C Hamano
Nicely done.
builtin/submodule--helper.c static char *get_submodule_displaypath(const char *path, const char *prefix) · line 286
Quoted 1 line
builtin/submodule--helper.c +1
-0+if (!capture_command (&cp, &sb,0 ) && sb.len) {...
So, while it is not wrong per-se, I do not think we need to check revname[0] here. The helper never returns a non-NULL pointer that points at an empty string, right?On the other hand, if we dropped the "&& sb.len" check in the helper function to be more faithful to the original, then we must check revname[0] for an empty string.
Ah, ignore all of the above. This will all be discarded in the next step [4/4], as far as I can tell. Perhaps we should drop this step and get directly to it, making the result a three-patch series instead, then, no?
PATCH v5 4/4 submodule: port submodule subcommand 'status' from shell to C
In reply to Prathamesh ChavanThis 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_listed_submodule() looping through the list obtained.
Then for_each_listed_submodule() 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.
Helper function get_rev_name() removed after porting the above subcommand as it is no longer used.
--- builtin/submodule--helper.c | 162 +++++++++++++++++++++++++++++++++++++++----- git-submodule.sh | 49 +------------- 2 files changed, 147 insertions(+), 64 deletions(-)
builtin/submodule--helper.c +146 -16
@@ -293,21 +293,6 @@ static char *compute_rev_name(const char *sub_path, const char* object_id)return NULL ; } -static int get_rev_name (int argc,const char **argv,const char *prefix) -{ -char *revname; -if (argc !=3 ) -die ("get-rev-name only accepts two arguments: <path> <sha1>" ); - - revname =compute_rev_name (argv[1 ], argv[2 ]); -if (revname && revname[0 ]) -printf (" (%s)" , revname); -printf ("\n" ); - -free (revname); -return 0 ; -} -struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -559,6 +544,151 @@ 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, +const struct object_id *oid,const char *displaypath) +{ +if (info->quiet) +return ; + +printf ("%c%s %s" , state,oid_to_hex (oid), displaypath); + +if (state ==' ' || state =='+' ) +printf (" (%s)" ,compute_rev_name (path,oid_to_hex (oid))); + +printf ("\n" ); +} + +static int handle_submodule_head_ref (const char *refname, +const struct object_id *oid,int flags, +void *cb_data) +{ +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 *displaypath; +struct argv_array diff_files_args =ARGV_ARRAY_INIT ; + +if (!submodule_from_path (&null_oid, 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 (ce_stage (list_item)) { +print_status (info,'U' , list_item->name, + &null_oid, displaypath); + goto cleanup; + } + +if (!is_submodule_active (the_repository, list_item->name)) { +print_status (info,'-' , list_item->name, &list_item->oid, + 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, &list_item->oid, + displaypath); + }else { +if (!info->cached) { +struct object_id oid; + +if (refs_head_ref (get_submodule_ref_store (list_item->name), handle_submodule_head_ref, &oid)) +die (_ ("could not resolve HEAD ref inside the" +"submodule '%s'" ), list_item->name); + +print_status (info,'+' , list_item->name, &oid, + displaypath); + }else { +print_status (info,'+' , list_item->name, + &list_item->oid, 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, +"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: +argv_array_clear (&diff_files_args); +free (displaypath); +} + +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; + +for_each_listed_submodule (&list, status_submodule, &info); + +return 0 ; +} +static int module_name (int argc,const char **argv,const char *prefix) {const struct submodule *sub;
@@ -1355,8 +1485,8 @@ static struct cmd_struct commands[] = { {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, - {"get-rev-name", get_rev_name, 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},
git-submodule.sh +1 -48
55 lines not on this page · raw message
Signature · 1 line
2.13.0
Re: [PATCH v5 4/4] submodule: port submodule subcommand 'status' from shell to C
Quoted 3 lines · Prathamesh Chavan
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()andprint_status().
Well then it does not just aim to, but it does, doesn't it ;-)?
Quoted 26 lines
builtin/submodule--helper.c +22
-0@@ -559,6 +544,151 @@ 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, +const struct object_id *oid,const char *displaypath) +{ +if (info->quiet) +return ; + +printf ("%c%s %s" , state,oid_to_hex (oid), displaypath); + +if (state ==' ' || state =='+' ) +printf (" (%s)" ,compute_rev_name (path,oid_to_hex (oid))); + +printf ("\n" ); +} +
builtin/submodule--helper.c static int module_init(int argc, const char **argv, const char *prefix) · line 578
Quoted 10 lines
builtin/submodule--helper.c +10
-0+static int handle_submodule_head_ref (const char *refname, +const struct object_id *oid,int flags, +void *cb_data) +{ +struct object_id *output = cb_data; +if (oid) +oidcpy (output, oid); + +return 0 ; +}
All of the above look sensible.
Quoted 20 lines
builtin/submodule--helper.c +20
-0+static void status_submodule (const struct cache_entry *list_item,void *cb_data) +{ +struct status_cb *info = cb_data; +char *displaypath; +struct argv_array diff_files_args =ARGV_ARRAY_INIT ; + +if (!submodule_from_path (&null_oid, 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 (ce_stage (list_item)) { +print_status (info,'U' , list_item->name, + &null_oid, displaypath); + goto cleanup; + } + +if (!is_submodule_active (the_repository, list_item->name)) { + print_status(info, '-', list_item->name, &list_item->oid,
builtin/submodule--helper.c static int module_init(int argc, const char **argv, const char *prefix) · line 609
Quoted 10 lines
builtin/submodule--helper.c +10
-0+ 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)) {
Calling any cmd_foo() from places other than git.c, especially if it is done more than once, is a source of bug. I didn't check closely if cmd_diff_files() currently has any of the potential problems, but these functions are free to (1) modify global or file-scope static variables, assuming that they will not be called again, leaving these variables in a state different from the original and making cmd_foo() unsuitable to be called twice, and (2) exit instead of return at the end, among other things.
The functions in diff-lib.c (I think run_diff_files() for this application) are designed to be called multiple times as library-ish helpers; this caller should be using them instead.
builtin/submodule--helper.c static int module_init(int argc, const char **argv, const char *prefix) · line 613
Quoted 4 lines
builtin/submodule--helper.c +4
-0+print_status (info,' ' , list_item->name, &list_item->oid, + displaypath); + }else { +if (!info->cached) {
By using "else if (!info->cached) {" here, you can reduce the nesting level, perhaps?
Quoted 26 lines
builtin/submodule--helper.c +26
-0+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 ;
builtin/submodule--helper.c static int module_init(int argc, const char **argv, const char *prefix) · line 690
Quoted 10 lines
builtin/submodule--helper.c +10
-0+ + info.prefix = prefix; + info.quiet = !!quiet; + info.recursive = !!recursive; + info.cached = !!cached; + +for_each_listed_submodule (&list, status_submodule, &info); + +return 0 ; +}
The same comment as [2/4] on "init_submodule()? don't you want init_submodule_cb() instead?" applies to "status_submodule()". I suspect that in the future we would want more direct calls to show the status of one single submodule, without indirectly controlling what is chosen via the &list interface, and if that is the case, you'd want the interface into the leaf level helper function to be a more direct one that is not based on "void *cb_data", with a thin wrapper around it that is to be used as the callback function by for_each_listed_submodule().
Other than that, looks very cleanly done.
Thanks.
v6 cover Incremental rewrite of git-submodules
changes in v6:
* The function get_submodule_displaypath() was modified for the case when get_super_prefix() returns a non-null value. The condition to check if the super-prefix ends with a '/' is removed. To accomodate this change appropriate value of super_prefix is passed instead in the recursive calls of init_submodule() and status_submodule().
* To accomodate the possiblity of a direct call to the function init_submodule(), a callback function init_submodule_cb() is introduced which takes cache_entry and init_cb structures as input params, and calls init_submodule() with parameters which are more appropriate for a direct call of this function.
* Similar changes were even done for status_submodule(). But as it was observed that the number of params increased a lot due to flags like quiet, recursive, cached, etc, and keeping in mind the future subcommand's ported functions as well, a single unsigned int called cb_flags was introduced to store all of these flags, instead of having parameter for each one.
* Patches [3/4] and [4/4] from the previous series were merged as a single step.
* Call to function cmd_diff_files was avoided in the function status_submodule() and instead used the function run_diff_files() for the same purpose.
Since there were many changes the patches required, I took more time on making these changes. Thank you, Junio for the last reviews. They helped a lot for improving the patch series.
As before you can find this series at:
https://github.com/pratham-pc/git/commits/patch-series-1
And its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-1
Build #184
Prathamesh Chavan (3):
submodule--helper: introduce get_submodule_displaypath()
submodule--helper: introduce for_each_listed_submodule()
submodule: port submodule subcommand 'status' from shell to C
builtin/submodule--helper.c | 281 +++++++++++++++++++++++++++++++++++++++++--- git-submodule.sh | 61 +--------- 2 files changed, 265 insertions(+), 77 deletions(-)
Signature · 1 line
2.13.0
PATCH v6 1/3 submodule--helper: introduce get_submodule_displaypath()
Introduce function get_submodule_displaypath() to replace the code occurring in submodule_init() for generating displaypath of the submodule with a call to it.
This new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)
builtin/submodule--helper.c +23 -12
@@ -220,6 +220,26 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *prreturn 0 ; } +/* the result should be freed by the caller. */ +static char *get_submodule_displaypath (const char *path,const char *prefix) +{ +const char *super_prefix =get_super_prefix (); + +if (prefix && super_prefix) { +BUG ("cannot have prefix '%s' and superprefix '%s'" , + prefix, super_prefix); + }else if (prefix) { +struct strbuf sb =STRBUF_INIT ; +char *displaypath =xstrdup (relative_path (path, prefix, &sb)); +strbuf_release (&sb); +return displaypath; + }else if (super_prefix) { +return xstrfmt ("%s%s" , super_prefix, path); + }else { +return xstrdup (path); + } +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -335,15 +355,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; -if (prefix &&get_super_prefix ()) -die ("BUG: cannot have prefix and superprefix" ); -else if (prefix) - displaypath =xstrdup (relative_path (path, prefix, &sb)); -else if (get_super_prefix ()) { -strbuf_addf (&sb,"%s%s" ,get_super_prefix (), path); - displaypath =strbuf_detach (&sb,NULL ); - }else - displaypath =xstrdup (path); + displaypath =get_submodule_displaypath (path, prefix); sub =submodule_from_path (&null_oid, path);
@@ -358,9 +370,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); +strbuf_reset (&sb); }/*
@@ -368,7 +380,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* To look up the url in .git/config, we must not fall back to * .gitmodules, so look it up directly. */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.url" , sub->name);if (git_config_get_string (sb.buf, &url)) {if (!sub->url)
@@ -405,9 +416,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath); } +strbuf_reset (&sb);/* Copy "update" setting when it is not set yet */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.update" , sub->name);if (git_config_get_string (sb.buf, &upd) && sub->update_strategy.type !=SM_UPDATE_UNSPECIFIED ) {
Signature · 1 line
2.13.0
This step looks good to me. Thanks.
PATCH v6 2/3 submodule--helper: introduce for_each_listed_submodule()
In reply to Prathamesh ChavanIntroduce function for_each_listed_submodule() and replace a loop in module_init() with a call to it.
The new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)
builtin/submodule--helper.c +34 -5
@@ -14,6 +14,11 @@#include "refs.h" #include "connect.h" +#define CB_OPT_QUIET (1<<0) + +typedef void (*each_submodule_fn )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;
@@ -349,7 +354,22 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_listed_submodule (const struct module_list *list, +each_submodule_fn fn,void *cb_data) +{ +int i; +for (i =0 ; i < list->nr; i++) +fn (list->entries[i], cb_data); +} + +struct init_cb { +const char *prefix; +unsigned int cb_flags; +}; +#define INIT_CB_INIT { NULL, 0 } + +static void init_submodule (const char *path,const char *prefix, +unsigned int cb_flags) {const struct submodule *sub;struct strbuf sb =STRBUF_INIT ;
@@ -411,7 +431,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)if (git_config_set_gently (sb.buf, url))die (_ ("Failed to register url for submodule path '%s'" ), displaypath); -if (!quiet) +if (!(cb_flags &CB_OPT_QUIET ))fprintf (stderr,_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath);
@@ -438,12 +458,18 @@ static void init_submodule(const char *path, const char *prefix, int quiet)free (upd); } +static void init_submodule_cb (const struct cache_entry *list_item,void *cb_data) +{ +struct init_cb *info = cb_data; +init_submodule (list_item->name, info->prefix, info->cb_flags); +} +static int module_init (int argc,const char **argv,const char *prefix) { +struct init_cb info =INIT_CB_INIT ;struct pathspec pathspec;struct module_list list =MODULE_LIST_INIT ;int quiet =0 ; -int i;struct option module_init_options[] = {OPT__QUIET (&quiet,N_ ("Suppress output for initializing a submodule" )),
@@ -468,8 +494,11 @@ static int module_init(int argc, const char **argv, const char *prefix)if (!argc &&git_config_get_value_multi ("submodule.active" ))module_list_active (&list); -for (i =0 ; i < list.nr; i++) -init_submodule (list.entries[i]->name, prefix, quiet); + info.prefix = prefix; +if (quiet) + info.cb_flags |=CB_OPT_QUIET ; + +for_each_listed_submodule (&list, init_submodule_cb, &info);return 0 ; }
Signature · 1 line
2.13.0
Re: [PATCH v6 2/3] submodule--helper: introduce for_each_listed_submodule()
Quoted 18 lines · Prathamesh Chavan
Introduce function
for_each_listed_submodule()and replace a loop inmodule_init()with a call to it.The new function will also be used in other parts of the system in later patches.
Mentored-by Christian Couder <[email protected]>Mentored-by Stefan Beller <[email protected]>Signed-off-by Prathamesh Chavan <[email protected]>--- builtin/submodule--helper.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)builtin/submodule--helper.c +0
-0
builtin/submodule--helper.c line 17
Quoted 4 lines
builtin/submodule--helper.c +1
-0@@ -14,6 +14,11 @@#include "refs.h" #include "connect.h" +#define CB_OPT_QUIET (1<<0)
Is the purpose of this bit to make the callback quiet? I do not think so. Is there a reason why we cannot call it just OPT_QUIET or something instead?
When the set of functions that pay attention to these flags include both ones that are callable for a single submodule and ones meant as callbacks for for-each interface, having to flip bit whose name screams "CallBack!" in a caller of a single-short version feels very wrong.
"make style" tells me to format the above like so:
#define OPT_QUIET (1 << 0)
and I think I agree.
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 363
Quoted 12 lines
builtin/submodule--helper.c +7
-1@@ -349,7 +354,22 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_listed_submodule (const struct module_list *list, +each_submodule_fn fn,void *cb_data) +{ +int i; +for (i =0 ; i < list->nr; i++) +fn (list->entries[i], cb_data); +}
Good.
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 365
Quoted 1 line
builtin/submodule--helper.c +1
-0+struct init_cb {
I take it is a short-hand for "submodule init callback"? As long as the name stays inside this file, I think we are OK.
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 367
Quoted 2 lines
builtin/submodule--helper.c +2
-0+const char *prefix; +unsigned int cb_flags;
Call this just "flags"; call-back ness is plenty clear from the fact that it lives in a structure meant as a callback interface already.
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 368
Quoted 1 line
builtin/submodule--helper.c +1
-0+};
Blank line here?
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 372
Quoted 4 lines
builtin/submodule--helper.c +4
-0+#define INIT_CB_INIT { NULL, 0 } + +static void init_submodule (const char *path,const char *prefix, +unsigned int cb_flags)
Call this also "flags"; a direct caller of this function that wants to initialize a single submodule without going thru the for-each callback interface would not be passing "callback flags"--they are just passing a set of flags.
PATCH v6 3/3 submodule: port submodule subcommand 'status' from shell to C
In reply to Prathamesh ChavanThis 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 four functions: module_status(), submodule_status_cb(), 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_listed_submodule() looping through the list obtained.
Then for_each_listed_submodule() calls submodule_status_cb() for each of the submodule in its list. The function submodule_status_cb() calls submodule_status() after passing appropriate arguments to the funciton. 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.
Function set_name_rev() is also ported from git-submodule to the submodule--helper builtin function compute_rev_name(), which now generates the value of the revision name as required.
--- builtin/submodule--helper.c | 207 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 61 +------------ 2 files changed, 208 insertions(+), 60 deletions(-)
builtin/submodule--helper.c +207 -0
@@ -13,8 +13,13 @@#include "remote.h" #include "refs.h" #include "connect.h" +#include "revision.h" +#include "diffcore.h" +#include "diff.h" #define CB_OPT_QUIET (1<<0) +#define CB_OPT_CACHED (1<<1) +#define CB_OPT_RECURSIVE (1<<2)typedef void (*each_submodule_fn )(const struct cache_entry *list_item,void *cb_data);
@@ -245,6 +250,53 @@ static char *get_submodule_displaypath(const char *path, const char *prefix) } } +static char *compute_rev_name (const char *sub_path,const char * object_id) +{ +struct strbuf sb =STRBUF_INIT ; +const char ***d; + +static const char *describe_bare[] = { +NULL + }; + +static const char *describe_tags[] = { +"--tags" ,NULL + }; + +static const char *describe_contains[] = { +"--contains" ,NULL + }; + +static const char *describe_all_always[] = { +"--all" ,"--always" ,NULL + }; + +static const char **describe_argv[] = { + describe_bare, describe_tags, describe_contains, + describe_all_always,NULL + }; + +for (d = describe_argv; *d; d++) { +struct child_process cp =CHILD_PROCESS_INIT ; +prepare_submodule_repo_env (&cp.env_array); + cp.dir = sub_path; + cp.git_cmd =1 ; + cp.no_stderr =1 ; + +argv_array_push (&cp.args,"describe" ); +argv_array_pushv (&cp.args, *d); +argv_array_push (&cp.args, object_id); + +if (!capture_command (&cp, &sb,0 )) { +strbuf_strip_suffix (&sb,"\n" ); +return strbuf_detach (&sb,NULL ); + } + } + +strbuf_release (&sb); +return NULL ; +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -503,6 +555,160 @@ static int module_init(int argc, const char **argv, const char *prefix)return 0 ; } +struct status_cb { +const char *prefix; +unsigned int cb_flags; +}; +#define STATUS_CB_INIT { NULL, 0 } + +static void print_status (unsigned int flags,char state,const char *path, +const struct object_id *oid,const char *displaypath) +{ +if (flags &CB_OPT_QUIET ) +return ; + +printf ("%c%s %s" , state,oid_to_hex (oid), displaypath); + +if (state ==' ' || state =='+' ) +printf (" (%s)" ,compute_rev_name (path,oid_to_hex (oid))); + +printf ("\n" ); +} + +static int handle_submodule_head_ref (const char *refname, +const struct object_id *oid,int flags, +void *cb_data) +{ +struct object_id *output = cb_data; +if (oid) +oidcpy (output, oid); + +return 0 ; +} + +static void status_submodule (const char *path,const struct object_id *ce_oid, +unsigned int ce_flags,const char *prefix, +unsigned int cb_flags) +{ +char *displaypath; +struct argv_array diff_files_args =ARGV_ARRAY_INIT ; +struct rev_info rev; +int diff_files_result; + +if (!submodule_from_path (&null_oid, path)) +die (_ ("no submodule mapping found in .gitmodules for path '%s'" ), + path); + + displaypath =get_submodule_displaypath (path, prefix); + +if ((CE_STAGEMASK & ce_flags) >>CE_STAGESHIFT ) { +print_status (cb_flags,'U' , path, &null_oid, displaypath); + goto cleanup; + } + +if (!is_submodule_active (the_repository, path)) { +print_status (cb_flags,'-' , path, ce_oid, displaypath); + goto cleanup; + } + +argv_array_pushl (&diff_files_args,"diff-files" , +"--ignore-submodules=dirty" ,"--quiet" ,"--" , + path,NULL ); + +git_config (git_diff_basic_config,NULL ); +init_revisions (&rev, prefix); + rev.abbrev =0 ; +precompose_argv (diff_files_args.argc, diff_files_args.argv); + diff_files_args.argc =setup_revisions (diff_files_args.argc, + diff_files_args.argv, + &rev,NULL ); + diff_files_result =run_diff_files (&rev,0 ); + +if (!diff_result_code (&rev.diffopt, diff_files_result)) { +print_status (cb_flags,' ' , path, ce_oid, + displaypath); + }else if (!(cb_flags &CB_OPT_CACHED )) { +struct object_id oid; + +if (refs_head_ref (get_submodule_ref_store (path), + handle_submodule_head_ref, &oid)) +die (_ ("could not resolve HEAD ref inside the" +"submodule '%s'" ), path); + +print_status (cb_flags,'+' , path, &oid, displaypath); + }else { +print_status (cb_flags,'+' , path, ce_oid, displaypath); + } + +if (cb_flags &CB_OPT_RECURSIVE ) { +struct child_process cpr =CHILD_PROCESS_INIT ; + + cpr.git_cmd =1 ; + cpr.dir = path; +prepare_submodule_repo_env (&cpr.env_array); + +argv_array_push (&cpr.args,"--super-prefix" ); +argv_array_pushf (&cpr.args,"%s/" , displaypath); +argv_array_pushl (&cpr.args,"submodule--helper" ,"status" , +"--recursive" ,NULL ); + +if (cb_flags &CB_OPT_CACHED ) +argv_array_push (&cpr.args,"--cached" ); + +if (cb_flags &CB_OPT_QUIET ) +argv_array_push (&cpr.args,"--quiet" ); + +if (run_command (&cpr)) +die (_ ("failed to recurse into submodule '%s'" ), path); + } + +cleanup: +argv_array_clear (&diff_files_args); +free (displaypath); +} + +static void status_submodule_cb (const struct cache_entry *list_item, +void *cb_data) +{ +struct status_cb *info = cb_data; +status_submodule (list_item->name, &list_item->oid, list_item->ce_flags, + info->prefix, info->cb_flags); +} + +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 ; + +struct option module_status_options[] = { +OPT__QUIET (&quiet,N_ ("Suppress submodule status output" )), +OPT_BIT (0 ,"cached" , &info.cb_flags,N_ ("Use commit stored in the index instead of the one stored in the submodule HEAD" ),CB_OPT_CACHED ), +OPT_BIT (0 ,"recursive" , &info.cb_flags,N_ ("recurse into nested submodules" ),CB_OPT_RECURSIVE ), +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; +if (quiet) + info.cb_flags |=CB_OPT_QUIET ; + +for_each_listed_submodule (&list, status_submodule_cb, &info); + +return 0 ; +} +static int module_name (int argc,const char **argv,const char *prefix) {const struct submodule *sub;
@@ -1300,6 +1506,7 @@ static struct cmd_struct commands[] = { {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 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},
git-submodule.sh +1 -60
73 lines not on this page · raw message
Signature · 1 line
2.13.0
Re: [PATCH v6 3/3] submodule: port submodule subcommand 'status' from shell to C
builtin/submodule--helper.c line 22
Quoted 4 lines · Prathamesh Chavan
#define
CB_OPT_QUIET(1<<0)builtin/submodule--helper.c +2
-0+#define CB_OPT_CACHED (1<<1) +#define CB_OPT_RECURSIVE (1<<2)
Same comments on both naming and formatting.
Quoted 19 lines
builtin/submodule--helper.c +15
-0@@ -245,6 +250,53 @@ static char *get_submodule_displaypath(const char *path, const char *prefix) } } +static char *compute_rev_name (const char *sub_path,const char * object_id) +{ +struct strbuf sb =STRBUF_INIT ; +const char ***d; + +static const char *describe_bare[] = { +NULL + }; + +static const char *describe_tags[] = { +"--tags" ,NULL + }; + +static const char *describe_contains[] = { +"--contains" ,NULL
builtin/submodule--helper.c static char *get_submodule_displaypath(const char *path, const char *prefix) · line 277
Quoted 10 lines
builtin/submodule--helper.c +10
-0+ }; + +static const char *describe_all_always[] = { +"--all" ,"--always" ,NULL + }; + +static const char **describe_argv[] = { + describe_bare, describe_tags, describe_contains, + describe_all_always,NULL + };
"make style" seems to suggest a lot more compact version to be used for the above, and I tend to agree with its diagnosis.
builtin/submodule--helper.c static int module_init(int argc, const char **argv, const char *prefix) · line 562
Quoted 9 lines
builtin/submodule--helper.c +5
-0@@ -503,6 +555,160 @@ static int module_init(int argc, const char **argv, const char *prefix)return 0 ; } +struct status_cb { +const char *prefix; +unsigned int cb_flags; +}; +#define STATUS_CB_INIT { NULL, 0 }
Same three comments as the previous "init_cb" patch apply.
builtin/submodule--helper.c static int module_init(int argc, const char **argv, const char *prefix) · line 618
Quoted 5 lines
builtin/submodule--helper.c +5
-0+argv_array_pushl (&diff_files_args,"diff-files" , +"--ignore-submodules=dirty" ,"--quiet" ,"--" , + path,NULL ); + +git_config (git_diff_basic_config,NULL );
Should this be called every time? The config file is not changing, no?
builtin/submodule--helper.c static int module_init(int argc, const char **argv, const char *prefix) · line 620
Quoted 2 lines
builtin/submodule--helper.c +2
-0+init_revisions (&rev, prefix); + rev.abbrev =0 ;
This part looks OK.
builtin/submodule--helper.c static int module_init(int argc, const char **argv, const char *prefix) · line 621
Quoted 1 line
builtin/submodule--helper.c +1
-0+precompose_argv (diff_files_args.argc, diff_files_args.argv);
I do not think this is correct. We certainly did not get the path argument (i.e. args.argv) from the command line of macOS X box and the correction for UTF-8 canonicalization should not be necessary. Even if we did get path from the command line, I think the UTF-8 correction should have been done for us for any command (like "git submodule--helper") that uses parse-optoins API already.
Just dropping the line should be sufficient to correct this, I think.
The remainder of the patch looked more-or-less OK, but I'd revisit it later to make sure.
Thanks.
v7 cover Incremental rewrite of git-submodules
Changes in v7:
* Instead of using cb_flags in the callback data's struct, 'flags' is used.
* Similar changes were applied to the CB_OPT_QUIET and other bits.
* The function compute_rev_name() was formatted in accordance with the "make style", into a compact version.
* Call to precompose_argv() in the function status_submodule() was dropped as the call was unnecessary.
As before you can find this series at:
https://github.com/pratham-pc/git/commits/patch-series-1
And its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-1
Build #190
The above changes were based on master branch.
Another branch, similar to the above, was created, but was based on the 'next' branch.
Complete build report of that is also available at:
https://travis-ci.org/pratham-pc/git/builds
Branch: patch-series-1-next
Build #189
The above changes are also push on github and are available at:
https://github.com/pratham-pc/git/commits/patch-series-1-next
Prathamesh Chavan (3):
submodule--helper: introduce get_submodule_displaypath()
submodule--helper: introduce for_each_listed_submodule()
submodule: port submodule subcommand 'status' from shell to C
builtin/submodule--helper.c | 273 +++++++++++++++++++++++++++++++++++++++++--- git-submodule.sh | 61 +--------- 2 files changed, 257 insertions(+), 77 deletions(-)
Signature · 1 line
2.14.2
PATCH v7 1/3 submodule--helper: introduce get_submodule_displaypath()
Introduce function get_submodule_displaypath() to replace the code occurring in submodule_init() for generating displaypath of the submodule with a call to it.
This new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)
builtin/submodule--helper.c +23 -12
@@ -219,6 +219,26 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *prreturn 0 ; } +/* the result should be freed by the caller. */ +static char *get_submodule_displaypath (const char *path,const char *prefix) +{ +const char *super_prefix =get_super_prefix (); + +if (prefix && super_prefix) { +BUG ("cannot have prefix '%s' and superprefix '%s'" , + prefix, super_prefix); + }else if (prefix) { +struct strbuf sb =STRBUF_INIT ; +char *displaypath =xstrdup (relative_path (path, prefix, &sb)); +strbuf_release (&sb); +return displaypath; + }else if (super_prefix) { +return xstrfmt ("%s%s" , super_prefix, path); + }else { +return xstrdup (path); + } +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -334,15 +354,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; -if (prefix &&get_super_prefix ()) -die ("BUG: cannot have prefix and superprefix" ); -else if (prefix) - displaypath =xstrdup (relative_path (path, prefix, &sb)); -else if (get_super_prefix ()) { -strbuf_addf (&sb,"%s%s" ,get_super_prefix (), path); - displaypath =strbuf_detach (&sb,NULL ); - }else - displaypath =xstrdup (path); + displaypath =get_submodule_displaypath (path, prefix); sub =submodule_from_path (&null_oid, path);
@@ -357,9 +369,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); +strbuf_reset (&sb); }/*
@@ -367,7 +379,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* To look up the url in .git/config, we must not fall back to * .gitmodules, so look it up directly. */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.url" , sub->name);if (git_config_get_string (sb.buf, &url)) {if (!sub->url)
@@ -404,9 +415,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath); } +strbuf_reset (&sb);/* Copy "update" setting when it is not set yet */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.update" , sub->name);if (git_config_get_string (sb.buf, &upd) && sub->update_strategy.type !=SM_UPDATE_UNSPECIFIED ) {
Signature · 1 line
2.14.2
I didn't find a URL in the cover letter pointing at previous iterations of this patch series and related discussions, so forgive me if comments below merely repeat what was said earlier...
Quoted 20 lines · Prathamesh Chavan
Introduce function
get_submodule_displaypath()to replace the code occurring insubmodule_init()for generating displaypath of the submodule with a call to it.This new function will also be used in other parts of the system in later patches.
Signed-off-by Prathamesh Chavan <[email protected]>---
builtin/submodule--helper.c +9
-0@@ -219,6 +219,26 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr +/* the result should be freed by the caller. */ +static char *get_submodule_displaypath (const char *path,const char *prefix) +{ +const char *super_prefix =get_super_prefix (); + +if (prefix && super_prefix) { +BUG ("cannot have prefix '%s' and superprefix '%s'" , + prefix, super_prefix); + }else if (prefix) {
builtin/submodule--helper.c static int resolve_relative_url_test(int argc, const char **argv, const char *pr · line 237
Quoted 10 lines
builtin/submodule--helper.c +10
-0+struct strbuf sb =STRBUF_INIT ; +char *displaypath =xstrdup (relative_path (path, prefix, &sb)); +strbuf_release (&sb); +return displaypath; + }else if (super_prefix) { +return xstrfmt ("%s%s" , super_prefix, path); + }else { +return xstrdup (path); + } +}
At first glance, this appears to be a simple code-movement patch which shouldn't require deep inspection by a reviewer, however, upon closer examination, it turns out that it is doing rather more than that, which increases reviewer burden, especially since these additional changes are not mentioned in the commit message. At a minimum, it includes these changes:
* factors out calls to get_super_prefix()
* adds extra context to the "BUG" message
* changes die("BUG...") to BUG(...)
* allocates/releases a strbuf
* changes assignments to returns
The final two are obvious necessary (or clarifying) changes which a reviewer would expect to see in a patch which factors code out to its own function; the others not so.
This isn't to say that the other changes are not reasonable -- they are -- but if one of your goals is to make the patches easy for reviewers to digest, then you should make the changes as obvious as possible for reviewers to spot. One way would be to mention in the commit message that you're taking the opportunity to also make these particular cleanups to the code. A more common approach is to place the various cleanups in preparatory patches before this one, with one cleanup per patch. I'd prefer to see the latter (if my opinion carries any weight).
More below...
Quoted 18 lines
builtin/submodule--helper.c +1
-9@@ -334,15 +354,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)struct strbuf sb =STRBUF_INIT ;char *upd =NULL , *url =NULL , *displaypath; -if (prefix &&get_super_prefix ()) -die ("BUG: cannot have prefix and superprefix" ); -else if (prefix) - displaypath =xstrdup (relative_path (path, prefix, &sb)); -else if (get_super_prefix ()) { -strbuf_addf (&sb,"%s%s" ,get_super_prefix (), path); - displaypath =strbuf_detach (&sb,NULL ); - }else - displaypath =xstrdup (path); + displaypath =get_submodule_displaypath (path, prefix); sub =submodule_from_path (&null_oid, path);
builtin/submodule--helper.c static void init_submodule(const char *path, const char *prefix, int quiet) · line 374
Quoted 7 lines
builtin/submodule--helper.c +1
-1@@ -357,9 +369,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* Set active flag for the submodule being initialized */ if (!is_submodule_active (the_repository, path)) { -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.active" , sub->name);git_config_set_gently (sb.buf,"true" ); +strbuf_reset (&sb);
This strbuf_reset() movement, and those below, are pretty much just "noise" changes. They add extra burden to the review process without really improving the code. The reason they add to reviewer burden is that they do not seem to be related to the intention stated in the commit message, so the reviewer must spend extra time trying to understand their purpose and correctness.
More serious, though, is that these strbuf_reset() movements may actually increase the burden on someone changing the code in the future. Presumably, your reason for making these changes is that you reviewed the code after factoring out the get_submodule_displaypath() logic and discovered that the strbuf was no longer touched before this point, therefore resetting it before strbuf_addf() is unnecessary. While this may be true today, it may not be so in the future. If someone comes along and adds code above this point which does touch the strbuf, then these code movements either need to be reverted by that person (more noise) or that person needs to remember to add a strbuf_reset() at the end of the new code.
Moreover, it's somewhat easier to reason about the strbuf_reset()'s and the corresponding strbuf_addf()'s when they are kept together, as in the original code, so, for that reason alone, one could argue that moving the strbuf_reset()'s does not really improve the code.
I'd suggest dropping these changes in the re-roll.
Quoted 24 lines
builtin/submodule--helper.c +1
-3}/* @@ -367,7 +379,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)* To look up the url in .git/config, we must not fall back to * .gitmodules, so look it up directly. */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.url" , sub->name);if (git_config_get_string (sb.buf, &url)) {if (!sub->url)@@ -404,9 +415,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath); } +strbuf_reset (&sb);/* Copy "update" setting when it is not set yet */ -strbuf_reset (&sb);strbuf_addf (&sb,"submodule.%s.update" , sub->name);if (git_config_get_string (sb.buf, &upd) && sub->update_strategy.type !=SM_UPDATE_UNSPECIFIED ) { --2.14.2
PATCH v7 2/3 submodule--helper: introduce for_each_listed_submodule()
In reply to Prathamesh ChavanIntroduce function for_each_listed_submodule() and replace a loop in module_init() with a call to it.
The new function will also be used in other parts of the system in later patches.
--- builtin/submodule--helper.c | 40 +++++++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-)
builtin/submodule--helper.c +35 -5
@@ -14,6 +14,11 @@#include "refs.h" #include "connect.h" +#define OPT_QUIET (1 << 0) + +typedef void (*each_submodule_fn )(const struct cache_entry *list_item, +void *cb_data); +static char *get_default_remote (void ) {char *dest =NULL , *ret;
@@ -348,7 +353,23 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ; } -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_listed_submodule (const struct module_list *list, +each_submodule_fn fn,void *cb_data) +{ +int i; +for (i =0 ; i < list->nr; i++) +fn (list->entries[i], cb_data); +} + +struct init_cb { +const char *prefix; +unsigned int flags; +}; + +#define INIT_CB_INIT { NULL, 0 } + +static void init_submodule (const char *path,const char *prefix, +unsigned int flags) {const struct submodule *sub;struct strbuf sb =STRBUF_INIT ;
@@ -410,7 +431,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)if (git_config_set_gently (sb.buf, url))die (_ ("Failed to register url for submodule path '%s'" ), displaypath); -if (!quiet) +if (!(flags &OPT_QUIET ))fprintf (stderr,_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath);
@@ -437,12 +458,18 @@ static void init_submodule(const char *path, const char *prefix, int quiet)free (upd); } +static void init_submodule_cb (const struct cache_entry *list_item,void *cb_data) +{ +struct init_cb *info = cb_data; +init_submodule (list_item->name, info->prefix, info->flags); +} +static int module_init (int argc,const char **argv,const char *prefix) { +struct init_cb info =INIT_CB_INIT ;struct pathspec pathspec;struct module_list list =MODULE_LIST_INIT ;int quiet =0 ; -int i;struct option module_init_options[] = {OPT__QUIET (&quiet,N_ ("Suppress output for initializing a submodule" )),
@@ -467,8 +494,11 @@ static int module_init(int argc, const char **argv, const char *prefix)if (!argc &&git_config_get_value_multi ("submodule.active" ))module_list_active (&list); -for (i =0 ; i < list.nr; i++) -init_submodule (list.entries[i]->name, prefix, quiet); + info.prefix = prefix; +if (quiet) + info.flags |=OPT_QUIET ; + +for_each_listed_submodule (&list, init_submodule_cb, &info);return 0 ; }
Signature · 1 line
2.14.2
Re: [PATCH v7 2/3] submodule--helper: introduce for_each_listed_submodule()
Same disclaimer as in my review of patch 1/3: I didn't see a URL in the cover letter pointing at discussions of earlier iterations, so below comments may be at odds with what went on previously...
Quoted 10 lines · Prathamesh Chavan
Introduce function
for_each_listed_submodule()and replace a loop inmodule_init()with a call to it.The new function will also be used in other parts of the system in later patches.
Signed-off-by Prathamesh Chavan <[email protected]>---
builtin/submodule--helper.c line 20
Quoted 7 lines
builtin/submodule--helper.c +4
-0@@ -14,6 +14,11 @@#include "refs.h" #include "connect.h" +#define OPT_QUIET (1 << 0) + +typedef void (*each_submodule_fn )(const struct cache_entry *list_item, +void *cb_data);
What is the reason for having the definition of 'each_submodule_fn' so far removed textually from its first reference by for_each_listed_submodule() below?
Quoted 5 lines
builtin/submodule--helper.c +0
-0static char *get_default_remote (void ) {char *dest =NULL , *ret;@@ -348,7 +353,23 @@ static int module_list(int argc, const char **argv, const char *prefix)return 0 ;
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 362
Quoted 10 lines
builtin/submodule--helper.c +7
-1} -static void init_submodule (const char *path,const char *prefix,int quiet) +static void for_each_listed_submodule (const struct module_list *list, +each_submodule_fn fn,void *cb_data) +{ +int i; +for (i =0 ; i < list->nr; i++) +fn (list->entries[i], cb_data); +}
I'm very curious about the justification for introducing a for-each function for what amounts to the simplest sort of loop possible: a canonical for-loop with a one-line body. I could easily understand the desire for such a function if either the loop conditions or the body of the loop, or both, were complex, but this does not seem to be the case. Even the callers of this new function, in this patch and in 3/3, are as simple as possible: one-liners (simple function calls).
Although this sort of for-each function can, at times, be helpful, there are costs: extra boilerplate and increased complexity for clients since it requires callback functions and (optionally) callback data. The separation of logic into a callback function can make code more difficult to reason about than when it is simply the body of a for-loop.
So, unless the plan for the future is that this for-each function will have considerable additional functionality baked into it, I'm having a difficult time understanding why this change is desirable.
builtin/submodule--helper.c static int module_list(int argc, const char **argv, const char *prefix) · line 369
Quoted 6 lines
builtin/submodule--helper.c +6
-0+struct init_cb { +const char *prefix; +unsigned int flags; +}; + +#define INIT_CB_INIT { NULL, 0 }
Why are these definitions so far removed from init_submodule_cb() below?
builtin/submodule--helper.c static void init_submodule(const char *path, const char *prefix, int quiet) · line 434
Quoted 11 lines
builtin/submodule--helper.c +3
-1+static void init_submodule (const char *path,const char *prefix, +unsigned int flags) {const struct submodule *sub;struct strbuf sb =STRBUF_INIT ;@@ -410,7 +431,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)if (git_config_set_gently (sb.buf, url))die (_ ("Failed to register url for submodule path '%s'" ), displaypath); -if (!quiet) +if (!(flags &OPT_QUIET ))
This change of having init_submodule() accept a 'flags' argument, rather than a single boolean, increases reviewer burden, since the reviewer is forced to puzzle out how this change relates to the stated intention of the patch since it is not mentioned at all by the commit message.
It's also conceptually unrelated to the introduction of a for-each function, thus should be instead be done by a separate preparatory patch.
Quoted 39 lines
builtin/submodule--helper.c +12
-4fprintf (stderr,_ ("Submodule '%s' (%s) registered for path '%s'\n" ), sub->name, url, displaypath);@@ -437,12 +458,18 @@ static void init_submodule(const char *path, const char *prefix, int quiet)free (upd); } +static void init_submodule_cb (const struct cache_entry *list_item,void *cb_data) +{ +struct init_cb *info = cb_data; +init_submodule (list_item->name, info->prefix, info->flags); +} +static int module_init (int argc,const char **argv,const char *prefix) { +struct init_cb info =INIT_CB_INIT ;struct pathspec pathspec;struct module_list list =MODULE_LIST_INIT ;int quiet =0 ; -int i;struct option module_init_options[] = {OPT__QUIET (&quiet,N_ ("Suppress output for initializing a submodule" )),@@ -467,8 +494,11 @@ static int module_init(int argc, const char **argv, const char *prefix)if (!argc &&git_config_get_value_multi ("submodule.active" ))module_list_active (&list); -for (i =0 ; i < list.nr; i++) -init_submodule (list.entries[i]->name, prefix, quiet); + info.prefix = prefix; +if (quiet) + info.flags |=OPT_QUIET ; + +for_each_listed_submodule (&list, init_submodule_cb, &info);return 0 ; } --2.14.2
PATCH v7 3/3 submodule: port submodule subcommand 'status' from shell to C
In reply to Prathamesh ChavanThis 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 four functions: module_status(), submodule_status_cb(), 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_listed_submodule() looping through the list obtained.
Then for_each_listed_submodule() calls submodule_status_cb() for each of the submodule in its list. The function submodule_status_cb() calls submodule_status() after passing appropriate arguments to the funciton. 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.
Function set_name_rev() is also ported from git-submodule to the submodule--helper builtin function compute_rev_name(), which now generates the value of the revision name as required.
--- builtin/submodule--helper.c | 198 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 61 +------------- 2 files changed, 199 insertions(+), 60 deletions(-)
builtin/submodule--helper.c +198 -0
@@ -13,8 +13,13 @@#include "remote.h" #include "refs.h" #include "connect.h" +#include "revision.h" +#include "diffcore.h" +#include "diff.h" #define OPT_QUIET (1 << 0) +#define OPT_CACHED (1 << 1) +#define OPT_RECURSIVE (1 << 2)typedef void (*each_submodule_fn )(const struct cache_entry *list_item,void *cb_data);
@@ -244,6 +249,44 @@ static char *get_submodule_displaypath(const char *path, const char *prefix) } } +static char *compute_rev_name (const char *sub_path,const char * object_id) +{ +struct strbuf sb =STRBUF_INIT ; +const char ***d; + +static const char *describe_bare[] = {NULL }; + +static const char *describe_tags[] = {"--tags" ,NULL }; + +static const char *describe_contains[] = {"--contains" ,NULL }; + +static const char *describe_all_always[] = {"--all" ,"--always" ,NULL }; + +static const char **describe_argv[] = { describe_bare, describe_tags, + describe_contains, + describe_all_always,NULL }; + +for (d = describe_argv; *d; d++) { +struct child_process cp =CHILD_PROCESS_INIT ; +prepare_submodule_repo_env (&cp.env_array); + cp.dir = sub_path; + cp.git_cmd =1 ; + cp.no_stderr =1 ; + +argv_array_push (&cp.args,"describe" ); +argv_array_pushv (&cp.args, *d); +argv_array_push (&cp.args, object_id); + +if (!capture_command (&cp, &sb,0 )) { +strbuf_strip_suffix (&sb,"\n" ); +return strbuf_detach (&sb,NULL ); + } + } + +strbuf_release (&sb); +return NULL ; +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -503,6 +546,160 @@ static int module_init(int argc, const char **argv, const char *prefix)return 0 ; } +struct status_cb { +const char *prefix; +unsigned int flags; +}; + +#define STATUS_CB_INIT { NULL, 0 } + +static void print_status (unsigned int flags,char state,const char *path, +const struct object_id *oid,const char *displaypath) +{ +if (flags &OPT_QUIET ) +return ; + +printf ("%c%s %s" , state,oid_to_hex (oid), displaypath); + +if (state ==' ' || state =='+' ) +printf (" (%s)" ,compute_rev_name (path,oid_to_hex (oid))); + +printf ("\n" ); +} + +static int handle_submodule_head_ref (const char *refname, +const struct object_id *oid,int flags, +void *cb_data) +{ +struct object_id *output = cb_data; +if (oid) +oidcpy (output, oid); + +return 0 ; +} + +static void status_submodule (const char *path,const struct object_id *ce_oid, +unsigned int ce_flags,const char *prefix, +unsigned int flags) +{ +char *displaypath; +struct argv_array diff_files_args =ARGV_ARRAY_INIT ; +struct rev_info rev; +int diff_files_result; + +if (!submodule_from_path (&null_oid, path)) +die (_ ("no submodule mapping found in .gitmodules for path '%s'" ), + path); + + displaypath =get_submodule_displaypath (path, prefix); + +if ((CE_STAGEMASK & ce_flags) >>CE_STAGESHIFT ) { +print_status (flags,'U' , path, &null_oid, displaypath); + goto cleanup; + } + +if (!is_submodule_active (the_repository, path)) { +print_status (flags,'-' , path, ce_oid, displaypath); + goto cleanup; + } + +argv_array_pushl (&diff_files_args,"diff-files" , +"--ignore-submodules=dirty" ,"--quiet" ,"--" , + path,NULL ); + +git_config (git_diff_basic_config,NULL ); +init_revisions (&rev, prefix); + rev.abbrev =0 ; + diff_files_args.argc =setup_revisions (diff_files_args.argc, + diff_files_args.argv, + &rev,NULL ); + diff_files_result =run_diff_files (&rev,0 ); + +if (!diff_result_code (&rev.diffopt, diff_files_result)) { +print_status (flags,' ' , path, ce_oid, + displaypath); + }else if (!(flags &OPT_CACHED )) { +struct object_id oid; + +if (refs_head_ref (get_submodule_ref_store (path), + handle_submodule_head_ref, &oid)) +die (_ ("could not resolve HEAD ref inside the" +"submodule '%s'" ), path); + +print_status (flags,'+' , path, &oid, displaypath); + }else { +print_status (flags,'+' , path, ce_oid, displaypath); + } + +if (flags &OPT_RECURSIVE ) { +struct child_process cpr =CHILD_PROCESS_INIT ; + + cpr.git_cmd =1 ; + cpr.dir = path; +prepare_submodule_repo_env (&cpr.env_array); + +argv_array_push (&cpr.args,"--super-prefix" ); +argv_array_pushf (&cpr.args,"%s/" , displaypath); +argv_array_pushl (&cpr.args,"submodule--helper" ,"status" , +"--recursive" ,NULL ); + +if (flags &OPT_CACHED ) +argv_array_push (&cpr.args,"--cached" ); + +if (flags &OPT_QUIET ) +argv_array_push (&cpr.args,"--quiet" ); + +if (run_command (&cpr)) +die (_ ("failed to recurse into submodule '%s'" ), path); + } + +cleanup: +argv_array_clear (&diff_files_args); +free (displaypath); +} + +static void status_submodule_cb (const struct cache_entry *list_item, +void *cb_data) +{ +struct status_cb *info = cb_data; +status_submodule (list_item->name, &list_item->oid, list_item->ce_flags, + info->prefix, info->flags); +} + +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 ; + +struct option module_status_options[] = { +OPT__QUIET (&quiet,N_ ("Suppress submodule status output" )), +OPT_BIT (0 ,"cached" , &info.flags,N_ ("Use commit stored in the index instead of the one stored in the submodule HEAD" ),OPT_CACHED ), +OPT_BIT (0 ,"recursive" , &info.flags,N_ ("recurse into nested submodules" ),OPT_RECURSIVE ), +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; +if (quiet) + info.flags |=OPT_QUIET ; + +for_each_listed_submodule (&list, status_submodule_cb, &info); + +return 0 ; +} +static int module_name (int argc,const char **argv,const char *prefix) {const struct submodule *sub;
@@ -1300,6 +1497,7 @@ static struct cmd_struct commands[] = { {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 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},
git-submodule.sh +1 -60
73 lines not on this page · raw message
Signature · 1 line
2.14.2
Re: [PATCH v7 0/3] Incremental rewrite of git-submodules
In reply to Prathamesh ChavanQuoted 1 line · Prathamesh Chavan
Changes in v7:
FWIW, the previous one is at
https://public-inbox.org/git/[email protected]
Alternatively, the References link can be followed back from the cover letter to go back to quite an early iteration of the series.
Hope that helps ;-)
Re: [PATCH v7 0/3] Incremental rewrite of git-submodules
Quoted 3 lines · Junio C Hamano
FWIW, the previous one is at
https://public-inbox.org/git/[email protected]Hope that helps ;-)
Thanks, it does help.
Having scanned discussions of previous versions, I see that some of my comments do indeed overlap (and sometimes are at odds) with comments from other reviewers.
Re: [PATCH v6 0/3] Incremental rewrite of git-submodules
In reply to Prathamesh ChavanQuoted 5 lines · Prathamesh Chavan
* The function
get_submodule_displaypath()was modified for the case whenget_super_prefix()returns a non-null value. The condition to check if the super-prefix ends with a '/' is removed. To accomodate this change appropriate value ofsuper_prefixis passed instead in the recursive calls ofinit_submodule()andstatus_submodule().
OK.
Quoted 5 lines
* To accomodate the possiblity of a direct call to the function
init_submodule(), a callback functioninit_submodule_cb()is introduced which takescache_entryandinit_cbstructures as input params, and callsinit_submodule()with parameters which are more appropriate for a direct call of this function.
OK.
Quoted 6 lines
* Similar changes were even done for
status_submodule(). But as it was observed that the number of params increased a lot due to flags like quiet, recursive, cached, etc, and keeping in mind the future subcommand's ported functions as well, a single unsigned int calledcb_flagswas introduced to store all of these flags, instead of having parameter for each one.
Use of a flag word instead of many bit parameter is a very good idea. I do not think it is brilliant to call a field in a structure that is used as an interface to the callback interface cb_flags, though, especially when it is already clear that it is about the callback from the name the structure. Just name them "flags".
I may or may not leave more detailed comments on individual patches.
Thanks.
PATCH v2 3/4 submodule: port set_name_rev() from shell to C
In reply to Prathamesh ChavanFunction set_name_rev() is ported from git-submodule to the submodule--helper builtin. The function compute_rev_name() generates the value of the revision name as required.
The function get_rev_name() calls compute_rev_name() and receives the revision name, and later handles its formating and printing.
--- builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 16 ++---------- 2 files changed, 65 insertions(+), 14 deletions(-)
builtin/submodule--helper.c +63 -0
@@ -245,6 +245,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix) } } +static char *compute_rev_name (const char *sub_path,const char * object_id) +{ +struct strbuf sb =STRBUF_INIT ; +const char ***d; + +static const char *describe_bare[] = { +NULL + }; + +static const char *describe_tags[] = { +"--tags" ,NULL + }; + +static const char *describe_contains[] = { +"--contains" ,NULL + }; + +static const char *describe_all_always[] = { +"--all" ,"--always" ,NULL + }; + +static const char **describe_argv[] = { + describe_bare, describe_tags, describe_contains, + describe_all_always,NULL + }; + +for (d = describe_argv; *d; d++) { +struct child_process cp =CHILD_PROCESS_INIT ; +prepare_submodule_repo_env (&cp.env_array); + cp.dir = sub_path; + cp.git_cmd =1 ; + cp.no_stderr =1 ; + +argv_array_push (&cp.args,"describe" ); +argv_array_pushv (&cp.args, *d); +argv_array_push (&cp.args, object_id); + +if (!capture_command (&cp, &sb,0 ) && sb.len) { +strbuf_strip_suffix (&sb,"\n" ); +return strbuf_detach (&sb,NULL ); + } + } + +strbuf_release (&sb); +return NULL ; +} + +static int get_rev_name (int argc,const char **argv,const char *prefix) +{ +char *revname; +if (argc !=3 ) +die ("get-rev-name only accepts two arguments: <path> <sha1>" ); + + revname =compute_rev_name (argv[1 ], argv[2 ]); +if (revname && revname[0 ]) +printf (" (%s)" , revname); +printf ("\n" ); + +free (revname); +return 0 ; +} +struct module_list {const struct cache_entry **entries;int alloc, nr;
@@ -1243,6 +1305,7 @@ static struct cmd_struct commands[] = { {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url", resolve_relative_url, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, + {"get-rev-name", get_rev_name, 0}, {"init", module_init, SUPPORT_SUPER_PREFIX}, {"remote-branch", resolve_remote_submodule_branch, 0}, {"push-check", push_check, 0},
git-submodule.sh +2 -14
@@ -759,18 +759,6 @@ cmd_update() } } -set_name_rev () { - revname=$( ( - sanitize_submodule_env - cd "$1" && { - git describe "$2" 2>/dev/null || - git describe --tags "$2" 2>/dev/null || - git describe --contains "$2" 2>/dev/null || - git describe --all --always "$2" - } - ) ) - test -z "$revname" || revname=" ($revname)" -} # # Show commit summary for submodules in index or working tree #
@@ -1042,14 +1030,14 @@ cmd_status() fi if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path" then - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$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 - set_name_rev "$sm_path" "$sha1" + revname=$(git submodule--helper get-rev-name "$sm_path" "$sha1") say "+$sha1 $displaypath$revname" fi
Signature · 1 line
2.13.0
PATCH v2 4/4 submodule: port submodule subcommand 'status' from shell to C
In reply to Prathamesh ChavanThis 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() looping through the list obtained.
Then for_each_submodule() 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.
--- builtin/submodule--helper.c | 156 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 49 +------------- 2 files changed, 157 insertions(+), 48 deletions(-)
builtin/submodule--helper.c +156 -0
@@ -561,6 +561,161 @@ 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, +const struct object_id *oid,const char *displaypath) +{ +if (info->quiet) +return ; + +printf ("%c%s %s" , state,oid_to_hex (oid), displaypath); + +if (state ==' ' || state =='+' ) { +struct argv_array get_rev_args =ARGV_ARRAY_INIT ; + +argv_array_pushl (&get_rev_args,"get-rev-name" , + path,oid_to_hex (oid),NULL ); +get_rev_name (get_rev_args.argc, get_rev_args.argv, + info->prefix); + +argv_array_clear (&get_rev_args); + }else { +printf ("\n" ); + } +} + +static int handle_submodule_head_ref (const char *refname, +const struct object_id *oid,int flags, +void *cb_data) +{ +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 *displaypath; +struct argv_array diff_files_args =ARGV_ARRAY_INIT ; + +if (!submodule_from_path (&null_oid, 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 (ce_stage (list_item)) { +print_status (info,'U' , list_item->name, + &null_oid, displaypath); + goto cleanup; + } + +if (!is_submodule_active (the_repository, list_item->name)) { +print_status (info,'-' , list_item->name, &list_item->oid, + 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, &list_item->oid, + displaypath); + }else { +if (!info->cached) { +struct object_id oid; + +if (head_ref_submodule (list_item->name, + handle_submodule_head_ref, &oid)) +die (_ ("could not resolve HEAD ref inside the" +"submodule '%s'" ), list_item->name); + +print_status (info,'+' , list_item->name, &oid, + displaypath); + }else { +print_status (info,'+' , list_item->name, + &list_item->oid, 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, +"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: +argv_array_clear (&diff_files_args); +free (displaypath); +} + +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, status_submodule, &info); + +return 0 ; +} +static int module_name (int argc,const char **argv,const char *prefix) {const struct submodule *sub;
@@ -1307,6 +1462,7 @@ static struct cmd_struct commands[] = { {"resolve-relative-url-test", resolve_relative_url_test, 0}, {"get-rev-name", get_rev_name, 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},
git-submodule.sh +1 -48
55 lines not on this page · raw message
Signature · 1 line
2.13.0