Re: [PATCH 4/8] builtin/commit-graph.c: extract 'read_one_commit()'

On Thu, May 07, 2020 at 04:03:05PM -0400, Jeff King wrote:

On Mon, May 04, 2020 at 07:13:46PM -0600, Taylor Blau wrote:

quoted

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.

quoted

+static int read_one_commit(struct oidset *commits, char *hash)

This could be "const char *hash", couldn't it?

Yep, thanks.

quoted

+	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.

Yeah, I know the -1 is more idiomatic than what I had written here. This
was done so that I could use the return value from 'read_one_commit' as
an exit code from 'graph_write()', but I don't mind switching this to
'return error(...)' and then checking at the caller for a non-zero
return value and returning 1 there instead.

quoted

+		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.

Yeah, it's much clearer as two separate cases. I'll send something like
that shortly once I get to the rest of your review.

quoted

+
 		UNLEAK(buf);

Not new in your patch, but this UNLEAK() has always bugged me. ;) Why
not just strbuf_release() it?

I snuck it in! ;)

-Peff

Thanks,
Taylor