v3, 3 partsgit-push: clean up some of the output from git push --porcelain

Having it go to standard output interferes with git-push --porcelain.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

* don't emit long explanatory message about non-fast-forward updates.

* send "To dest" lines to standard out so whoever is reading standard out knows
  which ref updates went to which remotes.

* only send the "Everything up-to-date" line if verbose.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    2 +-
 transport.c    |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Larry D'Anna <larry@elder-gods.org> writes:

This makes sense as a goal, but at the same time as an implementation it would be cleaner to flip "advice" off under --porcelain, instead of doing "s/if (advice_blah)/if (advice_blah && !porcelain)/;".

This is doubly important if you consider longer term maintainability. I do not want to see the next person who tries to add new advice messages to copy and paste the long if() statement you added in this patch.

Makes sense. s/standard out/the standard output/, and s/reading .*knows/reading from the process knows/, perhaps.

Don't you want to send this also to stdout?

I realize that I more-or-less repeated what I already said for the second round, so let's try a different approach.

How about replacing the three patches with something like this?

-- >8 --
Subject: push --porcelain: usability updates

"git push --porcelain" is meant for Porcelain scripts to read from; there is no reason to give advice messages meant for the user under that mode.

When reporting the update status, the name of the destination and "Everything up-to-date" message were shown to the standard error stream, but that is unfriendly when a Porcelain script is reading from us. Send them to the standard output to make it easier for them.

---
 builtin-push.c |    5 +++++
 transport.c    |    5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

On Mon, Feb 08, 2010 at 01:13:36PM -0800, Junio C Hamano wrote:

I think this is sane.

But note here that you are changing the --porcelain format, as callers which were keeping only the stdout (and letting stderr go to /dev/null, or spew to the user) saw only the ref lines. So this may be breaking such callers.

I think you argued elsewhere (and I agree) that with multiple push urls, this information is useful. Which means that the original porcelain format was perhaps not very well thought-out. :( So we have to choose now whether to fix it and break compatibility, or leave it broken. If the former, then we should make sure there are not other design issues in need of fixing, so we can just break compatibility _once_.

This one, on the other hand, seems to me to be just noise. What does a --porcelain caller learn by seeing "Everything up-to-date" that it did not already know from seeing the list of refs?

-Peff

* Jeff King (peff@peff.net) [100208 16:32]:

I agree. I don't see how sending this message to stdout could possibly help the --porcelain caller.

            --larry

Jeff King <peff@peff.net> writes:

I think the purpose of the patches that started this thread was to admit that 1965ff7 (add --porcelain option to git-push, 2009-06-22) was not well thought out, and to break compatibility to fix it.

Having said that, I would say that what 1965ff7 specified was only these two:

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

so everything else that do not match this pattern is a fair game, most importantly, the line that begins with "To" would not be mistaken with this pattern, I think.

I do not care too much about this hunk either way. We could leave it as is, as we will be giving some other stuff to the standard error stream without squelching anyway, even with the three-patch series. We could squelch only this message, but it is dubious what it is buying us. If you forced me to decide, I would probably say "let's just drop this hunk and keep the code as-is".

As to the exit status, do you have any thoughts, by the way?

I am not convinced that it would be necessary nor even a good idea to make it behave inconsistently between the normal case and Porcelain case, only to make it easier to special case the "remote side would reject due to non-fast-forward" failure mode (iow, even if the calling script knows that it would fail due to non-fast-forward but otherwise everything else would be fine, what good would it do?)

* Junio C Hamano (gitster@pobox.com) [100208 17:22]:

also these

X TAB ...
! TAB ...
* TAB ...
+ TAB ...
SPACE TAB ...

I'll respond to this message with another iteration of the patch.

     --larry

* send the "To prevent you from losing history, non-fast-forward...." message to
  the standard error.  That's where these sort of advice messages typically go

* for git push --porcelain: squelch the above advice.

* for git push --porcelain: send "To dest" lines to the standard output so
  whoever is reading from the process which ref updates went to which remotes.

* for git push --porcelain: only send the "Everything up-to-date" line if
  verbose.

* for git push --porcelain --dry-run: exit with status 0 even if updates will be
  rejected.  Whoever is reading the output of git push--dry-run --porcelain can
  clearly see if updates will be rejected.  However, it will probably need to
  distinguish this condition from other unknown errors that it does not know how
  to handle.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c      |   11 ++++++++---
 builtin-send-pack.c |    4 ++++
 send-pack.h         |    1 +
 transport.c         |    7 ++++---
 4 files changed, 17 insertions(+), 6 deletions(-)

Larry D'Anna <larry@elder-gods.org> writes:

Is this documented anywhere?

Much of what your patches in this thread tried to do does not interest me very much rignt now, i.e. during the pre-release freeze period [*1*].

If you are already showing '*' and '+' without telling the users in the documentation, it is a much more urgent issue that needs fixing, and it would be preferrable if we can do so before the release.

Documentation/git-push.txt seems to say that only ' ', '!' and '=' have any defined meaning:

    flag::
            A single character indicating the status of the ref. This is
            blank for a successfully pushed ref, `!` for a ref that was
            rejected or failed to push, and '=' for a ref that was up to
            date and did not need pushing (note that the status of up to
            date refs is shown only when `git push` is running verbosely).

[Footnote]

*1* As I hinted repeatedly, I think many of them are mere churn, except for "don't advice porcelain scripts" (good) and perhaps "exit with failure status upon only this kind of failure" (I am undecided). Especially I want to see the "send advice to the standard error stream" one as a separate patch, if you insist including that change in the series.

But they are something we can decide after 1.7.0 happens.

* Junio C Hamano (gitster@pobox.com) [100208 17:48]:

Actually it won't to 'X'. print_one_push_status has a case for 'X', but it never gets called.

aparnetly not.  patch to follow.

          --larry

Larry D'Anna <larry@elder-gods.org> writes:

Thanks.

git-push prints a flag indicating the status of each ref that was pushed, or
attempted to be pushed.  The documentation did not correctly list all of the
possible flags.
---
 Documentation/git-push.txt |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

Larry D'Anna <larry@elder-gods.org> writes:

Thanks.

I assume it is Ok to copy your sign-off from your other messages ;-)

