Re: [RFC PATCH 07/15] cache_tree_update(): Capability to handle tree entries missing from index

On Mon, Sep 6, 2010 at 2:42 PM, Elijah Newren [off-list ref] wrote:

On Sun, Sep 5, 2010 at 3:09 PM, Elijah Newren [off-list ref] wrote:

quoted

n Sun, Sep 5, 2010 at 1:54 AM, Nguyen Thai Ngoc Duy [off-list ref] wrote:

quoted

On Sun, Sep 5, 2010 at 10:13 AM, Elijah Newren [off-list ref] wrote:

quoted

cache_tree_update() will write trees using the index.  With sparse clones,
the index will only contain entries matching the sparse limits, meaning
that the index does not provide enough information to write complete tree
objects.  Having cache_tree_update() take a tree (typically HEAD), will
allow new complete trees to be constructed by using entries from the
specified tree outside the sparse limits together with the index.

You are moving it closer to the index (from my view because I changed
in commit_tree()). This makes me think, why don't you move the base
tree into the index itself?

The index is supposed to save the image of full worktree. While you
don't have all path names, you have the clue to all of them, the base
tree. To me, that means it belongs to the index. That would reduce
code change to
 - cache-tree.c (generate new tree from the base tree and index)
 - read-cache.c (new sparse-clone index extension)
 - index writing operations (save the base tree in index): read-tree and merge

That's a really good idea.  I'll look into that.

Hmm..maybe before I get ahead of myself, I should back up for a
second.  Which way do we think is better -- handling this in
cache_tree_update() or doing a fixup in commit_tree()?  If the latter,
then I should just drop my 7/15 and 8/15 for your 13/17 and 14/17.  If
the former, then it makes sense to look into this change you suggest.
In that case, we'd probably keep my 7/15 but drop 8/15 for patch(es)
that implement your idea above.  But you're more familiar with the
index format than I am and it's your idea, so you'd probably be the
better one to implement it.

Thoughts?

The former makes more sense. I still keep an eye on remote merge
support. Think of this case: you request a remote merge and have a
completely new base tree, different from HEAD. Then you get some
conflicts inside narrow/sparse area. By the time you resolve all
conflicts and are about to commit, where do you get the base tree?

It could follow the same way merge does, store it in a file in
$GIT_DIR, similar to $GIT_DIR/MERGE_HEAD, and make "git commit" pick
it up. Or just store it in index. You would need to (or should) change
the index anyway, to prevent naive git implementation from using it.
It makes more sense to put some more in index rather than a separate
file. Some commands (especially "git commit") make use of temporary
index. I think putting the base tree in index is a good point here,
but I'm not quite sure.

Anyway if you want to play with it, apply this on top of my 04/17
(then change index_state.narrow_base to something suitable for sparse
clone)

diff --git a/cache.h b/cache.h
index 9c014ef..c7d626d 100644
--- a/cache.h
+++ b/cache.h

@@ -293,6 +293,7 @@ struct index_state {
 	struct cache_tree *cache_tree;
 	struct cache_time timestamp;
 	char *narrow_prefix;
+	unsigned char narrow_base[20];
 	void *alloc;
 	unsigned name_hash_initialized : 1,
 		 initialized : 1;

diff --git a/read-cache.c b/read-cache.c
index 250013c..15c1c9e 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -1191,7 +1191,8 @@ static int read_index_extension(struct

index_state *istate,
 		istate->resolve_undo = resolve_undo_read(data, sz);
 		break;
 	case CACHE_EXT_NARROW:
-		istate->narrow_prefix = xstrdup(data);
+		hashcpy(istate->narrow_base, data);
+		istate->narrow_prefix = xstrdup(data+sizeof(istate->narrow_base));
 		break;
 	default:
 		if (*ext < 'A' || 'Z' < *ext)

@@ -1396,6 +1397,7 @@ int discard_index(struct index_state *istate)
 	istate->alloc = NULL;
 	istate->initialized = 0;
 	free(istate->narrow_prefix);
+	hashcpy(istate->narrow_base, (const unsigned char *)EMPTY_TREE_SHA1_BIN);
 	istate->narrow_prefix = NULL;

 	/* no need to throw away allocated active_cache */

@@ -1627,11 +1629,15 @@ int write_index(struct index_state *istate, int newfd)
 			return -1;
 	}
 	if (get_narrow_prefix()) {
+		struct strbuf sb = STRBUF_INIT;
 		char *buf = get_narrow_string();
-		int len = strlen(buf)+1;
-		err = write_index_ext_header(&c, newfd, CACHE_EXT_NARROW, len) < 0 ||
-			ce_write(&c, newfd, buf, len) < 0;
+		int len = 20+strlen(buf)+1;
+		strbuf_add(&sb, istate->narrow_base, 20);
+		strbuf_addstr(&sb, buf);
 		free(buf);
+		err = write_index_ext_header(&c, newfd, CACHE_EXT_NARROW, len) < 0 ||
+			ce_write(&c, newfd, sb.buf, len) < 0;
+		strbuf_release(&sb);
 		if (err)
 			return -1;
 	}

-- 
Duy