Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

On Thu, Jul 9, 2009 at 10:37, Johan Herland[off-list ref] wrote:

On Thursday 09 July 2009, Alex Riesen wrote:

quoted

On Wed, Jul 8, 2009 at 18:01, Johan Herland[off-list ref] wrote:

quoted

On Wednesday 08 July 2009, Johannes Sixt wrote:

quoted

... don't you get this error message with errno set to EPIPE?
Previously, there was no error message.

Indeed, you are correct. I guess the following should be added to the
patch:

       if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) <
0 || write_in_full(revlist.in, "\n", 1) < 0) {
-               error("failed write to rev-list");
-               err = errno;
+               if (errno != EPIPE) {
+                       error("failed write to rev-list");
+                       err = errno;

You'll loose errno this way: error() does not save it.

Not sure what you mean here. Should I move "err = errno;" outside the
innermost "if"?

put it before error("failed write to rev-list"); or even before the
"if (err != EPIPE)".
Otherwise it is 0 after fprintf to stderr (which is the error() call).

From my POV, if errno != EPIPE, we save it into err, and return that
(overridden by finish_command()'s return value, if non-zero). If errno ==
EPIPE, we're not interested in saving it, because we expect finish_command()
to return non-zero in any case.

And you think this expectation makes the code simpler to understand?