Both [horizontal] and three colons are something we never have used in the existing documentation set. How confident are you that various versions of deployed AsciiDoc people would use all support this?

I am not _complaining_; I am just being cautious to see if I have to look into the issue myself (if your answer is "not at all") or not (otherwise).

Junio C Hamano <gitster@pobox.com> writes:

Unfortunately, http://www.methods.co.nz/asciidoc/newlists.html labels the syntax as "new in 8.3.0", and Debian 5.0.X and Fedora 11 both use AsciiDoc that are older than that (8.2.7 and 8.2.5).

I'll flatten the description into prose for now.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Actually, we already use another syntax for nested enumerations, namely double-semicolon, in config.txt, so let's mimic that.

-- >8 --
Subject: git-push: document all the status flags used by --porcelain output 

We didn't talk about '-' (deletion), '*' (addition), nor '+' (forced).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-push.txt |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

* Junio C Hamano (gitster@pobox.com) [100208 19:45]:

the flags are used by normal git-push as well

    --larry

Larry D'Anna <larry@elder-gods.org> writes:

I noticed it after I sent it out and reworded "s/by.*/in the output/". Thanks for being careful.

* Junio C Hamano (gitster@pobox.com) [100208 19:24]:

Apparently three colons is really new, its only in asciidoc 8.5.3, released this January. [horizontal] goes back to 8.3.0, from 2008.

          --arry

* Junio C Hamano (gitster@pobox.com) [100208 17:48]:

What about the "To" lines? It seems that they really should go to stdout (if --porcelain is selected). Otherwise, how does the reader of stdout know which refs got pushed to which remote?

Larry D'Anna <larry@elder-gods.org> writes:

I think that probably is an improvement.

We would probably need documentation updates to cover the parsing of "To", at least; it might be cleaner to have a separate section for porcelain writers. And tests. But this series won't hit next until 1.7.0 ships so we have enough time to polish the details.

I queued the series and once merged it to 'pu' but seeing some tests fail, I reverted the merge. The error is somewhat curious...

$ while sh t5401-*.sh -i; do :; done
... wait for a while ...
* FAIL 12: send-pack stderr contains hook messages

                grep ^remote: send.err | sed "s/ *\$//" >actual &&
                test_cmp - actual <expect

