v4transport-helper: report errors properly

If a push fails because the remote-helper died (with fast-export), the user won't see any error message. So let's add one.

At the same time lets add tests to ensure this error is reported, and while we are at it, check the error from fast-import

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-remote-testgit        | 13 +++++++++++++
 t/t5801-remote-helpers.sh | 21 +++++++++++++++++++++
 transport-helper.c        |  2 +-
 3 files changed, 35 insertions(+), 1 deletion(-)

On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Does the user know what a remote helper is? Could we point them at some helpful docs in case they don't?

-- Cheers,

Sverre Rabbelier

On Mon, Apr 08, 2013 at 11:20:15AM -0700, Sverre Rabbelier wrote:

That's a good point. I wonder if it would be enough to say:

  fatal: Reading from helper git-remote-X failed

That might make it more clear what the helper's role is, and showing the command name gives the user a starting point for running "man".

-Peff

On Mon, Apr 08, 2013 at 09:40:04AM -0500, Felipe Contreras wrote:

Thanks, I think this patch is definitely the right direction.

It seems like there is a lot of back-story that had to be clarified during the review/discussion. Is there a reason not to summarize it here so later readers of this commit are enlightened?

I'm thinking something like:

  If a push fails because the remote-helper died (with fast-export), the
  user does not see any error message. We do correctly die with a failed
  exit code, as we notice that the helper has died while reading back
  the ref status from the helper. However, we don't print any message.
  This is OK if the helper itself printed a useful error message, but we
  cannot count on that; let's let the user know that the helper failed.

  In the long run, it may make more sense to propagate the error back up
  to push, so that it can present the usual status table and give a
  nicer message. But this is a much simpler fix that can help
  immediately.

  While we're adding tests, let's also confirm that the remote-helper
  dying is also detect when importing refs. We currently do so robustly
  when the helper uses the "done" feature (and that is what we test). We
  cannot do so reliably when the helper does not use the "done" feature,
  but it is not even worth testing; the right solution is for the helper
  to start using "done".

We can do away with this sleep with:

  while read line; do
          test "$line" = "done" && break
  done

The version I posted yesterday had both the read and the sleep, but the sleep was only necessary there to demonstrate the race with check_command.

I think we can drop this comment now, right?

-Peff

Felipe Contreras <felipe.contreras@gmail.com> writes:

[...]

There appears to be a race in the version that is in today's pu (5eb25f737b). I reproduced with this:

  cd git/t
  i=1
  while ./t5801-remote-helpers.sh --root=/dev/shm --valgrind
  do
    i=$(($i+1))
  done

Two out of six of these loops quit within 1 and 2 iterations, respectively, both with an error along the lines of:

  expecting success: 
          (GIT_REMOTE_TESTGIT_FAILURE=1 &&
          export GIT_REMOTE_TESTGIT_FAILURE &&
          cd local &&
          test_must_fail git push --all 2> error &&
          cat error &&
          grep -q "Reading from remote helper failed" error
          )

  error: fast-export died of signal 13
  fatal: Error while running fast-export
  not ok 21 - proper failure checks for pushing

I haven't been able to reproduce outside of valgrind tests. Is this an expected issue, caused by overrunning the sleep somehow? If so, can you increase the sleep delay under valgrind so as to not cause intermittent failures in the test suite?

On Tue, Apr 09, 2013 at 11:38:05PM +0200, Thomas Rast wrote:

Yeah, I am not too surprised. The failing helper sleeps before exiting so that fast-export puts all of its data into the pipe buffer before the helper dies, and does not get SIGPIPE. But obviously the sleep is just delaying the problem if your fast-export runs really slowly (which, if you are running under valgrind, is a possibility).

The helper should instead just consume all of fast-export's input before exiting, which accomplishes the same thing, finishes sooner in the normal case, and doesn't race. And I think it also simulates a reasonable real-world setup (a helper reads and converts the data, but then dies while writing the output to disk, the network, or whatever).

I posted review comments, including that, and I'm assuming that Felipe is going to re-roll at some point.

-Peff

I think this topic is close to being done, so I just wanted to move it along.

  [1/2]: transport-helper: report errors properly

    This is Felipe's v4 patch with the adjustments I suggested in
    review. It explains more in the commit message, and should fix
    Thomas's valgrind failures (it consumes fast-export's data before
    dying rather than sleeping and hoping that fast-export is done
    writing).

  [2/2]: transport-helper: mention helper name when it dies

    This changes the error message, to help with the issue raised by
    Sverre.

-Peff

From: Felipe Contreras <felipe.contreras@gmail.com>

If a push fails because the remote-helper died (with fast-export), the user does not see any error message. We do correctly die with a failed exit code, as we notice that the helper has died while reading back the ref status from the helper. However, we don't print any message. This is OK if the helper itself printed a useful error message, but we cannot count on that; let's let the user know that the helper failed.

In the long run, it may make more sense to propagate the error back up to push, so that it can present the usual status table and give a nicer message. But this is a much simpler fix that can help immediately.

While we're adding tests, let's also confirm that the remote-helper dying is also detect when importing refs. We currently do so robustly when the helper uses the "done" feature (and that is what we test). We cannot do so reliably when the helper does not use the "done" feature, but it is not even worth testing; the right solution is for the helper to start using "done".

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Felipe,

Can you acknowledge that it's OK to stick your name on this, as it's not exactly what you submitted before?

 git-remote-testgit        | 19 +++++++++++++++++++
 t/t5801-remote-helpers.sh | 20 ++++++++++++++++++++
 transport-helper.c        |  2 +-
 3 files changed, 40 insertions(+), 1 deletion(-)

On Wed, Apr 10, 2013 at 2:15 PM, Jeff King <peff@peff.net> wrote:

The fixes you made to this patch make a lot of sense, glad to not have a 'sleep 1' in our tests.

Acked-by: Sverre Rabbelier <srabbelier@gmail.com>

-- Cheers,

Sverre Rabbelier

On Wed, Apr 10, 2013 at 5:15 PM, Jeff King <peff@peff.net> wrote:

s/detect/detected/

On Wed, Apr 10, 2013 at 4:15 PM, Jeff King <peff@peff.net> wrote:

This explained the same thing:

Granted, depending on the way the remote-helper died, an error might or might not been printed, so s/won't/might not/.

The fact that an exit code was returned before is not relevant, neither is how the exit was returned, and for that matter neither is all the other things that are happening in this code. It's just noise.

The only thing that is relevant is this:

-               exit(128);
+               die("Reading from remote helper failed");

It's a simple change, and simple to explain.

Yes it might, and it might make sense to rewrite much of this code, but that's not relevant.

That is enough explanation.

This doesn't help anyone, and it's not even accurate. I think it might be possible enforce remote-helpers to implement the "done" feature, and we might want to do that later. But of course, discussing what bad things remote-helpers could do, and how we should test and babysit them is not relevant here.

If it was important to explain the subtleties and reasoning behind this change, it should be a separate patch.

I would add:

[jk: rewrote every piece of text]

I think this is explanation enough.

I don't understand what is being said here. What is "that case"?

LGTM.

Cheers.

-- Felipe Contreras

On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote:

I am OK with adding the test for import as a separate patch. What I am not OK with (and this goes for the rest of the commit message, too) is failing to explain any back-story at all for why the change is done in the way it is.

_You_ may understand it _right now_, but that is not the primary audience of the message. The primary audience is somebody else a year from now who is wondering why this patch was done the way it was. When they are trying to find out why git does not detect errors in a helper, and they notice that our test for failure only check the "done" case, isn't it more helpful to say "we considered the other case, but it was not worth fixing" rather than leaving them to guess?

I may be more verbose than necessary in some of my commit messages, but I would much rather err on the side of explaining too much than too little.

The case that fast-export gets SIGPIPE. I was trying to explain not just _what_ we are doing, but _why_ it is important. Perhaps a better wording would be:

  # consume input so fast-export doesn't get SIGPIPE;
  # we do not technically need to do so in order for
  # git to notice the failure to export, as it will
  # detect problems either with fast-export or with
  # the helper failing to report ref status. But since
  # we are trying to demonstrate that the latter
  # check works, we must avoid the SIGPIPE, which would
  # trigger the former.

-Peff

On Thu, Apr 11, 2013 at 11:18 AM, Jeff King <peff@peff.net> wrote:

Who would be this person? Somebody who wonders why this test is using "feature done"? I doubt such a person would exist, as using this feature is standard, as can be seen below this chunk. *If* the test was *not* using this "feature done", *then* sure, an explanation would be needed.

But why is this test doing something expected is not a question anybody would benefit from asking.

If you are worried about such hypothetical people, they would be better served by a comment in the source code of the test, or even better, the c file, or even better, to document that remote helpers should use this feature. But wait:

