Ben Boeckel <mathstuf@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html