public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Ben Boeckel <mathstuf@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH v4] remote: add get-url subcommand
Date: Fri, 04 Sep 2015 15:40:58 -0700	[thread overview]
Message-ID: <xmqqwpw5erk5.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1441377024-23832-1-git-send-email-mathstuf@gmail.com> (Ben Boeckel's message of "Fri, 4 Sep 2015 10:30:24 -0400")

Ben Boeckel <mathstuf@gmail•com> 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.

next prev parent reply	other threads:[~2015-09-04 22:41 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-31 17:38 [PATCH] add ls-remote --get-push-url option Ben Boeckel
2015-07-31 17:38 ` [PATCH] ls-remote: add " Ben Boeckel
2015-07-31 18:40 ` [PATCH] add ls-remote " Junio C Hamano
2015-07-31 18:56   ` Ben Boeckel
2015-07-31 19:02     ` Junio C Hamano
2015-07-31 19:04       ` Ben Boeckel
2015-07-31 19:16         ` Junio C Hamano
2015-07-31 20:51           ` Ben Boeckel
2015-08-03 21:00 ` [PATCH v2] add git-url subcommand Ben Boeckel
2015-08-03 21:00 ` [PATCH v2] remote: add get-url subcommand Ben Boeckel
2015-08-03 23:38   ` Eric Sunshine
2015-08-04  0:16     ` Ben Boeckel
2015-08-04  0:45       ` Eric Sunshine
2015-08-04 14:56 ` [PATCH v3] " Ben Boeckel
2015-08-05 20:34   ` Junio C Hamano
2015-08-05 21:33     ` Ben Boeckel
2015-08-05 21:39       ` Junio C Hamano
2015-09-04 14:30 ` [PATCH v4] " Ben Boeckel
2015-09-04 22:40   ` Junio C Hamano [this message]
2015-09-16  1:53 ` [PATCH v5] " Ben Boeckel
2015-09-16 22:51   ` Junio C Hamano
2015-09-17  0:19 ` [PATCH v6] " Ben Boeckel

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

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

  git send-email \
    --in-reply-to=xmqqwpw5erk5.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=mathstuf@gmail$(echo .)com \
    /path/to/YOUR_REPLY

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox