Suggestion on git-push --porcelain

Suggestion on git-push --porcelain

[Tay Ray Chuan]

Hi,

this is regarding the recent patch series from Larry. (I thought
replying to any of the patch messages was appropriate, I couldn't find
a cover-letter, so I'm starting a new thread.)

Around June last year, a patch from Larry was made to add the
--porcelain option, so as to produce machine-readable output regarding
ref status.

The latest patch series goes a step further, and tries to change
output - for example, suppressing user-friendly advice and giving "To:
<destination>".

I think this is an untenable path - adding/suppressing output of
certain messages for porcelain writers, while trying to keep things
fixed enough for porcelain writers to depend on. We will also have to
keep and eye out for future patches from adding fprintfs to stdout and
stderr that may break porcelain scripts.

I believe a better approach would be to prefix messages intended for
porcelain writers. For example, a push session might look like this:

  $ git push --porcelain
  PORCELAIN To git://foo.com/git/myrepo.git
  PORCELAIN uptodate refs/heads/baz:refs/heads/baz 1234ab ba4321
  PORCELAIN nonff refs/heads/bar:refs/heads/bar 2345cd 3456de

This is an "positive" approach, in the sense that we don't remove
anything from the current output; we just add more printf("PORCELAIN")
lines to wherever is appropriate.

-- 
Cheers,
Ray Chuan

Re: Suggestion on git-push --porcelain

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

>   $ git push --porcelain
>   PORCELAIN To git://foo.com/git/myrepo.git
>   PORCELAIN uptodate refs/heads/baz:refs/heads/baz 1234ab ba4321
>   PORCELAIN nonff refs/heads/bar:refs/heads/bar 2345cd 3456de
>
> This is an "positive" approach, in the sense that we don't remove
> anything from the current output; we just add more printf("PORCELAIN")
> lines to wherever is appropriate.

Sorry, but I don't see what that would solve.  For example, we used not to
give the destination to the standard output stream, but that line carries
a necessary information and Larry's series corrects that.

In your "prefix with PORCELAIN" scheme, such a change will start adding a
new line "PORCELAIN To ..." that older implementations may not be prepared
to see.

Other than making the output more noisy, I do not see an improvement here.

Re: Suggestion on git-push --porcelain

[Larry D'Anna]

* Tay Ray Chuan (rctay89@gmail.com) [100209 21:34]:
> Hi,
> 
> this is regarding the recent patch series from Larry. (I thought
> replying to any of the patch messages was appropriate, I couldn't find
> a cover-letter, so I'm starting a new thread.)
> 
> Around June last year, a patch from Larry was made to add the
> --porcelain option, so as to produce machine-readable output regarding
> ref status.
> 
> The latest patch series goes a step further, and tries to change
> output - for example, suppressing user-friendly advice and giving "To:
> <destination>".
> 
> I think this is an untenable path - adding/suppressing output of
> certain messages for porcelain writers, while trying to keep things
> fixed enough for porcelain writers to depend on. We will also have to
> keep and eye out for future patches from adding fprintfs to stdout and
> stderr that may break porcelain scripts.

While I agree with you in principle, I'm still advocating that we make these two
changes.  *Especially* suppressing the advice.  That advice did not exist last
June, so the output format has already been changed.  Also, the advice never
should have gone to the standard output in the first place.  All the other
instances of output like that go to standard error.

As for the "To: " lines, unfortunately the lack of them was a pretty serious
design flaw in the original patch :(

> I believe a better approach would be to prefix messages intended for
> porcelain writers. For example, a push session might look like this:
> 
>   $ git push --porcelain
>   PORCELAIN To git://foo.com/git/myrepo.git
>   PORCELAIN uptodate refs/heads/baz:refs/heads/baz 1234ab ba4321
>   PORCELAIN nonff refs/heads/bar:refs/heads/bar 2345cd 3456de
> 
> This is an "positive" approach, in the sense that we don't remove
> anything from the current output; we just add more printf("PORCELAIN")
> lines to wherever is appropriate.

Actually, What I'm proposing is something very similar to this: I think that the
output for the porcelain writer and *only* the output for the porcelain writer
should go to standard output, and everything else should go to standard error.


       --larry

Re: Suggestion on git-push --porcelain

[Jeff King]

On Tue, Feb 09, 2010 at 06:57:46PM -0800, Junio C Hamano wrote:

> Tay Ray Chuan <rctay89@gmail.com> writes:
> 
> >   $ git push --porcelain
> >   PORCELAIN To git://foo.com/git/myrepo.git
> >   PORCELAIN uptodate refs/heads/baz:refs/heads/baz 1234ab ba4321
> >   PORCELAIN nonff refs/heads/bar:refs/heads/bar 2345cd 3456de
> >
> > This is an "positive" approach, in the sense that we don't remove
> > anything from the current output; we just add more printf("PORCELAIN")
> > lines to wherever is appropriate.
> 
> Sorry, but I don't see what that would solve.  For example, we used not to
> give the destination to the standard output stream, but that line carries
> a necessary information and Larry's series corrects that.

I think he is trying to future-proof any additional output that push (or
remote helpers) produce. I don't think it is really worth it, though.
All of that should be going to stderr, and thus would be, at worst,
noise on the terminal. I don't think it is that hard or error-prone a
rule to send such cruft to stderr.

-Peff

Re: Suggestion on git-push --porcelain

[Tay Ray Chuan]

Hi,

On Wed, Feb 10, 2010 at 10:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Tay Ray Chuan <rctay89@gmail.com> writes:
>
>>   $ git push --porcelain
>>   PORCELAIN To git://foo.com/git/myrepo.git
>>   PORCELAIN uptodate refs/heads/baz:refs/heads/baz 1234ab ba4321
>>   PORCELAIN nonff refs/heads/bar:refs/heads/bar 2345cd 3456de
>>
>> This is an "positive" approach, in the sense that we don't remove
>> anything from the current output; we just add more printf("PORCELAIN")
>> lines to wherever is appropriate.
>
> Sorry, but I don't see what that would solve.  For example, we used not to
> give the destination to the standard output stream, but that line carries
> a necessary information and Larry's series corrects that.

I sense a chicken-and-egg situation here.

Printing a "To: <destination>" to stdout is a correction as far as the
current non-prefixed output scheme is concerned.

If a prefixed scheme (or some other output scheme) is adopted, then
where, or whether, the "To: <destination>" is printed, is not relevant
to porcelain script writers - they just get ignored.

> In your "prefix with PORCELAIN" scheme, such a change will start adding a
> new line "PORCELAIN To ..." that older implementations may not be prepared
> to see.

The original --porcelain patch only "porcelained" the output for ref
updates, such as

  = TAB refs/heads/master:refs/heads/master TAB [up to date]
  - TAB :refs/heads/foobar TAB [deleted]
 ...

In addition, the "To: <destination>" patch --porcelain is fairly
recent - I don't think it has left 'pu', has it? Should we worry about
breaking compatibility on something not out yet?

-- 
Cheers,
Ray Chuan

Re: Suggestion on git-push --porcelain

[Tay Ray Chuan]

Hi,

On Wed, Feb 10, 2010 at 12:54 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 09, 2010 at 06:57:46PM -0800, Junio C Hamano wrote:
>
>> Tay Ray Chuan <rctay89@gmail.com> writes:
>>
>> >   $ git push --porcelain
>> >   PORCELAIN To git://foo.com/git/myrepo.git
>> >   PORCELAIN uptodate refs/heads/baz:refs/heads/baz 1234ab ba4321
>> >   PORCELAIN nonff refs/heads/bar:refs/heads/bar 2345cd 3456de
>> >
>> > This is an "positive" approach, in the sense that we don't remove
>> > anything from the current output; we just add more printf("PORCELAIN")
>> > lines to wherever is appropriate.
>>
>> Sorry, but I don't see what that would solve.  For example, we used not to
>> give the destination to the standard output stream, but that line carries
>> a necessary information and Larry's series corrects that.
>
> I think he is trying to future-proof any additional output that push (or
> remote helpers) produce.

Yes. By using this (or some other output scheme), we're deliberately
marking out output for porcelain scripts.

> I don't think it is really worth it, though.
> All of that should be going to stderr, and thus would be, at worst,
> noise on the terminal. I don't think it is that hard or error-prone a
> rule to send such cruft to stderr.

on examining the code paths, there's a fair bit of work needed to
ensure this. Apart from transport.c (transport-helper.c and by
extension, remote helpers, looks ok), we would have to look at the
individual builtin-transport implementations for git and bundle.

Also, as you mentioned above, we would also have to take care future
transport implementations send messages in the desired fashion so that
we wouldn't break porcelain scripts. If we have an output scheme, then
such risks are minimized.

-- 
Cheers,
Ray Chuan

Re: Suggestion on git-push --porcelain

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> I sense a chicken-and-egg situation here.
>
> Printing a "To: <destination>" to stdout is a correction as far as the
> current non-prefixed output scheme is concerned.
>
> If a prefixed scheme (or some other output scheme) is adopted, then
> where, or whether, the "To: <destination>" is printed, is not relevant
> to porcelain script writers - they just get ignored.

Sorry.  I don't get this.  Are you saying that you will write "To" without
the prefix in order to make older scripts ignore it?  What then would newer
scripts that want to use the information do?  Pay attention to lines that
do not start with your prefix?  Then what good does it do to introduce
such a prefix to the output?

I think what you need is not a prefix or any syntax but a rule that says
"ignore things that you do not understand" and possibly a way for the
output stream to say "if you do not understand this, you are too old to
correctly process this stream---please abort, because ignoring this will
make you produce an incorrect result".