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