--- Just like 'push', a batch sequence of one or more 'import' is terminated with a blank line. For each batch of 'import', the remote helper should produce a fast-import stream terminated by a 'done' command. ---

So it's already explained, if somebody fails to follow this documentation, it's dubious a commit message that introduces a test would help. Surely, the writer of this bad remote helper would _never_ look there.

I wouldn't. The only thing an overload of information achieves is that the reader would simply skip or skim it.

If we are trying to avoid SIGPIPE wouldn't that imply that git notices the SIGPIPE?

# consume input so fast-export doesn't get SIGPIPE; we want to test the remote-helper's code after fast-export.

-- Felipe Contreras

On Thu, Apr 11, 2013 at 11:49:11AM -0500, Felipe Contreras wrote:

If it was so obvious, why did your initial patch not use "feature done"? If it was so obvious, why did our email discussion go back and forth so many times before arriving at this patch?

It was certainly not obvious to me when this email thread started. So in response to your question: *I* am that person. I was him two weeks ago, and there is a good chance that I will be him a year from now. Much of my work on git is spent tracking down bugs in older code, and those commit messages are extremely valuable to me in understanding what happened at the time.

But I give up on you. I find most of your commit messages lacking in details and motivation, making assumptions that the reader is as familiar with the code when reading the commit as you are when you wrote it. I tried to help by suggesting in review that you elaborate. That didn't work. So I tried to help by writing the text myself. But clearly I am not going to convince you that it is valuable, even if it requires no work at all from you, so I have nothing else to say on the matter.

-Peff

On Thu, Apr 11, 2013 at 11:59 AM, Jeff King <peff@peff.net> wrote:

Because I didn't want to test the obvious, I wanted to test something else.

This patch has absolutely nothing to do with that, in fact, forget about it, such a minor check is not worth this time and effort: http://article.gmane.org/gmane.comp.version-control.git/220899

No, you are not. I didn't send a patch with "feature done" originally, the only reason you wondered about the patch with "feature done" is that you saw one without it. It will _never_ happen again.

Lets make a bet. Let's push the simpler version, and when you hit this commit message retrospectively and find that you don't understand what is happening, I loose, and I will forever accept verbose commit messages. It will never happen.

Me neither. I picked your solution, but that's not enough, you *always* want me to do EXACTLY what you want, and never argue back.

It's not going to happen. There's nothing wrong with disagreeing.

Cheers.

-- Felipe Contreras

Felipe Contreras <felipe.contreras@gmail.com> writes:

Heh, it seems that I was late for the party.

Writing only minimally sufficient in the log messages is fine for your own project. We won't decide nor dictate the policy for your project for you.

But _this_ project wants its log messages to be understandable by people who you may disagree with and who may have shorter memory span than you do. Disagreeing with that policy is fine. You need to learn to disagree but accept to be part of the project.

Thanks.

On Thu, Apr 11, 2013 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:

Having a shorter memory span is irrelevant when you are _never_ going to go back and ask the question the commit message is answering. And if it indeed is an important question, the answer belongs in the code comments.

Yeah, I accept that you will commit whatever you want, but I still don't think this verbosity serves the purpose you think it serves. Some one-liners deserve pages of commit messages, but this one is not one of them. People are easily deceived, and because you saw one commit message that needed more information, you think all of them do, but no, some don't don't, and this is one of them. It's not serving any real purpose.

-- Felipe Contreras

Felipe Contreras <felipe.contreras@gmail.com> writes:

I agree with you that s/does not see/may not see/ would be more helpful here, so I'll squash it in while queuing.

It is a good reminder for people who later inspect this part of the code and wonder if it was a conscious design choice not to propagate the error or just being "simple and sufficient for now", I think. It would help them by making it clear that it is the latter, no?

Yes, all these are possible and I think writing it down explicitly will serve as a reminder for our future selves, I think.

In my first reading, it felt to me that it was natural to interpret that this is "even if we didn't have this loop that avoids killing fast-export with SIGPIPE, we would notice death of fast-export by SIGPIPE".

On Thu, Apr 11, 2013 at 1:44 PM, Junio C Hamano <gitster@pobox.com> wrote:

No. Design choices is what code comments are for, of which Git only has too few, according to ohloh[1]. No wonder they are so few, people are spending time writing novels on commit messages and forgetting there's also code where you should clarify things.