$ t/(364643e...); cat trash\ directory.t5401-update-hooks/actual remote: STDOUT pre-receive remote: STDERR pre-receive remote: STDOUT update refs/heads/master remote: STDERR update refs/heads/master remote: STDOUT update refs/heads/tofail remote: STDOUT post-receive remote: STDERR post-receive remote: STDOUT post-update remote: STDERR post-update $ t/(364643e...); cat trash\ directory.t5401-update-hooks/expect remote: STDOUT pre-receive remote: STDERR pre-receive remote: STDOUT update refs/heads/master remote: STDERR update refs/heads/master remote: STDOUT update refs/heads/tofail remote: STDERR update refs/heads/tofail remote: STDOUT post-receive remote: STDERR post-receive remote: STDOUT post-update remote: STDERR post-update

* Junio C Hamano (gitster@pobox.com) [100209 02:31]:

I bisected, and this was introduced by  

6d525d3 receive-pack: Send hook output over side band #2

  --larry

Junio C Hamano <gitster@pobox.com> wrote:

A quick visual inspection shows that only the STDERR tofail message is missing here. That sounds to me like a race condition in the recv_sideband decoder. Or, a race condition in the hook code in builtin-receive-pack.c.

I doubt its in receive-pack. run_update_hook() directly calls the copy_to_sideband() function, and that reads until EOF on the hook's stderr stream before it returns and waits for the hook's exit status. So we should be pulling everything and dumping it into the sideband.

builtin-send-pack.c clearly isn't stopping early while processing the stream, since we see later messages from the post-receive and post-update hooks just fine.

So I think the only code that is in question is the case 2 arm of recv_sideband(). But to be honest, I can't find any fault with it.

I've been running this test in a while loop for a while now, and I can't make it trigger this failure. Maybe its possible that its the update hook itself, not flushing its stderr buffer before it terminates?

On Tue, 9 Feb 2010, Shawn O. Pearce wrote:

Note that strict order of messages passed through the sideband can't be relied upon. Often you have sideband 1 connected to stdin and sideband 2 connected to stderr, and they are linked with pipes, and various factors such as stdio buffering or even printf implementation in the C lib, pipe buffers in the OS, random scheduling between the processes involved, and other factors such as disk or network speed vs CPU speed, are all adding to a certain degree of randomness affecting how the data end up in various channels wrt each other.

See my efforts to reduce that issue so far with those commits:

6b59f51b312f06d9420d34c09fa408c658aac6d2

6b9c42b4daabe3d0471d9f90d2ae472c9d994e1c

d048a96ee9bec968be0bdc9c43ffce61169545be

ebe8fa738dcf6911fe520adce0cfa0cb26dee5e2

Nicolas

Nicolas Pitre <nico@fluxnic.net> wrote:

Oh.  Sure.  But that isn't really the case here.

The messages are all coming down side band #2, before we write anything down side band #1. The missing message in question should have appeared somewhere in the middle of that side band #2 stream that we are seeing in the test output. Given that its all serialized down into a single stream by the parent process receive-pack, we really shouldn't see the messages out of order.

The only way I can see this missing message happening is if the C library isn't flushing the stdio buffer before the hook process exits. Given that the hook process is a /bin/sh shell script, and its using echo to print its messages... I'm at a loss for how to fix that in Git.

Unless its the recv_sideband() somehow skipping a line. But I can't see it doing that.

"Shawn O. Pearce" <spearce@spearce.org> writes:

The detection method of test is fooled by intermixed message.

This is what send.err has, and you grep for '^remote:' in it.

-- >8 --
warning: updating the current branch
warning: Updating the currently cheremote: STDERR pre-receive        
,
warning: as the index and work tree do not reflect changes that are in HEAD.
warning: As a result, you may see the changes you just pushed into it
warning: reverted when you run 'git diff' over there, and you may want
warning: to run 'git reset --hard' before starting to work to recover.
warning: 
warning: You can set 'receive.denyCurrentBranch' configuration variable to
warning: 'refuse' in the remote repository to forbid pushing into its
warning: current branch.
warning: To allow pushing into the current branch, you can set it to 'ignore';
warning: but this is not recommended unless you arranged to update its work
warning: tree to match what you pushed in some other way.
warning: 
warning: To squelch this message, you can set it to 'warn'.
warning: 
warning: Note that the default will change in a future version of git
warning: to refuse updating the current branch unless you have the
warning: configuration variable set to either 'ignore' or 'warn'.
remote: STDOUT update refs/heads/master        
remote: STDERR update refs/heads/master        
remote: STDOUT update refs/heads/tofail        
remote: STDERR update refs/heads/tofail        
error: hook declined to update refs/heads/tofail
remote: STDOUT post-receive        
remote: STDERR post-receive        
remote: STDOUT post-update        
remote: STDERR post-update        
To ./victim/.git
   d4f8e4a..b07e5b8  master -> master
 ! [remote rejected] tofail -> tofail (hook declined)
