On Sun, Apr 7, 2013 at 9:33 PM, Jeff King <peff@xxxxxxxx> 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 enough.
That's not what I said. My complaint is why _now_, and not six months ago.
>> If this current patch series is not good enough, I think it's best to
>> leave things as they are for me.
>
> That's certainly your choice. It just seems like we can be more robust
> while adding less code, as I tried to show.
After reading back all the mails and checking all the different
situations, I think my main problem was that my test was not for the
right thing. The test for import should be using the 'done' feature,
and we should mostly ignore the rest.
While my patch does improve the situation when the remote-helper is
not using the 'done' feature, it's probably not worth the effort, and
we might want to enforce the remote helpers to implement this somehow.
In the meantime, your patch should probably deal with all the real
world situations, so I've updated the tests and I'll resend.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html