Skip to content

Szeder's commit-graph cleanups #650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
derrickstolee wants to merge 10 commits into from
Closed

Szeder's commit-graph cleanups #650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
af84c25
tree-walk.c: don't match submodule entries for 'submod/anything'
szeder Jun 4, 2020
1e1671e
commit-graph: fix parsing the Chunk Lookup table
szeder May 29, 2020
a09d0bd
commit-graph-format.txt: all multi-byte numbers are in network byte o…
szeder May 29, 2020
32a6f11
commit-slab: add a function to deep free entries on the slab
szeder May 29, 2020
1a70ff0
diff.h: drop diff_tree_oid() & friends' return value
szeder Jun 4, 2020
636c206
commit-graph: clean up #includes
szeder Jun 4, 2020
cd9e033
commit-graph: simplify parse_commit_graph() #1
szeder May 29, 2020
83641b5
commit-graph: simplify parse_commit_graph() #2
szeder Jun 4, 2020
9b818b9
commit-graph: simplify write_commit_graph_file() #1
szeder Jun 4, 2020
5984fb0
commit-graph: simplify write_commit_graph_file() #2
szeder May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Documentation/technical/commit-graph-format.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ the body into "chunks" and provide a binary lookup table at the beginning
of the body. The header includes certain values, such as number of chunks
and hash type.

All 4-byte numbers are in network order.
All multi-byte numbers are in network byte order.

HEADER:

Expand Down
113 changes: 48 additions & 65 deletions commit-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#include "cache.h"
#include "config.h"
#include "dir.h"
#include "git-compat-util.h"
#include "config.h"
#include "lockfile.h"
#include "pack.h"
#include "packfile.h"
Expand Down Expand Up @@ -232,8 +230,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
const unsigned char *data, *chunk_lookup;
uint32_t i;
struct commit_graph *graph;
uint64_t last_chunk_offset;
uint32_t last_chunk_id;
uint64_t next_chunk_offset;
uint32_t graph_signature;
unsigned char graph_version, hash_version;

Expand Down Expand Up @@ -274,25 +271,26 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
graph->data = graph_map;
graph->data_len = graph_size;

