git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] implemented strbuf_write_or_die()

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Mar 3, 2014, 19:46 UTC
Message-ID
<CAPig+cTmejtWXRzr6qk-kd+P8j4b6xMJSUVnNnqObqNXc-S9UA@mail.gmail.com>
In-Reply-To
<xmqqvbvvqglc.fsf@gitster.dls.corp.google.com>
On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 29 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun <sunheehnus@gmail.com> wrote:
>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari <faiz.off93@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))
>>>>                 exit(1);
>>>>         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 [1]. 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.

[1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md
Previous: Junio C HamanoNext: David Kastrup
Message 14 of 23 in “implemented strbuf_write_or_die()”
  1. implemented strbuf_write_or_die()Faiz Kothari, Mar 1, 2014
  2. He SunMar 1, 2014
  3. implemented strbuf_write_or_die()Faiz Kothari, Mar 1, 2014
  4. Michael HaggertyMar 1, 2014
  5. implemented strbuf_write_or_die()Faiz Kothari, Mar 2, 2014
  6. Eric SunshineMar 2, 2014
  7. Eric SunshineMar 2, 2014
  8. 1/2 Introduce strbuf_write_or_die()Faiz Kothari, Mar 2, 2014
  9. 2/2 use strbuf_write_or_die()Faiz Kothari, Mar 2, 2014
  10. Eric SunshineMar 2, 2014
  11. Eric SunshineMar 2, 2014
  12. Eric SunshineMar 2, 2014
  13. Junio C HamanoMar 3, 2014
  14. Eric SunshineMar 3, 2014
  15. David KastrupMar 3, 2014
  16. Junio C HamanoMar 3, 2014
  17. Eric SunshineMar 3, 2014
  18. Michael HaggertyMar 4, 2014
  19. Faiz KothariMar 4, 2014
  20. Junio C HamanoMar 4, 2014
  21. Johannes SixtMar 1, 2014
  22. Junio C HamanoMar 3, 2014
  23. Fwd: [PATCH] implemented strbuf_write_or_die()Faiz Kothari, Mar 3, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.