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

2018-02-05 Thread Derrick Stolee

On 2/1/2018 7:23 PM, Jonathan Tan wrote:

On Tue, 30 Jan 2018 16:39:35 -0500
Derrick Stolee  wrote:


Teach git-commit-graph to read commit graph files and summarize their contents.

One overall question - is the "read" command meant to be a command used
by the end user, or is it here just to test that some aspects of reading
works? If the former, I'm not sure how useful it is. And if the latter,
I think that it is more useful to just implementing something that reads
it, then make the 11/14 change (modifying parse_commit_gently) and
include a perf test to show that your commit graph reading is both
correct and (performance-)effective.


The "read" command is intended for use with the tests to verify that the 
different --write commands write the correct number of commits at a 
time. For example, we can verify that the closure under reachability 
works when using --stdin-commits, that we get every commit in a pack 
when using --stdin-packs, and that we don't get more commits than we should.


It doesn't serve much purpose on the user-facing side, but this is 
intended to be a plumbing command that is called by other porcelain 
commands.


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

2018-02-01 Thread Jonathan Tan
On Tue, 30 Jan 2018 16:39:35 -0500
Derrick Stolee  wrote:

> Teach git-commit-graph to read commit graph files and summarize their 
> contents.

One overall question - is the "read" command meant to be a command used
by the end user, or is it here just to test that some aspects of reading
works? If the former, I'm not sure how useful it is. And if the latter,
I think that it is more useful to just implementing something that reads
it, then make the 11/14 change (modifying parse_commit_gently) and
include a perf test to show that your commit graph reading is both
correct and (performance-)effective.


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

2018-02-01 Thread SZEDER Gábor

> Teach git-commit-graph to read commit graph files and summarize their 
> contents.
> 
> Use the --read option to verify the contents of a commit graph file in the
> tests.
> 
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/git-commit-graph.txt |   7 ++
>  builtin/commit-graph.c |  55 +++
>  commit-graph.c | 138 
> -
>  commit-graph.h |  25 +++
>  t/t5318-commit-graph.sh|  28 ++--
>  5 files changed, 247 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index 3f3790d9a8..09aeaf6c82 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  
>  [verse]
>  'git commit-graph' --write  [--pack-dir ]
> +'git commit-graph' --read  [--pack-dir ]

Again, what does this option do?

>  EXAMPLES
>  
> @@ -20,6 +21,12 @@ EXAMPLES
>  $ git commit-graph --write
>  
>  
> +* Read basic information from a graph file.
> ++
> +
> +$ git commit-graph --read --graph-hash=
> +
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 7affd512f1..218740b1f8 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c

> +int close_commit_graph(struct commit_graph *g)

static, perhaps?  I see it's declared as extern in the headeer file
below, but I don't see it called outside of this source file by the
end of the patch series.

> +{
> + if (g->graph_fd < 0)
> + return 0;
> +
> + munmap((void *)g->data, g->data_len);
> + g->data = 0;
> +
> + close(g->graph_fd);
> + g->graph_fd = -1;
> +
> + return 1;
> +}
> +
> +static void free_commit_graph(struct commit_graph **g)
> +{
> + if (!g || !*g)
> + return;
> +
> + close_commit_graph(*g);
> +
> + free(*g);
> + *g = NULL;
> +}
> +
> +struct commit_graph *load_commit_graph_one(const char *graph_file, const 
> char *pack_dir)
> +{
> + void *graph_map;
> + const unsigned char *data;
> + struct commit_graph_header *hdr;
> + size_t graph_size;
> + struct stat st;
> + uint32_t i;
> + struct commit_graph *graph;
> + int fd = git_open(graph_file);
> + uint64_t last_chunk_offset;
> + uint32_t last_chunk_id;
> +
> + if (fd < 0)
> + return 0;
> + if (fstat(fd, &st)) {
> + close(fd);
> + return 0;
> + }
> + graph_size = xsize_t(st.st_size);
> +
> + if (graph_size < GRAPH_MIN_SIZE) {
> + close(fd);
> + die("graph file %s is too small", graph_file);
> + }
> + graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
> + data = (const unsigned char *)graph_map;
> +
> + hdr = graph_map;
> + if (ntohl(hdr->graph_signature) != GRAPH_SIGNATURE) {
> + uint32_t signature = ntohl(hdr->graph_signature);
> + munmap(graph_map, graph_size);
> + close(fd);
> + die("graph signature %X does not match signature %X",
> + signature, GRAPH_SIGNATURE);
> + }
> + if (hdr->graph_version != GRAPH_VERSION) {
> + unsigned char version = hdr->graph_version;
> + munmap(graph_map, graph_size);
> + close(fd);
> + die("graph version %X does not match version %X",
> + version, GRAPH_VERSION);
> + }
> +
> + graph = alloc_commit_graph(strlen(pack_dir) + 1);
> +
> + graph->hdr = hdr;
> + graph->graph_fd = fd;
> + graph->data = graph_map;
> + graph->data_len = graph_size;
> +
> + last_chunk_id = 0;
> + last_chunk_offset = (uint64_t)sizeof(*hdr);
> + for (i = 0; i < hdr->num_chunks; i++) {
> + uint32_t chunk_id = ntohl(*(uint32_t*)(data + sizeof(*hdr) + 12 
> * i));
> + uint64_t chunk_offset1 = ntohl(*(uint32_t*)(data + sizeof(*hdr) 
> + 12 * i + 4));
> + uint32_t chunk_offset2 = ntohl(*(uint32_t*)(data + sizeof(*hdr) 
> + 12 * i + 8));

There are a lot of magic number in these three lines, but at least
they are all multiples of 4.

> + uint64_t chunk_offset = (chunk_offset1 << 32) | chunk_offset2;
> +
> + if (chunk_offset > graph_size - GIT_MAX_RAWSZ)
> + die("improper chunk offset %08x%08x", 
> (uint32_t)(chunk_offset >> 32),
> + (uint32_t)chunk_offset);
> +
> + switch (chunk_id) {
> + case GRAPH_CHUNKID_OIDFANOUT:
> + graph->chunk_oid_fanout = data + chunk_offset;
> + break;
> +
> + case GRAPH_CHUNKID_OIDLOOKUP:
> +   

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

2018-01-30 Thread Stefan Beller
> +static void free_commit_graph(struct commit_graph **g)
> +{
> +   if (!g || !*g)
> +   return;
> +
> +   close_commit_graph(*g);
> +
> +   free(*g);
> +   *g = NULL;

nit: You may want to use FREE_AND_NULL(*g) instead.