last_chunk_id = 0;
last_chunk_offset = 8;
if (graph_size < GRAPH_HEADER_SIZE +
(graph->num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH +
GRAPH_FANOUT_SIZE + the_hash_algo->rawsz) {
error(_("commit-graph file is too small to hold %u chunks"),
graph->num_chunks);
free(graph);
return NULL;
}

chunk_lookup = data + 8;
next_chunk_offset = get_be64(chunk_lookup + 4);
for (i = 0; i < graph->num_chunks; i++) {
uint32_t chunk_id;
uint64_t chunk_offset;
uint64_t chunk_offset = next_chunk_offset;
int chunk_repeated = 0;

if (data + graph_size - chunk_lookup <
GRAPH_CHUNKLOOKUP_WIDTH) {
error(_("commit-graph chunk lookup table entry missing; file may be incomplete"));
free(graph);
return NULL;
}

chunk_id = get_be32(chunk_lookup + 0);
chunk_offset = get_be64(chunk_lookup + 4);

chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;
next_chunk_offset = get_be64(chunk_lookup + 4);

if (chunk_offset > graph_size - the_hash_algo->rawsz) {
error(_("commit-graph improper chunk offset %08x%08x"), (uint32_t)(chunk_offset >> 32),
Expand All @@ -312,8 +310,11 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
case GRAPH_CHUNKID_OIDLOOKUP:
if (graph->chunk_oid_lookup)
chunk_repeated = 1;
else
else {
graph->chunk_oid_lookup = data + chunk_offset;
graph->num_commits = (next_chunk_offset - chunk_offset)
/ graph->hash_len;
}
break;

case GRAPH_CHUNKID_DATA:
Expand Down Expand Up @@ -368,15 +369,6 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
free(graph);
return NULL;
}

if (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)
{
graph->num_commits = (chunk_offset - last_chunk_offset)
/ graph->hash_len;
}

last_chunk_id = chunk_id;
last_chunk_offset = chunk_offset;
}

if (graph->chunk_bloom_indexes && graph->chunk_bloom_data) {
Expand Down Expand Up @@ -1530,17 +1522,22 @@ static int write_graph_chunk_base(struct hashfile *f,
return 0;
}

struct chunk_info {
uint32_t id;
uint64_t size;
};

static int write_commit_graph_file(struct write_commit_graph_context *ctx)
{
uint32_t i;
int fd;
struct hashfile *f;
struct lock_file lk = LOCK_INIT;
uint32_t chunk_ids[MAX_NUM_CHUNKS + 1];
uint64_t chunk_offsets[MAX_NUM_CHUNKS + 1];
struct chunk_info chunks[MAX_NUM_CHUNKS + 1];
const unsigned hashsz = the_hash_algo->rawsz;
struct strbuf progress_title = STRBUF_INIT;
int num_chunks = 3;
uint64_t chunk_offset;
struct object_id file_hash;
const struct bloom_filter_settings bloom_settings = DEFAULT_BLOOM_FILTER_SETTINGS;

Expand Down Expand Up @@ -1580,51 +1577,34 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
}

chunk_ids[0] = GRAPH_CHUNKID_OIDFANOUT;
chunk_ids[1] = GRAPH_CHUNKID_OIDLOOKUP;
chunk_ids[2] = GRAPH_CHUNKID_DATA;
chunks[0].id = GRAPH_CHUNKID_OIDFANOUT;
chunks[0].size = GRAPH_FANOUT_SIZE;
chunks[1].id = GRAPH_CHUNKID_OIDLOOKUP;
chunks[1].size = hashsz * ctx->commits.nr;
chunks[2].id = GRAPH_CHUNKID_DATA;
chunks[2].size = (hashsz + 16) * ctx->commits.nr;
if (ctx->num_extra_edges) {
chunk_ids[num_chunks] = GRAPH_CHUNKID_EXTRAEDGES;
chunks[num_chunks].id = GRAPH_CHUNKID_EXTRAEDGES;
chunks[num_chunks].size = 4 * ctx->num_extra_edges;
num_chunks++;
}
if (ctx->changed_paths) {
chunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMINDEXES;
chunks[num_chunks].id = GRAPH_CHUNKID_BLOOMINDEXES;
chunks[num_chunks].size = sizeof(uint32_t) * ctx->commits.nr;
num_chunks++;
chunk_ids[num_chunks] = GRAPH_CHUNKID_BLOOMDATA;
chunks[num_chunks].id = GRAPH_CHUNKID_BLOOMDATA;
chunks[num_chunks].size = sizeof(uint32_t) * 3
+ ctx->total_bloom_filter_data_size;
num_chunks++;
}
if (ctx->num_commit_graphs_after > 1) {
chunk_ids[num_chunks] = GRAPH_CHUNKID_BASE;
num_chunks++;
}

chunk_ids[num_chunks] = 0;

chunk_offsets[0] = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;
chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
chunk_offsets[2] = chunk_offsets[1] + hashsz * ctx->commits.nr;
chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * ctx->commits.nr;

num_chunks = 3;
if (ctx->num_extra_edges) {
chunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +
4 * ctx->num_extra_edges;
chunks[num_chunks].id = GRAPH_CHUNKID_BASE;
chunks[num_chunks].size = hashsz * (ctx->num_commit_graphs_after - 1);
num_chunks++;
}
if (ctx->changed_paths) {
chunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +
sizeof(uint32_t) * ctx->commits.nr;
num_chunks++;

chunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +
sizeof(uint32_t) * 3 + ctx->total_bloom_filter_data_size;
num_chunks++;
}
if (ctx->num_commit_graphs_after > 1) {
chunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +
hashsz * (ctx->num_commit_graphs_after - 1);
num_chunks++;
}
chunks[num_chunks].id = 0;
chunks[num_chunks].size = 0;

hashwrite_be32(f, GRAPH_SIGNATURE);

Expand All @@ -1633,13 +1613,16 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
hashwrite_u8(f, num_chunks);
hashwrite_u8(f, ctx->num_commit_graphs_after - 1);

chunk_offset = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;
for (i = 0; i <= num_chunks; i++) {
uint32_t chunk_write[3];

chunk_write[0] = htonl(chunk_ids[i]);
chunk_write[1] = htonl(chunk_offsets[i] >> 32);
chunk_write[2] = htonl(chunk_offsets[i] & 0xffffffff);
chunk_write[0] = htonl(chunks[i].id);
chunk_write[1] = htonl(chunk_offset >> 32);
chunk_write[2] = htonl(chunk_offset & 0xffffffff);
hashwrite(f, chunk_write, 12);

chunk_offset += chunks[i].size;
}

if (ctx->report_progress) {
Expand Down
6 changes: 3 additions & 3 deletions commit-graph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
#define COMMIT_GRAPH_H

#include "git-compat-util.h"
#include "repository.h"
#include "string-list.h"
#include "cache.h"
#include "object-store.h"

#define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
Expand All @@ -22,6 +19,9 @@ void git_test_write_commit_graph_or_die(void);

struct commit;
struct bloom_filter_settings;
struct repository;
struct raw_object_store;
struct string_list;

char *get_commit_graph_filename(struct object_directory *odb);
int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
Expand Down
1 change: 1 addition & 0 deletions commit-slab-decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ struct slabname { \
void init_ ##slabname## _with_stride(struct slabname *s, unsigned stride); \
Copy link

@gitgitgadget gitgitgadget bot Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, René Scharfe wrote (reply to this):

Am 05.06.20 um 15:00 schrieb SZEDER Gábor via GitGitGadget:
> From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= <[email protected]>
>
> clear_##slabname() frees only the memory allocated for a commit slab
> itself, but entries in the commit slab might own additional memory
> outside the slab that should be freed as well.  We already have (at
> least) one such commit slab, and this patch series is about to add one
> more.
>
> To free all additional memory owned by entries on the commit slab the
> user of such a slab could iterate over all commits it knows about,
> peek whether there is a valid entry associated with each commit, and
> free the additional memory, if any.  Or it could rely on intimate
> knowledge about the internals of the commit slab implementation, and
> could itself iterate directly through all entries in the slab, and
> free the additional memory.  Or it could just leak the additional
> memory...
>
> Introduce deep_clear_##slabname() to allow releasing memory owned by
> commit slab entries by invoking the 'void free_fn(elemtype *ptr)'
> function specified as parameter for each entry in the slab.

Adding a new function instead of extending the existing ones makes
sense, as this is a rare requirement.

>
> Use it in get_shallow_commits() in 'shallow.c' to replace an
> open-coded iteration over a commit slab's entries.
>
> Signed-off-by: SZEDER Gábor <[email protected]>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
>  commit-slab-decl.h |  1 +
>  commit-slab-impl.h | 13 +++++++++++++
>  commit-slab.h      | 10 ++++++++++
>  shallow.c          | 14 +++++---------
>  4 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/commit-slab-decl.h b/commit-slab-decl.h
> index adc7b46c83b..286164b7e27 100644
> --- a/commit-slab-decl.h
> +++ b/commit-slab-decl.h
> @@ -32,6 +32,7 @@ struct slabname {							\
>  void init_ ##slabname## _with_stride(struct slabname *s, unsigned stride); \
>  void init_ ##slabname(struct slabname *s);				\
>  void clear_ ##slabname(struct slabname *s);				\
> +void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *ptr)); \
>  elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \
>  elemtype *slabname## _at(struct slabname *s, const struct commit *c);	\
>  elemtype *slabname## _peek(struct slabname *s, const struct commit *c)
> diff --git a/commit-slab-impl.h b/commit-slab-impl.h
> index 5c0eb91a5d1..557738df271 100644
> --- a/commit-slab-impl.h
> +++ b/commit-slab-impl.h
> @@ -38,6 +38,19 @@ scope void clear_ ##slabname(struct slabname *s)			\
>  	FREE_AND_NULL(s->slab);						\
>  }									\
>  									\
> +scope void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *)) \
> +{									\
> +	unsigned int i;							\
> +	for (i = 0; i < s->slab_count; i++) {				\
> +		unsigned int j;						\
> +		if (!s->slab[i])					\
> +			continue;					\
> +		for (j = 0; j < s->slab_size; j++)			\
> +			free_fn(&s->slab[i][j * s->stride]);		\
> +	}								\
> +	clear_ ##slabname(s);						> +}									\


Why pass an elemtype pointer to the callback function instead of
a plain elemtype?  Because it matches the return type of _at() and
_peek().  Consistency, good.  Handing it a pointer allows the
callback to pass it on to free(), though, which would be bad,
since we do that in clear_() as well.  Hmm.

> +									\
>  scope elemtype *slabname## _at_peek(struct slabname *s,			\
>  						  const struct commit *c, \
>  						  int add_if_missing)   \
> diff --git a/commit-slab.h b/commit-slab.h
> index 05b3f2804e7..8e72a305365 100644
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -47,6 +47,16 @@
>   *
>   *   Call this function before the slab falls out of scope to avoid
>   *   leaking memory.
> + *
> + * - void deep_clear_indegree(struct indegree *, void (*free_fn)(int*))
> + *
> + *   Empties the slab, similar to clear_indegree(), but in addition it
> + *   calls the given 'free_fn' for each slab entry to release any
> + *   additional memory that might be owned by the entry (but not the
> + *   entry itself!).
> + *   Note that 'free_fn' might be called even for entries for which no
> + *   indegree_at() call has been made; in this case 'free_fn' is invoked
> + *   with a pointer to a zero-initialized location.
>   */
>
>  #define define_commit_slab(slabname, elemtype) \
> diff --git a/shallow.c b/shallow.c
> index 7fd04afed19..c4ac8a73273 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -84,6 +84,10 @@ int is_repository_shallow(struct repository *r)
>   * supports a "valid" flag.
>   */
>  define_commit_slab(commit_depth, int *);
> +static void free_depth_in_slab(int **ptr)
> +{
> +	FREE_AND_NULL(*ptr);
> +}

Why FREE_AND_NULL?  The original loop below called free().  The slabs
are all released by deep_clear_() immediately after the callbacks are
done anyway, so what's the point in zeroing these pointers?

>  struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>  		int shallow_flag, int not_shallow_flag)
>  {
> @@ -150,15 +154,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>  			}
>  		}
>  	}
> -	for (i = 0; i < depths.slab_count; i++) {
> -		int j;
> -
> -		if (!depths.slab[i])
> -			continue;
> -		for (j = 0; j < depths.slab_size; j++)
> -			free(depths.slab[i][j]);
> -	}
> -	clear_commit_depth(&depths);
> +	deep_clear_commit_depth(&depths, free_depth_in_slab);

