Re: [PATCH v2] remote: add get-url subcommand

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 3, 2015, 23:38 UTC
Message-ID: <CAPig+cQO_jXZmJG=EccwQ408TiAk7_BiQF1UKA31phXF7CXy6w@mail.gmail.com>
In-Reply-To: <1438635614-11174-2-git-send-email-mathstuf@gmail.com>

On Mon, Aug 3, 2015 at 5:00 PM, Ben Boeckel <mathstuf@gmail.com> wrote:

Show 21 quoted lines

> Expanding `insteadOf` is a part of ls-remote --url and there is no way
> to expand `pushInsteadOf` as well. Add a get-url subcommand to be able
> to query both as well as a way to get all configured urls.
>
> Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
> ---
> diff --git a/builtin/remote.c b/builtin/remote.c
> index f4a6ec9..9278a83 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1497,6 +1503,53 @@ static int set_branches(int argc, const char **argv)
> +static int get_url(int argc, const char **argv)
> +{
> +       int i, push_mode = 0, all_mode = 0;
> +       const char *remotename = NULL;
> +       struct remote *remote;
> +       const char **url;
> +       int url_nr;
> +       struct option options[] = {
> +               OPT_BOOL('\0', "push", &push_mode,
> +                        N_("query push URLs")),

A bit more explanatory:

    "query push URLs rather than fetch URLs"

Show 6 quoted lines

> +               OPT_BOOL('\0', "all", &all_mode,
> +                        N_("return all URLs")),
> +               OPT_END()
> +       };
> +       argc = parse_options(argc, argv, NULL, options, builtin_remote_geturl_usage,
> +                            PARSE_OPT_KEEP_ARGV0);

What is the reason for PARSE_OPT_KEEP_ARGV0 in this case?

> +       if (argc < 1 || argc > 2)
> +               usage_with_options(builtin_remote_geturl_usage, options);

So,  'argc' must be 1 or 2, which in 'argv' terms is argv[0] and argv[1]).

> +       remotename = argv[1];

But here, argv[1] is accessed unconditionally, even though 'argc' may have been 1, thus out of bounds.

Show 24 quoted lines

> +       if (!remote_is_configured(remotename))
> +               die(_("No such remote '%s'"), remotename);
> +       remote = remote_get(remotename);
> +
> +       if (push_mode) {
> +               url = remote->pushurl;
> +               url_nr = remote->pushurl_nr;
> +       } else {
> +               url = remote->url;
> +               url_nr = remote->url_nr;
> +       }
> +
> +       if (!url_nr)
> +               die(_("no URLs configured for remote '%s'"), remotename);
> +
> +       if (all_mode) {
> +               for (i = 0; i < url_nr; i++)
> +                       printf_ln("%s", url[i]);
> +       } else {
> +               printf_ln("%s", *url);
> +       }
> +
> +       return 0;
> +}