On Fri, Feb 26, 2016 at 11:18 AM, Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:
> From: Jacob Keller <jacob.keller@xxxxxxxxx>
>
> git submodule--helper clone usage specified that paths come after the --
> as a sequence. However, the actual implementation does not, and requires
> only a single path passed in via --path. In addition, argc was
> unchecked. (allowing arbitrary extra arguments that were silently
> ignored).
>
> Fix the usage description to match implementation. Add an argc check to
> enforce no extra arguments. Fix a bug in the argument passing in
> git-submodule.sh which would pass --reference and --depth as empty
> strings when they were unused, resulting in extra argc after parsing
> options.
>
> Signed-off-by: Jacob Keller <jacob.keller@xxxxxxxxx>
> ---
> builtin/submodule--helper.c | 5 ++++-
> git-submodule.sh | 4 ++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f4c3eff179b5..072d9bbd12a8 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -187,13 +187,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)
> const char *const git_submodule_helper_usage[] = {
> N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
> "[--reference <repository>] [--name <name>] [--url <url>]"
> - "[--depth <depth>] [--] [<path>...]"),
> + "[--depth <depth>] [--path <path>]"),
Right, no extra arguments.
> NULL
> };
>
> argc = parse_options(argc, argv, prefix, module_clone_options,
> git_submodule_helper_usage, 0);
>
> + if (argc)
> + usage(*git_submodule_helper_usage);
> +
This is the fix to check for more arguments.
> strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
> sm_gitdir = strbuf_detach(&sb, NULL);
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 9bc5c5f94d1d..2dd29b3df0e6 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -347,7 +347,7 @@ Use -f if you really want to add it." >&2
> echo "$(eval_gettext "Reactivating local git directory for submodule '\$sm_name'.")"
> fi
> fi
> - git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" "$reference" "$depth" || exit
> + git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${depth:+"$depth"} || exit
By having this additional fix (i.e. no '--depth', '<empty string>' is
passed to the
submodule helper, we can improve the submodule helper further
in clone_submodule we can drop the double check for `depth` and `reference`
(as well as `gitdir`, that double check is unneeded as of now already),
by just checking for the pointer to be non NULL and not further checking
the dereferenced pointer.
That can go in either squashed into this commit or on top of it, either is fine.
That said:
Reviewed-by: Stefan Beller <sbeller@xxxxxxxxxx>
> (
> clear_local_git_env
> cd "$sm_path" &&
> @@ -709,7 +709,7 @@ Maybe you want to use 'update --init'?")"
>
> if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
> then
> - git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
> + git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" ${reference:+"$reference"} ${depth:+"$depth"} || exit
> cloned_modules="$cloned_modules;$name"
> subsha1=
> else
> --
> 2.7.1.429.g45cd78e
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html