From: Stefan Beller <sbeller@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH] push options: fail properly in the stateless case
Date: Wed, 8 Feb 2017 12:51:00 -0800 [thread overview]
Message-ID: <CAGZ79kaszczpnu+VEV3y+61qgji-0+vRFVLk8Q1+BbSDgiHz1Q@mail.gmail.com> (raw)
In-Reply-To: <xmqqpoiswo0l.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 8, 2017 at 10:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> When using non-builtin protocols relying on a transport helper
>> (such as http), push options are not propagated to the helper.
>>
>> Fix this by propagating the push options to the transport helper and
>
> The description up to this point is VERY readable and sensible. But
> that makes the title sound a bit strange.
Yes, the title was there first and then I massaged the commit message
until it was readable. Originally I thought the issue is with stateless
protocols, but that is wrong. The underlying issue is just the transport
helper communication lacking.
> I read it as if it were
> saying "stateless case can never support push-options so fail if the
> caller attempts to use one", but that does not seem to be what is
> going on.
Indeed the subject is wrong.
>
>> adding a test that push options using http fail properly.
>
> Sounds sensible. What end-user visible effect does this fix have?
> IOW, what feature do we use "push-option" for?
The Gerrit world started using push options for having a better git
experience, not needing to navigate to the web UI, e.g.:
# push for review and set me as a reviewer:
git push --push-option reviewer=sbeller@google.com ssh://gerrit...
# same with http, it looks like it worked, but the push option
# never made it to the server
git push --push-option reviewer=sbeller@google.com http://gerrit...
# This patch will make the second command fail, reporting
# the http helper not supporting push options.
>
> Ahh, OK, so you need to describe that there are two issues in order
> to be understood by the readers:
>
> (1) the helper protocol does not propagate push-option
> (2) the http helper is not prepared to handle push-option
>
> You fix (1), and you take advantage of the fact (2) to ensure that
> (1) is fixed in the new test.
>
> With such an understanding, the title makes (sort of) sense and you
> wouldn't have to be asked "what end-user visible effect/benefit does
> this have?"
Your analysis is correct.
>
>> +'option push-option <c-string>::
>> + Transmit this push option.
>> +
>
> There is no "c-string" in the current documentation used or
> defined. The closest thing I found is
>
> ... that field will be quoted in the manner of a C string ...
>
> in git-status page, but I do not think you send the value for an
> push-option after running quote_c_style(), so I am puzzled.
When implementing push options, we discussed that and according to
Documentation/git-push:
The given string must not contain a NUL or LF character.
>
> I'd rather see 'option push-option <string>' as the bullet item, and
> in its description say how arbitrary values (if you allow them, that
> is) can be used, e.g. "Transmit <string> encoded in such and such
> way a the value of the push-option".
okay.
next prev parent reply other threads:[~2017-02-08 20:51 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-08 1:09 [PATCH] push options: fail properly in the stateless case Stefan Beller
2017-02-08 18:11 ` Junio C Hamano
2017-02-08 20:51 ` Stefan Beller [this message]
2017-02-08 23:01 ` Junio C Hamano
2017-02-08 22:04 ` [PATCHv2] push options: pass push options to the transport helper Stefan Beller
[not found] ` <CAFzf2XzLNOKbNbkCfzpVJtN-ROW8m8WVBsWM-z7Bxdnv55b45Q@mail.gmail.com>
2017-02-10 18:44 ` Stefan Beller
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGZ79kaszczpnu+VEV3y+61qgji-0+vRFVLk8Q1+BbSDgiHz1Q@mail.gmail.com \
--to=sbeller@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).