On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Eric Sunshine <sunsh...@sunshineco.com> writes:
>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun <sunheeh...@gmail.com> wrote:
>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari <faiz.of...@gmail.com>:
>>>> diff --git a/remote-curl.c b/remote-curl.c
>>>> index 10cb011..dee8716 100644
>>>> --- a/remote-curl.c
>>>> +++ b/remote-curl.c
>>>> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct
>>>> discovery *heads)
>>>> if (start_command(&client))
>>>> if (preamble)
>>>> - write_or_die(client.in, preamble->buf, preamble->len);
>>>> + strbuf_write_or_die(client.in, preamble);
>>>> if (heads)
>>>> write_or_die(client.in, heads->buf, heads->len);
>>> This should be changed. May be you can use Ctrl-F to search write_or_die().
>>> Or if you are using vim, use "/ and n" to find all.
>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,
>> so Faiz correctly left this invocation alone.
> That is a very good sign why this change is merely a code-churn and
> not an improvement, isn't it? We know (and any strbuf user should
> know) that ->buf and ->len are the ways to learn the pointer and the
> length the strbuf holds. Why anybody thinks it is benefitial to
> introduce another function that is _only_ for writing out strbuf and
> cannot be used to write out a plain buffer is simply beyond me.
As a potential GSoC student and newcomer to the project, Faiz would
not have known that this would be considered unwanted churn when he
chose the task from the GSoC microproject page . Perhaps it would
be a good idea to retire this item from the list?
On the other hand, it did expose Faiz to the iterative code review
process on this project and gave him a taste of what would be expected
of him as a GSoC student, so the microproject achieved that important
goal, and thus wasn't an utter failure.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html