Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
Eric Sunshine writes: >> + grep "remote-helper doesn.t support push; refspec needed" error > > Is "doesn.t" intentional? It certainly works by accident in grep, but > did you mean s/doesn.t/doesn't/ ? The pattern matching the expected string is not by accident, but by design. It of course can be made more strict to reject "doesnot" and require "doesn't" by doing something like this: grep "remote-helper doesn'\''t support push; refspec needed" error but at some point, it simply stops being worth it to tighten the pattern. For that matter, it could be as loose as grep "support push; refspec needed" error if you know the string is unique enough. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
On Thu, Apr 18, 2013 at 7:27 PM, Eric Sunshine wrote: > On Thu, Apr 18, 2013 at 12:14 AM, Felipe Contreras > wrote: >> 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 > > Is "doesn.t" intentional? It certainly works by accident in grep, but > did you mean s/doesn.t/doesn't/ ? In all git test cases we are already inside single quotes; can't do that. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
On Thu, Apr 18, 2013 at 12:14 AM, Felipe Contreras wrote: > 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 Is "doesn.t" intentional? It certainly works by accident in grep, but did you mean s/doesn.t/doesn't/ ? > ' > > test_expect_success 'pulling without marks' ' -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
On Thu, Apr 18, 2013 at 10:29:30AM -0700, Junio C Hamano wrote:
> John Keeping writes:
> >> 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");
> >
> > I think the "refspec needed" text is likely to be confusing if an
> > end-user ever sees this message. I'm not sure how we can provide useful
> > feedback for both remote helper authors and end-users though.
>
> This "refspecs" only come via the helper and not directly from the
> end user, no?
>
> If that is the case, I do not think "confusing" is much of an issue.
> Not _("localizing") is also the right thing to do. We may want to
> say "BUG: " at front to clarify it is not the end-user's fault, but
> a problem they may want to report. If we at this point know what
> helper attempted export without giving refspecs, it may help to show
> it here, so that developers will know with what helper the user
> had problems with.
I like this idea. Perhaps we should say "Bug in remote helper; refspec
needed with export", so that it clearly indicates to both end-users and
remote helper authors that the error is in the remote helper.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
John Keeping writes:
>> +It is recommended that all importers providing the 'import'
>> +capability use this. It's mandatory for 'export'.
>
> s/It's/It is/
I personally do not care _too_ deeply either way, but I agree our
documentation tends to use the latter more and being consistent
would be good.
>> 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");
>
> I think the "refspec needed" text is likely to be confusing if an
> end-user ever sees this message. I'm not sure how we can provide useful
> feedback for both remote helper authors and end-users though.
This "refspecs" only come via the helper and not directly from the
end user, no?
If that is the case, I do not think "confusing" is much of an issue.
Not _("localizing") is also the right thing to do. We may want to
say "BUG: " at front to clarify it is not the end-user's fault, but
a problem they may want to report. If we at this point know what
helper attempted export without giving refspecs, it may help to show
it here, so that developers will know with what helper the user
had problems with.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
Felipe Contreras 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
> ---
> 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);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
On Thu, Apr 18, 2013 at 05:14:18AM -0500, Felipe Contreras wrote: > On Thu, Apr 18, 2013 at 5:11 AM, John Keeping wrote: > > On Wed, Apr 17, 2013 at 11:14:30PM -0500, Felipe Contreras wrote: > >> This has never worked, since it's inception the code simply skips all > >> the refs, essentially telling fast-export to do nothing. > >> > >> Let's at least tell the user what's going on. > >> > >> Signed-off-by: Felipe Contreras > >> --- > >> 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'. > > > > s/It's/It is/ > > What's the difference? "It's" is considered informal, while we do adopt that tone on the mailing list and sometimes in the documentation, in general I think we should avoid contractions in the formal documentation. In this case it is particularly jarring because it immediately follows a sentence where we use the full "It is" form. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
On Thu, Apr 18, 2013 at 5:11 AM, John Keeping wrote:
> On Wed, Apr 17, 2013 at 11:14:30PM -0500, Felipe Contreras wrote:
>> This has never worked, since it's inception the code simply skips all
>> the refs, essentially telling fast-export to do nothing.
>>
>> Let's at least tell the user what's going on.
>>
>> Signed-off-by: Felipe Contreras
>> ---
>> 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'.
>
> s/It's/It is/
What's the difference?
>> --- 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");
>
> I think the "refspec needed" text is likely to be confusing if an
> end-user ever sees this message. I'm not sure how we can provide useful
> feedback for both remote helper authors and end-users though.
It doesn't have to be. They'll google it, or they'll post a bug report
with this warning verbatim, or they'll just ignore it. I don't think
there's much more we can do to help in either of these cases.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs
On Wed, Apr 17, 2013 at 11:14:30PM -0500, Felipe Contreras wrote:
> This has never worked, since it's inception the code simply skips all
> the refs, essentially telling fast-export to do nothing.
>
> Let's at least tell the user what's going on.
>
> Signed-off-by: Felipe Contreras
> ---
> 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'.
s/It's/It is/
> +
> A helper advertising the capability
> `refspec refs/heads/*:refs/svn/origin/branches/*`
> 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
> '
>
> test_expect_success 'pulling without marks' '
> 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");
I think the "refspec needed" text is likely to be confusing if an
end-user ever sees this message. I'm not sure how we can provide useful
feedback for both remote helper authors and end-users though.
> +
> 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);
> --
> 1.8.2.1.679.g509521a
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/6] transport-helper: clarify pushing without refspecs
This has never worked, since it's inception the code simply skips all
the refs, essentially telling fast-export to do nothing.
Let's at least tell the user what's going on.
Signed-off-by: Felipe Contreras
---
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'.
+
A helper advertising the capability
`refspec refs/heads/*:refs/svn/origin/branches/*`
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
'
test_expect_success 'pulling without marks' '
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);
--
1.8.2.1.679.g509521a
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
