Re: [PATCH] implemented strbuf_write_or_die()

Eric Sunshine [off-list ref] writes:

On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano [off-list ref] wrote:

quoted

Eric Sunshine [off-list ref] writes:

quoted

On Sat, Mar 1, 2014 at 7:51 AM, He Sun [off-list ref] wrote:

quoted

2014-03-01 19:21 GMT+08:00 Faiz Kothari [off-list ref]:

quoted

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?

I don't think I saw this on the microproject suggestion page when I
last looked at it, and assumed that this was on the student's own
initiative.

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

Surely.

I would have to say that this is not a good sample exercise to
suggest to new students and I'd encourage dropping it from the list.
You could argue that it is an effective way to cull people with bad
design taste to mix suggestions to make the codebase worse and see
who picks them, but I do not think it is very fair ;-)

On Mon, Mar 3, 2014 at 3:35 PM, Junio C Hamano [off-list ref] wrote:

Eric Sunshine [off-list ref] writes:

quoted

On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano [off-list ref] wrote:

quoted

Eric Sunshine [off-list ref] writes:

quoted

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?

I don't think I saw this on the microproject suggestion page when I
last looked at it, and assumed that this was on the student's own
initiative.

I also had not seen it earlier on the microprojects page and had the
same reaction until I re-checked the page and found that it had been
added [1].

The microprojects page already instructs students to indicate that a
submission is for GSoC [2] (and many have followed the advice), but
perhaps we can avoid this sort of misunderstanding in the future by
making it more explicit: for instance, tell them to add [GSoC] to the
Subject:.

[1]: https://github.com/git/git.github.io/commit/f314120a2b5e831459673c612a3630ad953d9954
[2]: https://github.com/git/git.github.io/blame/master/SoC-2014-Microprojects.md#L83

quoted

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

Surely.

I would have to say that this is not a good sample exercise to
suggest to new students and I'd encourage dropping it from the list.
You could argue that it is an effective way to cull people with bad
design taste to mix suggestions to make the codebase worse and see
who picks them, but I do not think it is very fair ;-)

Agreed. The item should be dropped from the list.