Re: [PATCH] git-rev-list: give better diagnostic for failed write

From: Jim Meyering <jim@meyering.net>
Date: Jun 25, 2007, 21:52 UTC
Message-ID: <878xa7u2gh.fsf@rho.meyering.net>
In-Reply-To: <alpine.LFD.0.98.0706251349540.8675@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> wrote:

Show 16 quoted lines

> On Mon, 25 Jun 2007, Jim Meyering wrote:
>>
>> [this patch depends on the one I posted here:
>>  http://marc.info/?l=git&m=118280134031923&w=2 ]
>>
>> Without this patch, git-rev-list unnecessarily omits strerror(errno)
>> from its diagnostic, upon write failure:
>
> And this is a perfect example of what's wrong with the whole thing.
>
> Dammit, how many times do I need to say this:
>
>  - If you want reliable errors, don't use stdio!
>
> That fflush is there FOR A REASON. You removed it FOR A MUCH LESS
> IMPORTANT REASON!

Wow. No need to curse and get into ALL_CAPS_MODE every time you reply to me. It does not advance your cause.

Remember: I'm trying to improve existing code here.
You should save some of your ire for the person who wrote that code.

Show 5 quoted lines

> That fflush is there exactly because WE DO NOT WANT TO BUFFER the list of
> commits, because that thing is meant very much to be used for pipelines,
> and it's quite common that the receiving end is going to do something
> asynchronous with the result, and can - and does - want the results as
> soon as possible.

That's good to know. I'm glad you pointed it out. It would have been nice to have a comment. However, wouldn't it be better at least to check for and report fflush failure? fflush usually does a write, after all. Most of the rest of the code is careful to diagnose write errors at the source. Why not here?

I've posted a revised patch.

Show 21 quoted lines

> IOW, things like "gitk" use git-rev-list exactly to get the list of
> commits, and they want that list *incrementally*. They don't want to wait
> for git-rev-list to have filled up some 8kB buffer of commits. Especially
> since generating those commits can be slow if we're talking about a big
> tree and some path-limited stuff.
>
> So for example, do something like
>
> 	git rev-list HEAD -- drivers/char/drm/Makefile
>
> and if you don't see the result scroll a line at a time on a slower
> machine, there's something *wrong*.
>
> Junio, I'm NAK'ing this very forcefully!
>
> Jim: I don't know what I'm doing wrong, but I'm apparently not reaching
> you. So let me try one more time:
>
>  - stdio really isn't very good with error handling
>
>  - if you use stdio, YOU HAD BETTER ACCEPT THAT

Using stdio is fine, as long as you know and respect its limitations.

It's a real shame that you have to intersperse your often-valuable feedback with such vitriol.