Re: [PATCH v3 06/14] commit-graph: implement 'git-commit-graph read'

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <stolee@gmail.com> writes:

> +'read'::
> +
> +Read a graph file given by the graph-head file and output basic
> +details about the graph file.

"a graph file", assuming that there must be only one in the
specified place?  Or if there are more than one, read all of them?
Or is it an error to have more than one?

    Do not answer questions in a message that is a response to _me_;
    the purpose of a review is not to educate reviewers---it is to
    improve the patch.

> +With `--graph-hash=<hash>` option, consider the graph file
> +graph-<hash>.graph in the pack directory.

I think it is more in line with how plumbing works to just let the
full pathname be specifiable (e.g. learn from how pack-objects takes
"pack-" prefix from the command line, even though in practice names
of all packs you see in any repos start from "pack-").

> +struct commit_graph *load_commit_graph_one(const char *graph_file, const char *pack_dir)

This somehow smells like a screwed up API.  It gets a filename to
read from that is directly passed to git_open().  Why does an
instance of graph has to know and remember the path to the directory
(i.e. pack_dir) that was given when it was constructed?  "I am an
instance that holds commit topology learned from this object
database" is something it might want to know and remember, but "I am
told that I'll eventually be written back to there when I was
created" does not sound like a useful thing to have.