What a relief!

>
>  	return result;
>  }
>

Copy link

@gitgitgadget gitgitgadget bot Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 6/18/2020 4:59 PM, René Scharfe wrote:
> Am 05.06.20 um 15:00 schrieb SZEDER Gábor via GitGitGadget:
>> From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= <[email protected]>
>>
>> clear_##slabname() frees only the memory allocated for a commit slab
>> itself, but entries in the commit slab might own additional memory
>> outside the slab that should be freed as well.  We already have (at
>> least) one such commit slab, and this patch series is about to add one
>> more.
>>
>> To free all additional memory owned by entries on the commit slab the
>> user of such a slab could iterate over all commits it knows about,
>> peek whether there is a valid entry associated with each commit, and
>> free the additional memory, if any.  Or it could rely on intimate
>> knowledge about the internals of the commit slab implementation, and
>> could itself iterate directly through all entries in the slab, and
>> free the additional memory.  Or it could just leak the additional
>> memory...
>>
>> Introduce deep_clear_##slabname() to allow releasing memory owned by
>> commit slab entries by invoking the 'void free_fn(elemtype *ptr)'
>> function specified as parameter for each entry in the slab.
> 
> Adding a new function instead of extending the existing ones makes
> sense, as this is a rare requirement.
> 
>>
>> Use it in get_shallow_commits() in 'shallow.c' to replace an
>> open-coded iteration over a commit slab's entries.
>>
>> Signed-off-by: SZEDER Gábor <[email protected]>
>> Signed-off-by: Derrick Stolee <[email protected]>
>> ---
>>  commit-slab-decl.h |  1 +
>>  commit-slab-impl.h | 13 +++++++++++++
>>  commit-slab.h      | 10 ++++++++++
>>  shallow.c          | 14 +++++---------
>>  4 files changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/commit-slab-decl.h b/commit-slab-decl.h
>> index adc7b46c83b..286164b7e27 100644
>> --- a/commit-slab-decl.h
>> +++ b/commit-slab-decl.h
>> @@ -32,6 +32,7 @@ struct slabname {							\
>>  void init_ ##slabname## _with_stride(struct slabname *s, unsigned stride); \
>>  void init_ ##slabname(struct slabname *s);				\
>>  void clear_ ##slabname(struct slabname *s);				\
>> +void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *ptr)); \
>>  elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \
>>  elemtype *slabname## _at(struct slabname *s, const struct commit *c);	\
>>  elemtype *slabname## _peek(struct slabname *s, const struct commit *c)
>> diff --git a/commit-slab-impl.h b/commit-slab-impl.h
>> index 5c0eb91a5d1..557738df271 100644
>> --- a/commit-slab-impl.h
>> +++ b/commit-slab-impl.h
>> @@ -38,6 +38,19 @@ scope void clear_ ##slabname(struct slabname *s)			\
>>  	FREE_AND_NULL(s->slab);						\
>>  }									\
>>  									\
>> +scope void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *)) \
>> +{									\
>> +	unsigned int i;							\
>> +	for (i = 0; i < s->slab_count; i++) {				\
>> +		unsigned int j;						\
>> +		if (!s->slab[i])					\
>> +			continue;					\
>> +		for (j = 0; j < s->slab_size; j++)			\
>> +			free_fn(&s->slab[i][j * s->stride]);		\
>> +	}								\
>> +	clear_ ##slabname(s);						> +}									\
> 
> 
> Why pass an elemtype pointer to the callback function instead of
> a plain elemtype?  Because it matches the return type of _at() and
> _peek().  Consistency, good.  Handing it a pointer allows the
> callback to pass it on to free(), though, which would be bad,
> since we do that in clear_() as well.  Hmm.
> 
>> +									\
>>  scope elemtype *slabname## _at_peek(struct slabname *s,			\
>>  						  const struct commit *c, \
>>  						  int add_if_missing)   \
>> diff --git a/commit-slab.h b/commit-slab.h
>> index 05b3f2804e7..8e72a305365 100644
>> --- a/commit-slab.h
>> +++ b/commit-slab.h
>> @@ -47,6 +47,16 @@
>>   *
>>   *   Call this function before the slab falls out of scope to avoid
>>   *   leaking memory.
>> + *
>> + * - void deep_clear_indegree(struct indegree *, void (*free_fn)(int*))
>> + *
>> + *   Empties the slab, similar to clear_indegree(), but in addition it
>> + *   calls the given 'free_fn' for each slab entry to release any
>> + *   additional memory that might be owned by the entry (but not the
>> + *   entry itself!).
>> + *   Note that 'free_fn' might be called even for entries for which no
>> + *   indegree_at() call has been made; in this case 'free_fn' is invoked
>> + *   with a pointer to a zero-initialized location.
>>   */
>>
>>  #define define_commit_slab(slabname, elemtype) \
>> diff --git a/shallow.c b/shallow.c
>> index 7fd04afed19..c4ac8a73273 100644
>> --- a/shallow.c
>> +++ b/shallow.c
>> @@ -84,6 +84,10 @@ int is_repository_shallow(struct repository *r)
>>   * supports a "valid" flag.
>>   */
>>  define_commit_slab(commit_depth, int *);
>> +static void free_depth_in_slab(int **ptr)
>> +{
>> +	FREE_AND_NULL(*ptr);
>> +}
> 
> Why FREE_AND_NULL?  The original loop below called free().  The slabs
> are all released by deep_clear_() immediately after the callbacks are
> done anyway, so what's the point in zeroing these pointers?

