git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	Jeff King <peff@peff.net>,
	Mark Strapetz <marc.strapetz@syntevo.com>,
	Junio C Hamano <gitster@pobox.com>,
	Jacob Keller <jacob.keller@gmail.com>
Subject: Re: [PATCH v4 2/3] sumodule--helper: fix submodule--helper clone usage and check argc count
Date: Fri, 26 Feb 2016 11:31:06 -0800	[thread overview]
Message-ID: <CAGZ79kbD7_CYo9KT185oTxrjqch-otwfG0mWqAV2QiwrC-ch9Q@mail.gmail.com> (raw)
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:
> 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.

>                 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@google.com>


>                 (
>                         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
>

  reply	other threads:[~2016-02-26 19:31 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-26 19:18 [PATCH v4 1/3] t/lib-httpd: load mod_unixd Jacob Keller
2016-02-26 19:18 ` [PATCH v4 2/3] sumodule--helper: fix submodule--helper clone usage and check argc count Jacob Keller
2016-02-26 19:31   ` Stefan Beller [this message]
2016-02-26 22:08     ` Jacob Keller
2016-02-26 19:18 ` [PATCH v4 3/3] git: submodule honor -c credential.* from command line Jacob Keller
2016-02-26 22:05   ` Jeff King
2016-02-26 22:20     ` Jacob Keller
2016-02-27  0:01       ` Jacob Keller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGZ79kbD7_CYo9KT185oTxrjqch-otwfG0mWqAV2QiwrC-ch9Q@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jacob.e.keller@intel.com \
    --cc=jacob.keller@gmail.com \
    --cc=marc.strapetz@syntevo.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).