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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 4, 2015, 22:40 UTC
Message-ID: <xmqqwpw5erk5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1441377024-23832-1-git-send-email-mathstuf@gmail.com>

Ben Boeckel <mathstuf@gmail.com> writes:

Show 11 quoted lines

> +	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.

Show 19 quoted lines

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

Show 21 quoted lines

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