On Fri, 7 Mar 2008, Johannes Schindelin wrote:
>
> > @@ -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.
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.
> > @@ -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).
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(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 0cdf198..afa9c9d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -366,6 +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");
+ 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)
--
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
