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

[Taylor Blau <me@ttaylorr.com>]

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:
>
> > 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?

Yep, thanks.

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

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

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