Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
Jeff King writes: > On Mon, Apr 08, 2013 at 02:08:03PM -0700, Junio C Hamano wrote: > >> OK, so you are envisioning that transport-helper would read from the >> helper after importer is done? If so, perhaps it is a prudent >> solution to disconnect in this version (to fix), and then in a >> separate patch that adds such an extension (I imagine it would >> involve that the helper advertising a capability or being invoked >> with an option to let transport-helper somehow know that it should >> continue the conversation once fast-import is done) to disable the >> disconnect here when that extension is in use? > > At this point, I am of the opinion that it's OK to just do nothing; > modern helpers should be using the "done" flag, and if they aren't, then > that is the right place for the fix. Then we don't have to worry about > any side effects of disconnecting, or adding a new capability flag. Sounds sane. Thanks for unconfusing me ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
On Mon, Apr 08, 2013 at 02:08:03PM -0700, Junio C Hamano wrote: > OK, so you are envisioning that transport-helper would read from the > helper after importer is done? If so, perhaps it is a prudent > solution to disconnect in this version (to fix), and then in a > separate patch that adds such an extension (I imagine it would > involve that the helper advertising a capability or being invoked > with an option to let transport-helper somehow know that it should > continue the conversation once fast-import is done) to disable the > disconnect here when that extension is in use? At this point, I am of the opinion that it's OK to just do nothing; modern helpers should be using the "done" flag, and if they aren't, then that is the right place for the fix. Then we don't have to worry about any side effects of disconnecting, or adding a new capability flag. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
Felipe Contreras writes: > On Mon, Apr 8, 2013 at 1:46 PM, Junio C Hamano wrote: > ... But if we keep helper running, who will be communicating with it via these open pipes? The process that is calling finish_command() on fast-import and disconnecting from the helper won't be, as read/write to the pipe, even if we do not disconnect from here, will result in errors if the helper has already exited at this point. >>> >>> Nobody will send any further input, but in theory we could redirect >>> the pipe and send more commands. That's how it was designed. >> >> Who does the redirection to whom? > > The one that is doing all the redirections, transport-helper. > >> How would the process tree and >> piping constructed around the current system? > > I cannot parse that correctly, Sorry, s/the current system/& look like/; > but transport-helper is already > receiving the output from the remote-helper. OK, so you are envisioning that transport-helper would read from the helper after importer is done? If so, perhaps it is a prudent solution to disconnect in this version (to fix), and then in a separate patch that adds such an extension (I imagine it would involve that the helper advertising a capability or being invoked with an option to let transport-helper somehow know that it should continue the conversation once fast-import is done) to disable the disconnect here when that extension is in use? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
On Mon, Apr 08, 2013 at 09:38:28AM -0500, Felipe Contreras wrote: > > Nowhere does it say that we should not check fast-import's exit value, > > and indeed, the patch does not replace that check at all. It comes > > immediately after. It replaces the WNOHANG check of the helper's exit > > code (i.e., check_command) with a synchronous check. > > Yeah, and in the process it's changing the design of transport-helper, > where the pipes are closed only at the very end. Yes. And I tried to explain why this is acceptable in the "comprehensive essay" of a comment. If you think there are reasons not to do so, please elaborate. But talking about fast-import's exit value is irrelevant. > > I don't see your point. If a helper reports success when it has failed, > > and doesn't feed anything to fast-import, we can do nothing. That case > > is indistinguishable from a helper which has nothing to fetch from the > > remote, and exits. My patch doesn't help with such a broken helper, but > > neither does yours, because they check the exact same thing. > > No, my code checks that the remote-helper is *still running*, you code > doesn't. And after all, that is the design of transport-helper. I see. That would have been a very helpful thing to put in the commit message, no? But the issue still remains: a "still running" check is subject to race conditions. A disconnect solution should fail not only on a failed exit code, but also if we are unable to send the disconnect command (i.e., it should be checking EPIPE rather than ignoring it). So my solution was not sufficient, either. But I think it is moot, as I agree with your statements below and the follow-up patch. > >> The done-import capability *is already there*. The remote helper fails > >> to say "done", fast-import detects that there was a problem and exits > >> with -1 (or whatever), but it doesn't matter, because we > >> (transport-helper) are oblivious of that return status until it's too > >> late. > > > > Wait, then why is this a problem at all? We check the exit code of > > fast-import right before the code in question. > > It's not, *If* the remote-helper is using "done", and a small one if it's not. Something like this might have been helpful in the commit message to make it more clear what the point of the patch is: If the remote-helper is using the "done" feature in the export stream, then it will signal to fast-import that it correctly output all of the data to the stream. We can trust that the exit code of fast-import will report such a problem to it, and do not have to worry. But for helpers which do not use the "done" feature and exit early, fast-import has no idea whether all of the data was written or not. The helper does not report any status to us, so we are left to guess at whether it worked successfully. We can make such a guess by seeing whether the process is still alive. This does not cover all cases, as it is possible for us to see the process still present just before it exits. However, it covers the common case that the helper dies with an error, closing the pipe to fast-import at the same time. Since we reap fast-import first, by the time we check whether the helper is still alive, it will have exited, too. That may miss some cases, but it is the best we can do for such a helper, given the current protocol. A remote helper which wants to be robust should use the "done" feature. Maybe you thought about all of those issues. Maybe you even communicated them somewhere on the list. But I did not see them in the commit message nor in a comment, which is the main item I look at while reviewing. If you did not realize it until after the discussion, that is fine, too. That is why we have the discussions. Now we can perhaps improve the patch (either by explaining there what is happening, or deciding that this case is not worth caring about). > > Why is your patch to recvline so much more complicated than my > > one-liner? Why do you go to lengths to pass out an error code from it > > just so that we can end up dying anyway? > > Because I think dying inside a generic function is bad design. I think > the error should be returned, and let the caller decide what to do > with it. We already die inside the function; my patch only added a message. If you want to change the dying, fine, but that cleanup is a separate issue. > In the current version of the code we seem to receive lines > only from the remote-helper, so I guess your patch is fine and works. > But I still think the current code is not good. It's a static inside transport-helper.c. So I think it is OK for it to assume that the transport-helper code will be the only caller. In such a situation, it _may_ be worth changing later if it becomes a more generic and widely accessible function, but I do not think that is the case here. The function does nothing except report remote-helper debugging information and ask strbuf_getline to do the heavy li
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
On Mon, Apr 8, 2013 at 1:46 PM, Junio C Hamano wrote: >>> ... But if we keep >>> helper running, who will be communicating with it via these open >>> pipes? The process that is calling finish_command() on fast-import >>> and disconnecting from the helper won't be, as read/write to the >>> pipe, even if we do not disconnect from here, will result in errors >>> if the helper has already exited at this point. >> >> Nobody will send any further input, but in theory we could redirect >> the pipe and send more commands. That's how it was designed. > > Who does the redirection to whom? The one that is doing all the redirections, transport-helper. > How would the process tree and > piping constructed around the current system? I cannot parse that correctly, but transport-helper is already receiving the output from the remote-helper. > I am not trying to say it is just theoretical mental exercise (which > I have seen you do not do at all on this list). I am trying to find > out what the practical use case is that you have in mind, because > disconnecting will prove to be not an improvement but a regression > for that use case. But you do not have to answer this question > directly, because... As the gitifyhg developers effusively pointed out, it's useful to see which mercurial revision corresponds to certain git revision, and this is best achieved through notes. Implementing this in the remote-helper doesn't make much sense (I won't go into details). >> And in fact, I'm thinking doing exactly that, so we can send another >> command to fetch the foreign commit ids and append notes with them. > > ...we will see the answer in that change anyway. That doesn't change the fact that transport-helper would end up with mixed and matched design. Maybe the use-case above wouldn't be prevented by this change, but I think further changes would need to be done to the file to not end up in this weird state. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
On Mon, Apr 08, 2013 at 01:31:43PM -0500, Felipe Contreras wrote: > > Reviewers have limited time allocated for each individual topic. To > > an original patch with say 3 problems, the review may only point out > > issues in 2 and suggest a concrete improvement for only 1 and that > > is sufficient to suggest a reroll. That is not being unhelpful by > > deliberately withholding 1 and half reviews in the initial round. > > I'm not talking about the time it took to come up with the criticism > below, I'm talking about the comment regarding the commit message. If > the commit message did indeed provide *zero* explanation, that's > certainly something that should be immediately visible, no? It could > have been mentioned six months ago. I do not recall this series at all from six months ago. I reviewed a series (with no version markers, nor any mention of the previous round) posted on April 1st, and assumed it was a new series. I raised the same issues there that I did in v3, though in less detail (because I had not yet looked into it as thoroughly). I searched the archive after reading this mail and found the original series. Yes, it existed. I didn't review it. I guess I had something more important to do that day. But is all of this even important? If there are technical issues with this series, does it matter when we find out about them? They still deserve to be fixed, no? Yes, it is nice if things get reviewed promptly, but the limited bandwidth and attention of reviewers is a fact of life. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
Felipe Contreras writes: > I'm not talking about the time it took to come up with the criticism > below, I'm talking about the comment regarding the commit message. If > the commit message did indeed provide *zero* explanation, that's > certainly something that should be immediately visible, no? It could > have been mentioned six months ago. Given that all of your log messages tend to be missing or terser than necessary, I wouldn't be surprised Peff stopped bothering about it. But now we know this needs to be better explained, and your v4 has better explanation that will help you avoid having to repeat yourself in the discussion, so let's not dwell on it too much. >> ... But if we keep >> helper running, who will be communicating with it via these open >> pipes? The process that is calling finish_command() on fast-import >> and disconnecting from the helper won't be, as read/write to the >> pipe, even if we do not disconnect from here, will result in errors >> if the helper has already exited at this point. > > Nobody will send any further input, but in theory we could redirect > the pipe and send more commands. That's how it was designed. Who does the redirection to whom? How would the process tree and piping constructed around the current system? I am not trying to say it is just theoretical mental exercise (which I have seen you do not do at all on this list). I am trying to find out what the practical use case is that you have in mind, because disconnecting will prove to be not an improvement but a regression for that use case. But you do not have to answer this question directly, because... > And in fact, I'm thinking doing exactly that, so we can send another > command to fetch the foreign commit ids and append notes with them. ...we will see the answer in that change anyway. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
On Mon, Apr 8, 2013 at 12:43 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> On Sun, Apr 7, 2013 at 9:33 PM, Jeff King wrote: >>> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote: >> And you think that is desirable? User-friendly? >>> >>> No, as you probably realized if you read the rest of my email. My point >>> in bringing it up was to try to explain exactly what is going on in each >>> case. Your commit message provided zero explanation of what the current >>> problem is, nor why your fix is the right thing to do. >> >> I have explained this many times, I wonder why when the patch is close >> to be merged does this become important, and not before. > > There are at least a few reasons I can think of off the top of my > head. > > Reviewers have limited time allocated for each individual topic. To > an original patch with say 3 problems, the review may only point out > issues in 2 and suggest a concrete improvement for only 1 and that > is sufficient to suggest a reroll. That is not being unhelpful by > deliberately withholding 1 and half reviews in the initial round. I'm not talking about the time it took to come up with the criticism below, I'm talking about the comment regarding the commit message. If the commit message did indeed provide *zero* explanation, that's certainly something that should be immediately visible, no? It could have been mentioned six months ago. That's a comprehensive essay, unfortunately, it's not correct. The exit status of the remote-helper is not important, it's the one of fast-import that we really care about. >>> >>> Nowhere does it say that we should not check fast-import's exit value, >>> and indeed, the patch does not replace that check at all. It comes >>> immediately after. It replaces the WNOHANG check of the helper's exit >>> code (i.e., check_command) with a synchronous check. >> >> Yeah, and in the process it's changing the design of transport-helper, >> where the pipes are closed only at the very end. > > I agree that the disconnection here closes the pipes a lot earlier > than we used to. But I am not sure what the practical consequences > of doing so are. We know the fast-import process has successfully > read everything from the helper (we called finish_command() on it). > We expect at this point the helper is still running or successfully > exited (the other alternative, the helper died with non-zero status, > is an error check_command() patch wanted to detect). But if we keep > helper running, who will be communicating with it via these open > pipes? The process that is calling finish_command() on fast-import > and disconnecting from the helper won't be, as read/write to the > pipe, even if we do not disconnect from here, will result in errors > if the helper has already exited at this point. Nobody will send any further input, but in theory we could redirect the pipe and send more commands. That's how it was designed. And in fact, I'm thinking doing exactly that, so we can send another command to fetch the foreign commit ids and append notes with them. > What I am trying to get at is if "changing the design", which I > agree is a change, is an improvement, or a backward incompatible > possible breakage. It's an improvement I guess, but it's "changing the design" only in one part of the code, while the rest follows a different design. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
Felipe Contreras writes: > On Sun, Apr 7, 2013 at 9:33 PM, Jeff King wrote: >> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote: > >>> And you think that is desirable? User-friendly? >> >> No, as you probably realized if you read the rest of my email. My point >> in bringing it up was to try to explain exactly what is going on in each >> case. Your commit message provided zero explanation of what the current >> problem is, nor why your fix is the right thing to do. > > I have explained this many times, I wonder why when the patch is close > to be merged does this become important, and not before. There are at least a few reasons I can think of off the top of my head. Reviewers have limited time allocated for each individual topic. To an original patch with say 3 problems, the review may only point out issues in 2 and suggest a concrete improvement for only 1 and that is sufficient to suggest a reroll. That is not being unhelpful by deliberately withholding 1 and half reviews in the initial round. Reviewers will see the same patch with fresh eyes after 6 months and will notice different issues. That is not being unhelpful by deliberately withholding a crucial point in the initial round of the review. I would not be surprised if one ends up repeating oneself in multiple review threads; the log message of a rerolled patch is a better place to avoid the need for the repetition. >>> That's a comprehensive essay, unfortunately, it's not correct. The >>> exit status of the remote-helper is not important, it's the one of >>> fast-import that we really care about. >> >> Nowhere does it say that we should not check fast-import's exit value, >> and indeed, the patch does not replace that check at all. It comes >> immediately after. It replaces the WNOHANG check of the helper's exit >> code (i.e., check_command) with a synchronous check. > > Yeah, and in the process it's changing the design of transport-helper, > where the pipes are closed only at the very end. I agree that the disconnection here closes the pipes a lot earlier than we used to. But I am not sure what the practical consequences of doing so are. We know the fast-import process has successfully read everything from the helper (we called finish_command() on it). We expect at this point the helper is still running or successfully exited (the other alternative, the helper died with non-zero status, is an error check_command() patch wanted to detect). But if we keep helper running, who will be communicating with it via these open pipes? The process that is calling finish_command() on fast-import and disconnecting from the helper won't be, as read/write to the pipe, even if we do not disconnect from here, will result in errors if the helper has already exited at this point. What I am trying to get at is if "changing the design", which I agree is a change, is an improvement, or a backward incompatible possible breakage. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
On Sun, Apr 7, 2013 at 9:33 PM, Jeff King wrote:
> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:
>> And you think that is desirable? User-friendly?
>
> No, as you probably realized if you read the rest of my email. My point
> in bringing it up was to try to explain exactly what is going on in each
> case. Your commit message provided zero explanation of what the current
> problem is, nor why your fix is the right thing to do.
I have explained this many times, I wonder why when the patch is close
to be merged does this become important, and not before.
>> That's a comprehensive essay, unfortunately, it's not correct. The
>> exit status of the remote-helper is not important, it's the one of
>> fast-import that we really care about.
>
> Nowhere does it say that we should not check fast-import's exit value,
> and indeed, the patch does not replace that check at all. It comes
> immediately after. It replaces the WNOHANG check of the helper's exit
> code (i.e., check_command) with a synchronous check.
Yeah, and in the process it's changing the design of transport-helper,
where the pipes are closed only at the very end.
> The argument that the exit status of remote-helper is not important
> would mean your series is pointless, too, no?
The main point of my patch was not to add a check for import, but for
export. Adding such a check here is not important, but it doesn't hurt
either, as it would improve the situation if the remote-helper is not
using the "done" feature, which at the moment is a possibility.
>> > + if (disconnect_helper(transport) != 0)
>> > die("Error while running helper");
>>
>> Yeah, that's good, *if* the remote-helper is implemented correctly,
>> but try this:
>>
>>if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
>>then
>>exit 0
>>fi
>
> I don't see your point. If a helper reports success when it has failed,
> and doesn't feed anything to fast-import, we can do nothing. That case
> is indistinguishable from a helper which has nothing to fetch from the
> remote, and exits. My patch doesn't help with such a broken helper, but
> neither does yours, because they check the exact same thing.
No, my code checks that the remote-helper is *still running*, you code
doesn't. And after all, that is the design of transport-helper.
>> > argv_array_free_detached(fastimport.argv);
>> >
>> > which passes both your original test and the more strict one above. Of
>> > course adding a done-import capability would be nice to fix the protocol
>> > deficiency, but it is more code.
>>
>> The done-import capability *is already there*. The remote helper fails
>> to say "done", fast-import detects that there was a problem and exits
>> with -1 (or whatever), but it doesn't matter, because we
>> (transport-helper) are oblivious of that return status until it's too
>> late.
>
> Wait, then why is this a problem at all? We check the exit code of
> fast-import right before the code in question.
It's not, *If* the remote-helper is using "done", and a small one if it's not.
>> Yeah, I already explored this option, and I said it was possible on
>> pushing, but now the problem is fetching.
>>
>> http://article.gmane.org/gmane.comp.version-control.git/219760
>>
>> And to be frank, I'm tired of this. I keep repeating the same things
>> over and over, when I ask for input on different ways to achieve this
>> I get nothing, and when the patch is getting closer to be merged, then
>> I receive criticism, only so I can repeat the same again.
>
> I don't even know what to say to this. I reviewed your original patch,
> raised some concerns. You responded. I had not yet responded back,
> because I have many other things to work on. In the meantime you
> submitted another round. Now I have the opportunity to respond and did
> so, having read both threads.
I sent the first version of this patch series on October of last year.
> Why is your patch to recvline so much more complicated than my
> one-liner? Why do you go to lengths to pass out an error code from it
> just so that we can end up dying anyway?
Because I think dying inside a generic function is bad design. I think
the error should be returned, and let the caller decide what to do
with it. In the current version of the code we seem to receive lines
only from the remote-helper, so I guess your patch is fine and works.
But I still think the current code is not good.
> Why do you need a done
> capability for push when the blank line already signals the end of the
> status?
I suppose we don't.
> I did not respond to 219760 separately because those are all questions
> that _don't matter_ if you follow the analysis in my last email that
> lead to my one-liner. Maybe there are interesting answers to those
> questions, and my one-liner isn't sufficient. But I'll never know
> because instead of pointing them out, you are complaining that I didn't
> respond fast eno
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:
> > I'm still not very excited about this approach, as it is racy to detect
> > errors. E.g., there is nothing to say that the helper does not close the
> > pipe to fast-import prematurely and then die afterwards, leaving the
> > refs unwritten, fast-import happy, but a failed exit code from the
> > helper.
>
> That's right, and that would leave us in the situation we are right
> now, even with "done" check.
Yes. I do not think your patch makes anything worse. But it also does
not fix the problem completely.
> > The import test still passes even without the check_command part of your
> > patch because some of the refs in refs/testgit do not exist prior to the
> > failed fetch, and therefore also do not exist afterwards. When fetch
> > tries to feed their sha1s to check_everything_connected, you get the
> > funny:
> >
> > fatal: bad object
> > error: testgit::/home/peff/compile/git/t/trash
> >directory.t5801-remote-helpers/server did not send all necessary
> >objects
>
> And you think that is desirable? User-friendly?
No, as you probably realized if you read the rest of my email. My point
in bringing it up was to try to explain exactly what is going on in each
case. Your commit message provided zero explanation of what the current
problem is, nor why your fix is the right thing to do.
> > - if (!check_command(data->helper))
> > + /*
> > +* We must disconnect from the helper at this point, because even
> > +* though fast-import may have succeeded, it may only be because the
> > +* helper was not able to feed fast-import all of the data, and what
> > +* fast-import got looked OK (e.g., it may have got nothing if the
> > +* helper died early). We still need to check the return code of the
> > +* helper to make sure it is happy with what it sent.
> > +*
> > +* Since the import command does not require the helper to ever
> > report
> > +* success/failure of the import, we have no mechanism to check for
> > +* problems except to check its exit status.
> > +*
> > +* Callers of the transport code are allowed to make more requests
> > +* of our helper, so we may be disconnecting before they expect in
> > that
> > +* case. However:
> > +*
> > +* 1. Current callers don't do that; after fetching refs, there
> > +* is nothing left for the helper to do.
> > +*
> > +* 2. We transparently start the helper as necessary, so if we
> > were
> > +* to get another request (e.g., to import more refs), we would
> > +* simply start a new instantiation of the helper.
> > +*
> > +*/
>
> That's a comprehensive essay, unfortunately, it's not correct. The
> exit status of the remote-helper is not important, it's the one of
> fast-import that we really care about.
Nowhere does it say that we should not check fast-import's exit value,
and indeed, the patch does not replace that check at all. It comes
immediately after. It replaces the WNOHANG check of the helper's exit
code (i.e., check_command) with a synchronous check.
The argument that the exit status of remote-helper is not important
would mean your series is pointless, too, no?
> > + if (disconnect_helper(transport) != 0)
> > die("Error while running helper");
>
> Yeah, that's good, *if* the remote-helper is implemented correctly,
> but try this:
>
>if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
>then
>exit 0
>fi
I don't see your point. If a helper reports success when it has failed,
and doesn't feed anything to fast-import, we can do nothing. That case
is indistinguishable from a helper which has nothing to fetch from the
remote, and exits. My patch doesn't help with such a broken helper, but
neither does yours, because they check the exact same thing.
> > argv_array_free_detached(fastimport.argv);
> >
> > which passes both your original test and the more strict one above. Of
> > course adding a done-import capability would be nice to fix the protocol
> > deficiency, but it is more code.
>
> The done-import capability *is already there*. The remote helper fails
> to say "done", fast-import detects that there was a problem and exits
> with -1 (or whatever), but it doesn't matter, because we
> (transport-helper) are oblivious of that return status until it's too
> late.
Wait, then why is this a problem at all? We check the exit code of
fast-import right before the code in question.
> Yeah, I already explored this option, and I said it was possible on
> pushing, but now the problem is fetching.
>
> http://article.gmane.org/gmane.comp.version-control.git/219760
>
> And to be frank, I'm tired of this. I keep repeating the same things
> over an
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
On Sun, Apr 7, 2013 at 7:51 PM, Jeff King wrote:
> On Sun, Apr 07, 2013 at 01:45:06AM -0600, Felipe Contreras wrote:
>
>> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
>> index f387027..efe67e2 100755
>> --- a/t/t5801-remote-helpers.sh
>> +++ b/t/t5801-remote-helpers.sh
>> @@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' '
>> compare_refs local dup server dup
>> '
>>
>> +test_expect_success 'proper failure checks for fetching' '
>> + (GIT_REMOTE_TESTGIT_FAILURE=1 &&
>> + export GIT_REMOTE_TESTGIT_FAILURE &&
>> + cd local &&
>> + test_must_fail git fetch 2> error &&
>> + grep "Error while running helper" error
>> + )
>> +'
>
> Hmm. If you drop the final "grep" (which is looking for the error
> message you add elsewhere in this patch), this test passes even without
> the addition of the check_command calls added by your patch. Which made
> me wonder if we should be checking something else (repo state, error
> messages, etc), since we seem to otherwise be detecting the error. More
> analysis below on exactly what is going on there.
It fails all right, because the fetch process assumes the
remote-helper succeeded and only later eventually fails with an error
that is anything but user-friendly:
fatal: bad object
error: testgit::/home/felipec/dev/git/t/trash
directory.t5801-remote-helpers/server did not send all necessary
objects
>> +# We sleep to give fast-export a chance to catch the SIGPIPE
>
> I'm not sure what this means; I don't see a sleep anywhere.
That's right, I'll remove the comment. It was cruft from the next
patch (see below).
>> +test_expect_failure 'proper failure checks for pushing' '
>> + (GIT_REMOTE_TESTGIT_FAILURE=1 &&
>> + export GIT_REMOTE_TESTGIT_FAILURE &&
>> + cd local &&
>> + test_must_fail git push --all 2> error &&
>> + grep "Error while running helper" error
>> + )
>> +'
>
> I wondered why this one is marked for failure.
Because it fails to report the right error.
> Even without check_command, the push produces:
>
> error: fast-export died of signal 13
> fatal: Error while running fast-export
>
> which explains why the test fails: it does not even make it to the
> check_command call at all. Why do we need check_command here, then? Is
> it racy/non-deterministic for fast-export to die due to the pipe?
It does not make it to check_command because the wait is missing. I
added that in a separate patch:
http://article.gmane.org/gmane.comp.version-control.git/219715
With that patch the check_command happens reliably, the error is
reported correctly, and the test passes. However, the way to trigger
it is ugly.
In a real use-case the chances of check_command catching the issue are
much higher..
>> diff --git a/transport-helper.c b/transport-helper.c
>> index cb3ef7d..dfdfa7a 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport
>> *transport,
>>
>> if (finish_command(&fastimport))
>> die("Error while running fast-import");
>> +
>> + if (!check_command(data->helper))
>> + die("Error while running helper");
>> +
>> argv_array_free_detached(fastimport.argv);
>
> I'm still not very excited about this approach, as it is racy to detect
> errors. E.g., there is nothing to say that the helper does not close the
> pipe to fast-import prematurely and then die afterwards, leaving the
> refs unwritten, fast-import happy, but a failed exit code from the
> helper.
That's right, and that would leave us in the situation we are right
now, even with "done" check.
> The import test still passes even without the check_command part of your
> patch because some of the refs in refs/testgit do not exist prior to the
> failed fetch, and therefore also do not exist afterwards. When fetch
> tries to feed their sha1s to check_everything_connected, you get the
> funny:
>
> fatal: bad object
> error: testgit::/home/peff/compile/git/t/trash
>directory.t5801-remote-helpers/server did not send all necessary
>objects
And you think that is desirable? User-friendly?
> we can see that without your check_command, the failure in the second
> fetch is not noticed. Adding in your patch does detect this. _But_ it is
> only checking a specific failure mode of the remote-helper: process
> death that results in closing the fast-import pipe, which is how your
> GIT_REMOTE_TESTGIT_FAILURE is implemented (closing the pipe first and
> then dying is racy).
>
> If we then add this on top of your series (fixing the "status" bug in
> patch 1 that Junio mentioned), the test will start failing (both the
> original, and the more robust one I showed above):
>
> diff --git a/git-remote-testgit b/git-remote-testgit
> index ca0cf09..41b0780 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -64,6 +64,11 @@
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
On Sun, Apr 07, 2013 at 01:45:06AM -0600, Felipe Contreras wrote:
> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
> index f387027..efe67e2 100755
> --- a/t/t5801-remote-helpers.sh
> +++ b/t/t5801-remote-helpers.sh
> @@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' '
> compare_refs local dup server dup
> '
>
> +test_expect_success 'proper failure checks for fetching' '
> + (GIT_REMOTE_TESTGIT_FAILURE=1 &&
> + export GIT_REMOTE_TESTGIT_FAILURE &&
> + cd local &&
> + test_must_fail git fetch 2> error &&
> + grep "Error while running helper" error
> + )
> +'
Hmm. If you drop the final "grep" (which is looking for the error
message you add elsewhere in this patch), this test passes even without
the addition of the check_command calls added by your patch. Which made
me wonder if we should be checking something else (repo state, error
messages, etc), since we seem to otherwise be detecting the error. More
analysis below on exactly what is going on there.
> +# We sleep to give fast-export a chance to catch the SIGPIPE
I'm not sure what this means; I don't see a sleep anywhere.
> +test_expect_failure 'proper failure checks for pushing' '
> + (GIT_REMOTE_TESTGIT_FAILURE=1 &&
> + export GIT_REMOTE_TESTGIT_FAILURE &&
> + cd local &&
> + test_must_fail git push --all 2> error &&
> + grep "Error while running helper" error
> + )
> +'
I wondered why this one is marked for failure.
Even without check_command, the push produces:
error: fast-export died of signal 13
fatal: Error while running fast-export
which explains why the test fails: it does not even make it to the
check_command call at all. Why do we need check_command here, then? Is
it racy/non-deterministic for fast-export to die due to the pipe?
> diff --git a/transport-helper.c b/transport-helper.c
> index cb3ef7d..dfdfa7a 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,
>
> if (finish_command(&fastimport))
> die("Error while running fast-import");
> +
> + if (!check_command(data->helper))
> + die("Error while running helper");
> +
> argv_array_free_detached(fastimport.argv);
I'm still not very excited about this approach, as it is racy to detect
errors. E.g., there is nothing to say that the helper does not close the
pipe to fast-import prematurely and then die afterwards, leaving the
refs unwritten, fast-import happy, but a failed exit code from the
helper.
The import test still passes even without the check_command part of your
patch because some of the refs in refs/testgit do not exist prior to the
failed fetch, and therefore also do not exist afterwards. When fetch
tries to feed their sha1s to check_everything_connected, you get the
funny:
fatal: bad object
error: testgit::/home/peff/compile/git/t/trash
directory.t5801-remote-helpers/server did not send all necessary
objects
But if we change the test like this:
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index efe67e2..e968ea9 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -167,11 +167,11 @@ test_expect_success 'proper failure checks for fetching' '
'
test_expect_success 'proper failure checks for fetching' '
- (GIT_REMOTE_TESTGIT_FAILURE=1 &&
+ (cd local &&
+ git fetch &&
+ GIT_REMOTE_TESTGIT_FAILURE=1 &&
export GIT_REMOTE_TESTGIT_FAILURE &&
- cd local &&
- test_must_fail git fetch 2> error &&
- grep "Error while running helper" error
+ test_must_fail git fetch 2> error
)
'
we can see that without your check_command, the failure in the second
fetch is not noticed. Adding in your patch does detect this. _But_ it is
only checking a specific failure mode of the remote-helper: process
death that results in closing the fast-import pipe, which is how your
GIT_REMOTE_TESTGIT_FAILURE is implemented (closing the pipe first and
then dying is racy).
If we then add this on top of your series (fixing the "status" bug in
patch 1 that Junio mentioned), the test will start failing (both the
original, and the more robust one I showed above):
diff --git a/git-remote-testgit b/git-remote-testgit
index ca0cf09..41b0780 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -64,6 +64,11 @@ do
if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
then
+ # close stdout
+ exec >/dev/null
+ # now sleep to make sure fast-import
+ # has time to die before we exit
+ sleep 1
exit -1
fi
I agree that the failure mode from your patch is probably the most
common order for helpers to fail in (i.e., they just die and that's what
kills the pip
Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive
Felipe Contreras writes:
> Otherwise transport-helper will continue checking for refs and other
> things what will confuse the user more.
> ---
Sign-off?
> git-remote-testgit| 11 +++
> t/t5801-remote-helpers.sh | 19 +++
> transport-helper.c| 8
> 3 files changed, 38 insertions(+)
>
> diff --git a/git-remote-testgit b/git-remote-testgit
> index b395c8d..ca0cf09 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -61,12 +61,23 @@ do
> echo "feature import-marks=$gitmarks"
> echo "feature export-marks=$gitmarks"
> fi
> +
> + if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
> + then
> + exit -1
It is rather unusual to see a negative return value returned from a
shell script. Shouldn't this be something like 127 (or 1)?
> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
> index f387027..efe67e2 100755
> --- a/t/t5801-remote-helpers.sh
> +++ b/t/t5801-remote-helpers.sh
> @@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' '
> compare_refs local dup server dup
> '
>
> +test_expect_success 'proper failure checks for fetching' '
> + (GIT_REMOTE_TESTGIT_FAILURE=1 &&
> + export GIT_REMOTE_TESTGIT_FAILURE &&
> + cd local &&
> + test_must_fail git fetch 2> error &&
> + grep "Error while running helper" error
> + )
> +'
> +
> +# We sleep to give fast-export a chance to catch the SIGPIPE
> +test_expect_failure 'proper failure checks for pushing' '
> + (GIT_REMOTE_TESTGIT_FAILURE=1 &&
> + export GIT_REMOTE_TESTGIT_FAILURE &&
> + cd local &&
> + test_must_fail git push --all 2> error &&
> + grep "Error while running helper" error
> + )
> +'
> +
> test_done
> diff --git a/transport-helper.c b/transport-helper.c
> index cb3ef7d..dfdfa7a 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,
>
> if (finish_command(&fastimport))
> die("Error while running fast-import");
> +
> + if (!check_command(data->helper))
> + die("Error while running helper");
> +
> argv_array_free_detached(fastimport.argv);
>
> /*
> @@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport
> *transport,
>
> if (finish_command(&exporter))
> die("Error while running fast-export");
> +
> + if (!check_command(data->helper))
> + die("Error while running helper");
> +
> push_update_refs_status(data, remote_refs);
> return 0;
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
