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

From: Junio C Hamano <gitster@pobox.com>
Date: May 14, 2020, 18:27 UTC
Message-ID: <xmqq8shuqsnf.fsf@gitster.c.googlers.com>
In-Reply-To: <20200514180200.GA86181@syl.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 5 quoted lines

>  static int graph_write(int argc, const char **argv)
>  {
> -	struct string_list *pack_indexes = NULL;
> +	struct strbuf buf = STRBUF_INIT;
> +	struct string_list pack_indexes;

I would strongly prefer this to be initialized here, not ...

Show 15 quoted lines

>  	struct oidset commits = OIDSET_INIT;
>  	struct object_directory *odb = NULL;
> -	struct string_list lines;
>  	int result = 0;
>  	enum commit_graph_write_flags flags = 0;
>
> @@ -209,44 +221,34 @@ static int graph_write(int argc, const char **argv)
>  		return 0;
>  	}
>
> -	string_list_init(&lines, 0);
> -	if (opts.stdin_packs || opts.stdin_commits) {
> -		struct strbuf buf = STRBUF_INIT;
> +	if (opts.stdin_packs) {
> +		string_list_init(&pack_indexes, 0);

... down here. I know that the use of it is guarded with a ternary (opts.stdin_packs ? &pack_indexes : NULL), but that is why you need to UNLEAK() it, intead of unconditionally releasing the strbuf, at the end of this function, no?

Show 52 quoted lines

>  		while (strbuf_getline(&buf, stdin) != EOF)
> -			string_list_append(&lines, strbuf_detach(&buf, NULL));
> +			string_list_append(&pack_indexes,
> +					   strbuf_detach(&buf, NULL));
> +	} else if (opts.stdin_commits) {
> +		oidset_init(&commits, 0);
> +		flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
>
> -		if (opts.stdin_packs)
> -			pack_indexes = &lines;
> -		if (opts.stdin_commits) {
> -			struct string_list_item *item;
> -			oidset_init(&commits, lines.nr);
> -			for_each_string_list_item(item, &lines) {
> -				struct object_id oid;
> -				const char *end;
> -
> -				if (parse_oid_hex(item->string, &oid, &end)) {
> -					error(_("unexpected non-hex object ID: "
> -						"%s"), item->string);
> -					return 1;
> -				}
> -
> -				oidset_insert(&commits, &oid);
> +		while (strbuf_getline(&buf, stdin) != EOF) {
> +			if (read_one_commit(&commits, buf.buf)) {
> +				result = 1;
> +				goto cleanup;
>  			}
> -			flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
>  		}
> -
> -		UNLEAK(buf);
>  	}
>
>  	if (write_commit_graph(odb,
> -			       pack_indexes,
> +			       opts.stdin_packs ? &pack_indexes : NULL,
>  			       opts.stdin_commits ? &commits : NULL,
>  			       flags,
>  			       &split_opts))
>  		result = 1;
>
> -	UNLEAK(lines);
> +cleanup:
> +	UNLEAK(pack_indexes);
> +	strbuf_release(&buf);
>  	return result;
>  }
>
> --
> 2.26.0.113.ge9739cdccc