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

[Junio C Hamano]

Ben Boeckel  writes:

> + url_nr = 0;
> + if (push_mode) {
> + url = remote->pushurl;
> + url_nr = remote->pushurl_nr;
> + }
> +
> + /* Fall back to the fetch URL if no push URLs are set. */
> + if (!url_nr) {
> + url = remote->url;
> + url_nr = remote->url_nr;
> + }

While the code does the right thing, the comment and the logic
looked somewhat on the side of funny than cute.  url_nr would be
zero when we are asking for fetch URL, and it would be zero also
when we are asking for push URL but there is no push URL defined.
So this statement covers both cases, but "Fall back to" talks only
about the case where the user asked for push URL.

> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 7a8499c..2cfd3cb 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -919,6 +919,18 @@ test_expect_success 'new remote' '
>   cmp expect actual
>  '
>  
> +test_expect_success 'get-url on new remote' '
> + echo foo >expect &&
> + git remote get-url someremote >actual &&
> + cmp expect actual &&
> + git remote get-url --all someremote >actual &&
> + cmp expect actual &&
> + git remote get-url --push someremote >actual &&
> + cmp expect actual &&
> + git remote get-url --push --all someremote >actual &&
> + cmp expect actual
> +'

In the pre-context of this hunk, I can see that you inherited this
habit from existing tests, but breakage can be made easier to see
if you used test_cmp instead of cmp.

> @@ -961,6 +973,24 @@ test_expect_success 'remote set-url --push zot' '
>   cmp expect actual
>  '
>  
> +test_expect_success 'get-url with different urls' '
> + echo baz >expect &&
> + echo "YYY" >>expect &&
> + echo baz >>expect &&
> + echo "YYY" >>expect &&
> + echo zot >>expect &&
> + echo "YYY" >>expect &&
> + echo zot >>expect &&
> + git remote get-url someremote >actual &&
> + echo "YYY" >>actual &&
> + git remote get-url --all someremote >>actual &&
> + echo "YYY" >>actual &&
> + git remote get-url --push someremote >>actual &&
> + echo "YYY" >>actual &&
> + git remote get-url --push --all someremote >>actual &&
> + cmp expect actual
> +'

I am not sure what these YYY are about. Is this an attempt to make
it easier to see which of the output from four logically separate
tests are broken?

I am wondering if something along this line might be easier to read
and maintain:

get_url_test () {
cat >expect &&
test_expect_success "get-url $*" '
git remote get-url $* >actual &&
test_cmp expect actual
'
}

echo baz | get_url_test someremote
echo baz | get_url_test --all someremote

Then later when you have more than one pushURL to someremote, you
would do something like:

get_url_test --all --push someremote <<\-EOF
foo
aaa
EOF

This comment applies to the remainder of this patch that has YYY
sprinkled all over it.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] remote: add get-url subcommand

[Ben Boeckel]

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 
---
 Documentation/git-remote.txt | 10 
 builtin/remote.c | 58 
 t/t5505-remote.sh| 53 
 3 files changed, 121 insertions(+)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4c6d6de..3c9bf45 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 'git remote remove' 
 'git remote set-head'  (-a | --auto | -d | --delete | )
 'git remote set-branches' [--add]  ...
+'git remote get-url' [--push] [--all] 
 'git remote set-url' [--push]   []
 'git remote set-url --add' [--push]  
 'git remote set-url --delete' [--push]  
@@ -131,6 +132,15 @@ The named branches will be interpreted as if specified 
with the
 With `--add`, instead of replacing the list of currently tracked
 branches, adds to that list.
 
+'get-url'::
+
+Retrieves the URLs for a remote. Configurations for `insteadOf` and
+`pushInsteadOf` are expanded here. By default, only the first URL is listed.
++
+With '--push', push URLs are queried rather than fetch URLs.
++
+With '--all', all URLs for the remote will be listed.
+
 'set-url'::
 
 Changes URLs for the remote. Sets first URL for remote  that matches
diff --git a/builtin/remote.c b/builtin/remote.c
index 181668d..192b9cb 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,6 +18,7 @@ static const char * const builtin_remote_usage[] = {
N_("git remote prune [-n | --dry-run] "),
N_("git remote [-v | --verbose] update [-p | --prune] [( | 
)...]"),
N_("git remote set-branches [--add]  ..."),
+   N_("git remote get-url [--push] [--all] "),
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
N_("git remote set-url --delete  "),
@@ -65,6 +66,11 @@ static const char * const builtin_remote_update_usage[] = {
NULL
 };
 
+static const char * const builtin_remote_geturl_usage[] = {
+   N_("git remote get-url [--push] [--all] "),
+   NULL
+};
+
 static const char * const builtin_remote_seturl_usage[] = {
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
@@ -1467,6 +1473,56 @@ static int set_branches(int argc, const char **argv)
return set_remote_branches(argv[0], argv + 1, add_mode);
 }
 
+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 rather than fetch URLs")),
+   OPT_BOOL('\0', "all", &all_mode,
+N_("return all URLs")),
+   OPT_END()
+   };
+   argc = parse_options(argc, argv, NULL, options, 
builtin_remote_geturl_usage, 0);
+
+   if (argc != 1)
+   usage_with_options(builtin_remote_geturl_usage, options);
+
+   remotename = argv[0];
+
+   if (!remote_is_configured(remotename))
+   die(_("No such remote '%s'"), remotename);
+   remote = remote_get(remotename);
+
+   url_nr = 0;
+   if (push_mode) {
+   url = remote->pushurl;
+   url_nr = remote->pushurl_nr;
+   }
+
+   /* Fall back to the fetch URL if no push URLs are set. */
+   if (!url_nr) {
+   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;
+}
+
 static int set_url(int argc, const char **argv)
 {
int i, push_mode = 0, add_mode = 0, delete_mode = 0;
@@ -1576,6 +1632,8 @@ int cmd_remote(int argc, const char **argv, const char 
*prefix)
result = set_head(argc, argv);
else if (!strcmp(argv[0], "set-branches"))
result = set_branches(argc, argv);
+   else if (!strcmp(argv[0], "get-url"))
+   result = get_url(argc, argv);
else if (!strcmp(argv[0], "set-url"))
result = set_url(argc, argv);
else if (!strcmp(argv[0], "show"))
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 7a8499c..2cfd3cb 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -919,6 +919,18 @@ test_expect_success 'new remote' '
cmp expect actual
 '
 
+test_expect_success 'get-url on new remote' '
+   echo foo >expect &&
+   git remote get-url someremote