On Sat, Mar 1, 2014 at 7:18 PM, Faiz Kothari <faiz.off93@xxxxxxxxx> wrote:
> Subject: implemented strbuf_write_or_die()
Imperative tone is preferred. The commit message tells what it is
doing, not what it did.
Subject: introduce strbuf_write_or_die()
> Signed-off-by: Faiz Kothari <faiz.off93@xxxxxxxxx>
> ---
> Implementing this clearly distinguishes between writing a normal buffer
> and writing a strbuf. Also, it provides an interface to write strbuf
> directly without knowing the private members of strbuf, making strbuf
> completely opaque. Also, makes the code more readable.
These three sentences which justify the change should go above the
"---" line so they are recorded in the project history for future
readers to understand why the change was made. As for their actual
value, the first and third sentences are nebulous; the second sentence
may be suitable (although strbuf's buf and len are actually considered
public, so the justification not be supportable).
A couple other comments:
Justification is important because many topics are in-flight at any
given time. Changes like this one which touch an arbitrary number of
files may conflict with other in-flight topics, which places extra
burden on the project maintainer (Junio). The justification needs to
be strong enough to outweigh that added burden.
Introduction of a new function is conceptually distinct from the act
of updating existing callers to invoke it, thus this submission should
probably be decomposed into two or more patches where the first patch
introduces the function, and following patches convert existing
callers.
More below.
> diff --git a/cache.h b/cache.h
> index db223e8..8898bf4 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);
> extern int copy_file(const char *dst, const char *src, int mode);
> extern int copy_file_with_time(const char *dst, const char *src, int mode);
> extern void write_or_die(int fd, const void *buf, size_t count);
> +extern void strbuf_write_or_die(const struct strbuf *sbuf, int fd);
You still have the layering violation here mentioned by both Johannes
and Michael. This declaration belongs in strbuf.h, not cache.h
Also, for bonus points, you should document this new function in
Documentation/technical/api-strbuf.txt.
> extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
> extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
> extern void fsync_or_die(int fd, const char *);
> diff --git a/write_or_die.c b/write_or_die.c
> index b50f99a..373450e 100644
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -64,6 +64,16 @@ void write_or_die(int fd, const void *buf, size_t count)
> }
> }
>
> +void strbuf_write_or_die(const struct strbuf *sbuf, int fd)
Ditto regarding the layering violation. This implementation belongs in strbuf.c.
> +{
> + if(!sbuf)
> + fprintf(stderr, "write error\n");
This is a programmer error, rather than a run-time I/O error. As such,
an assertion would be appropriate:
assert(sbuf);
> + else if (write_in_full(fd, sbuf->buf, sbuf->len) < 0) {
> + check_pipe(errno);
> + die_errno("write error");
He Sun correctly pointed out in his review that you should simply
invoke the lower-level write_or_die() here rather than copying/pasting
its functionality. (You fixed it in an earlier version of the patch
but somehow reverted that fix when sending this version.)
> + }
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html