Re: [PATCH 2/3] Support for partial HTTP transfers

[Junio C Hamano <junkio@cox.net>]

Nick Hengeveld <nickh@reactrix.com> writes:

> +	unlink(prevfile);
> +	rename(tmpfile, prevfile);
> +	unlink(tmpfile);

If rename() succeeds then tmpfile is no more.  If rename() fails
because there were no tmpfile to begin with, it is not an error
(i.e. you are not recovering) and that case there would not be
tmpfile either.  Otherwise, if tmpfile still remains to unlink()
because rename() failed for any other reason, wouldn't you
rather report it as an error and abort?

I wonder what happens if by mistake or intentionally we run two
http-fetch instances simultaneously.  IIRC, the current code is
safe -- the resulting object database will have the object file
fetched by one of the instance, and the updating of ref is done
via write_ref_sha1(), so it also is safe.  But your change may
introduce an interesting case where one creates a tmpfile, the
other one moves it to prevfile and starts using its partial
contents, and possibly gets confused -- it will probabaly fail
at the end detecting inconsistent object so it is probably not a
big loss.

Personally, I do not think people would mind much if we
introduce the BKL at the beginning of git-fetch.sh to prevent
multiple fetches stomping on each other, if somebody cared
enough (hint, hint).

> -	snprintf(tmpfile, sizeof(tmpfile), "%s/obj_XXXXXX",
> -		 get_object_directory());
> +	local = open(tmpfile, O_WRONLY | O_CREAT | O_EXCL, 0666);
> -	local = mkstemp(tmpfile);

I introduced this mkstemp() part recently to mimic the local
object creation where newly created object files are built in
the same directory, hoping that they would be allocated close to
each other when more than one are created in sequence.  I do not
know it matters much in practice -- has anybody measured, and
does anybody care?

> +	/* Reset inflate/SHA1 if there was an error reading the previous temp
> +	   file; also rewind to the beginning of the local file. */

Maybe not just rewind but truncate as well?  It probably does
not matter in practice much, but previous representation your
fetch was interrupted in the middle could have been much larger
than the representation you are slurping right now.

There was a discussion about an object file of the same SHA1 and
the same contents can have different compressed representations
(we hash then compress so the resulting filesize depends on the
compression level without affecting the contents of the object).
In a "doctor, it hurts when I do this -- don't do it, then" kind
of corner case, a DNS rotated pair of webservers could be
serving the same object in different representations and you may
get interrupted while fetching from one, and restart the
transfer from the other.  The SHA1 check at the end hopefully
would catch this kind of situation, and that round of http-fetch
would fail -- the user needs to re-run the fetch so it is not a
big loss, but it is something to keep in mind.