Yes, but not writing them here. By spending so much time in commit messages you neglect the code, and the wiki (which is actually the place to write these things on.

And if all you want is to write them down, we already did, right here. There's no need to punish the readers of the commit messages in the future only so we can flex our memory, because we already did.

And if you must, you might was well label them with "REMINDER", no, wait, that's what "TODO" comments are for, where people can see them, and not *forget* them.

Cheers.

[1] https://www.ohloh.net/p/git/factoids#FactoidCommentsLow

-- Felipe Contreras

Felipe Contreras <felipe.contreras@gmail.com> writes:

Yeah, good point.

On Thu, Apr 11, 2013 at 6:05 PM, Junio C Hamano <gitster@pobox.com> wrote:

Moreover, I think there's a clear double standard. Consider this commit:

commit 99d3206010ba1fcc9311cbe8376c0b5e78f4a136
Author: Antoine Pelisse <apelisse@gmail.com>
Date:   Sat Mar 23 18:23:28 2013 +0100

    combine-diff: coalesce lost lines optimally

    This replaces the greedy implementation to coalesce lost lines by using
    dynamic programming to find the Longest Common Subsequence.

    The O(n²) time complexity is obviously bigger than previous
    implementation but it can produce shorter diff results (and most likely
    easier to read).

    List of lost lines is now doubly-linked because we reverse-read it when
    reading the direction matrix.

The commit message is 9 lines, and the diffstat 320 insertions(+), 64 deletions(-). Moreover, there are some important bits of information on the mailing list that never made it to the commit message:

--- Best-case analysis: All p parents have the same n lines. We will find LCS and provide a n lines (the same lines) new list in O(n²), and then run it again in O(n²) with the next parent, etc. It will end-up being O(pn²).

Worst-case analysis: All p parents have no lines in common. We will find LCS and provide a 2n new list in O(n²). Then we run it again in O(2n x n), and again O(3n x n), etc, until O(pn x n). When we sum these all, we end-up with O(p² x n²) ---

--- Unfortunately on a commit that would remove A LOT of lines (10000) from 7 parents, the times goes from 0.01s to 1.5s... I'm pretty sure that scenario is quite uncommon though. ---

This is not mentioned in the commit message; on which situations this implementation would be worst and why it's OK either way.

--- As you can see the last test is broken because the solution is not optimal for more than two parents. It would probably require to extend the dynamic programming to a k-dimension matrix (for k parents) but the result would end-up being O(n^k) (when removing n consecutives lines from p parents). I'm not sure there is any better solution known yet to the k-LCS problem. Implementing the dynamic solution with the k-dimension matrix would probably require to re-hash the strings (I guess it's already done by xdiff), as the number of string comparisons would increase. ---

The fact that the last test is broken is not mentioned at all.

Now let's compare to the final version of my patch which is 19 lines 40 insertions(+), 1 deletion(-). The ration of commit message lines vs. code changed lines is 19/41(0.46) whereas Antoine's patch is 3/128(0.02), a difference of over 19 times. Granted, some single-line changes do require a good chunk of explanation, but this is not one of them; this single line patch doesn't even change the behavior of the code, simply changes a silent error exit to a verbose error exit, that's all. Antoine's patch has a lot more potential to trigger something unexpected.

And the chances that somebody would have to look at Antoine's patch is quite high, especially since a failing test-case is introduced. The chances that anybody would look at mine are very very low.

So either Antoine's commit message was fine, and so was mine, or it was sorely lacking explanation.

To me, the reality is obvious: my patch didn't require such a big commit message, the short version was fine, the only reason Jeff King insisted on a longer version is because the patch came from me. Antoine's patch might have benefited from a little more explanation, but not every issue that was discussed in the mailing list was necessary (in my patch virtually every issue discussed was added to the commit message).

This is the definition of double standard.

Cheers.

On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote:

Get over yourself. The reason I suggested a longer commit message for your commit is because after spending several hours figuring out what the current code did, and what it should be doing instead, I wanted to document that effort so that I and other readers did not have to do it again later. I didn't even review the other patch you mention, so I could not possibly have come to the same point with it.

But hey, if you want to have paranoid fantasies that I'm persecuting you (by writing the longer commit messages for you!), go ahead.

If you don't want me to review your patches, that's fine by me, too; our discussions often end up frustrating, and it's clear we do not agree on very much with respect to process or design. But if you don't want that, please stop cc'ing me when you send out the patches.

-Peff

On Sat, Apr 13, 2013 at 1:00 AM, Jeff King <peff@peff.net> wrote:

The double standard might not come from you, perhaps you subject all the patches you review to the same standard, it comes from the fact that the patches you review have an unfair disadvantage.