I think the point was that a later change was going to free
elements in the slab on a one-by-one basis while computing
the filters, to save memory overall. To be future-proof
against such a change, we need to NULL the pointers here.

Perhaps that viewpoint also answers your other comment about
"why pass the pointer?"

Thanks,
-Stolee

Copy link

@gitgitgadget gitgitgadget bot Jun 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Fri, Jun 05, 2020 at 01:00:26PM +0000, SZEDER Gábor via GitGitGadget wrote:
> From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= <[email protected]>
> 
> clear_##slabname() frees only the memory allocated for a commit slab
> itself, but entries in the commit slab might own additional memory
> outside the slab that should be freed as well.  We already have (at
> least) one such commit slab, and this patch series is about to add one
> more.

This was only true in my original submission, but not anymore: this
patch series doesn't add another such slab, and, more importantly, now
we have at least two such commit slabs.  deep_clear_##slabnmae() could
be used to clear the bloom_filter_slab and all memory attached to it,
which at the moment is just leaked.

> To free all additional memory owned by entries on the commit slab the
> user of such a slab could iterate over all commits it knows about,
> peek whether there is a valid entry associated with each commit, and
> free the additional memory, if any.  Or it could rely on intimate
> knowledge about the internals of the commit slab implementation, and
> could itself iterate directly through all entries in the slab, and
> free the additional memory.  Or it could just leak the additional
> memory...
> 
> Introduce deep_clear_##slabname() to allow releasing memory owned by
> commit slab entries by invoking the 'void free_fn(elemtype *ptr)'
> function specified as parameter for each entry in the slab.
> 
> Use it in get_shallow_commits() in 'shallow.c' to replace an
> open-coded iteration over a commit slab's entries.
> 
> Signed-off-by: SZEDER Gábor <[email protected]>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
>  commit-slab-decl.h |  1 +
>  commit-slab-impl.h | 13 +++++++++++++
>  commit-slab.h      | 10 ++++++++++
>  shallow.c          | 14 +++++---------
>  4 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/commit-slab-decl.h b/commit-slab-decl.h
> index adc7b46c83b..286164b7e27 100644
> --- a/commit-slab-decl.h
> +++ b/commit-slab-decl.h
> @@ -32,6 +32,7 @@ struct slabname {							\
>  void init_ ##slabname## _with_stride(struct slabname *s, unsigned stride); \
>  void init_ ##slabname(struct slabname *s);				\
>  void clear_ ##slabname(struct slabname *s);				\
> +void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *ptr)); \
>  elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \
>  elemtype *slabname## _at(struct slabname *s, const struct commit *c);	\
>  elemtype *slabname## _peek(struct slabname *s, const struct commit *c)
> diff --git a/commit-slab-impl.h b/commit-slab-impl.h
> index 5c0eb91a5d1..557738df271 100644
> --- a/commit-slab-impl.h
> +++ b/commit-slab-impl.h
> @@ -38,6 +38,19 @@ scope void clear_ ##slabname(struct slabname *s)			\
>  	FREE_AND_NULL(s->slab);						\
>  }									\
>  									\
> +scope void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *)) \
> +{									\
> +	unsigned int i;							\
> +	for (i = 0; i < s->slab_count; i++) {				\
> +		unsigned int j;						\
> +		if (!s->slab[i])					\
> +			continue;					\
> +		for (j = 0; j < s->slab_size; j++)			\
> +			free_fn(&s->slab[i][j * s->stride]);		\
> +	}								\
> +	clear_ ##slabname(s);						\
> +}									\
> +									\
>  scope elemtype *slabname## _at_peek(struct slabname *s,			\
>  						  const struct commit *c, \
>  						  int add_if_missing)   \
> diff --git a/commit-slab.h b/commit-slab.h
> index 05b3f2804e7..8e72a305365 100644
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -47,6 +47,16 @@
>   *
>   *   Call this function before the slab falls out of scope to avoid
>   *   leaking memory.
> + *
> + * - void deep_clear_indegree(struct indegree *, void (*free_fn)(int*))
> + *
> + *   Empties the slab, similar to clear_indegree(), but in addition it
> + *   calls the given 'free_fn' for each slab entry to release any
> + *   additional memory that might be owned by the entry (but not the
> + *   entry itself!).
> + *   Note that 'free_fn' might be called even for entries for which no
> + *   indegree_at() call has been made; in this case 'free_fn' is invoked
> + *   with a pointer to a zero-initialized location.
>   */
>  
>  #define define_commit_slab(slabname, elemtype) \
> diff --git a/shallow.c b/shallow.c
> index 7fd04afed19..c4ac8a73273 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -84,6 +84,10 @@ int is_repository_shallow(struct repository *r)
>   * supports a "valid" flag.
>   */
>  define_commit_slab(commit_depth, int *);
> +static void free_depth_in_slab(int **ptr)
> +{
> +	FREE_AND_NULL(*ptr);
> +}
>  struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>  		int shallow_flag, int not_shallow_flag)
>  {
> @@ -150,15 +154,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>  			}
>  		}
>  	}
> -	for (i = 0; i < depths.slab_count; i++) {
> -		int j;
> -
> -		if (!depths.slab[i])
> -			continue;
> -		for (j = 0; j < depths.slab_size; j++)
> -			free(depths.slab[i][j]);
> -	}
> -	clear_commit_depth(&depths);
> +	deep_clear_commit_depth(&depths, free_depth_in_slab);
>  
>  	return result;
>  }
> -- 
> gitgitgadget
>

