Re: [PATCH 17/18] http*: add helper methods for fetching packs

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2009, 20:44 UTC
Message-ID: <7v7i06teng.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20090524222006.cd71f8dc.rctay89@gmail.com>

Tay Ray Chuan <rctay89@gmail.com> writes:

Show 10 quoted lines

> The code handling the fetching of packs in http-push.c and
> http-walker.c have been refactored into new methods and a new struct
> (http_pack_request) in http.c. They are not meant to be invoked
> elsewhere.
>
> The new methods in http.c are
>  *new_http_pack_request
>  *finish_http_pack_request
>  *release_http_pack_request
> and the new struct is http_pack_request.

Please do not use '*' not followed by any whitespace for enumerations; use of '*' when you talk about code is confusing (e.g "is he talking about a pointer?").

Show 7 quoted lines

> ***http-push.c***
> The local_stream member of the transfer_request has been removed, as
> the packfile pointer will be managed in the struct http_pack_request.
>
> The code moved out from start_fetch_packed to new_http_pack_request
> deals with filenames, file and curl options, and does not change
> its behaviour.

Sorry, I cannot parse these.  Do you mean

	Refactor code from start_fetch_packed() to a new function,
	new_http_pack_request(); this deals with filenames, file and curl
	options.

I am a bit puzzled about the description split along the filename boundary of the source files. You also describe the new_http_pack_request() function below, where you talk about changes to http-walker.c. It would be much easier to review if you don't split them, e.g.

	Add a function, new_http_pack_request(), that deals with the
	details of coming up with the filename to store the retrieved
	packfile, resuming a previously aborted request, and making a new
	curl request.  Update start_fetch_packed() in http-push.c and
	fetch_pack() in http-walker.c to use this.