-- 8< --

But there indeed _is_ some skipping. "Updating the currently che" is interrupted by the output from the pre-receive hook, and I do not see the remainder "cked out branch may cause confusion,\n" anywhere.

Oh, I hate myself ;-)

This seems to fix the test.  Could somebody please explain why?

I am guessing it has something to do with O_APPEND but I cannot come up with a commit log message that clearly describes the fix.

 t/t5401-update-hooks.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Junio C Hamano <gitster@pobox.com> wrote:

..

Uh.  I got the problem now, thanks.

What's going on is, other messages inside of builtin-receive-pack are being sent to stderr, while hook output is going over the multiplexed side-band through stdout, where its parsed and written to stderr by send-pack.

What I missed in my patch was changing all of these other messages inside of receive-pack to also go over the side-band #2 if we have use_sideband enabled.

Patch coming in a few minutes.

* Junio C Hamano (gitster@pobox.com) [100208 17:48]:

For some reason I thought you wanted the series squashed into one patch. Here it is again (to follow) with each discrete change as a single patch, and without messing with "Everything up-to-date"

        --larry

On Mon, Feb 08, 2010 at 02:21:26PM -0800, Junio C Hamano wrote:

I thought it was simply to remove stdout cruft that had crept in, and at the same time to remove some stderr cruft that was simply noise on the terminal. That being said, I am in favor of fixing it even if it means a slight compatibility breakage.

It depends on the parser. If the parser was something like:

  switch (line[0]) {
    case '=': ...; break;
    case '-': ...; break;
    default: die("wtf: %s", line);
  }

then we are not introducing any ambiguity, but we are causing a breakage. The problem is that we did not specify the format anywhere, so it is hard to say whether we are breaking any promises we made.

I have not actually been running these patches, just reading them, but my impression was the goal _was_ to squelch all of the stderr cruft. But if we are not even close, then probably we should just give up and callers should "2>/dev/null".

