Re: [PATCH v4 2/3] sumodule--helper: fix submodule--helper clone usage and check argc count
- From
Stefan Beller <sbeller@google.com>
- Date
- Feb 26, 2016, 19:31 UTC
- Message-ID
- <CAGZ79kbD7_CYo9KT185oTxrjqch-otwfG0mWqAV2QiwrC-ch9Q@mail.gmail.com>
- In-Reply-To
- <1456514328-10153-2-git-send-email-jacob.e.keller@intel.com>
On Fri, Feb 26, 2016 at 11:18 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
Show 30 quoted lines
> From: Jacob Keller <jacob.keller@gmail.com>
>
> 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@gmail.com>
> ---
> 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.
Show 9 quoted lines
> 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.
Show 13 quoted lines
> 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"} || exitBy 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@google.com>
Show 15 quoted lines
> (
> 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
>