Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 18, 2013, 17:28 UTC
- Message-ID
- <7va9ov7npq.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1366258473-12841-4-git-send-email-felipe.contreras@gmail.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.
Show 22 quoted lines
> > 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...
Show 19 quoted lines
> 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.
Show 23 quoted lines
> 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);