void init_ ##slabname(struct slabname *s); \
void clear_ ##slabname(struct slabname *s); \
void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *ptr)); \
elemtype *slabname## _at_peek(struct slabname *s, const struct commit *c, int add_if_missing); \
elemtype *slabname## _at(struct slabname *s, const struct commit *c); \
elemtype *slabname## _peek(struct slabname *s, const struct commit *c)
Expand Down
13 changes: 13 additions & 0 deletions commit-slab-impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,19 @@ scope void clear_ ##slabname(struct slabname *s) \
FREE_AND_NULL(s->slab); \
} \
\
scope void deep_clear_ ##slabname(struct slabname *s, void (*free_fn)(elemtype *)) \
{ \
unsigned int i; \
for (i = 0; i < s->slab_count; i++) { \
unsigned int j; \
if (!s->slab[i]) \
continue; \
for (j = 0; j < s->slab_size; j++) \
free_fn(&s->slab[i][j * s->stride]); \
} \
clear_ ##slabname(s); \
} \
\
scope elemtype *slabname## _at_peek(struct slabname *s, \
const struct commit *c, \
int add_if_missing) \
Expand Down
10 changes: 10 additions & 0 deletions commit-slab.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@
*
* Call this function before the slab falls out of scope to avoid
* leaking memory.
*
* - void deep_clear_indegree(struct indegree *, void (*free_fn)(int*))
*
* Empties the slab, similar to clear_indegree(), but in addition it
* calls the given 'free_fn' for each slab entry to release any
* additional memory that might be owned by the entry (but not the
* entry itself!).
* Note that 'free_fn' might be called even for entries for which no
* indegree_at() call has been made; in this case 'free_fn' is invoked
* with a pointer to a zero-initialized location.
*/

