Make unpack_trees() do separate source and destination indexes
cover Make unpack_trees() do separate source and destination indexes
Ok, this goes on top of the five-commit series yesterday. The first two patches are pure fixes and cleanups, the third one does a fairly mechanical thing to get rid of implied usage of "the_index", and the last one actually splits up the source and destination index handling.
Linus Torvalds (4):
Fix tree-walking compare_entry() in the presense of --prefix Add 'const' where appropriate to index handling functions Make 'unpack_trees()' take the index to work on as an argument Make 'unpack_trees()' have a separate source and destination index
The nice thing to see is how this clearly removes more lines than it adds, and the code is a bit more readable too, I think.
builtin-checkout.c | 9 +++ builtin-commit.c | 2 + builtin-merge-recursive.c | 2 + builtin-read-tree.c | 23 +------ cache.h | 10 ++-- diff-lib.c | 47 ++----------- hash.c | 6 +- hash.h | 4 +- read-cache.c | 12 ++-- tree-walk.c | 3 + unpack-trees.c | 158 ++++++++++++++++++++++----------------------- unpack-trees.h | 15 +++-- 12 files changed, 128 insertions(+), 163 deletions(-)
PATCH 1/4 Fix tree-walking compare_entry() in the presense of --prefix
When we make the "root" tree-walk info entry have a pathname in it, we need to have a ->prev pointer so that compare_entry will actually notice and traverse into the root.
--- tree-walk.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
tree-walk.c +3 -0
@@ -107,6 +107,7 @@ int tree_entry(struct tree_desc *desc, struct name_entry *entry)void setup_traverse_info (struct traverse_info *info,const char *base) {int pathlen =strlen (base); +static struct traverse_info dummy;memset (info,0 ,sizeof (*info));if (pathlen && base[pathlen-1 ] =='/' )
@@ -114,6 +115,8 @@ void setup_traverse_info(struct traverse_info *info, const char *base) info->pathlen = pathlen ? pathlen +1 :0 ; info->name.path = base; info->name.sha1 = (void *)(base + pathlen +1 ); +if (pathlen) + info->prev = &dummy; }char *make_traverse_path (char *path,const struct traverse_info *info,const struct name_entry *n)
Signature · 1 line
1.5.4.3.452.g67136
PATCH 2/4 Add 'const' where appropriate to index handling functions
In reply to Linus TorvaldsThis is in an effort to make the source index of 'unpack_trees()' as being const, and thus making the compiler help us verify that we only access it for reading.
The constification also extended to some of the hashing helpers that get called indirectly.
--- cache.h | 10 +++++----- hash.c | 6 +++--- hash.h | 4 ++-- read-cache.c | 12 ++++++------ 4 files changed, 16 insertions(+), 16 deletions(-)
cache.h +5 -5
@@ -346,12 +346,12 @@ extern void verify_non_filename(const char *prefix, const char *name);/* Initialize and use the cache information */ extern int read_index (struct index_state *);extern int read_index_from (struct index_state *,const char *path); -extern int write_index (struct index_state *,int newfd); +extern int write_index (const struct index_state *,int newfd);extern int discard_index (struct index_state *); -extern int unmerged_index (struct index_state *); +extern int unmerged_index (const struct index_state *);extern int verify_path (const char *path);extern int index_name_exists (struct index_state *istate,const char *name,int namelen); -extern int index_name_pos (struct index_state *,const char *name,int namelen); +extern int index_name_pos (const struct index_state *,const char *name,int namelen);#define ADD_CACHE_OK_TO_ADD 1/* Ok to add */ #define ADD_CACHE_OK_TO_REPLACE 2/* Ok to replace file/directory */ #define ADD_CACHE_SKIP_DFCHECK 4/* Ok to skip DF conflict checks */
@@ -368,8 +368,8 @@ extern int ce_same_name(struct cache_entry *a, struct cache_entry *b);#define CE_MATCH_IGNORE_VALID 01/* do not check the contents but report dirty on racily-clean entries */ #define CE_MATCH_RACY_IS_DIRTY 02 -extern int ie_match_stat (struct index_state *,struct cache_entry *,struct stat *,unsigned int ); -extern int ie_modified (struct index_state *,struct cache_entry *,struct stat *,unsigned int ); +extern int ie_match_stat (const struct index_state *,struct cache_entry *,struct stat *,unsigned int ); +extern int ie_modified (const struct index_state *,struct cache_entry *,struct stat *,unsigned int );extern int ce_path_match (const struct cache_entry *ce,const char **pathspec);extern int index_fd (unsigned char *sha1,int fd,struct stat *st,int write_object,enum object_type type,const char *path);
hash.c +3 -3
@@ -9,7 +9,7 @@* the existing entry, or the empty slot if none existed. The caller * can then look at the (*ptr) to see whether it existed or not. */ -static struct hash_table_entry *lookup_hash_entry (unsigned int hash,struct hash_table *table) +static struct hash_table_entry *lookup_hash_entry (unsigned int hash,const struct hash_table *table) {unsigned int size = table->size, nr = hash % size;struct hash_table_entry *array = table->array;
@@ -66,7 +66,7 @@ static void grow_hash_table(struct hash_table *table)free (old_array); } -void *lookup_hash (unsigned int hash,struct hash_table *table) +void *lookup_hash (unsigned int hash,const struct hash_table *table) {if (!table->array)return NULL ;
@@ -81,7 +81,7 @@ void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table)return insert_hash_entry (hash, ptr, table); } -int for_each_hash (struct hash_table *table,int (*fn)(void *)) +int for_each_hash (const struct hash_table *table,int (*fn)(void *)) {int sum =0 ;unsigned int i;
hash.h +2 -2
@@ -28,9 +28,9 @@ struct hash_table {struct hash_table_entry *array; }; -extern void *lookup_hash (unsigned int hash,struct hash_table *table); +extern void *lookup_hash (unsigned int hash,const struct hash_table *table);extern void **insert_hash (unsigned int hash,void *ptr,struct hash_table *table); -extern int for_each_hash (struct hash_table *table,int (*fn)(void *)); +extern int for_each_hash (const struct hash_table *table,int (*fn)(void *));extern void free_hash (struct hash_table *table);static inline void init_hash (struct hash_table *table)
read-cache.c +6 -6
@@ -255,13 +255,13 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)return changed; } -static int is_racy_timestamp (struct index_state *istate,struct cache_entry *ce) +static int is_racy_timestamp (const struct index_state *istate,struct cache_entry *ce) {return (istate->timestamp && ((unsigned int )istate->timestamp) <= ce->ce_mtime); } -int ie_match_stat (struct index_state *istate, +int ie_match_stat (const struct index_state *istate,struct cache_entry *ce,struct stat *st,unsigned int options) {
@@ -304,7 +304,7 @@ int ie_match_stat(struct index_state *istate,return changed; } -int ie_modified (struct index_state *istate, +int ie_modified (const struct index_state *istate,struct cache_entry *ce,struct stat *st,unsigned int options) {int changed, changed_fs;
@@ -412,7 +412,7 @@ int cache_name_compare(const char *name1, int flags1, const char *name2, int flareturn 0 ; } -int index_name_pos (struct index_state *istate,const char *name,int namelen) +int index_name_pos (const struct index_state *istate,const char *name,int namelen) {int first, last;
@@ -1201,7 +1201,7 @@ int discard_index(struct index_state *istate)return 0 ; } -int unmerged_index (struct index_state *istate) +int unmerged_index (const struct index_state *istate) {int i;for (i =0 ; i < istate->cache_nr; i++) {
@@ -1346,7 +1346,7 @@ static int ce_write_entry(SHA_CTX *c, int fd, struct cache_entry *ce)return ce_write (c, fd, ondisk, size); } -int write_index (struct index_state *istate,int newfd) +int write_index (const struct index_state *istate,int newfd) {SHA_CTX c;struct cache_header hdr;
Signature · 1 line
1.5.4.3.452.g67136
PATCH 3/4 Make 'unpack_trees()' take the index to work on as an argument
In reply to Linus TorvaldsThis is just a very mechanical conversion, and makes everybody set it to '&the_index' before calling, but at least it makes it more explicit where we work with the index.
The next stage would be to split that index usage up into a 'source' and a 'destination' index, so that we can unpack into a different index than we started out from.
--- builtin-checkout.c | 6 +++ builtin-commit.c | 1 + builtin-merge-recursive.c | 1 + builtin-read-tree.c | 1 + diff-lib.c | 2 + unpack-trees.c | 79 +++++++++++++++++++++++---------------------- unpack-trees.h | 1 + 7 files changed, 52 insertions(+), 39 deletions(-)
builtin-checkout.c +6 -0
@@ -152,6 +152,7 @@ static int reset_to_new(struct tree *tree, int quiet) {struct unpack_trees_options opts;struct tree_desc tree_desc; +memset (&opts,0 ,sizeof (opts)); opts.head_idx =-1 ; opts.update =1 ;
@@ -159,6 +160,7 @@ static int reset_to_new(struct tree *tree, int quiet) opts.merge =1 ; opts.fn = oneway_merge; opts.verbose_update = !quiet; + opts.index = &the_index;parse_tree (tree);init_tree_desc (&tree_desc, tree->buffer, tree->size);if (unpack_trees (1 , &tree_desc, &opts))
@@ -170,6 +172,7 @@ static void reset_clean_to_new(struct tree *tree, int quiet) {struct unpack_trees_options opts;struct tree_desc tree_desc; +memset (&opts,0 ,sizeof (opts)); opts.head_idx =-1 ; opts.skip_unmerged =1 ;
@@ -177,6 +180,7 @@ static void reset_clean_to_new(struct tree *tree, int quiet) opts.merge =1 ; opts.fn = oneway_merge; opts.verbose_update = !quiet; + opts.index = &the_index;parse_tree (tree);init_tree_desc (&tree_desc, tree->buffer, tree->size);if (unpack_trees (1 , &tree_desc, &opts))
@@ -224,8 +228,10 @@ static int merge_working_tree(struct checkout_opts *opts,struct tree_desc trees[2 ];struct tree *tree;struct unpack_trees_options topts; +memset (&topts,0 ,sizeof (topts)); topts.head_idx =-1 ; + topts.index = &the_index;refresh_cache (REFRESH_QUIET );
builtin-commit.c +1 -0
@@ -198,6 +198,7 @@ static void create_base_index(void) opts.head_idx =1 ; opts.index_only =1 ; opts.merge =1 ; + opts.index = &the_index; opts.fn = oneway_merge; tree =parse_tree_indirect (head_sha1);
builtin-merge-recursive.c +1 -0
@@ -213,6 +213,7 @@ static int git_merge_trees(int index_only, opts.merge =1 ; opts.head_idx =2 ; opts.fn = threeway_merge; + opts.index = &the_index;init_tree_desc_from_tree (t+0 , common);init_tree_desc_from_tree (t+1 , head);
builtin-read-tree.c +1 -0
@@ -102,6 +102,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)memset (&opts,0 ,sizeof (opts)); opts.head_idx =-1 ; + opts.index = &the_index;git_config (git_default_config);
diff-lib.c +2 -0
@@ -734,6 +734,7 @@ int run_diff_index(struct rev_info *revs, int cached) opts.merge =1 ; opts.fn = oneway_diff; opts.unpack_data = revs; + opts.index = &the_index;init_tree_desc (&t, tree->buffer, tree->size);if (unpack_trees (1 , &t, &opts))
@@ -787,6 +788,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt) opts.merge =1 ; opts.fn = oneway_diff; opts.unpack_data = &revs; + opts.index = &the_index;init_tree_desc (&t, tree->buffer, tree->size);if (unpack_trees (1 , &t, &opts))
unpack-trees.c +40 -39
248 lines not on this page · raw message
unpack-trees.h +1 -0
@@ -28,6 +28,7 @@ struct unpack_trees_options {struct cache_entry *df_conflict_entry;void *unpack_data; +struct index_state *index; }; extern int unpack_trees(unsigned n, struct tree_desc *t,
Signature · 1 line
1.5.4.3.452.g67136
Quoted 4 lines · Linus Torvalds
Ok, this goes on top of the five-commit series yesterday. The first two patches are pure fixes and cleanups, the third one does a fairly mechanical thing to get rid of implied usage of "
the_index", and the last one actually splits up the source and destination index handling.
What I have been wondering about this series was if we can re-enable cache-tree for more of the unpack_trees() users.
Currently, all unpack_trees() users, other than a single-tree read-tree, invalidates the whole cache-tree information, as Daniel's "pop one, decide what to put back, all in the original index" had too many manual index manipulations and sprinkling cache_tree_invalidate() call everywhere was too much cluttering of already hard-to-follow codepath.
I'd want to see at least two-way read-tree preserve as much cache-tree information as possible. People often work on one branch to completion (i.e. make a commit, which involves a write-tree, which leaves a fully valid cache-tree in the index), switch branches (i.e. two-way read-tree, which unfortunately decimates cache-tree in the current implementation) to commit a small fix. If major parts of the cache-tree were left intact during branch switching, these unchanged subdirectories do not have to be rehashed in write-tree when making this final commit.
Quoted 9 lines · Junio C Hamano
What I have been wondering about this series was if we can re-enable cache-tree for more of the
unpack_trees()users.Currently, all
unpack_trees()users, other than a single-tree read-tree, invalidates the whole cache-tree information, as Daniel's "pop one, decide what to put back, all in the original index" had too many manual index manipulations and sprinklingcache_tree_invalidate()call everywhere was too much cluttering of already hard-to-follow codepath.
It should work as-is, but not optimally.
The reason I say that is that the result-tree won't have any cache tree *at*all* with this series, which is a possible peformance poblem (but there are performance upsides to the series too, so I'm not sure it's all that noticeable). So the "source" index will keep its cache-tree unmodified, because it simply never got modified in the first place.
Of course, all _current_ users will have src_index == dst_index (except for the "diff_oneway" thing that doesn't have a destination at all, because it doesn't actually want any index manipulation), so for all current users this is all pure downside: we throw away the good cache_tree for the incoming index, and we aren't even able to use it for the result.
But because I envisioned that the people who really *care* will be the ones that want to have different incoming and outgoing indexes, I didn't actually worry about it. Ie all the "git status" and "git commit" stuff would presumably end up using different indexes, and then they can re-use the (unmodified) source, and that would generally be the one that they actually want.
But I dunno. Maybe we'd actually want to try to re-use the cache_tree from the source some way, if we normally actually want it in the destination.
So this series really isn't a final thing, but I do think that in the form I posted it, it's a step forward in (a) readability/maintainability and
(b) flexibility. And I think all the really *core* stuff got converted, but now there's a lot of small details hanging around that just don't take advantage of the new capabilities.
I basically tried to keep things bug-for-bug compatible. I also didn't try to optimize some things that I know I'll want to eventually.
For example, since we now create a new "index_state" entirely, that means that we may need to re-hash the cache entries in the name hash, and that means that we now allocate a new one *and*copy*it* rather than just re-use cache entries directly.
So that's the reason for the new add_entry() function that actually allocates a new cache_entry. And it actually has room for various very obvious optimizations that I did *not* do:
- if the cache_entry comes from a tree (common), just re-use it, since it wasn't hashed in the first place
- if "o->dst_index" is NULL, just don't do anything, because we'll not use the result anyway.
- if "o->dst_index == o->src_index" and the cache_entry doesn't have CE_HASHED set, just re-use it directly.
adn things like that. So there were a lot of small micro-optimizations that I didn't do, because quite frankly, this conversion was really painful in the first place (the set of patches I sent out may have _looked_ fairly clean, but that's only because I didn't show you the pain it was to create them ;)
Linus
PATCH 4/4 Make 'unpack_trees()' have a separate source and destination index
In reply to Linus TorvaldsWe will always unpack into our own internal index, but we will take the source from wherever specified, and we will optionally write the result to a specified index (optionally, because not everybody even _wants_ any
result: the index diffing really wants to just walk the tree and index in parallel).
This ends up removing a fair number more lines than it adds, for the simple reason that we can now skip all the crud that tried to be oh-so-careful about maintaining our position in the index as we were traversing and modifying it. Since we don't actually modify the source index any more, we can just update the 'o->pos' pointer without worrying about whether an index entry got removed or replaced or added to.
--- builtin-checkout.c | 9 ++- builtin-commit.c | 3 +- builtin-merge-recursive.c | 3 +- builtin-read-tree.c | 24 +------- diff-lib.c | 49 +++------------ unpack-trees.c | 149 ++++++++++++++++++++++----------------------- unpack-trees.h | 16 +++-- 7 files changed, 101 insertions(+), 152 deletions(-)
builtin-checkout.c +6 -3
@@ -160,7 +160,8 @@ static int reset_to_new(struct tree *tree, int quiet) opts.merge =1 ; opts.fn = oneway_merge; opts.verbose_update = !quiet; - opts.index = &the_index; + opts.src_index = &the_index; + opts.dst_index = &the_index;parse_tree (tree);init_tree_desc (&tree_desc, tree->buffer, tree->size);if (unpack_trees (1 , &tree_desc, &opts))
@@ -180,7 +181,8 @@ static void reset_clean_to_new(struct tree *tree, int quiet) opts.merge =1 ; opts.fn = oneway_merge; opts.verbose_update = !quiet; - opts.index = &the_index; + opts.src_index = &the_index; + opts.dst_index = &the_index;parse_tree (tree);init_tree_desc (&tree_desc, tree->buffer, tree->size);if (unpack_trees (1 , &tree_desc, &opts))
@@ -231,7 +233,8 @@ static int merge_working_tree(struct checkout_opts *opts,memset (&topts,0 ,sizeof (topts)); topts.head_idx =-1 ; - topts.index = &the_index; + topts.src_index = &the_index; + topts.dst_index = &the_index;refresh_cache (REFRESH_QUIET );
builtin-commit.c +2 -1
@@ -198,7 +198,8 @@ static void create_base_index(void) opts.head_idx =1 ; opts.index_only =1 ; opts.merge =1 ; - opts.index = &the_index; + opts.src_index = &the_index; + opts.dst_index = &the_index; opts.fn = oneway_merge; tree =parse_tree_indirect (head_sha1);
builtin-merge-recursive.c +2 -1
@@ -213,7 +213,8 @@ static int git_merge_trees(int index_only, opts.merge =1 ; opts.head_idx =2 ; opts.fn = threeway_merge; - opts.index = &the_index; + opts.src_index = &the_index; + opts.dst_index = &the_index;init_tree_desc_from_tree (t+0 , common);init_tree_desc_from_tree (t+1 , head);
builtin-read-tree.c +2 -22
@@ -102,7 +102,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)memset (&opts,0 ,sizeof (opts)); opts.head_idx =-1 ; - opts.index = &the_index; + opts.src_index = &the_index; + opts.dst_index = &the_index;git_config (git_default_config);
@@ -221,27 +222,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)if ((opts.dir && !opts.update))die ("--exclude-per-directory is meaningless unless -u" ); -if (opts.prefix) { -int pfxlen =strlen (opts.prefix); -int pos; -if (opts.prefix[pfxlen-1 ] !='/' ) -die ("prefix must end with /" ); -if (stage !=2 ) -die ("binding merge takes only one tree" ); - pos =cache_name_pos (opts.prefix, pfxlen); -if (0 <= pos) -die ("corrupt index file" ); - pos = -pos-1 ; -if (pos < active_nr && - !strncmp (active_cache[pos]->name, opts.prefix, pfxlen)) -die ("subdirectory '%s' already exists." , opts.prefix); - pos =cache_name_pos (opts.prefix, pfxlen-1 ); -if (0 <= pos) -die ("file '%.*s' already exists." , - pfxlen-1 , opts.prefix); - opts.pos =-1 - pos; - } -if (opts.merge) {if (stage <2 )die ("just how do you expect me to merge %d trees?" , stage-1 );
diff-lib.c +8 -41
91 lines not on this page · raw message
unpack-trees.c +72 -77
409 lines not on this page · raw message
unpack-trees.h +9 -7
@@ -4,8 +4,7 @@struct unpack_trees_options ;typedef int (*merge_fn_t )(struct cache_entry **src, -struct unpack_trees_options *options, -int remove); +struct unpack_trees_options *options);struct unpack_trees_options {int reset;
@@ -28,15 +27,18 @@ struct unpack_trees_options {struct cache_entry *df_conflict_entry;void *unpack_data; - struct index_state *index; + +struct index_state *dst_index; +const struct index_state *src_index; + struct index_state result; };extern int unpack_trees (unsigned n,struct tree_desc *t,struct unpack_trees_options *options); -int threeway_merge (struct cache_entry **stages,struct unpack_trees_options *o,int ); -int twoway_merge (struct cache_entry **src,struct unpack_trees_options *o,int ); -int bind_merge (struct cache_entry **src,struct unpack_trees_options *o,int ); -int oneway_merge (struct cache_entry **src,struct unpack_trees_options *o,int ); +int threeway_merge (struct cache_entry **stages,struct unpack_trees_options *o); +int twoway_merge (struct cache_entry **src,struct unpack_trees_options *o); +int bind_merge (struct cache_entry **src,struct unpack_trees_options *o); +int oneway_merge (struct cache_entry **src,struct unpack_trees_options *o);#endif
Signature · 1 line
1.5.4.3.452.g67136
Re: [PATCH 4/4] Make 'unpack_trees()' have a separate source and destination index
Quoted 6 lines · Linus Torvalds
We will always unpack into our own internal index, but we will take the source from wherever specified, and we will optionally write the result to a specified index (optionally, because not everybody even _wants_ any
result: the index diffing really wants to just walk the tree and index in parallel).
Quoted 6 lines
This ends up removing a fair number more lines than it adds, for the simple reason that we can now skip all the crud that tried to be oh-so-careful about maintaining our position in the index as we were traversing and modifying it. Since we don't actually modify the source index any more, we can just update the '
o->pos' pointer without worrying about whether an index entry got removed or replaced or added to.
It looks to me like it's leaking stuff stored in the index it creates if it ends up failing. I'm not entirely sure of the index lifecycle stuff, but it seems like it would be necessary. Aside from that, I think it should be right, although I haven't really gone over the resulting code in detail.
-Daniel
*This .sig left intentionally blank*
Re: [PATCH 4/4] Make 'unpack_trees()' have a separate source and destination index
Quoted 3 lines · Daniel Barkalow
It looks to me like it's leaking stuff stored in the index it creates if it ends up failing.
Yes, adding a "discard_index(&o->result)" to the failure path sounds like a good idea. Patch appended.
Quoted 2 lines
I'm not entirely sure of the index lifecycle stuff, but it seems like it would be necessary.
Well, "necessary" may not be the right word, because it's a purely internal index thing, so all that happens is a memory leak. But it's certainly easy to do and correct.
That said, the *big* memory leak comes from the fact that we leak the cache_entry things left and right. That's a very traditional leak: because we used to build up the cache entries by just mapping them directly in from the index file (and we emulate that in modern times by allocating them from one big array), we can't actually free them one-by-one.
So doing the "discard_index()" will free the hash tables etc, which is good, and it will free the "istate->alloc" but that is never set on the result because we don't get the result from the index read. So we don't actually free the individual cache entries themselves that got created from the trees.
That's not something new, btw. We never did. But some day we should just add a flag to the cache_entry() that it's a "free one by one" kind, and then we could/should do it. In the meantime, this one-liner will fix *some* of the memory leaks, but not that old traditional one.
Linus
--- unpack-trees.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
unpack-trees.c +1 -0
@@ -315,6 +315,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, strstatic int unpack_failed (struct unpack_trees_options *o,const char *message) { +discard_index (&o->result);if (!o->gently) {if (message)return error (message);
Re: [PATCH 4/4] Make 'unpack_trees()' have a separate source and destination index
In reply to Linus TorvaldsHi,
Quoted 6 lines · Linus Torvalds
We will always unpack into our own internal index, but we will take the source from wherever specified, and we will optionally write the result to a specified index (optionally, because not everybody even _wants_ any
result: the index diffing really wants to just walk the tree and index in parallel).
Quoted 6 lines
This ends up removing a fair number more lines than it adds, for the simple reason that we can now skip all the crud that tried to be oh-so-careful about maintaining our position in the index as we were traversing and modifying it. Since we don't actually modify the source index any more, we can just update the '
o->pos' pointer without worrying about whether an index entry got removed or replaced or added to.
Nice. Although you could have done better on the account of remove/add ratio if you did not add empty lines ;-) But then, you removed a few curly brackets around single lines, too ;-)
Quoted 15 lines
builtin-read-tree.c +0
-11@@ -221,27 +222,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)if ((opts.dir && !opts.update))die ("--exclude-per-directory is meaningless unless -u" ); -if (opts.prefix) { -int pfxlen =strlen (opts.prefix); -int pos; -if (opts.prefix[pfxlen-1 ] !='/' ) -die ("prefix must end with /" ); -if (stage !=2 ) -die ("binding merge takes only one tree" ); - pos =cache_name_pos (opts.prefix, pfxlen); -if (0 <= pos) -die ("corrupt index file" ); - pos = -pos-1 ;
builtin-read-tree.c int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) · line 244
Quoted 10 lines
builtin-read-tree.c +0
-10-if (pos < active_nr && - !strncmp (active_cache[pos]->name, opts.prefix, pfxlen)) -die ("subdirectory '%s' already exists." , opts.prefix); - pos =cache_name_pos (opts.prefix, pfxlen-1 ); -if (0 <= pos) -die ("file '%.*s' already exists." , - pfxlen-1 , opts.prefix); - opts.pos =-1 - pos; - } -
Was the wholesale removal intentional? I think there are a few sanity checks, and I did not see the checks moved to somewhere else. But then, there could be redundant checks somewhere else that I missed.
unpack-trees.c int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options · line 375
Quoted 11 lines
unpack-trees.c +3
-0@@ -360,7 +366,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsif (o->trivial_merges_only && o->nontrivial_merge)return unpack_failed (o,"Merge requires file-level merging" ); + o->src_index =NULL ;check_updates (o); +if (o->dst_index) + *o->dst_index = o->result;return 0 ; }
I wonder if you should discard_index(o->dst_index) if o->src_index == o->dst_index (before you set it to NULL, of course).
I cannot really comment on the rest, since the whole unpack_trees() logic was too complicated for me. Maybe I have a chance after this patch.
Ciao,
Dscho
Re: [PATCH 4/4] Make 'unpack_trees()' have a separate source and destination index
Quoted 22 lines · Johannes Schindelin
Quoted 22 lines
(quoted diff) +0
-17@@ -221,27 +222,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) if ((opts.dir && !opts.update)) die("--exclude-per-directory is meaningless unless -u"); - if (opts.prefix) { - int pfxlen = strlen(opts.prefix); - int pos; - if (opts.prefix[pfxlen-1] != '/') - die("prefix must end with /"); - if (stage != 2) - die("binding merge takes only one tree"); - pos = cache_name_pos(opts.prefix, pfxlen); - if (0 <= pos) - die("corrupt index file"); - pos = -pos-1; - if (pos < active_nr && - !strncmp(active_cache[pos]->name, opts.prefix, pfxlen)) - die("subdirectory '%s' already exists.", opts.prefix); - pos = cache_name_pos(opts.prefix, pfxlen-1); - if (0 <= pos) - die("file '%.*s' already exists.",
Quoted 8 lines
Review int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) · line 244
Quoted 4 lines
(quoted diff) +0
-4- pfxlen-1, opts.prefix); - opts.pos = -1 - pos; - } -Was the wholesale removal intentional? I think there are a few sanity checks, and I did not see the checks moved to somewhere else. But then, there could be redundant checks somewhere else that I missed.
It was intentional. The "bind_merge()" function already checks for overlap in the individual entries, so those sanity checks don't actually buy you anything. And those games with "opt.pos" actualyl made the new model not work, because it would mean that we'd skip the old entries before that "pos" entirely (instead of moving them over to the new index).
However, you're right to point it out. The old checks are somewhat different from the new ones - "bind_merge()" will check for overlap in individual entries, but it doesn't disallow merging non-overlapping subdirectories. I thought that extending the functionality would be a feature, but maybe somebody really wants those checks to be there on purpose.
So maybe we could have left a few of those checks. I just thought it was pretty interesting how we disallowed doing
- index:
subdirectory/Makefile
- tree:
.. anything *but* a 'Makefile' entry ..
- git-read-tree --prefix subdirectory/ <tree>
and without the checks it actually works fine (it creates a "union" of the index and the incoming tree).
If somebody doesn't want that, I think they should check up-front (easy
enough: just test if "git ls-files subdirectory/" returns anything), so I actually think the sanity checks just remove capabilities rather than add anything.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0
-0@@ -360,7 +366,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options if (o->trivial_merges_only && o->nontrivial_merge) return unpack_failed(o, "Merge requires file-level merging");
Quoted 10 lines
Review int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options · line 375
Quoted 7 lines
(quoted diff) +3
-0+ o->src_index = NULL; check_updates(o); + if (o->dst_index) + *o->dst_index = o->result; return 0; }I wonder if you should
discard_index(o->dst_index)ifo->src_index==o->dst_index(before you set it to NULL, of course).
Not quite yet. I _really_ wanted to, but we can't do it - there's a few users that want to use "the_index" that I was not ready to work out what the right thing to do was.
In particular, when we do write_entry() of the finished index, that one does:
check_updates
checkout_entry()
write_entry
convert_to_working_tree
git_checkattr
bootstrap_attr_stack
read_attr
read_index_data
index_name_pos(&the_index)
and the thing is, if we discard the old "the_index()" early (like we *should* do), that callchain breaks because the attribute code will use "the_index" rather than the index we are actually using!
Now, the fact is, I think that's a bug in the attribute code (well, "historical artifact" from the fact that we always used to use just "the_index" for everything). It should no longer (in my opinion) just blindly use "the_index" when we are writing out something else than "the_index", but hey, that's what it does.
If you want to play with it, here's a patch that I did *not* post on top of the whole series that makes the issue more obvious. That "memset()" is just to poison any users of the index that isn't finalized yet, to make you get a nice SIGSEGV rather than just inexplicable failures.
Anyway, to fix that thing, we'd have to pass the correct index around all the way, which is definitely worth doing regardless, but it was kind of beyond the aim of _this_ particular patch series.
Here's the patch to get you started :)
Linus
--- unpack-trees.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
unpack-trees.c +4 -0
@@ -366,6 +366,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsif (o->trivial_merges_only && o->nontrivial_merge)return unpack_failed (o,"Merge requires file-level merging" ); +if (o->dst_index) { +discard_index (o->dst_index); +memset (o->dst_index,0x55 ,sizeof (*o->dst_index)); + } o->src_index =NULL ;check_updates (o);if (o->dst_index)