Hi,
On Thu, 6 Mar 2008, Linus Torvalds wrote:
> 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).
>
> 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 ;-)
> @@ -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;
> - }
> -
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.
> @@ -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");
>
> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