#define define_commit_slab(slabname, elemtype) \
Expand Down
10 changes: 5 additions & 5 deletions diff.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,11 +431,11 @@ struct combine_diff_path *diff_tree_paths(
struct combine_diff_path *p, const struct object_id *oid,
const struct object_id **parents_oid, int nparent,
struct strbuf *base, struct diff_options *opt);
int diff_tree_oid(const struct object_id *old_oid,
const struct object_id *new_oid,
const char *base, struct diff_options *opt);
int diff_root_tree_oid(const struct object_id *new_oid, const char *base,
struct diff_options *opt);
void diff_tree_oid(const struct object_id *old_oid,
const struct object_id *new_oid,
const char *base, struct diff_options *opt);
void diff_root_tree_oid(const struct object_id *new_oid, const char *base,
struct diff_options *opt);

struct combine_diff_path {
struct combine_diff_path *next;
Expand Down
9 changes: 3 additions & 6 deletions revision.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,9 +791,7 @@ static int rev_compare_tree(struct rev_info *revs,

tree_difference = REV_TREE_SAME;
revs->pruning.flags.has_changes = 0;
if (diff_tree_oid(&t1->object.oid, &t2->object.oid, "",
&revs->pruning) < 0)
return REV_TREE_DIFFERENT;
diff_tree_oid(&t1->object.oid, &t2->object.oid, "", &revs->pruning);

if (!nth_parent)
if (bloom_ret == 1 && tree_difference == REV_TREE_SAME)
Expand All @@ -804,17 +802,16 @@ static int rev_compare_tree(struct rev_info *revs,

static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)
{
int retval;
struct tree *t1 = get_commit_tree(commit);

if (!t1)
return 0;

tree_difference = REV_TREE_SAME;
revs->pruning.flags.has_changes = 0;
retval = diff_tree_oid(NULL, &t1->object.oid, "", &revs->pruning);
diff_tree_oid(NULL, &t1->object.oid, "", &revs->pruning);

return retval >= 0 && (tree_difference == REV_TREE_SAME);
return tree_difference == REV_TREE_SAME;
}

struct treesame_state {
Expand Down
14 changes: 5 additions & 9 deletions shallow.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ int is_repository_shallow(struct repository *r)
* supports a "valid" flag.
*/
define_commit_slab(commit_depth, int *);
static void free_depth_in_slab(int **ptr)
{
FREE_AND_NULL(*ptr);
}
struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
int shallow_flag, int not_shallow_flag)
{
Expand Down Expand Up @@ -150,15 +154,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
}
}
}
for (i = 0; i < depths.slab_count; i++) {
int j;

if (!depths.slab[i])
continue;
for (j = 0; j < depths.slab_size; j++)
free(depths.slab[i][j]);
}
clear_commit_depth(&depths);
deep_clear_commit_depth(&depths, free_depth_in_slab);

return result;
}
Expand Down
4 changes: 3 additions & 1 deletion t/t4010-diff-pathspec.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ test_expect_success 'setup submodules' '
test_expect_success 'diff-tree ignores trailing slash on submodule path' '
git diff --name-only HEAD^ HEAD submod >expect &&
git diff --name-only HEAD^ HEAD submod/ >actual &&
test_cmp expect actual
test_cmp expect actual &&
git diff --name-only HEAD^ HEAD -- submod/whatever >actual &&
test_must_be_empty actual
'

test_expect_success 'diff multiple wildcard pathspecs' '
Expand Down
5 changes: 3 additions & 2 deletions t/t5318-commit-graph.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ test_expect_success 'detect bad hash version' '
'

test_expect_success 'detect low chunk count' '
corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\02" \
corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\01" \
"missing the .* chunk"
'

Expand Down Expand Up @@ -574,7 +574,8 @@ test_expect_success 'detect invalid checksum hash' '

test_expect_success 'detect incorrect chunk count' '
corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \
"chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET
"commit-graph file is too small to hold [0-9]* chunks" \
$GRAPH_CHUNK_LOOKUP_OFFSET
'

test_expect_success 'git fsck (checks commit-graph)' '
Expand Down
Loading