On Mon, May 04, 2020 at 07:13:46PM -0600, Taylor Blau wrote:
> In the case of '--stdin-commits', feed each line to a new
> 'read_one_commit' helper, which (for now) will merely call
> 'parse_oid_hex'.
Makes sense.
> +static int read_one_commit(struct oidset *commits, char *hash)
This could be "const char *hash", couldn't it?
> + struct object_id oid;
> + const char *end;
> +
> + if (parse_oid_hex(hash, &oid, &end)) {
> + error(_("unexpected non-hex object ID: %s"), hash);
> + return 1;
> + }
Returning "-1" for error is more idiomatic in our code base (though I
know some of the commit-graph code doesn't follow that, I think we
should slowly try to move it back in the other direction.
> + while (strbuf_getline(&buf, stdin) != EOF) {
> + char *line = strbuf_detach(&buf, NULL);
> + if (opts.stdin_commits) {
> + int result = read_one_commit(&commits, line);
> + if (result)
> + return result;
> + } else
> + string_list_append(&pack_indexes, line);
> + }
This leaks "line" for each commit in stdin_commits mode (it used to get
added to a string list). I think you want:
while (strbuf_getline(&buf, stdin) != EOF) {
if (opts.stdin_commits) {
if (read_one_commit(&commits, buf.buf)) {
strbuf_release(&buf);
return 1;
}
} else {
string_list_append(&pack_indexes, strbuf_detach(&buf));
}
}
Though I think it might be easier to follow if each mode simply has its
own while loop.
> +
> UNLEAK(buf);
Not new in your patch, but this UNLEAK() has always bugged me. ;) Why
not just strbuf_release() it?
-Peff
