Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs

[Junio C Hamano <gitster@pobox.com>]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> This has never worked, since it's inception the code simply skips all
> the refs, essentially telling fast-export to do nothing.

Good description.

>
> Let's at least tell the user what's going on.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/gitremote-helpers.txt | 4 ++--
>  t/t5801-remote-helpers.sh           | 6 +++---
>  transport-helper.c                  | 5 +++--
>  3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
> index ba7240c..4d26e37 100644
> --- a/Documentation/gitremote-helpers.txt
> +++ b/Documentation/gitremote-helpers.txt
> @@ -162,8 +162,8 @@ Miscellaneous capabilities
>  	For remote helpers that implement 'import' or 'export', this capability
>  	allows the refs to be constrained to a private namespace, instead of
>  	writing to refs/heads or refs/remotes directly.
> -	It is recommended that all importers providing the 'import' or 'export'
> -	capabilities use this.
> +	It is recommended that all importers providing the 'import'
> +	capability use this. It's mandatory for 'export'.

As [1/6] said "*:*" does not work and has never worked, and
especially on the push side the patch below makes it clear it was a
glorified no-op, it may be better to say a bit more than "use
this. It's mandatory".  It is not like any value makes sense, right?

Perhaps the documentation will be further updated in a later step in
the series to clarify it.  Let's read on til the end of the series...

> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
> index cd1873c..3eeb309 100755
> --- a/t/t5801-remote-helpers.sh
> +++ b/t/t5801-remote-helpers.sh
> @@ -111,13 +111,13 @@ test_expect_success 'pulling without refspecs' '
>  	compare_refs local2 HEAD server HEAD
>  '
>  
> -test_expect_failure 'pushing without refspecs' '
> +test_expect_success 'pushing without refspecs' '
>  	test_when_finished "(cd local2 && git reset --hard origin)" &&
>  	(cd local2 &&
>  	echo content >>file &&
>  	git commit -a -m ten &&
> -	GIT_REMOTE_TESTGIT_REFSPEC="" git push) &&
> -	compare_refs local2 HEAD server HEAD
> +	GIT_REMOTE_TESTGIT_REFSPEC="" test_must_fail git push 2> ../error) &&
> +	grep "remote-helper doesn.t support push; refspec needed" error
>  '

I somehow like this change that turns a _failure into a _success by
expecting test_must_fail, especially because it is clear why the
tested "push" should fail when you look at the rest of the patch ;-)

Fun.

> diff --git a/transport-helper.c b/transport-helper.c
> index cea787c..4d98567 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -785,6 +785,9 @@ static int push_refs_with_export(struct transport *transport,
>  	struct string_list revlist_args = STRING_LIST_INIT_NODUP;
>  	struct strbuf buf = STRBUF_INIT;
>  
> +	if (!data->refspecs)
> +		die("remote-helper doesn't support push; refspec needed");
> +
>  	helper = get_helper(transport);
>  
>  	write_constant(helper->in, "export\n");
> @@ -795,8 +798,6 @@ static int push_refs_with_export(struct transport *transport,
>  		char *private;
>  		unsigned char sha1[20];
>  
> -		if (!data->refspecs)
> -			continue;
>  		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
>  		if (private && !get_sha1(private, sha1)) {
>  			strbuf_addf(&buf, "^%s", private);