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

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

On Mon, Sep 6, 2010 at 2:42 PM, Elijah Newren <newren@gmail.com> wrote:
> On Sun, Sep 5, 2010 at 3:09 PM, Elijah Newren <newren@gmail.com> wrote:
>> n Sun, Sep 5, 2010 at 1:54 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>>> On Sun, Sep 5, 2010 at 10:13 AM, Elijah Newren <newren@gmail.com> wrote:
>>>> 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