You don't persecute me, you persecute my patches. I could almost picture the moment you see a patch is coming from me, you have already decided to rewrite the commit message, even before reading it. Antoine is not me, so you simply didn't review that patch.

This comment was directed towards Junio, I do hope he is able to see the double standard. As for you, I think your reviews have value, but I also think you dwelling in irrelevant details do slow things down, which is not too bad, what is bad is that you assume that your opinions are facts (e.g. the commit message need to be bigger), and get angry when somebody disagrees with them.

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

There are reviewers who share the basic values [*1*] with I and the tradition of this project and whose judgement I can trust.

When somebody (like Peff) whose judgement I trust spends time to review a series, and writes his thought process to the degree that he thinks is appropriate, that's his judgement and I trust it.

You cannot expect perfect evenness from multiple people. For that matter, you cannot expect perfect evenness even from a single person, either.

When reviewing a proposed change to an area that I am intimately familiar with, I may immediately know some subtleties involved in a proposed solution without even reading the patch, and I may not even realize that such subtleties are hard to know without being somebody who are already familiar with that part of the codebase, and either in-code comment or in the log message may better spelled them out. On the other hand, when the change is in another area that I am not familiar with, I may request more explanation, if only for me to understand the issue.

I try to avoid the "I may know too well" pitfalls, but I am not perfect. I will not speak for Peff or any other reviewers whose jugement I trust, but I would be very surprised if any of them claimed he is perfectly even.

"Double" may only be showing that we do not have enough trusted maintainers; ideally I would like it to have "Triple" or more.

[Footnote]

*1* A few examples of core values.

 - we should make sure that future developers who wonder why a part
   of the code is how it is can find out what thought process
   brought the code into the current shape.

 - when we add something, we try not to overengineer or to shoot for
   unattainable perfection, but we still try to make sure we will
   not paint ourselves into an unescapable corner when we later want
   to extend it.

On Sun, Apr 14, 2013 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:

A double or triple review raises *a single* standard higher, but having more than one standard is never good. But apparently your trust in Jeff means more than caring about the double standard.

But good to know, my patches will always have an unfair disadvantages to everybody else's.

And there is such a thing as being too cautious, to the point where you walk WAY too slowly for the fear of tumbling, because it happened a few times in the fast, when you were running. But how would you even know that you are being too cautions? If you don't dare to walk a bit faster to find out.

Cheers.

When we try to read from a remote-helper and get EOF or an error, we print a message indicating that the helper died. However, users may not know that a remote helper was in use (e.g., when using git-over-http), or even what a remote helper is.

Let's print the name of the helper (e.g., "git-remote-https"); this makes it more obvious what the program is for, and provides a useful token for reporting bugs or searching for more information (e.g., in manpages).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5801-remote-helpers.sh | 2 +-
 transport-helper.c        | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

On Wed, Apr 10, 2013 at 2:16 PM, Jeff King <peff@peff.net> wrote:

Better than nothing:

Acked-by: Sverre Rabbelier <srabbelier@gmail.com>

-- Cheers,

Sverre Rabbelier

On Wed, Apr 10, 2013 at 02:23:56PM -0700, Sverre Rabbelier wrote:

Now that's the kind of whole-hearted endorsement I strive for. :)

If you have better wording, I'm open to it. I do note that we don't actually have a manpage for "git-remote-https", though we do for others. Probably "man git-remote-helpers" is the most sensible thing to point the user to. But I don't even think this is worthy of a big advice message. It's a bug in the helper, it shouldn't really happen, and giving the user a token they can use to report or google for the error is probably good enough.

-Peff

On Wed, Apr 10, 2013 at 2:28 PM, Jeff King <peff@peff.net> wrote:

It's nothing wrong with your patch, the main problem is that there's not really a good place to point users at.

Yeah, exactly. man git-remote-helpers is more a place for developers to read how to implement a git-remote-helper, not so much a place for users to read what they are, and/or how to use them.

-- Cheers,

Sverre Rabbelier

On Mon, 8 Apr 2013 09:40:04 -0500 Felipe Contreras <felipe.contreras@gmail.com> wrote:

....

Do I read this correctly? If I'm in debug mode the remote helper quit but if not in debug mode it failed? Debuggers never fail they only quit!

On Wed, Apr 10, 2013 at 04:13:20PM -0700, rh wrote:

In debug mode, it prints both messages. The debug version is superfluous at this point, though, and we can probably just drop it.

-Peff