I had initially endorsed it, but now I am having second thoughts. Especially if the "usual" calling convention is to redirect stderr as above, then we are probably missing out on any useful error messages that accompany a failure return, anyway. So maybe the sane thing to do is to leave the exit code alone, and include a --porcelain output line that either says "Everything was OK, see individual ref status" or "We couldn't even talk to the other side". Then the status code is irrelevant, and stdout contains all of the useful information (and if you don't get an error or OK message, you know there was some serious error like a broken git installation).

-Peff

* Jeff King (peff@peff.net) [100210 00:41]:

Personally, I don't really care about squelching stderr cruft. All I really want is for what goes to stdout be sane and the calling script to be able to unambiguously figure out what happened, including

* which refs go to which remotes 

* whether or not some mysterious error occurred (beyond those mentioned in the
  ref status lines)

That serves my purposes as well as the exit code would.  Is this the consensus?

     --larry

Hi,

On Wed, Feb 10, 2010 at 1:55 PM, Larry D'Anna <larry@elder-gods.org> wrote:

does this mean the ugly return status mangling patch (git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected) would be dropped? If so, I'm all for it. =p

Jeff King <peff@peff.net> writes:

I am tempted to suggest adding "clear_advice(void)" in advice.[ch], so that people adding new advices do not have to hunt for even the above hunk. It would be a good direction to go in general _if_ we will have more like this --porcelain thing in other parts of the system.

I didn't do so because that "_if_" is still iffy.

On Mon, Feb 08, 2010 at 02:59:01PM -0800, Junio C Hamano wrote:

Would it make sense to clear _all_ advice if we are just in porcelain mode for git-push? That is, let's say I am in "push --porcelain" and I try to write a reflog entry for a local tracking ref, but my identity is implicitly picked up from the hostname[1]. Should it trigger advice_implicit_identity and say "by the way, your identity is implicit" on stderr or not?

I would say yes. The advice output should all be on stderr, and the porcelain output should all be on stdout. So there is no parsing conflict. And stderr either goes to /dev/null (because the porcelain is not terminal-based, or doesn't want the terminal screwed up), in which case the advice does nothing, or stderr goes to the terminal (because the porcelain is some simple script), in which case the message is probably something the user would want to see.

-Peff

[1] I am actually not sure if you can trigger the implicit identity advice in this way. But I am arguing from the perspective of "assume you have triggered some random advice through some sub-action", which may include advice to be added in the future.

Jeff King <peff@peff.net> writes:

Hmm, if we make it a rule to send all the advice messages to the standard error stream, wouldn't the logical conclusion be that the hunk you are commenting is unnecessary? "push --porcelain" should give advice just as usual (but we would simply send the advice to the standard error stream).

On Thu, Feb 11, 2010 at 03:23:09PM -0800, Junio C Hamano wrote:

Yes and no. The advice it is giving is specifically related to things you can't see, so it is not really as helpful as it could be. And in that sense, squelching may be positive.

But it seems that the thread has come to the conclusion that we're still going to have messages on stderr. There is not much point in suppressing one particular message if there is a bunch of other cruft. Either the user should see stuff on stderr, or they shouldn't, and the caller can decide by redirecting stderr. So yes, squelching that advice probably should just be dropped.

-Peff

The script calling git push --dry-run --porcelain can see clearly from the output that the updates will be rejected. However, it will probably need to distinguish this condition from the push failing for other reasons, such as the remote not being reachable.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-send-pack.c |    4 ++++
 send-pack.h         |    1 +
 transport.c         |    3 ++-
 3 files changed, 7 insertions(+), 1 deletions(-)

Larry D'Anna <larry@elder-gods.org> writes:

I am not sure about this reasoning. If you are telling the script writers to decide what happened by reading from the output, shouldn't the program say "I fail because I cannot reach the other side" to its standard output so that the script can read it as well?

Having said that, I don't think it matters either way. If a script wants to know if push would fully succeed or not, it will run without --porcelain (perhaps while discarding the standard error) and check the status. Even without this patch, if a script runs with --porcelain and gets non-zero status, it can inspect the output and if it got rejection, that is a sure sign that it at least reached the other end to get enough information to decide that it will be rejected, no?

* Junio C Hamano (gitster@pobox.com) [100208 15:59]:

Wouldn't that just complicate life for the script writer? If you send such messages to the standard output, the script would have to include logic to distinguish error messages from the rest of the output. If you send them to the standard error then the script knows exactly where to find them.

Yes, it's a sure sign that it reached the other side, but how does the script know nothing else went wrong? What if a malloc failed? OK that's a bit far fetched, but it's really, really nice to be able to get an unambiguous status bit out of a command so you know if the requested operation succeeded or not. Without this patch, a script calling git push --porcelain --dry-run has no way of distinguishing these two situations

1) "git push" would try to push a ref that would get rejected, but everything
else is fine.

2) "git push" would try to push a ref that would get rejected, and also some
unknown type of error occurred that this script has no idea how to handle.

        --larry
            

Larry D'Anna <larry@elder-gods.org> writes:

Hmph, you are doing (rewritten in an easier to follow format)

	if (--dry-run && --porcelain)
		err = 0;
	else
		err = push_add_errors(remote_refs);

here, which I think changes the semantics of what follows immediately after this hunk, namely:

	if (!quiet || err)
		print_push_status(transport->url, remote_refs,
				verbose | porcelain, porcelain,
				nonfastforward);

Earlier, the logic said "even if you asked for --quiet, we would report if there is an error" but now you are changing the rule to "under --dry-run and --porcelain, --quiet means don't ever report the status, even when there are errors".

I don't necessarily think it is a bad change, but in any case the semantic change is worth documenting.

* Junio C Hamano (gitster@pobox.com) [100209 17:25]:

Now that you point it out, I can't really think of any reason why this patch ought to mess with the semantics of --quiet.

      --larry

The script calling git push --dry-run --porcelain can see clearly from the output that the updates will be rejected. However, it will probably need to distinguish this condition from the push failing for other reasons, such as the remote not being reachable.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-send-pack.c |    4 ++++
 send-pack.h         |    1 +
 transport.c         |    4 +++-
 3 files changed, 8 insertions(+), 1 deletions(-)

* Junio C Hamano (gitster@pobox.com) [100209 17:25]:

Which version of the --quiet behavior do you want for the next version of this series? My feeling is that "even if you asked for --quiet, we would report if there is an error" is the best policy, even under --dry-run --porcelain.

         --larry

Larry D'Anna <larry@elder-gods.org> writes:

I think that is probably a sane and the least-surprising thing to do from the end user's point of view.

Larry D'Anna <larry@elder-gods.org> writes:

Could you at least update this rationale?

You will be squelching this in [2/3] when --porcelain is used, so the above is no longer a good justification. It won't interfere at all.