* [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first @ 2015-08-31 19:19 Stefan Beller 2015-08-31 19:19 ` [PATCH 1/3] submodule: implement `module_list` as a builtin helper Stefan Beller ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Stefan Beller @ 2015-08-31 19:19 UTC (permalink / raw) To: gitster Cc: peff, git, jrnieder, johannes.schindelin, Jens.Lehmann, Stefan Beller This patch series is the first part of the larger series I sent daily last week and contains only those patches which have had good review by Jeff, Dscho and Junio doing only internal refactoring, not introducing any parallelism yet. It replaces all patches on top of (5a1ba6b48a62b, Merge 'hv/submodule-config' to 'sb/submodule-helper') Changes are only small nits: * whitespace correction in free((char *)pointer); * break a long line, * git submodule--helper -h adheres to the standard help output One of the mistakes I made last week was mixing these quite stable patches with highly RFC/WIP patches and not pointing out loud which is which. Thanks, Stefan Stefan Beller (3): submodule: implement `module_list` as a builtin helper submodule: implement `module_name` as a builtin helper submodule: implement `module_clone` as a builtin helper .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/submodule--helper.c | 286 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 166 +++---------------------- git.c | 1 + 6 files changed, 306 insertions(+), 150 deletions(-) create mode 100644 builtin/submodule--helper.c -- 2.5.0.264.geed6e44.dirty ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/3] submodule: implement `module_list` as a builtin helper 2015-08-31 19:19 [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first Stefan Beller @ 2015-08-31 19:19 ` Stefan Beller 2015-08-31 21:55 ` Eric Sunshine 2015-08-31 19:19 ` [PATCH 2/3] submodule: implement `module_name` " Stefan Beller ` (2 subsequent siblings) 3 siblings, 1 reply; 21+ messages in thread From: Stefan Beller @ 2015-08-31 19:19 UTC (permalink / raw) To: gitster Cc: peff, git, jrnieder, johannes.schindelin, Jens.Lehmann, Stefan Beller Most of the submodule operations work on a set of submodules. Calculating and using this set is usually done via: module_list "$@" | { while read mode sha1 stage sm_path do # the actual operation done } Currently the function `module_list` is implemented in the git-submodule.sh as a shell script wrapping a perl script. The rewrite is in C, such that it is faster and can later be easily adapted when other functions are rewritten in C. git-submodule.sh similar to the builtin commands will navigate to the top most directory of the repository and keeping the subdirectories as a variable. As the helper is called from within the git-submodule.sh script, we are already navigated to the root level, but the path arguments are stil relative to the subdirectory we were in when calling git-submodule.sh. That's why there is a `--prefix` option pointing to an alternative path where to anchor relative path arguments. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/submodule--helper.c | 114 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 54 +++------------------ git.c | 1 + 6 files changed, 124 insertions(+), 48 deletions(-) create mode 100644 builtin/submodule--helper.c diff --git a/.gitignore b/.gitignore index 4fd81ba..1c2f832 100644 --- a/.gitignore +++ b/.gitignore @@ -155,6 +155,7 @@ /git-status /git-stripspace /git-submodule +/git-submodule--helper /git-svn /git-symbolic-ref /git-tag diff --git a/Makefile b/Makefile index 24b636d..d434e73 100644 --- a/Makefile +++ b/Makefile @@ -901,6 +901,7 @@ BUILTIN_OBJS += builtin/shortlog.o BUILTIN_OBJS += builtin/show-branch.o BUILTIN_OBJS += builtin/show-ref.o BUILTIN_OBJS += builtin/stripspace.o +BUILTIN_OBJS += builtin/submodule--helper.o BUILTIN_OBJS += builtin/symbolic-ref.o BUILTIN_OBJS += builtin/tag.o BUILTIN_OBJS += builtin/unpack-file.o diff --git a/builtin.h b/builtin.h index 839483d..924e6c4 100644 --- a/builtin.h +++ b/builtin.h @@ -119,6 +119,7 @@ extern int cmd_show(int argc, const char **argv, const char *prefix); extern int cmd_show_branch(int argc, const char **argv, const char *prefix); extern int cmd_status(int argc, const char **argv, const char *prefix); extern int cmd_stripspace(int argc, const char **argv, const char *prefix); +extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix); extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); extern int cmd_tag(int argc, const char **argv, const char *prefix); extern int cmd_tar_tree(int argc, const char **argv, const char *prefix); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c new file mode 100644 index 0000000..beaab7d --- /dev/null +++ b/builtin/submodule--helper.c @@ -0,0 +1,114 @@ +#include "builtin.h" +#include "cache.h" +#include "parse-options.h" +#include "quote.h" +#include "pathspec.h" +#include "dir.h" +#include "utf8.h" + +static const struct cache_entry **ce_entries; +static int ce_alloc, ce_used; +static const char *alternative_path; + +static int module_list_compute(int argc, const char **argv, + const char *prefix, + struct pathspec *pathspec) +{ + int i; + char *max_prefix, *ps_matched = NULL; + int max_prefix_len; + parse_pathspec(pathspec, 0, + PATHSPEC_PREFER_FULL | + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP, + prefix, argv); + + /* Find common prefix for all pathspec's */ + max_prefix = common_prefix(pathspec); + max_prefix_len = max_prefix ? strlen(max_prefix) : 0; + + if (pathspec->nr) + ps_matched = xcalloc(pathspec->nr, 1); + + if (read_cache() < 0) + die("index file corrupt"); + + for (i = 0; i < active_nr; i++) { + const struct cache_entry *ce = active_cache[i]; + + if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), + max_prefix_len, ps_matched, + S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode))) + continue; + + if (S_ISGITLINK(ce->ce_mode)) { + ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc); + ce_entries[ce_used++] = ce; + } + + while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name)) + /* + * Skip entries with the same name in different stages + * to make sure an entry is returned only once. + */ + i++; + } + free(max_prefix); + + if (ps_matched && report_path_error(ps_matched, pathspec, prefix)) + return -1; + + return 0; +} + +static int module_list(int argc, const char **argv, const char *prefix) +{ + int i; + static struct pathspec pathspec; + + struct option module_list_options[] = { + OPT_STRING(0, "prefix", &alternative_path, + N_("path"), + N_("alternative anchor for relative paths")), + OPT_END() + }; + + static const char * const git_submodule_helper_usage[] = { + N_("git submodule--helper module_list [--prefix=<path>] [<path>...]"), + NULL + }; + + argc = parse_options(argc, argv, prefix, module_list_options, + git_submodule_helper_usage, 0); + + if (module_list_compute(argc, argv, alternative_path + ? alternative_path + : prefix, &pathspec) < 0) { + printf("#unmatched\n"); + return 1; + } + + for (i = 0; i < ce_used; i++) { + const struct cache_entry *ce = ce_entries[i]; + + if (ce_stage(ce)) { + printf("%06o %s U\t", ce->ce_mode, sha1_to_hex(null_sha1)); + } else { + printf("%06o %s %d\t", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce)); + } + + utf8_fprintf(stdout, "%s\n", ce->name); + } + return 0; +} + +int cmd_submodule__helper(int argc, const char **argv, const char *prefix) +{ + if (argc < 2) + goto usage; + + if (!strcmp(argv[1], "module_list")) + return module_list(argc - 1, argv + 1, prefix); + +usage: + usage("git submodule--helper module_list\n"); +} diff --git a/git-submodule.sh b/git-submodule.sh index 36797c3..af9ecef 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -145,48 +145,6 @@ relative_path () echo "$result$target" } -# -# Get submodule info for registered submodules -# $@ = path to limit submodule list -# -module_list() -{ - eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")" - ( - git ls-files -z --error-unmatch --stage -- "$@" || - echo "unmatched pathspec exists" - ) | - @@PERL@@ -e ' - my %unmerged = (); - my ($null_sha1) = ("0" x 40); - my @out = (); - my $unmatched = 0; - $/ = "\0"; - while (<STDIN>) { - if (/^unmatched pathspec/) { - $unmatched = 1; - next; - } - chomp; - my ($mode, $sha1, $stage, $path) = - /^([0-7]+) ([0-9a-f]{40}) ([0-3])\t(.*)$/; - next unless $mode eq "160000"; - if ($stage ne "0") { - if (!$unmerged{$path}++) { - push @out, "$mode $null_sha1 U\t$path\n"; - } - next; - } - push @out, "$_\n"; - } - if ($unmatched) { - print "#unmatched\n"; - } else { - print for (@out); - } - ' -} - die_if_unmatched () { if test "$1" = "#unmatched" @@ -532,7 +490,7 @@ cmd_foreach() # command in the subshell (and a recursive call to this function) exec 3<&0 - module_list | + git submodule--helper module_list --prefix "$wt_prefix"| while read mode sha1 stage sm_path do die_if_unmatched "$mode" @@ -592,7 +550,7 @@ cmd_init() shift done - module_list "$@" | + git submodule--helper module_list --prefix "$wt_prefix" "$@" | while read mode sha1 stage sm_path do die_if_unmatched "$mode" @@ -674,7 +632,7 @@ cmd_deinit() die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" fi - module_list "$@" | + git submodule--helper module_list --prefix "$wt_prefix" "$@" | while read mode sha1 stage sm_path do die_if_unmatched "$mode" @@ -790,7 +748,7 @@ cmd_update() fi cloned_modules= - module_list "$@" | { + git submodule--helper module_list --prefix "$wt_prefix" "$@" | { err= while read mode sha1 stage sm_path do @@ -1222,7 +1180,7 @@ cmd_status() shift done - module_list "$@" | + git submodule--helper module_list --prefix "$wt_prefix" "$@" | while read mode sha1 stage sm_path do die_if_unmatched "$mode" @@ -1299,7 +1257,7 @@ cmd_sync() esac done cd_to_toplevel - module_list "$@" | + git submodule--helper module_list --prefix "$wt_prefix" "$@" | while read mode sha1 stage sm_path do die_if_unmatched "$mode" diff --git a/git.c b/git.c index 55c327c..deecba0 100644 --- a/git.c +++ b/git.c @@ -469,6 +469,7 @@ static struct cmd_struct commands[] = { { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE }, { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, { "stripspace", cmd_stripspace }, + { "submodule--helper", cmd_submodule__helper, RUN_SETUP }, { "symbolic-ref", cmd_symbolic_ref, RUN_SETUP }, { "tag", cmd_tag, RUN_SETUP }, { "unpack-file", cmd_unpack_file, RUN_SETUP }, -- 2.5.0.264.geed6e44.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] submodule: implement `module_list` as a builtin helper 2015-08-31 19:19 ` [PATCH 1/3] submodule: implement `module_list` as a builtin helper Stefan Beller @ 2015-08-31 21:55 ` Eric Sunshine 0 siblings, 0 replies; 21+ messages in thread From: Eric Sunshine @ 2015-08-31 21:55 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Jeff King, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller <sbeller@google.com> wrote: > Most of the submodule operations work on a set of submodules. > Calculating and using this set is usually done via: > > module_list "$@" | { > while read mode sha1 stage sm_path > do > # the actual operation > done > } > > Currently the function `module_list` is implemented in the > git-submodule.sh as a shell script wrapping a perl script. > The rewrite is in C, such that it is faster and can later be > easily adapted when other functions are rewritten in C. > > git-submodule.sh similar to the builtin commands will navigate > to the top most directory of the repository and keeping the > subdirectories as a variable. ECANNOTPARSE Did you mean s/git-submodule.sh/&,/ s/commands/&,/ s/top most/top-most/ s/keeping/keep/ s/subdirectories/subdirectory/ ? > As the helper is called from > within the git-submodule.sh script, we are already navigated > to the root level, but the path arguments are stil relative s/stil/still/ > to the subdirectory we were in when calling git-submodule.sh. > That's why there is a `--prefix` option pointing to an alternative > path where to anchor relative path arguments. s/where to/at which to/ More below. > Signed-off-by: Stefan Beller <sbeller@google.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > new file mode 100644 > index 0000000..beaab7d > --- /dev/null > +++ b/builtin/submodule--helper.c > @@ -0,0 +1,114 @@ > +#include "builtin.h" > +#include "cache.h" > +#include "parse-options.h" > +#include "quote.h" > +#include "pathspec.h" > +#include "dir.h" > +#include "utf8.h" > + > +static const struct cache_entry **ce_entries; > +static int ce_alloc, ce_used; > +static const char *alternative_path; Why is 'alternative_path' declared at file scope? > +static int module_list_compute(int argc, const char **argv, > + const char *prefix, > + struct pathspec *pathspec) > +{ > + int i; > + char *max_prefix, *ps_matched = NULL; > + int max_prefix_len; > + parse_pathspec(pathspec, 0, > + PATHSPEC_PREFER_FULL | > + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP, > + prefix, argv); > + > + /* Find common prefix for all pathspec's */ > + max_prefix = common_prefix(pathspec); > + max_prefix_len = max_prefix ? strlen(max_prefix) : 0; > + > + if (pathspec->nr) > + ps_matched = xcalloc(pathspec->nr, 1); > + > + if (read_cache() < 0) > + die("index file corrupt"); > + > + for (i = 0; i < active_nr; i++) { > + const struct cache_entry *ce = active_cache[i]; > + > + if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), > + max_prefix_len, ps_matched, > + S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode))) > + continue; > + > + if (S_ISGITLINK(ce->ce_mode)) { > + ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc); > + ce_entries[ce_used++] = ce; > + } > + > + while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name)) > + /* > + * Skip entries with the same name in different stages > + * to make sure an entry is returned only once. > + */ > + i++; > + } > + free(max_prefix); > + > + if (ps_matched && report_path_error(ps_matched, pathspec, prefix)) > + return -1; > + > + return 0; Does 'ps_matched' need to be freed before these two 'return's? > +} > + > +static int module_list(int argc, const char **argv, const char *prefix) > +{ > + int i; > + static struct pathspec pathspec; Drop 'static'. > + > + struct option module_list_options[] = { > + OPT_STRING(0, "prefix", &alternative_path, > + N_("path"), > + N_("alternative anchor for relative paths")), > + OPT_END() > + }; > + > + static const char * const git_submodule_helper_usage[] = { You can drop this 'static' too. Style: *const > + N_("git submodule--helper module_list [--prefix=<path>] [<path>...]"), > + NULL > + }; > + > + argc = parse_options(argc, argv, prefix, module_list_options, > + git_submodule_helper_usage, 0); > + > + if (module_list_compute(argc, argv, alternative_path > + ? alternative_path > + : prefix, &pathspec) < 0) { > + printf("#unmatched\n"); > + return 1; > + } > + > + for (i = 0; i < ce_used; i++) { > + const struct cache_entry *ce = ce_entries[i]; > + > + if (ce_stage(ce)) { > + printf("%06o %s U\t", ce->ce_mode, sha1_to_hex(null_sha1)); > + } else { > + printf("%06o %s %d\t", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce)); > + } Style: drop unnecessary braces. > + utf8_fprintf(stdout, "%s\n", ce->name); > + } > + return 0; > +} > + > +int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > +{ > + if (argc < 2) > + goto usage; > + > + if (!strcmp(argv[1], "module_list")) > + return module_list(argc - 1, argv + 1, prefix); > + > +usage: > + usage("git submodule--helper module_list\n"); Aside: [bikeshedding on] I agree with Dscho that these subcommands would be better spelled with a hyphen than an underscore. If I recall correctly, the arguments for using underscore were (1) a less noisy diff, (2) these are internal commands nobody will be typing anyhow. However, (1) the diff noise will be the same with hyphens, and (2) people will want to test these commands manually anyhow, and its easier to type hyphens and easier to remember them since the precedent for hyphens in command-names is already well established. Also, the reason that the original shell code used underscores was because hyphens are not valid characters in shell function names, but that's an implementation detail which shouldn't be allowed to bleed over to user-facing interface design (and these subcommands are user-facing). [bikeshedding off] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] submodule: implement `module_name` as a builtin helper 2015-08-31 19:19 [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first Stefan Beller 2015-08-31 19:19 ` [PATCH 1/3] submodule: implement `module_list` as a builtin helper Stefan Beller @ 2015-08-31 19:19 ` Stefan Beller 2015-08-31 20:35 ` Junio C Hamano 2015-08-31 22:01 ` Eric Sunshine 2015-08-31 19:19 ` [PATCH 3/3] submodule: implement `module_clone` " Stefan Beller 2015-08-31 20:15 ` [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first Junio C Hamano 3 siblings, 2 replies; 21+ messages in thread From: Stefan Beller @ 2015-08-31 19:19 UTC (permalink / raw) To: gitster Cc: peff, git, jrnieder, johannes.schindelin, Jens.Lehmann, Stefan Beller This implements the helper `module_name` in C instead of shell, yielding a nice performance boost. Before this patch, I measured a time (best out of three): $ time ./t7400-submodule-basic.sh >/dev/null real 0m11.066s user 0m3.348s sys 0m8.534s With this patch applied I measured (also best out of three) $ time ./t7400-submodule-basic.sh >/dev/null real 0m10.063s user 0m3.044s sys 0m7.487s Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/submodule--helper.c | 28 +++++++++++++++++++++++++++- git-submodule.sh | 32 +++++++------------------------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index beaab7d..c8f7e0c 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -5,6 +5,9 @@ #include "pathspec.h" #include "dir.h" #include "utf8.h" +#include "submodule.h" +#include "submodule-config.h" +#include "string-list.h" static const struct cache_entry **ce_entries; static int ce_alloc, ce_used; @@ -101,6 +104,26 @@ static int module_list(int argc, const char **argv, const char *prefix) return 0; } +static int module_name(int argc, const char **argv, const char *prefix) +{ + const char *name; + const struct submodule *sub; + + if (argc != 1) + usage("git submodule--helper module_name <path>\n"); + + gitmodules_config(); + sub = submodule_from_path(null_sha1, argv[0]); + + if (!sub) + die("No submodule mapping found in .gitmodules for path '%s'", argv[0]); + + name = sub->name; + printf("%s\n", name); + + return 0; +} + int cmd_submodule__helper(int argc, const char **argv, const char *prefix) { if (argc < 2) @@ -109,6 +132,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix) if (!strcmp(argv[1], "module_list")) return module_list(argc - 1, argv + 1, prefix); + if (!strcmp(argv[1], "module_name")) + return module_name(argc - 2, argv + 2, prefix); + usage: - usage("git submodule--helper module_list\n"); + usage("git submodule--helper [module_list | module_name]\n"); } diff --git a/git-submodule.sh b/git-submodule.sh index af9ecef..e6ff38d 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -178,24 +178,6 @@ get_submodule_config () { printf '%s' "${value:-$default}" } - -# -# Map submodule path to submodule name -# -# $1 = path -# -module_name() -{ - # Do we have "submodule.<something>.path = $1" defined in .gitmodules file? - sm_path="$1" - re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g') - name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' | - sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' ) - test -z "$name" && - die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$sm_path'")" - printf '%s\n' "$name" -} - # # Clone a submodule # @@ -498,7 +480,7 @@ cmd_foreach() then displaypath=$(relative_path "$sm_path") say "$(eval_gettext "Entering '\$prefix\$displaypath'")" - name=$(module_name "$sm_path") + name=$(git submodule--helper module_name "$sm_path") ( prefix="$prefix$sm_path/" clear_local_git_env @@ -554,7 +536,7 @@ cmd_init() while read mode sha1 stage sm_path do die_if_unmatched "$mode" - name=$(module_name "$sm_path") || exit + name=$(git submodule--helper module_name "$sm_path") || exit displaypath=$(relative_path "$sm_path") @@ -636,7 +618,7 @@ cmd_deinit() while read mode sha1 stage sm_path do die_if_unmatched "$mode" - name=$(module_name "$sm_path") || exit + name=$(git submodule--helper module_name "$sm_path") || exit displaypath=$(relative_path "$sm_path") @@ -758,7 +740,7 @@ cmd_update() echo >&2 "Skipping unmerged submodule $prefix$sm_path" continue fi - name=$(module_name "$sm_path") || exit + name=$(git submodule--helper module_name "$sm_path") || exit url=$(git config submodule."$name".url) branch=$(get_submodule_config "$name" branch master) if ! test -z "$update" @@ -1022,7 +1004,7 @@ cmd_summary() { # Respect the ignore setting for --for-status. if test -n "$for_status" then - name=$(module_name "$sm_path") + name=$(git submodule--helper module_name "$sm_path") ignore_config=$(get_submodule_config "$name" ignore none) test $status != A && test $ignore_config = all && continue fi @@ -1184,7 +1166,7 @@ cmd_status() while read mode sha1 stage sm_path do die_if_unmatched "$mode" - name=$(module_name "$sm_path") || exit + name=$(git submodule--helper module_name "$sm_path") || exit url=$(git config submodule."$name".url) displaypath=$(relative_path "$prefix$sm_path") if test "$stage" = U @@ -1261,7 +1243,7 @@ cmd_sync() while read mode sha1 stage sm_path do die_if_unmatched "$mode" - name=$(module_name "$sm_path") + name=$(git submodule--helper module_name "$sm_path") url=$(git config -f .gitmodules --get submodule."$name".url) # Possibly a url relative to parent -- 2.5.0.264.geed6e44.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] submodule: implement `module_name` as a builtin helper 2015-08-31 19:19 ` [PATCH 2/3] submodule: implement `module_name` " Stefan Beller @ 2015-08-31 20:35 ` Junio C Hamano 2015-08-31 20:51 ` Stefan Beller 2015-08-31 22:01 ` Eric Sunshine 1 sibling, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2015-08-31 20:35 UTC (permalink / raw) To: Stefan Beller; +Cc: peff, git, jrnieder, johannes.schindelin, Jens.Lehmann Stefan Beller <sbeller@google.com> writes: > usage: > - usage("git submodule--helper module_list\n"); > + usage("git submodule--helper [module_list | module_name]\n"); To me, the above reads as if saying: The command takes one of the two subcommands at this stage, module_list that does not take any parameter, and module_name that does not take any parameter. which is not what you intended. I think that the help for individual options and arguments are sufficiently given in the implementation of each subcommand (e.g. module_list does its own parse_options() thing), so there is no need to duplicate them here. The only purpose of this usage serves is to tell the user that the subcommand name was not understood, and give the list of available subcommands. For that, I wonder if the usual single-liner "usage" is the best way to do so. $ git submodule--helper frotz fatal: 'frotz' is not a valid submodule--helper subcommand, which are module_list, module_name. or something along that line, perhaps, may be more appropriate? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] submodule: implement `module_name` as a builtin helper 2015-08-31 20:35 ` Junio C Hamano @ 2015-08-31 20:51 ` Stefan Beller 0 siblings, 0 replies; 21+ messages in thread From: Stefan Beller @ 2015-08-31 20:51 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, git@vger.kernel.org, Jonathan Nieder, johannes.schindelin, Jens Lehmann On Mon, Aug 31, 2015 at 1:35 PM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> usage: >> - usage("git submodule--helper module_list\n"); >> + usage("git submodule--helper [module_list | module_name]\n"); > > To me, the above reads as if saying: > > The command takes one of the two subcommands at this stage, > module_list that does not take any parameter, and module_name > that does not take any parameter. > > which is not what you intended. > > I think that the help for individual options and arguments are > sufficiently given in the implementation of each subcommand > (e.g. module_list does its own parse_options() thing), so there is > no need to duplicate them here. The only purpose of this usage serves > is to tell the user that the subcommand name was not understood, and > give the list of available subcommands. For that, I wonder if the > usual single-liner "usage" is the best way to do so. > > $ git submodule--helper frotz > fatal: 'frotz' is not a valid submodule--helper subcommand, which are > module_list, module_name. > > or something along that line, perhaps, may be more appropriate? As this is something that *should* not happen in the wild, (but it will of course), it sounds like a good idea to have a clear error message here. I'll send a patch for that. (either one on top of 3/3 to improve the message, or a reroll of the series, as you'd like) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] submodule: implement `module_name` as a builtin helper 2015-08-31 19:19 ` [PATCH 2/3] submodule: implement `module_name` " Stefan Beller 2015-08-31 20:35 ` Junio C Hamano @ 2015-08-31 22:01 ` Eric Sunshine 1 sibling, 0 replies; 21+ messages in thread From: Eric Sunshine @ 2015-08-31 22:01 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Jeff King, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller <sbeller@google.com> wrote: > This implements the helper `module_name` in C instead of shell, > yielding a nice performance boost. > > Signed-off-by: Stefan Beller <sbeller@google.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index beaab7d..c8f7e0c 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -101,6 +104,26 @@ static int module_list(int argc, const char **argv, const char *prefix) > +static int module_name(int argc, const char **argv, const char *prefix) > +{ > + const char *name; > + const struct submodule *sub; > + > + if (argc != 1) > + usage("git submodule--helper module_name <path>\n"); > + > + gitmodules_config(); > + sub = submodule_from_path(null_sha1, argv[0]); > + > + if (!sub) > + die("No submodule mapping found in .gitmodules for path '%s'", argv[0]); In the original shell code, this error message went through eval_gettext(), so don't you want: die(_("No ..."), ...); ? > + name = sub->name; > + printf("%s\n", name); Why the useless assignment to 'name'? Instead: printf("%s\n", sub->name); > + return 0; > +} > + > int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > { > if (argc < 2) > @@ -109,6 +132,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > if (!strcmp(argv[1], "module_list")) > return module_list(argc - 1, argv + 1, prefix); > > + if (!strcmp(argv[1], "module_name")) > + return module_name(argc - 2, argv + 2, prefix); > + > usage: > - usage("git submodule--helper module_list\n"); > + usage("git submodule--helper [module_list | module_name]\n"); > } ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/3] submodule: implement `module_clone` as a builtin helper 2015-08-31 19:19 [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first Stefan Beller 2015-08-31 19:19 ` [PATCH 1/3] submodule: implement `module_list` as a builtin helper Stefan Beller 2015-08-31 19:19 ` [PATCH 2/3] submodule: implement `module_name` " Stefan Beller @ 2015-08-31 19:19 ` Stefan Beller 2015-08-31 22:35 ` Eric Sunshine 2015-08-31 20:15 ` [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first Junio C Hamano 3 siblings, 1 reply; 21+ messages in thread From: Stefan Beller @ 2015-08-31 19:19 UTC (permalink / raw) To: gitster Cc: peff, git, jrnieder, johannes.schindelin, Jens.Lehmann, Stefan Beller `module_clone` is part of the update command, which I want to convert to C next. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/submodule--helper.c | 148 +++++++++++++++++++++++++++++++++++++++++++- git-submodule.sh | 80 +----------------------- 2 files changed, 150 insertions(+), 78 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index c8f7e0c..d29499c 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -8,6 +8,7 @@ #include "submodule.h" #include "submodule-config.h" #include "string-list.h" +#include "run-command.h" static const struct cache_entry **ce_entries; static int ce_alloc, ce_used; @@ -124,6 +125,147 @@ static int module_name(int argc, const char **argv, const char *prefix) return 0; } +static int clone_submodule(const char *path, const char *gitdir, const char *url, + const char *depth, const char *reference, int quiet) +{ + struct child_process cp; + child_process_init(&cp); + + argv_array_push(&cp.args, "clone"); + argv_array_push(&cp.args, "--no-checkout"); + if (quiet) + argv_array_push(&cp.args, "--quiet"); + if (depth && strcmp(depth, "")) { + argv_array_push(&cp.args, "--depth"); + argv_array_push(&cp.args, depth); + } + if (reference && strcmp(reference, "")) { + argv_array_push(&cp.args, "--reference"); + argv_array_push(&cp.args, reference); + } + if (gitdir) { + argv_array_push(&cp.args, "--separate-git-dir"); + argv_array_push(&cp.args, gitdir); + } + argv_array_push(&cp.args, url); + argv_array_push(&cp.args, path); + + cp.git_cmd = 1; + cp.env = local_repo_env; + + cp.no_stdin = 1; + cp.no_stdout = 1; + cp.no_stderr = 1; + + return run_command(&cp); +} + +static int module_clone(int argc, const char **argv, const char *prefix) +{ + const char *path = NULL, *name = NULL, *url = NULL; + const char *reference = NULL, *depth = NULL; + int quiet = 0; + FILE *submodule_dot_git; + const char *sm_gitdir, *p; + struct strbuf rel_path = STRBUF_INIT; + struct strbuf sb = STRBUF_INIT; + + struct option module_update_options[] = { + OPT_STRING(0, "prefix", &alternative_path, + N_("path"), + N_("alternative anchor for relative paths")), + OPT_STRING(0, "path", &path, + N_("path"), + N_("where the new submodule will be cloned to")), + OPT_STRING(0, "name", &name, + N_("string"), + N_("name of the new submodule")), + OPT_STRING(0, "url", &url, + N_("string"), + N_("url where to clone the submodule from")), + OPT_STRING(0, "reference", &reference, + N_("string"), + N_("reference repository")), + OPT_STRING(0, "depth", &depth, + N_("string"), + N_("depth for shallow clones")), + OPT_END() + }; + + static const char * const git_submodule_helper_usage[] = { + N_("git submodule--helper update [--prefix=<path>] [--quiet] [--remote] [-N|--no-fetch]" + "[-f|--force] [--rebase|--merge] [--reference <repository>]" + "[--depth <depth>] [--recursive] [--] [<path>...]"), + NULL + }; + + argc = parse_options(argc, argv, prefix, module_update_options, + git_submodule_helper_usage, 0); + + if (getenv("GIT_QUIET")) + quiet = 1; + + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); + sm_gitdir = strbuf_detach(&sb, NULL); + + if (!file_exists(sm_gitdir)) { + safe_create_leading_directories_const(sm_gitdir); + if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet)) + die(N_("Clone of '%s' into submodule path '%s' failed"), + url, path); + } else { + safe_create_leading_directories_const(path); + unlink(sm_gitdir); + } + + /* Write a .git file in the submodule to redirect to the superproject. */ + if (alternative_path && !strcmp(alternative_path, "")) { + p = relative_path(path, alternative_path, &sb); + strbuf_reset(&sb); + } else + p = path; + + if (safe_create_leading_directories_const(p) < 0) + die("Could not create directory '%s'", p); + + strbuf_addf(&sb, "%s/.git", p); + + if (safe_create_leading_directories_const(sb.buf) < 0) + die(_("could not create leading directories of '%s'"), sb.buf); + submodule_dot_git = fopen(sb.buf, "w"); + if (!submodule_dot_git) + die ("Cannot open file '%s': %s", sb.buf, strerror(errno)); + + fprintf(submodule_dot_git, "gitdir: %s\n", + relative_path(sm_gitdir, path, &rel_path)); + if (fclose(submodule_dot_git)) + die("Could not close file %s", sb.buf); + strbuf_reset(&sb); + + /* Redirect the worktree of the submodule in the superprojects config */ + if (!is_absolute_path(sm_gitdir)) { + char *s = (char*)sm_gitdir; + if (strbuf_getcwd(&sb)) + die_errno("unable to get current working directory"); + strbuf_addf(&sb, "/%s", sm_gitdir); + sm_gitdir = strbuf_detach(&sb, NULL); + free(s); + } + + if (strbuf_getcwd(&sb)) + die_errno("unable to get current working directory"); + strbuf_addf(&sb, "/%s", path); + + p = git_pathdup_submodule(path, "config"); + if (!p) + die("Could not get submodule directory for '%s'", path); + git_config_set_in_file(p, "core.worktree", + relative_path(sb.buf, sm_gitdir, &rel_path)); + strbuf_release(&sb); + free((char *)sm_gitdir); + return 0; +} + int cmd_submodule__helper(int argc, const char **argv, const char *prefix) { if (argc < 2) @@ -135,6 +277,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix) if (!strcmp(argv[1], "module_name")) return module_name(argc - 2, argv + 2, prefix); + if (!strcmp(argv[1], "module_clone")) + return module_clone(argc - 1, argv + 1, prefix); + usage: - usage("git submodule--helper [module_list | module_name]\n"); + usage("git submodule--helper [module_list | module_name | " + "module_clone]\n"); } diff --git a/git-submodule.sh b/git-submodule.sh index e6ff38d..fb5155e 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -178,80 +178,6 @@ get_submodule_config () { printf '%s' "${value:-$default}" } -# -# Clone a submodule -# -# $1 = submodule path -# $2 = submodule name -# $3 = URL to clone -# $4 = reference repository to reuse (empty for independent) -# $5 = depth argument for shallow clones (empty for deep) -# -# Prior to calling, cmd_update checks that a possibly existing -# path is not a git repository. -# Likewise, cmd_add checks that path does not exist at all, -# since it is the location of a new submodule. -# -module_clone() -{ - sm_path=$1 - name=$2 - url=$3 - reference="$4" - depth="$5" - quiet= - if test -n "$GIT_QUIET" - then - quiet=-q - fi - - gitdir= - gitdir_base= - base_name=$(dirname "$name") - - gitdir=$(git rev-parse --git-dir) - gitdir_base="$gitdir/modules/$base_name" - gitdir="$gitdir/modules/$name" - - if test -d "$gitdir" - then - mkdir -p "$sm_path" - rm -f "$gitdir/index" - else - mkdir -p "$gitdir_base" - ( - clear_local_git_env - git clone $quiet ${depth:+"$depth"} -n ${reference:+"$reference"} \ - --separate-git-dir "$gitdir" "$url" "$sm_path" - ) || - die "$(eval_gettext "Clone of '\$url' into submodule path '\$sm_path' failed")" - fi - - # We already are at the root of the work tree but cd_to_toplevel will - # resolve any symlinks that might be present in $PWD - a=$(cd_to_toplevel && cd "$gitdir" && pwd)/ - b=$(cd_to_toplevel && cd "$sm_path" && pwd)/ - # Remove all common leading directories after a sanity check - if test "${a#$b}" != "$a" || test "${b#$a}" != "$b"; then - die "$(eval_gettext "Gitdir '\$a' is part of the submodule path '\$b' or vice versa")" - fi - while test "${a%%/*}" = "${b%%/*}" - do - a=${a#*/} - b=${b#*/} - done - # Now chop off the trailing '/'s that were added in the beginning - a=${a%/} - b=${b%/} - - # Turn each leading "*/" component into "../" - rel=$(printf '%s\n' "$b" | sed -e 's|[^/][^/]*|..|g') - printf '%s\n' "gitdir: $rel/$a" >"$sm_path/.git" - - rel=$(printf '%s\n' "$a" | sed -e 's|[^/][^/]*|..|g') - (clear_local_git_env; cd "$sm_path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b") -} - isnumber() { n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1" @@ -301,7 +227,7 @@ cmd_add() shift ;; --depth=*) - depth=$1 + depth="$1" ;; --) shift @@ -412,7 +338,7 @@ Use -f if you really want to add it." >&2 echo "$(eval_gettext "Reactivating local git directory for submodule '\$sm_name'.")" fi fi - module_clone "$sm_path" "$sm_name" "$realrepo" "$reference" "$depth" || exit + git submodule--helper module_clone --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" "$reference" "$depth" || exit ( clear_local_git_env cd "$sm_path" && @@ -774,7 +700,7 @@ Maybe you want to use 'update --init'?")" if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git then - module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit + git submodule--helper module_clone --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit cloned_modules="$cloned_modules;$name" subsha1= else -- 2.5.0.264.geed6e44.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper 2015-08-31 19:19 ` [PATCH 3/3] submodule: implement `module_clone` " Stefan Beller @ 2015-08-31 22:35 ` Eric Sunshine 2015-09-01 17:49 ` Stefan Beller 0 siblings, 1 reply; 21+ messages in thread From: Eric Sunshine @ 2015-08-31 22:35 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Jeff King, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller <sbeller@google.com> wrote: > `module_clone` is part of the update command, > which I want to convert to C next. Hmm, place commentary below "---". > Signed-off-by: Stefan Beller <sbeller@google.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index c8f7e0c..d29499c 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -124,6 +125,147 @@ static int module_name(int argc, const char **argv, const char *prefix) > return 0; > } > > +static int clone_submodule(const char *path, const char *gitdir, const char *url, > + const char *depth, const char *reference, int quiet) > +{ > + struct child_process cp; > + child_process_init(&cp); > + > + argv_array_push(&cp.args, "clone"); > + argv_array_push(&cp.args, "--no-checkout"); > + if (quiet) > + argv_array_push(&cp.args, "--quiet"); > + if (depth && strcmp(depth, "")) { More idiomatic: if (depth && *depth) { > + argv_array_push(&cp.args, "--depth"); > + argv_array_push(&cp.args, depth); These would be easier to read if you consolidate them with argv_array_pushl(): argv_array_pushl(&cp.args, "--depth", depth, NULL); and it would allow you to drop the noisy braces. > + } > + if (reference && strcmp(reference, "")) { if (reference && *reference) { > + argv_array_push(&cp.args, "--reference"); > + argv_array_push(&cp.args, reference); argv_array_pushl(&cp.args, "--reference", reference, NULL); > + } > + if (gitdir) { Why is this case different than the others, in that it doesn't check for the zero-length string ""? > + argv_array_push(&cp.args, "--separate-git-dir"); > + argv_array_push(&cp.args, gitdir); argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL); > + } > + argv_array_push(&cp.args, url); > + argv_array_push(&cp.args, path); > + > + cp.git_cmd = 1; > + cp.env = local_repo_env; > + > + cp.no_stdin = 1; > + cp.no_stdout = 1; > + cp.no_stderr = 1; > + > + return run_command(&cp); > +} > + > +static int module_clone(int argc, const char **argv, const char *prefix) > +{ > + const char *path = NULL, *name = NULL, *url = NULL; > + const char *reference = NULL, *depth = NULL; > + int quiet = 0; > + FILE *submodule_dot_git; > + const char *sm_gitdir, *p; Why is 'sm_gitdir' declared const even though it's always pointing at memory which this function owns and needs to free? If you drop the 'const', then you won't have to cast it to non-const in a couple places below. > + struct strbuf rel_path = STRBUF_INIT; > + struct strbuf sb = STRBUF_INIT; > + > + struct option module_update_options[] = { > + OPT_STRING(0, "prefix", &alternative_path, > + N_("path"), > + N_("alternative anchor for relative paths")), > + OPT_STRING(0, "path", &path, > + N_("path"), > + N_("where the new submodule will be cloned to")), > + OPT_STRING(0, "name", &name, > + N_("string"), > + N_("name of the new submodule")), > + OPT_STRING(0, "url", &url, > + N_("string"), > + N_("url where to clone the submodule from")), > + OPT_STRING(0, "reference", &reference, > + N_("string"), > + N_("reference repository")), > + OPT_STRING(0, "depth", &depth, > + N_("string"), > + N_("depth for shallow clones")), > + OPT_END() > + }; > + > + static const char * const git_submodule_helper_usage[] = { You can drop this 'static'. > + N_("git submodule--helper update [--prefix=<path>] [--quiet] [--remote] [-N|--no-fetch]" > + "[-f|--force] [--rebase|--merge] [--reference <repository>]" > + "[--depth <depth>] [--recursive] [--] [<path>...]"), > + NULL > + }; > + > + argc = parse_options(argc, argv, prefix, module_update_options, > + git_submodule_helper_usage, 0); > + > + if (getenv("GIT_QUIET")) > + quiet = 1; I understand that you're simply replicating the behavior of the shell code, but this is "yuck". 'module_clone' is only called from two places in git-submodule.sh, so how about a cleanup patch which makes 'module_clone' accept a --quiet flag and have the two callers pass it explicitly? Finally, have this C replacement accept --quiet as a proper command-line option rather than via this hidden backdoor method. > + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); > + sm_gitdir = strbuf_detach(&sb, NULL); > + > + if (!file_exists(sm_gitdir)) { > + safe_create_leading_directories_const(sm_gitdir); Why is it that the return value of safe_create_leading_directories_const() is sometimes checked in this function and sometimes not? > + if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet)) > + die(N_("Clone of '%s' into submodule path '%s' failed"), > + url, path); > + } else { > + safe_create_leading_directories_const(path); > + unlink(sm_gitdir); Check the return value of unlink()? > + } > + > + /* Write a .git file in the submodule to redirect to the superproject. */ > + if (alternative_path && !strcmp(alternative_path, "")) { Maybe? if (alternative_path && !*alternative_path) { > + p = relative_path(path, alternative_path, &sb); > + strbuf_reset(&sb); > + } else > + p = path; > + > + if (safe_create_leading_directories_const(p) < 0) > + die("Could not create directory '%s'", p); > + > + strbuf_addf(&sb, "%s/.git", p); > + > + if (safe_create_leading_directories_const(sb.buf) < 0) > + die(_("could not create leading directories of '%s'"), sb.buf); Why the mix of capitalized and lowercase-only error messages in this function? Also, the original shell code consistently uses eval_gettext(), so _("...") seems warranted. > + submodule_dot_git = fopen(sb.buf, "w"); > + if (!submodule_dot_git) > + die ("Cannot open file '%s': %s", sb.buf, strerror(errno)); > + > + fprintf(submodule_dot_git, "gitdir: %s\n", > + relative_path(sm_gitdir, path, &rel_path)); > + if (fclose(submodule_dot_git)) > + die("Could not close file %s", sb.buf); > + strbuf_reset(&sb); > + > + /* Redirect the worktree of the submodule in the superprojects config */ s/superprojects/superproject's/ > + if (!is_absolute_path(sm_gitdir)) { > + char *s = (char*)sm_gitdir; > + if (strbuf_getcwd(&sb)) > + die_errno("unable to get current working directory"); > + strbuf_addf(&sb, "/%s", sm_gitdir); > + sm_gitdir = strbuf_detach(&sb, NULL); > + free(s); Why squirrel-away 'sm_gitdir' in 's' unnecessarily? Equivalent: if (strbuf_getcwd(&sb)) die_errno(...); strbuf_addf(&sb, "/%s, sm_gitdir); free(sm_gitdir); sm_gitdir = strbuf_detach(&sb, NULL); > + } > + > + if (strbuf_getcwd(&sb)) > + die_errno("unable to get current working directory"); The conditional block just above here also gets 'cwd'. If you move this code above the !is_absolute_path(sm_gitdir) conditional block, then you can avoid the duplication. > + strbuf_addf(&sb, "/%s", path); > + > + p = git_pathdup_submodule(path, "config"); > + if (!p) > + die("Could not get submodule directory for '%s'", path); > + git_config_set_in_file(p, "core.worktree", > + relative_path(sb.buf, sm_gitdir, &rel_path)); > + strbuf_release(&sb); > + free((char *)sm_gitdir); > + return 0; > +} > + > int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > { > if (argc < 2) > @@ -135,6 +277,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > if (!strcmp(argv[1], "module_name")) > return module_name(argc - 2, argv + 2, prefix); > > + if (!strcmp(argv[1], "module_clone")) > + return module_clone(argc - 1, argv + 1, prefix); > + > usage: > - usage("git submodule--helper [module_list | module_name]\n"); > + usage("git submodule--helper [module_list | module_name | " > + "module_clone]\n"); > } > diff --git a/git-submodule.sh b/git-submodule.sh > index e6ff38d..fb5155e 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -178,80 +178,6 @@ get_submodule_config () { > printf '%s' "${value:-$default}" > } ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper 2015-08-31 22:35 ` Eric Sunshine @ 2015-09-01 17:49 ` Stefan Beller 2015-09-01 18:35 ` Eric Sunshine 0 siblings, 1 reply; 21+ messages in thread From: Stefan Beller @ 2015-09-01 17:49 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Jeff King, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann took all suggestions except the one below. > > if (strbuf_getcwd(&sb)) > die_errno(...); > strbuf_addf(&sb, "/%s, sm_gitdir); > free(sm_gitdir); > sm_gitdir = strbuf_detach(&sb, NULL); > >> + } >> + >> + if (strbuf_getcwd(&sb)) >> + die_errno("unable to get current working directory"); > > The conditional block just above here also gets 'cwd'. If you move > this code above the !is_absolute_path(sm_gitdir) conditional block, > then you can avoid the duplication. I don't get it. We need to have strbuf_getcwd twice no matter how we arrange the code as we strbuf_detach will empty the strbuf. Do you mean to call strbuf_getcwd just once and then xstrdup the value, then reset the strbuf to just contain the cwd and append the other string ? > >> + strbuf_addf(&sb, "/%s", path); >> + >> + p = git_pathdup_submodule(path, "config"); >> + if (!p) >> + die("Could not get submodule directory for '%s'", path); >> + git_config_set_in_file(p, "core.worktree", >> + relative_path(sb.buf, sm_gitdir, &rel_path)); >> + strbuf_release(&sb); >> + free((char *)sm_gitdir); >> + return 0; >> +} ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper 2015-09-01 17:49 ` Stefan Beller @ 2015-09-01 18:35 ` Eric Sunshine 0 siblings, 0 replies; 21+ messages in thread From: Eric Sunshine @ 2015-09-01 18:35 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Jeff King, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann On Tue, Sep 1, 2015 at 1:49 PM, Stefan Beller <sbeller@google.com> wrote: > took all suggestions except the one below. > >> >> if (strbuf_getcwd(&sb)) >> die_errno(...); >> strbuf_addf(&sb, "/%s, sm_gitdir); >> free(sm_gitdir); >> sm_gitdir = strbuf_detach(&sb, NULL); >> >>> + } >>> + >>> + if (strbuf_getcwd(&sb)) >>> + die_errno("unable to get current working directory"); >> >> The conditional block just above here also gets 'cwd'. If you move >> this code above the !is_absolute_path(sm_gitdir) conditional block, >> then you can avoid the duplication. > > I don't get it. We need to have strbuf_getcwd twice no matter how we > arrange the code > as we strbuf_detach will empty the strbuf. > > Do you mean to call strbuf_getcwd just once and then xstrdup the value, > then reset the strbuf to just contain the cwd and append the other string ? Sorry, I have no idea what you are asking. All I am saying is that you're unnecessarily invoking getcwd() twice. It's value doesn't change between invocations, so the second call is entirely redundant and wasteful. You can instead call it just once and use the result in both places. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first 2015-08-31 19:19 [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first Stefan Beller ` (2 preceding siblings ...) 2015-08-31 19:19 ` [PATCH 3/3] submodule: implement `module_clone` " Stefan Beller @ 2015-08-31 20:15 ` Junio C Hamano 2015-09-01 0:40 ` [PATCHv3 " Stefan Beller 3 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2015-08-31 20:15 UTC (permalink / raw) To: Stefan Beller; +Cc: peff, git, jrnieder, johannes.schindelin, Jens.Lehmann Stefan Beller <sbeller@google.com> writes: > One of the mistakes I made last week was mixing these quite stable patches > with highly RFC/WIP patches and not pointing out loud which is which. I think it was very clear which is which to people who have been following (which are the only people you would care about anyway), but I agree that it would be easier to follow by others if you made two series, i.e. solid bits like this one, marked with "PATCH v$n", and the WIP part as a separate series, described as building on the v$n reroll of the other series in its own cover letter. Thanks, will replace. Let's move this part to 'next' and start cooking. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv3 0/3] submodule--helper: Have some refactoring only patches first 2015-08-31 20:15 ` [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first Junio C Hamano @ 2015-09-01 0:40 ` Stefan Beller 2015-09-01 0:40 ` [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper Stefan Beller ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Stefan Beller @ 2015-09-01 0:40 UTC (permalink / raw) To: gitster Cc: Stefan Beller, sunshine, git, jrnieder, johannes.schindelin, Jens.Lehmann, peff * Implemented different error messages as suggested by Junio * Implemented all of Erics suggestions, including renaming to module-with-dash-now Stefan Beller (3): submodule: implement `module-list` as a builtin helper submodule: implement `module-name` as a builtin helper submodule: implement `module-clone` as a builtin helper .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/submodule--helper.c | 282 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 166 +++----------------------- git.c | 1 + 6 files changed, 302 insertions(+), 150 deletions(-) create mode 100644 builtin/submodule--helper.c -- 2.5.0.256.gb62b30d.dirty ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper 2015-09-01 0:40 ` [PATCHv3 " Stefan Beller @ 2015-09-01 0:40 ` Stefan Beller 2015-09-01 2:22 ` Eric Sunshine 2015-09-01 0:40 ` [PATCHv3 2/3] submodule: implement `module-name` " Stefan Beller ` (2 subsequent siblings) 3 siblings, 1 reply; 21+ messages in thread From: Stefan Beller @ 2015-09-01 0:40 UTC (permalink / raw) To: gitster Cc: Stefan Beller, sunshine, git, jrnieder, johannes.schindelin, Jens.Lehmann, peff Most of the submodule operations work on a set of submodules. Calculating and using this set is usually done via: module_list "$@" | { while read mode sha1 stage sm_path do # the actual operation done } Currently the function `module_list` is implemented in the git-submodule.sh as a shell script wrapping a perl script. The rewrite is in C, such that it is faster and can later be easily adapted when other functions are rewritten in C. git-submodule.sh, similar to the builtin commands, will navigate to the top-most directory of the repository and keep the subdirectory as a variable. As the helper is called from within the git-submodule.sh script, we are already navigated to the root level, but the path arguments are still relative to the subdirectory we were in when calling git-submodule.sh. That's why there is a `--prefix` option pointing to an alternative path which to anchor relative path arguments. Signed-off-by: Stefan Beller <sbeller@google.com> --- .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/submodule--helper.c | 117 ++++++++++++++++++++++++++++++++++++++++++++ git-submodule.sh | 54 +++----------------- git.c | 1 + 6 files changed, 127 insertions(+), 48 deletions(-) create mode 100644 builtin/submodule--helper.c diff --git a/.gitignore b/.gitignore index 4fd81ba..1c2f832 100644 --- a/.gitignore +++ b/.gitignore @@ -155,6 +155,7 @@ /git-status /git-stripspace /git-submodule +/git-submodule--helper /git-svn /git-symbolic-ref /git-tag diff --git a/Makefile b/Makefile index 24b636d..d434e73 100644 --- a/Makefile +++ b/Makefile @@ -901,6 +901,7 @@ BUILTIN_OBJS += builtin/shortlog.o BUILTIN_OBJS += builtin/show-branch.o BUILTIN_OBJS += builtin/show-ref.o BUILTIN_OBJS += builtin/stripspace.o +BUILTIN_OBJS += builtin/submodule--helper.o BUILTIN_OBJS += builtin/symbolic-ref.o BUILTIN_OBJS += builtin/tag.o BUILTIN_OBJS += builtin/unpack-file.o diff --git a/builtin.h b/builtin.h index 839483d..924e6c4 100644 --- a/builtin.h +++ b/builtin.h @@ -119,6 +119,7 @@ extern int cmd_show(int argc, const char **argv, const char *prefix); extern int cmd_show_branch(int argc, const char **argv, const char *prefix); extern int cmd_status(int argc, const char **argv, const char *prefix); extern int cmd_stripspace(int argc, const char **argv, const char *prefix); +extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix); extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); extern int cmd_tag(int argc, const char **argv, const char *prefix); extern int cmd_tar_tree(int argc, const char **argv, const char *prefix); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c new file mode 100644 index 0000000..44310f5 --- /dev/null +++ b/builtin/submodule--helper.c @@ -0,0 +1,117 @@ +#include "builtin.h" +#include "cache.h" +#include "parse-options.h" +#include "quote.h" +#include "pathspec.h" +#include "dir.h" +#include "utf8.h" + +static const struct cache_entry **ce_entries; +static int ce_alloc, ce_used; + +static int module_list_compute(int argc, const char **argv, + const char *prefix, + struct pathspec *pathspec) +{ + int i, result = 0; + char *max_prefix, *ps_matched = NULL; + int max_prefix_len; + parse_pathspec(pathspec, 0, + PATHSPEC_PREFER_FULL | + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP, + prefix, argv); + + /* Find common prefix for all pathspec's */ + max_prefix = common_prefix(pathspec); + max_prefix_len = max_prefix ? strlen(max_prefix) : 0; + + if (pathspec->nr) + ps_matched = xcalloc(pathspec->nr, 1); + + if (read_cache() < 0) + die("index file corrupt"); + + for (i = 0; i < active_nr; i++) { + const struct cache_entry *ce = active_cache[i]; + + if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), + max_prefix_len, ps_matched, + S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode))) + continue; + + if (S_ISGITLINK(ce->ce_mode)) { + ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc); + ce_entries[ce_used++] = ce; + } + + while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name)) + /* + * Skip entries with the same name in different stages + * to make sure an entry is returned only once. + */ + i++; + } + free(max_prefix); + + if (ps_matched && report_path_error(ps_matched, pathspec, prefix)) + result = -1; + + free(ps_matched); + + return result; +} + +static int module_list(int argc, const char **argv, const char *prefix) +{ + int i; + struct pathspec pathspec; + const char *alternative_path; + + struct option module_list_options[] = { + OPT_STRING(0, "prefix", &alternative_path, + N_("path"), + N_("alternative anchor for relative paths")), + OPT_END() + }; + + const char *const git_submodule_helper_usage[] = { + N_("git submodule--helper module_list [--prefix=<path>] [<path>...]"), + NULL + }; + + argc = parse_options(argc, argv, prefix, module_list_options, + git_submodule_helper_usage, 0); + + if (module_list_compute(argc, argv, alternative_path + ? alternative_path + : prefix, &pathspec) < 0) { + printf("#unmatched\n"); + return 1; + } + + for (i = 0; i < ce_used; i++) { + const struct cache_entry *ce = ce_entries[i]; + + if (ce_stage(ce)) + printf("%06o %s U\t", ce->ce_mode, sha1_to_hex(null_sha1)); + else + printf("%06o %s %d\t", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce)); + + utf8_fprintf(stdout, "%s\n", ce->name); + } + return 0; +} + +int cmd_submodule__helper(int argc, const char **argv, const char *prefix) +{ + if (argc < 2) + die(N_("fatal: submodule--helper subcommand must be called with " + "a subcommand, which is module-list\n")); + + if (!strcmp(argv[1], "module-list")) + return module_list(argc - 1, argv + 1, prefix); + + die(N_("fatal: '%s' is not a valid submodule--helper subcommand, " + "which is module-list\n"), + argv[1]); +} diff --git a/git-submodule.sh b/git-submodule.sh index 36797c3..d4d710c 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -145,48 +145,6 @@ relative_path () echo "$result$target" } -# -# Get submodule info for registered submodules -# $@ = path to limit submodule list -# -module_list() -{ - eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")" - ( - git ls-files -z --error-unmatch --stage -- "$@" || - echo "unmatched pathspec exists" - ) | - @@PERL@@ -e ' - my %unmerged = (); - my ($null_sha1) = ("0" x 40); - my @out = (); - my $unmatched = 0; - $/ = "\0"; - while (<STDIN>) { - if (/^unmatched pathspec/) { - $unmatched = 1; - next; - } - chomp; - my ($mode, $sha1, $stage, $path) = - /^([0-7]+) ([0-9a-f]{40}) ([0-3])\t(.*)$/; - next unless $mode eq "160000"; - if ($stage ne "0") { - if (!$unmerged{$path}++) { - push @out, "$mode $null_sha1 U\t$path\n"; - } - next; - } - push @out, "$_\n"; - } - if ($unmatched) { - print "#unmatched\n"; - } else { - print for (@out); - } - ' -} - die_if_unmatched () { if test "$1" = "#unmatched" @@ -532,7 +490,7 @@ cmd_foreach() # command in the subshell (and a recursive call to this function) exec 3<&0 - module_list | + git submodule--helper module-list --prefix "$wt_prefix"| while read mode sha1 stage sm_path do die_if_unmatched "$mode" @@ -592,7 +550,7 @@ cmd_init() shift done - module_list "$@" | + git submodule--helper module-list --prefix "$wt_prefix" "$@" | while read mode sha1 stage sm_path do die_if_unmatched "$mode" @@ -674,7 +632,7 @@ cmd_deinit() die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")" fi - module_list "$@" | + git submodule--helper module-list --prefix "$wt_prefix" "$@" | while read mode sha1 stage sm_path do die_if_unmatched "$mode" @@ -790,7 +748,7 @@ cmd_update() fi cloned_modules= - module_list "$@" | { + git submodule--helper module-list --prefix "$wt_prefix" "$@" | { err= while read mode sha1 stage sm_path do @@ -1222,7 +1180,7 @@ cmd_status() shift done - module_list "$@" | + git submodule--helper module-list --prefix "$wt_prefix" "$@" | while read mode sha1 stage sm_path do die_if_unmatched "$mode" @@ -1299,7 +1257,7 @@ cmd_sync() esac done cd_to_toplevel - module_list "$@" | + git submodule--helper module-list --prefix "$wt_prefix" "$@" | while read mode sha1 stage sm_path do die_if_unmatched "$mode" diff --git a/git.c b/git.c index 55c327c..deecba0 100644 --- a/git.c +++ b/git.c @@ -469,6 +469,7 @@ static struct cmd_struct commands[] = { { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE }, { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, { "stripspace", cmd_stripspace }, + { "submodule--helper", cmd_submodule__helper, RUN_SETUP }, { "symbolic-ref", cmd_symbolic_ref, RUN_SETUP }, { "tag", cmd_tag, RUN_SETUP }, { "unpack-file", cmd_unpack_file, RUN_SETUP }, -- 2.5.0.256.gb62b30d.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper 2015-09-01 0:40 ` [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper Stefan Beller @ 2015-09-01 2:22 ` Eric Sunshine 0 siblings, 0 replies; 21+ messages in thread From: Eric Sunshine @ 2015-09-01 2:22 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann, Jeff King On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote: > Most of the submodule operations work on a set of submodules. > Calculating and using this set is usually done via: > > module_list "$@" | { > while read mode sha1 stage sm_path > do > # the actual operation > done > } > > Currently the function `module_list` is implemented in the > git-submodule.sh as a shell script wrapping a perl script. > The rewrite is in C, such that it is faster and can later be > easily adapted when other functions are rewritten in C. > > git-submodule.sh, similar to the builtin commands, will navigate > to the top-most directory of the repository and keep the > subdirectory as a variable. As the helper is called from > within the git-submodule.sh script, we are already navigated > to the root level, but the path arguments are still relative > to the subdirectory we were in when calling git-submodule.sh. > That's why there is a `--prefix` option pointing to an alternative > path which to anchor relative path arguments. > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > new file mode 100644 > index 0000000..44310f5 > --- /dev/null > +++ b/builtin/submodule--helper.c > +static int module_list_compute(int argc, const char **argv, > + const char *prefix, > + struct pathspec *pathspec) > +{ > + int i, result = 0; > + char *max_prefix, *ps_matched = NULL; > + int max_prefix_len; > + parse_pathspec(pathspec, 0, > + PATHSPEC_PREFER_FULL | > + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP, > + prefix, argv); > + > + /* Find common prefix for all pathspec's */ > + max_prefix = common_prefix(pathspec); > + max_prefix_len = max_prefix ? strlen(max_prefix) : 0; > + > + if (pathspec->nr) > + ps_matched = xcalloc(pathspec->nr, 1); > + > + if (read_cache() < 0) > + die("index file corrupt"); die(_("...")); > + for (i = 0; i < active_nr; i++) { > + const struct cache_entry *ce = active_cache[i]; > + > + if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), > + max_prefix_len, ps_matched, > + S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode))) > + continue; > + > + if (S_ISGITLINK(ce->ce_mode)) { > + ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc); > + ce_entries[ce_used++] = ce; > + } > + > + while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name)) > + /* > + * Skip entries with the same name in different stages > + * to make sure an entry is returned only once. > + */ > + i++; > + } > + free(max_prefix); > + > + if (ps_matched && report_path_error(ps_matched, pathspec, prefix)) > + result = -1; > + > + free(ps_matched); > + > + return result; > +} > + > +static int module_list(int argc, const char **argv, const char *prefix) > +{ > + int i; > + struct pathspec pathspec; > + const char *alternative_path; > + > + struct option module_list_options[] = { > + OPT_STRING(0, "prefix", &alternative_path, > + N_("path"), > + N_("alternative anchor for relative paths")), > + OPT_END() > + }; > + > + const char *const git_submodule_helper_usage[] = { > + N_("git submodule--helper module_list [--prefix=<path>] [<path>...]"), s/module_list/module-list/ > + NULL > + }; > + > + argc = parse_options(argc, argv, prefix, module_list_options, > + git_submodule_helper_usage, 0); > + > + if (module_list_compute(argc, argv, alternative_path > + ? alternative_path > + : prefix, &pathspec) < 0) { > + printf("#unmatched\n"); > + return 1; > + } > + > + for (i = 0; i < ce_used; i++) { > + const struct cache_entry *ce = ce_entries[i]; > + > + if (ce_stage(ce)) > + printf("%06o %s U\t", ce->ce_mode, sha1_to_hex(null_sha1)); > + else > + printf("%06o %s %d\t", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce)); > + > + utf8_fprintf(stdout, "%s\n", ce->name); > + } > + return 0; > +} > + > +int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > +{ > + if (argc < 2) > + die(N_("fatal: submodule--helper subcommand must be called with " > + "a subcommand, which is module-list\n")); > + > + if (!strcmp(argv[1], "module-list")) Can we drop the "module-" prefix altogether from these subcommands, please? Considering that the parent name is already "submodule--helper", the "module-" prefix is probably pure redundancy. Instead: submodule--helper list submodule--helper name submodule--helper clone > + return module_list(argc - 1, argv + 1, prefix); > + > + die(N_("fatal: '%s' is not a valid submodule--helper subcommand, " > + "which is module-list\n"), > + argv[1]); > +} ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper 2015-09-01 0:40 ` [PATCHv3 " Stefan Beller 2015-09-01 0:40 ` [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper Stefan Beller @ 2015-09-01 0:40 ` Stefan Beller 2015-09-01 2:31 ` Eric Sunshine 2015-09-01 0:40 ` [PATCHv3 3/3] submodule: implement `module-clone` " Stefan Beller 2015-09-01 2:09 ` [PATCHv3 0/3] submodule--helper: Have some refactoring only patches first Eric Sunshine 3 siblings, 1 reply; 21+ messages in thread From: Stefan Beller @ 2015-09-01 0:40 UTC (permalink / raw) To: gitster Cc: Stefan Beller, sunshine, git, jrnieder, johannes.schindelin, Jens.Lehmann, peff This implements the helper `module_name` in C instead of shell, yielding a nice performance boost. Before this patch, I measured a time (best out of three): $ time ./t7400-submodule-basic.sh >/dev/null real 0m11.066s user 0m3.348s sys 0m8.534s With this patch applied I measured (also best out of three) $ time ./t7400-submodule-basic.sh >/dev/null real 0m10.063s user 0m3.044s sys 0m7.487s Signed-off-by: Stefan Beller <sbeller@google.com> --- builtin/submodule--helper.c | 29 +++++++++++++++++++++++++++-- git-submodule.sh | 32 +++++++------------------------- 2 files changed, 34 insertions(+), 27 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 44310f5..91bd420 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -5,6 +5,9 @@ #include "pathspec.h" #include "dir.h" #include "utf8.h" +#include "submodule.h" +#include "submodule-config.h" +#include "string-list.h" static const struct cache_entry **ce_entries; static int ce_alloc, ce_used; @@ -102,16 +105,38 @@ static int module_list(int argc, const char **argv, const char *prefix) return 0; } +static int module_name(int argc, const char **argv, const char *prefix) +{ + const struct submodule *sub; + + if (argc != 1) + usage("git submodule--helper module_name <path>\n"); + + gitmodules_config(); + sub = submodule_from_path(null_sha1, argv[0]); + + if (!sub) + die(N_("No submodule mapping found in .gitmodules for path '%s'"), + argv[0]); + + printf("%s\n", sub->name); + + return 0; +} + int cmd_submodule__helper(int argc, const char **argv, const char *prefix) { if (argc < 2) die(N_("fatal: submodule--helper subcommand must be called with " - "a subcommand, which is module-list\n")); + "a subcommand, which are module-list, module-name\n")); if (!strcmp(argv[1], "module-list")) return module_list(argc - 1, argv + 1, prefix); + if (!strcmp(argv[1], "module-name")) + return module_name(argc - 2, argv + 2, prefix); + die(N_("fatal: '%s' is not a valid submodule--helper subcommand, " - "which is module-list\n"), + "which are module-list, module-name\n"), argv[1]); } diff --git a/git-submodule.sh b/git-submodule.sh index d4d710c..5f3dfc5 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -178,24 +178,6 @@ get_submodule_config () { printf '%s' "${value:-$default}" } - -# -# Map submodule path to submodule name -# -# $1 = path -# -module_name() -{ - # Do we have "submodule.<something>.path = $1" defined in .gitmodules file? - sm_path="$1" - re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g') - name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' | - sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' ) - test -z "$name" && - die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$sm_path'")" - printf '%s\n' "$name" -} - # # Clone a submodule # @@ -498,7 +480,7 @@ cmd_foreach() then displaypath=$(relative_path "$sm_path") say "$(eval_gettext "Entering '\$prefix\$displaypath'")" - name=$(module_name "$sm_path") + name=$(git submodule--helper module-name "$sm_path") ( prefix="$prefix$sm_path/" clear_local_git_env @@ -554,7 +536,7 @@ cmd_init() while read mode sha1 stage sm_path do die_if_unmatched "$mode" - name=$(module_name "$sm_path") || exit + name=$(git submodule--helper module-name "$sm_path") || exit displaypath=$(relative_path "$sm_path") @@ -636,7 +618,7 @@ cmd_deinit() while read mode sha1 stage sm_path do die_if_unmatched "$mode" - name=$(module_name "$sm_path") || exit + name=$(git submodule--helper module-name "$sm_path") || exit displaypath=$(relative_path "$sm_path") @@ -758,7 +740,7 @@ cmd_update() echo >&2 "Skipping unmerged submodule $prefix$sm_path" continue fi - name=$(module_name "$sm_path") || exit + name=$(git submodule--helper module-name "$sm_path") || exit url=$(git config submodule."$name".url) branch=$(get_submodule_config "$name" branch master) if ! test -z "$update" @@ -1022,7 +1004,7 @@ cmd_summary() { # Respect the ignore setting for --for-status. if test -n "$for_status" then - name=$(module_name "$sm_path") + name=$(git submodule--helper module-name "$sm_path") ignore_config=$(get_submodule_config "$name" ignore none) test $status != A && test $ignore_config = all && continue fi @@ -1184,7 +1166,7 @@ cmd_status() while read mode sha1 stage sm_path do die_if_unmatched "$mode" - name=$(module_name "$sm_path") || exit + name=$(git submodule--helper module-name "$sm_path") || exit url=$(git config submodule."$name".url) displaypath=$(relative_path "$prefix$sm_path") if test "$stage" = U @@ -1261,7 +1243,7 @@ cmd_sync() while read mode sha1 stage sm_path do die_if_unmatched "$mode" - name=$(module_name "$sm_path") + name=$(git submodule--helper module-name "$sm_path") url=$(git config -f .gitmodules --get submodule."$name".url) # Possibly a url relative to parent -- 2.5.0.256.gb62b30d.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper 2015-09-01 0:40 ` [PATCHv3 2/3] submodule: implement `module-name` " Stefan Beller @ 2015-09-01 2:31 ` Eric Sunshine 2015-09-01 16:18 ` Stefan Beller 0 siblings, 1 reply; 21+ messages in thread From: Eric Sunshine @ 2015-09-01 2:31 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann, Jeff King On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote: > This implements the helper `module_name` in C instead of shell, > yielding a nice performance boost. > > Before this patch, I measured a time (best out of three): > > $ time ./t7400-submodule-basic.sh >/dev/null > real 0m11.066s > user 0m3.348s > sys 0m8.534s > > With this patch applied I measured (also best out of three) > > $ time ./t7400-submodule-basic.sh >/dev/null > real 0m10.063s > user 0m3.044s > sys 0m7.487s > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 44310f5..91bd420 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -102,16 +105,38 @@ static int module_list(int argc, const char **argv, const char *prefix) > +static int module_name(int argc, const char **argv, const char *prefix) > +{ > + const struct submodule *sub; > + > + if (argc != 1) > + usage("git submodule--helper module_name <path>\n"); usage(_("...")); s/module_name/module-name/ I think you also need to drop the newline from the usage() argument. > + gitmodules_config(); > + sub = submodule_from_path(null_sha1, argv[0]); > + > + if (!sub) > + die(N_("No submodule mapping found in .gitmodules for path '%s'"), > + argv[0]); > + > + printf("%s\n", sub->name); > + > + return 0; > +} > + > int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > { > if (argc < 2) > die(N_("fatal: submodule--helper subcommand must be called with " > - "a subcommand, which is module-list\n")); > + "a subcommand, which are module-list, module-name\n")); Manually maintaining this list is likely to become a maintenance issue pretty quickly. Isn't there an OPT_CMDMODE for this sort of thing? Also, it would like be more readable if the possible commands were printed one per line rather than all squashed together. > if (!strcmp(argv[1], "module-list")) > return module_list(argc - 1, argv + 1, prefix); > > + if (!strcmp(argv[1], "module-name")) > + return module_name(argc - 2, argv + 2, prefix); > + > die(N_("fatal: '%s' is not a valid submodule--helper subcommand, " > - "which is module-list\n"), > + "which are module-list, module-name\n"), And, it's duplicated here, making it even more of a maintenance problem. > argv[1]); > } ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper 2015-09-01 2:31 ` Eric Sunshine @ 2015-09-01 16:18 ` Stefan Beller 0 siblings, 0 replies; 21+ messages in thread From: Stefan Beller @ 2015-09-01 16:18 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann, Jeff King On Mon, Aug 31, 2015 at 7:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: > On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote: >> This implements the helper `module_name` in C instead of shell, >> yielding a nice performance boost. >> >> Before this patch, I measured a time (best out of three): >> >> $ time ./t7400-submodule-basic.sh >/dev/null >> real 0m11.066s >> user 0m3.348s >> sys 0m8.534s >> >> With this patch applied I measured (also best out of three) >> >> $ time ./t7400-submodule-basic.sh >/dev/null >> real 0m10.063s >> user 0m3.044s >> sys 0m7.487s >> >> Signed-off-by: Stefan Beller <sbeller@google.com> >> --- >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> index 44310f5..91bd420 100644 >> --- a/builtin/submodule--helper.c >> +++ b/builtin/submodule--helper.c >> @@ -102,16 +105,38 @@ static int module_list(int argc, const char **argv, const char *prefix) >> +static int module_name(int argc, const char **argv, const char *prefix) >> +{ >> + const struct submodule *sub; >> + >> + if (argc != 1) >> + usage("git submodule--helper module_name <path>\n"); > > usage(_("...")); > > s/module_name/module-name/ > > I think you also need to drop the newline from the usage() argument. > >> + gitmodules_config(); >> + sub = submodule_from_path(null_sha1, argv[0]); >> + >> + if (!sub) >> + die(N_("No submodule mapping found in .gitmodules for path '%s'"), >> + argv[0]); >> + >> + printf("%s\n", sub->name); >> + >> + return 0; >> +} >> + >> int cmd_submodule__helper(int argc, const char **argv, const char *prefix) >> { >> if (argc < 2) >> die(N_("fatal: submodule--helper subcommand must be called with " >> - "a subcommand, which is module-list\n")); >> + "a subcommand, which are module-list, module-name\n")); > > Manually maintaining this list is likely to become a maintenance issue > pretty quickly. Isn't there an OPT_CMDMODE for this sort of thing? If we were using OPT_CMDMODE, we'd need to have all the options in one array, passing it to the option parser. I don't think we could have 2 layers of option parsing here. If the first layer only contains the check for argv[1] (list, name, clone), then it would fail not knowing the arguments for each of these sub commands. If we parse all the options together, then we would need to have lots of if (cmd == name) die ("it makes no sense to give --reference or --depth"); also the variables where the options are passed into would not be function specific, but file global. > > Also, it would like be more readable if the possible commands were > printed one per line rather than all squashed together. > >> if (!strcmp(argv[1], "module-list")) >> return module_list(argc - 1, argv + 1, prefix); >> >> + if (!strcmp(argv[1], "module-name")) >> + return module_name(argc - 2, argv + 2, prefix); >> + >> die(N_("fatal: '%s' is not a valid submodule--helper subcommand, " >> - "which is module-list\n"), >> + "which are module-list, module-name\n"), > > And, it's duplicated here, making it even more of a maintenance problem. > >> argv[1]); >> } Maybe we can do it similar to git.c to have our own array with name and function pointers, which can be used both in the help text as well as the actual function call. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv3 3/3] submodule: implement `module-clone` as a builtin helper 2015-09-01 0:40 ` [PATCHv3 " Stefan Beller 2015-09-01 0:40 ` [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper Stefan Beller 2015-09-01 0:40 ` [PATCHv3 2/3] submodule: implement `module-name` " Stefan Beller @ 2015-09-01 0:40 ` Stefan Beller 2015-09-01 2:41 ` Eric Sunshine 2015-09-01 2:09 ` [PATCHv3 0/3] submodule--helper: Have some refactoring only patches first Eric Sunshine 3 siblings, 1 reply; 21+ messages in thread From: Stefan Beller @ 2015-09-01 0:40 UTC (permalink / raw) To: gitster Cc: Stefan Beller, sunshine, git, jrnieder, johannes.schindelin, Jens.Lehmann, peff This converts implements the helper `module_clone`. This functionality is needed for converting `git submodule update` later on, which we want to add threading to. Signed-off-by: Stefan Beller <sbeller@google.com> --- builtin/submodule--helper.c | 144 +++++++++++++++++++++++++++++++++++++++++++- git-submodule.sh | 80 +----------------------- 2 files changed, 145 insertions(+), 79 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 91bd420..0669641 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -8,6 +8,7 @@ #include "submodule.h" #include "submodule-config.h" #include "string-list.h" +#include "run-command.h" static const struct cache_entry **ce_entries; static int ce_alloc, ce_used; @@ -124,11 +125,147 @@ static int module_name(int argc, const char **argv, const char *prefix) return 0; } +static int clone_submodule(const char *path, const char *gitdir, const char *url, + const char *depth, const char *reference, int quiet) +{ + struct child_process cp; + child_process_init(&cp); + + argv_array_push(&cp.args, "clone"); + argv_array_push(&cp.args, "--no-checkout"); + if (quiet) + argv_array_push(&cp.args, "--quiet"); + if (depth && *depth) + argv_array_pushl(&cp.args, "--depth", depth, NULL); + if (reference && *reference) + argv_array_pushl(&cp.args, "--reference", reference, NULL); + if (gitdir && *gitdir) + argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL); + + argv_array_push(&cp.args, url); + argv_array_push(&cp.args, path); + + cp.git_cmd = 1; + cp.env = local_repo_env; + + cp.no_stdin = 1; + cp.no_stdout = 1; + cp.no_stderr = 1; + + return run_command(&cp); +} + +static int module_clone(int argc, const char **argv, const char *prefix) +{ + const char *path = NULL, *name = NULL, *url = NULL; + const char *reference = NULL, *depth = NULL; + const char *alternative_path = NULL, *p; + int quiet = 0; + FILE *submodule_dot_git; + char *sm_gitdir; + struct strbuf rel_path = STRBUF_INIT; + struct strbuf sb = STRBUF_INIT; + + struct option module_update_options[] = { + OPT_STRING(0, "prefix", &alternative_path, + N_("path"), + N_("alternative anchor for relative paths")), + OPT_STRING(0, "path", &path, + N_("path"), + N_("where the new submodule will be cloned to")), + OPT_STRING(0, "name", &name, + N_("string"), + N_("name of the new submodule")), + OPT_STRING(0, "url", &url, + N_("string"), + N_("url where to clone the submodule from")), + OPT_STRING(0, "reference", &reference, + N_("string"), + N_("reference repository")), + OPT_STRING(0, "depth", &depth, + N_("string"), + N_("depth for shallow clones")), + OPT__QUIET(&quiet, "Suppress output for cloning a submodule"), + OPT_END() + }; + + static const char * const git_submodule_helper_usage[] = { + N_("git submodule--helper update [--prefix=<path>] [--quiet] [--remote] [-N|--no-fetch]" + "[-f|--force] [--rebase|--merge] [--reference <repository>]" + "[--depth <depth>] [--recursive] [--] [<path>...]"), + NULL + }; + + argc = parse_options(argc, argv, prefix, module_update_options, + git_submodule_helper_usage, 0); + + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); + sm_gitdir = strbuf_detach(&sb, NULL); + + if (!file_exists(sm_gitdir)) { + safe_create_leading_directories_const(sm_gitdir); + if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet)) + die(N_("Clone of '%s' into submodule path '%s' failed"), + url, path); + } else { + safe_create_leading_directories_const(path); + unlink(sm_gitdir); + } + + /* Write a .git file in the submodule to redirect to the superproject. */ + if (alternative_path && !strcmp(alternative_path, "")) { + p = relative_path(path, alternative_path, &sb); + strbuf_reset(&sb); + } else + p = path; + + if (safe_create_leading_directories_const(p) < 0) + die("Could not create directory '%s'", p); + + strbuf_addf(&sb, "%s/.git", p); + + if (safe_create_leading_directories_const(sb.buf) < 0) + die(_("could not create leading directories of '%s'"), sb.buf); + submodule_dot_git = fopen(sb.buf, "w"); + if (!submodule_dot_git) + die ("Cannot open file '%s': %s", sb.buf, strerror(errno)); + + fprintf(submodule_dot_git, "gitdir: %s\n", + relative_path(sm_gitdir, path, &rel_path)); + if (fclose(submodule_dot_git)) + die("Could not close file %s", sb.buf); + strbuf_reset(&sb); + + /* Redirect the worktree of the submodule in the superprojects config */ + if (!is_absolute_path(sm_gitdir)) { + char *s = (char*)sm_gitdir; + if (strbuf_getcwd(&sb)) + die_errno("unable to get current working directory"); + strbuf_addf(&sb, "/%s", sm_gitdir); + sm_gitdir = strbuf_detach(&sb, NULL); + free(s); + } + + if (strbuf_getcwd(&sb)) + die_errno("unable to get current working directory"); + strbuf_addf(&sb, "/%s", path); + + p = git_pathdup_submodule(path, "config"); + if (!p) + die("Could not get submodule directory for '%s'", path); + git_config_set_in_file(p, "core.worktree", + relative_path(sb.buf, sm_gitdir, &rel_path)); + strbuf_release(&sb); + free(sm_gitdir); + return 0; +} + int cmd_submodule__helper(int argc, const char **argv, const char *prefix) { if (argc < 2) die(N_("fatal: submodule--helper subcommand must be called with " - "a subcommand, which are module-list, module-name\n")); + "a subcommand, which are module-list, module-name, " + "module-clone\n")); if (!strcmp(argv[1], "module-list")) return module_list(argc - 1, argv + 1, prefix); @@ -136,7 +273,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix) if (!strcmp(argv[1], "module-name")) return module_name(argc - 2, argv + 2, prefix); + if (!strcmp(argv[1], "module-clone")) + return module_clone(argc - 1, argv + 1, prefix); + die(N_("fatal: '%s' is not a valid submodule--helper subcommand, " - "which are module-list, module-name\n"), + "which are module-list, module-name, module-clone\n"), argv[1]); } diff --git a/git-submodule.sh b/git-submodule.sh index 5f3dfc5..d1523ea 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -178,80 +178,6 @@ get_submodule_config () { printf '%s' "${value:-$default}" } -# -# Clone a submodule -# -# $1 = submodule path -# $2 = submodule name -# $3 = URL to clone -# $4 = reference repository to reuse (empty for independent) -# $5 = depth argument for shallow clones (empty for deep) -# -# Prior to calling, cmd_update checks that a possibly existing -# path is not a git repository. -# Likewise, cmd_add checks that path does not exist at all, -# since it is the location of a new submodule. -# -module_clone() -{ - sm_path=$1 - name=$2 - url=$3 - reference="$4" - depth="$5" - quiet= - if test -n "$GIT_QUIET" - then - quiet=-q - fi - - gitdir= - gitdir_base= - base_name=$(dirname "$name") - - gitdir=$(git rev-parse --git-dir) - gitdir_base="$gitdir/modules/$base_name" - gitdir="$gitdir/modules/$name" - - if test -d "$gitdir" - then - mkdir -p "$sm_path" - rm -f "$gitdir/index" - else - mkdir -p "$gitdir_base" - ( - clear_local_git_env - git clone $quiet ${depth:+"$depth"} -n ${reference:+"$reference"} \ - --separate-git-dir "$gitdir" "$url" "$sm_path" - ) || - die "$(eval_gettext "Clone of '\$url' into submodule path '\$sm_path' failed")" - fi - - # We already are at the root of the work tree but cd_to_toplevel will - # resolve any symlinks that might be present in $PWD - a=$(cd_to_toplevel && cd "$gitdir" && pwd)/ - b=$(cd_to_toplevel && cd "$sm_path" && pwd)/ - # Remove all common leading directories after a sanity check - if test "${a#$b}" != "$a" || test "${b#$a}" != "$b"; then - die "$(eval_gettext "Gitdir '\$a' is part of the submodule path '\$b' or vice versa")" - fi - while test "${a%%/*}" = "${b%%/*}" - do - a=${a#*/} - b=${b#*/} - done - # Now chop off the trailing '/'s that were added in the beginning - a=${a%/} - b=${b%/} - - # Turn each leading "*/" component into "../" - rel=$(printf '%s\n' "$b" | sed -e 's|[^/][^/]*|..|g') - printf '%s\n' "gitdir: $rel/$a" >"$sm_path/.git" - - rel=$(printf '%s\n' "$a" | sed -e 's|[^/][^/]*|..|g') - (clear_local_git_env; cd "$sm_path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b") -} - isnumber() { n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1" @@ -301,7 +227,7 @@ cmd_add() shift ;; --depth=*) - depth=$1 + depth="$1" ;; --) shift @@ -412,7 +338,7 @@ Use -f if you really want to add it." >&2 echo "$(eval_gettext "Reactivating local git directory for submodule '\$sm_name'.")" fi fi - module_clone "$sm_path" "$sm_name" "$realrepo" "$reference" "$depth" || exit + git submodule--helper module-clone ${GIT_QUIET:+--quiet} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" "$reference" "$depth" || exit ( clear_local_git_env cd "$sm_path" && @@ -774,7 +700,7 @@ Maybe you want to use 'update --init'?")" if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git then - module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit + git submodule--helper module-clone ${GIT_QUIET:+--quiet} --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit cloned_modules="$cloned_modules;$name" subsha1= else -- 2.5.0.256.gb62b30d.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCHv3 3/3] submodule: implement `module-clone` as a builtin helper 2015-09-01 0:40 ` [PATCHv3 3/3] submodule: implement `module-clone` " Stefan Beller @ 2015-09-01 2:41 ` Eric Sunshine 0 siblings, 0 replies; 21+ messages in thread From: Eric Sunshine @ 2015-09-01 2:41 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann, Jeff King On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote: > This converts implements the helper `module_clone`. This functionality is "converts implements"? > needed for converting `git submodule update` later on, which we want to > add threading to. > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > +static int module_clone(int argc, const char **argv, const char *prefix) > +{ > + const char *path = NULL, *name = NULL, *url = NULL; > + const char *reference = NULL, *depth = NULL; > + const char *alternative_path = NULL, *p; > + int quiet = 0; > + FILE *submodule_dot_git; > + char *sm_gitdir; > + struct strbuf rel_path = STRBUF_INIT; > + struct strbuf sb = STRBUF_INIT; > + > + struct option module_update_options[] = { s/update/clone/ maybe? > + OPT_STRING(0, "prefix", &alternative_path, > + N_("path"), > + N_("alternative anchor for relative paths")), > + OPT_STRING(0, "path", &path, > + N_("path"), > + N_("where the new submodule will be cloned to")), > + OPT_STRING(0, "name", &name, > + N_("string"), > + N_("name of the new submodule")), > + OPT_STRING(0, "url", &url, > + N_("string"), > + N_("url where to clone the submodule from")), > + OPT_STRING(0, "reference", &reference, > + N_("string"), > + N_("reference repository")), > + OPT_STRING(0, "depth", &depth, > + N_("string"), > + N_("depth for shallow clones")), > + OPT__QUIET(&quiet, "Suppress output for cloning a submodule"), > + OPT_END() > + }; > + > + static const char * const git_submodule_helper_usage[] = { You can still drop 'static'. > + N_("git submodule--helper update [--prefix=<path>] [--quiet] [--remote] [-N|--no-fetch]" "update"? > + "[-f|--force] [--rebase|--merge] [--reference <repository>]" > + "[--depth <depth>] [--recursive] [--] [<path>...]"), Also, what's all this "--force", "--rebase", "--remote", "--recursive" stuff? > + NULL > + }; > + > + argc = parse_options(argc, argv, prefix, module_update_options, > + git_submodule_helper_usage, 0); > + > + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); > + sm_gitdir = strbuf_detach(&sb, NULL); > + > + if (!file_exists(sm_gitdir)) { > + safe_create_leading_directories_const(sm_gitdir); Still inconsistent checking of return value of safe_create_leading_directories_const() in this function. In fact, it looks like pretty much all of my v2 review comments are still relevant to the remainder of this function. Rather than repeating them, I'll just give a reference[1]. [1]: http://article.gmane.org/gmane.comp.version-control.git/276952 > + if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet)) > + die(N_("Clone of '%s' into submodule path '%s' failed"), > + url, path); > + } else { > + safe_create_leading_directories_const(path); > + unlink(sm_gitdir); > + } > + > + /* Write a .git file in the submodule to redirect to the superproject. */ > + if (alternative_path && !strcmp(alternative_path, "")) { > + p = relative_path(path, alternative_path, &sb); > + strbuf_reset(&sb); > + } else > + p = path; > + > + if (safe_create_leading_directories_const(p) < 0) > + die("Could not create directory '%s'", p); > + > + strbuf_addf(&sb, "%s/.git", p); > + > + if (safe_create_leading_directories_const(sb.buf) < 0) > + die(_("could not create leading directories of '%s'"), sb.buf); > + submodule_dot_git = fopen(sb.buf, "w"); > + if (!submodule_dot_git) > + die ("Cannot open file '%s': %s", sb.buf, strerror(errno)); > + > + fprintf(submodule_dot_git, "gitdir: %s\n", > + relative_path(sm_gitdir, path, &rel_path)); > + if (fclose(submodule_dot_git)) > + die("Could not close file %s", sb.buf); > + strbuf_reset(&sb); > + > + /* Redirect the worktree of the submodule in the superprojects config */ > + if (!is_absolute_path(sm_gitdir)) { > + char *s = (char*)sm_gitdir; > + if (strbuf_getcwd(&sb)) > + die_errno("unable to get current working directory"); > + strbuf_addf(&sb, "/%s", sm_gitdir); > + sm_gitdir = strbuf_detach(&sb, NULL); > + free(s); > + } > + > + if (strbuf_getcwd(&sb)) > + die_errno("unable to get current working directory"); > + strbuf_addf(&sb, "/%s", path); > + > + p = git_pathdup_submodule(path, "config"); > + if (!p) > + die("Could not get submodule directory for '%s'", path); > + git_config_set_in_file(p, "core.worktree", > + relative_path(sb.buf, sm_gitdir, &rel_path)); > + strbuf_release(&sb); > + free(sm_gitdir); > + return 0; > +} ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCHv3 0/3] submodule--helper: Have some refactoring only patches first 2015-09-01 0:40 ` [PATCHv3 " Stefan Beller ` (2 preceding siblings ...) 2015-09-01 0:40 ` [PATCHv3 3/3] submodule: implement `module-clone` " Stefan Beller @ 2015-09-01 2:09 ` Eric Sunshine 3 siblings, 0 replies; 21+ messages in thread From: Eric Sunshine @ 2015-09-01 2:09 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Git List, Jonathan Nieder, Johannes Schindelin, Jens Lehmann, Jeff King On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote: > * Implemented different error messages as suggested by Junio > * Implemented all of Erics suggestions, > including renaming to module-with-dash-now In the future, please include an interdiff showing changes between versions. Thanks. > Stefan Beller (3): > submodule: implement `module-list` as a builtin helper > submodule: implement `module-name` as a builtin helper > submodule: implement `module-clone` as a builtin helper ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2015-09-01 18:35 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-31 19:19 [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first Stefan Beller 2015-08-31 19:19 ` [PATCH 1/3] submodule: implement `module_list` as a builtin helper Stefan Beller 2015-08-31 21:55 ` Eric Sunshine 2015-08-31 19:19 ` [PATCH 2/3] submodule: implement `module_name` " Stefan Beller 2015-08-31 20:35 ` Junio C Hamano 2015-08-31 20:51 ` Stefan Beller 2015-08-31 22:01 ` Eric Sunshine 2015-08-31 19:19 ` [PATCH 3/3] submodule: implement `module_clone` " Stefan Beller 2015-08-31 22:35 ` Eric Sunshine 2015-09-01 17:49 ` Stefan Beller 2015-09-01 18:35 ` Eric Sunshine 2015-08-31 20:15 ` [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first Junio C Hamano 2015-09-01 0:40 ` [PATCHv3 " Stefan Beller 2015-09-01 0:40 ` [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper Stefan Beller 2015-09-01 2:22 ` Eric Sunshine 2015-09-01 0:40 ` [PATCHv3 2/3] submodule: implement `module-name` " Stefan Beller 2015-09-01 2:31 ` Eric Sunshine 2015-09-01 16:18 ` Stefan Beller 2015-09-01 0:40 ` [PATCHv3 3/3] submodule: implement `module-clone` " Stefan Beller 2015-09-01 2:41 ` Eric Sunshine 2015-09-01 2:09 ` [PATCHv3 0/3] submodule--helper: Have some refactoring only patches first Eric Sunshine
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).