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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux