From: John Keeping Date: Thu, 18 Apr 2013 18:35:40 GMT Subject: Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs Message-ID: <20130418183540.GB2278@serenity.lan> In-Reply-To: <7v38un7nnp.fsf@alter.siamese.dyndns.org> 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.