"Nick Edelen" [off-list ref] writes:
Note that I didn't read the entire patch, so the parts I did not comment
on in this message may or may not have problematic parts.
- extra 'size' field added to commit, tag and blob objects, initialized in
parse_* functions and cache traversal
It is unclear which "size" this field refers to without comment.
I do not know if this change is justifiable. We (mostly Linus) spent
considerble effort to shrink the memory footprint of struct commit (and
struct object in general) exactly because revision traversal needs to
inspect tons of them.
+static unsigned int parse_size(const char *name)
+{
+ unsigned int size;
+ char *p;
+
+ size = strtol(name, &p, 10);
+ switch (*p) {
+ case 'k' :
+ case 'K' :
+ size *= 0x400;
+ break;
+ case 'm' :
+ case 'M' :
+ size *= 0x100000;
+ break;
+ default :
+ break;
+ }
+
+ return size;
+}
Looks vaguely familiar.
The configuration parser already knows about these size suffixes when told
to read "int". Can't that code, e.g. git_parse_ulong(), be reused here?
quoted hunk
@@ -219,7 +221,6 @@ unsigned char *get_cache_slice(struct commit *commit)
return 0;
ie = search_index(commit->object.sha1);
-
if (ie && ie->cache_index < idx_head.cache_nr)
return idx_caches + ie->cache_index * 20;
"Oops, fix an earlier mistake"...
quoted hunk
@@ -262,7 +282,6 @@ static int setup_traversal(struct cache_slice_header *head, unsigned char *map,
struct commit_list *prev, *wp, **wpp;
int retval;
- /* printf("adding %s\n", sha1_to_hex(commit->object.sha1)); */
Likewise.
It is unclear which "size" this field refers to without comment.
I do not know if this change is justifiable. We (mostly Linus) spent
considerble effort to shrink the memory footprint of struct commit (and
struct object in general) exactly because revision traversal needs to
inspect tons of them.
I originally added a `size` field to objects, a `unique` field to
commits and `name` ones to blobs/trees. This was (I reasoned) partly
for applications to take fuller advantage of rev-cache's abilities,
but largely to ease re-use of its information in slice regeneration.
However, in retrospect I think you're probably right. Not only do I
not really need to load the information into memory to re-use it, but
applications using rev-cache would probably have sufficiently
specialized needs to warrant direct access. In light of this I've
rewritten the fuse command to re-use data directly from the slice,
rather than requiring it to be loaded into memory first (hence
eliminating the all those extra fields). Furthermore I'll put all the
(renamed) structures and definitions in a seperate header, to allow
easy direct access to slices by third-parties.
Looks vaguely familiar.
The configuration parser already knows about these size suffixes when told
to read "int". Can't that code, e.g. git_parse_ulong(), be reused here?
You're also right here. I wrote it fast and hadn't checked if git had
already implemented it. I've changed it to use git_parse_ulong().
Sorry I haven't uploaded anything yet. I keep seeing new things that
could updated or revised; so far I've
- rewritten and added much to the documentation
- added support for an alternates-like mechanism
- changed --noobjects to --no-objects
- changed init_rci to init_rev_cache_info
- modified make_cache_slice to return actual start/end commits
- changed coagulate_ to fuse_
- added an (admittedly crude) escape plan in case of obscenely large
merges/branches
- cleaned up path generation
- replace parse_size with git_parse_ulong
- rewritten fuse to reuse objects verbatim, rather than via memory access
And on my todo list I've still got things such as renaming all the
structures/definitions, adding a more portable _ondisk version of
bitfield'ed structs and obviously cleanup the patchset (eliminating
fixes of earlier patches, etc.).
I hope to get everything finished in the next couple days, and upload
a v3 by friday at the latest.
- Nick